[Autocomplete] Prevent reset scroll position when new options are added #35735

sai6855 · 2023-01-05T13:00:26Z

working video: https://www.loom.com/share/6a38df21a3e2422c973c4b1e3ea9ba7f

working codesandbox demo: https://codesandbox.io/s/nervous-euclid-2ekej7

tests autocomplete performance demo in this PR: https://codesandbox.io/s/blazing-platform-io3mgu?file=/demo.tsx

tests autocomplete performance demo in master https://codesandbox.io/s/nameless-platform-nohxeo?file=/demo.tsx

open consoles for both demos and keep on navigating options and observe actualDuration for both demos, looks like both actualDuration times are in similar range (10-20ms).

I have followed (at least) the PR section of the contributing guide.

mui-bot · 2023-01-05T13:03:33Z

	Messages
📖	Netlify deploy preview: https://deploy-preview-35735--material-ui.netlify.app/

@material-ui/lab: parsed: +0.14% , gzip: +0.14%
@material-ui/unstyled: parsed: +0.27% , gzip: +0.30%

Details of bundle changes

Generated by 🚫 dangerJS against e6f5294

packages/mui-material/src/Autocomplete/Autocomplete.test.js

ZeeshanTamboli

Thanks for working on this.

I tried to review the code. Someone else familiar with the logic will take a deep look.

I would also recommend investigating why it stopped working in version 5. I would also suggest providing a preview package to the community in the related issue for them to test it out in real-world applications, as this looks complicated with heavy calculations.

packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js

packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx

sai6855 · 2023-01-14T16:23:40Z

@ZeeshanTamboli updated PR description which includes performance demos, and looks like there isn't much difference in render times b/w master and this branch. Let me know if there are any issues with implementation on how performance is calculated

packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js

packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx

ZeeshanTamboli

It looks good to me. Thanks for your work, @sai6855. This needs a review from @michaldudak, though. It would also help if we get a response on #29508 (comment).

vitalivasileuski-st · 2023-01-23T15:25:27Z

We at SensorTower tested this PR for AutoComplete with custom Option and Listbox components. ListBox inside uses InfiniteScroll component and some additional not related components. With useMemo for the ListBox it works perfectly fine.

sai6855 · 2023-01-27T10:32:20Z

Comment made in issue by one of the user that this PR is working fine in their set-up : #29508 (comment)

ZeeshanTamboli · 2023-01-27T14:17:31Z

@michaldudak Can you take a look?

packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js

michaldudak

Great! I couldn't find any scenario where it fails. Thank you for the fix!

sai6855 · 2023-01-30T09:41:50Z

Thanks!! Once this is merged I'll resume working on #35653

oliviertassinari

Nice to see this bug fixed, the solution looks nice 👍

oliviertassinari · 2023-01-31T21:23:28Z

packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js

+
+      if (previousHighlightedOption) {
+        const previousHighlightedOptionExists = filteredOptions.some((option) => {
+          return getOptionLabel(option) === getOptionLabel(previousHighlightedOption);


Are we sure about this comparison? I would expect the scroll to reset if no options are equal to be enough.

Suggested change

return getOptionLabel(option) === getOptionLabel(previousHighlightedOption);

return option === previousHighlightedOption;

But in the case when options are objects, it would always reset, with the strict equality always returning false because they point to different object instances.

it would always reset

Why? I would expect developers to fetch new options, append them, and keep the same reference to the existing options. At least, the bug reproduction we have seem to be compatible with this idea.

If developers re-fetch all the options which leads to creating new references, then maybe they would also expect a scroll reset?

So you're saying that, consider in the test, this is incorrect because developers re-fetch all the options (including a new one), so we should expect to reset? But shouldn't React developers treat objects/arrays as immutable when storing in state (options prop)? Then it would be a new reference?

Yes, in this test, these look like entirely new options. I think that a scroll reset can make sense.

#26492 makes the case that developers expect support for duplicated option labels.

Actually, we could use isOptionEqualToValue to determine if two options are the same instead of relying on labels. This should also fix #36114. I created a PR with this approach: #36116

@oliviertassinari resetting the highlight when a reference to an option changes but its value/label does not could lead to a worse DX, for example when a new set of options is received from a server, and it includes the currently highlighted one. I would expect the highlight to remain where it was.

packages/mui-material/src/Autocomplete/Autocomplete.test.js

waful · 2023-02-09T03:49:28Z

packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js

+      previousProps.filteredOptions &&
+      previousProps.filteredOptions.length !== filteredOptions.length &&
+      (multiple
+        ? previousProps.value.every((val, i) => getOptionLabel(value[i]) === getOptionLabel(val))


This PR introduces a runtime error TypeError: Cannot read properties of undefined (reading 'label') at getOptionLabel(value[i]) when under these conditions:

Autocomplete is using the multiple, disableCloseOnSelect, and filterSelectedOptions flags

There is currently one selected option

You delete the existing option while the dropdown is open

When you delete the option, value is now [] and so value[i] is undefined. When fed to getOptionLabel, it crashes because it expects option to not be undefined.

Line 100: getOptionLabel: getOptionLabelProp = (option) => option.label ?? option

EDIT: issue filed with more details and replication sandbox: #36114

honia19 · 2023-09-13T20:11:21Z

I'm really sorry, is it already released? If yes, what kind of version?

michaldudak · 2023-09-14T07:30:14Z

This was released in @mui/material 5.11.7

honia19 · 2023-09-14T07:43:55Z

This was released in @mui/material 5.11.7

But it's still not working with infinity scroll when I use the handle onScroll, I'm trying to make components with virtualization and infinity scroll together. And I still see issues with scroll to top when I review new data.

sai6855 · 2023-09-14T07:45:07Z

@honia19 can you create a new issue with reproduction

honia19 · 2023-09-14T07:46:17Z

@honia19 can you create a new issue with reproduction

Okay I will tag you

honia19 · 2024-01-18T20:48:23Z

@sai6855 Link to reproduce: https://codesandbox.io/s/autocomplete-scroll-bug-39wfv

quoc-jordan-an · 2024-11-07T20:21:37Z

@sai6855 Link to reproduce: https://codesandbox.io/s/autocomplete-scroll-bug-39wfv

Your problem can be solved by adding role: "list-box" to ListboxProps.

Kudos to this comment: #30249 (comment)

sai6855 added 2 commits January 5, 2023 18:27

autocomplete reset scroll

506224b

test name

28975c8

zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Jan 5, 2023

zannager requested a review from michaldudak January 5, 2023 13:04

sai6855 changed the title ~~[Autocomplete] prevent reset to scroll position when new options are added~~ [Autocomplete] prevent reset scroll position when new options are added Jan 5, 2023

sai6855 marked this pull request as draft January 5, 2023 13:14

added tests for joy ui

2733b10

sai6855 marked this pull request as ready for review January 5, 2023 13:29

sai6855 mentioned this pull request Jan 5, 2023

[Autocomplete] Add onScrollToBottom prop #35653

Closed

1 task

sai6855 marked this pull request as draft January 5, 2023 18:06

sai6855 added 3 commits January 6, 2023 11:08

updated logic and tests

7ba6ecc

logic patch

80221d6

logic patch

2e2a772

sai6855 marked this pull request as ready for review January 6, 2023 18:13

sai6855 added 3 commits January 6, 2023 23:49

logic patch

f3bf7d3

logic patch

429c2c1

reset

3c36115

sai6855 mentioned this pull request Jan 10, 2023

[Autocomplete] List gets scrolled to the top when new data is received #29508

Closed

2 tasks

ZeeshanTamboli added the bug 🐛 Something doesn't work label Jan 11, 2023

ZeeshanTamboli changed the title ~~[Autocomplete] prevent reset scroll position when new options are added~~ [Autocomplete] Prevent reset scroll position when new options are added Jan 11, 2023

ZeeshanTamboli reviewed Jan 11, 2023

View reviewed changes

packages/mui-material/src/Autocomplete/Autocomplete.test.js Show resolved Hide resolved

ZeeshanTamboli mentioned this pull request Jan 11, 2023

[Autocomplete] prevent highlight index from resetting when options are added #33645

Closed

1 task

ZeeshanTamboli reviewed Jan 14, 2023

View reviewed changes

packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js Outdated Show resolved Hide resolved

packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js Outdated Show resolved Hide resolved

packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx Outdated Show resolved Hide resolved

sai6855 added 4 commits January 14, 2023 15:44

changes requested

54cb9c3

added tests

c95c288

lint

14ccfd9

changed test name

33c970d

ZeeshanTamboli reviewed Jan 18, 2023

View reviewed changes

prop fix

fb77c56

sai6855 requested review from ZeeshanTamboli and removed request for michaldudak January 18, 2023 12:20

ZeeshanTamboli requested a review from michaldudak January 23, 2023 07:10

ZeeshanTamboli approved these changes Jan 23, 2023

View reviewed changes

michaldudak reviewed Jan 29, 2023

View reviewed changes

packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js Show resolved Hide resolved

extract code to function

c75f72e

sai6855 requested a review from michaldudak January 29, 2023 10:12

michaldudak approved these changes Jan 30, 2023

View reviewed changes

Merge branch 'master' into autocomplete-scrolltop

e6f5294

hbjORbj merged commit 14dba8d into mui:master Jan 31, 2023

oliviertassinari reviewed Jan 31, 2023

View reviewed changes

sai6855 deleted the autocomplete-scrolltop branch February 1, 2023 05:10

waful reviewed Feb 9, 2023

View reviewed changes

waful mentioned this pull request Feb 9, 2023

[Autocomplete] 5.11.7 Introduced a Crashing Bug For This Configuration of Autocomplete #36114

Closed

2 tasks

michaldudak mentioned this pull request Feb 9, 2023

[Autocomplete] Fix tag removal regression #36116

Merged

lensroman mentioned this pull request Mar 28, 2023

[Autocomplete] Prevent reset scroll position when new options are added lookfirst/mui-rff#1042

Closed

Incin mentioned this pull request Sep 14, 2023

Fix jarring auto scroll on multi select when selecting a variable at the bottom of the list trelliscope/trelliscopejs-lib#1107

Open

adi518 mentioned this pull request May 18, 2024

[Autocomplete] Adding items to options causes scroll to go to the top of the list (paging) #30249

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Autocomplete] Prevent reset scroll position when new options are added #35735

[Autocomplete] Prevent reset scroll position when new options are added #35735

sai6855 commented Jan 5, 2023 •

edited

Loading

mui-bot commented Jan 5, 2023 •

edited

Loading

ZeeshanTamboli left a comment

sai6855 commented Jan 14, 2023 •

edited

Loading

ZeeshanTamboli left a comment

vitalivasileuski-st commented Jan 23, 2023

sai6855 commented Jan 27, 2023

ZeeshanTamboli commented Jan 27, 2023

michaldudak left a comment

sai6855 commented Jan 30, 2023

oliviertassinari left a comment

oliviertassinari Jan 31, 2023

ZeeshanTamboli Feb 6, 2023 •

edited

Loading

oliviertassinari Feb 6, 2023 •

edited

Loading

ZeeshanTamboli Feb 8, 2023 •

edited

Loading

oliviertassinari Feb 8, 2023 •

edited

Loading

michaldudak Feb 9, 2023 •

edited

Loading

waful Feb 9, 2023 •

edited

Loading

honia19 commented Sep 13, 2023

michaldudak commented Sep 14, 2023

honia19 commented Sep 14, 2023

sai6855 commented Sep 14, 2023

honia19 commented Sep 14, 2023

honia19 commented Jan 18, 2024 •

edited

Loading

quoc-jordan-an commented Nov 7, 2024

	return getOptionLabel(option) === getOptionLabel(previousHighlightedOption);
	return option === previousHighlightedOption;

[Autocomplete] Prevent reset scroll position when new options are added #35735

[Autocomplete] Prevent reset scroll position when new options are added #35735

Conversation

sai6855 commented Jan 5, 2023 • edited Loading

mui-bot commented Jan 5, 2023 • edited Loading

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

sai6855 commented Jan 14, 2023 • edited Loading

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

vitalivasileuski-st commented Jan 23, 2023

sai6855 commented Jan 27, 2023

ZeeshanTamboli commented Jan 27, 2023

michaldudak left a comment

Choose a reason for hiding this comment

sai6855 commented Jan 30, 2023

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Jan 31, 2023

Choose a reason for hiding this comment

ZeeshanTamboli Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

ZeeshanTamboli Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

michaldudak Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

waful Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

honia19 commented Sep 13, 2023

michaldudak commented Sep 14, 2023

honia19 commented Sep 14, 2023

sai6855 commented Sep 14, 2023

honia19 commented Sep 14, 2023

honia19 commented Jan 18, 2024 • edited Loading

quoc-jordan-an commented Nov 7, 2024

sai6855 commented Jan 5, 2023 •

edited

Loading

mui-bot commented Jan 5, 2023 •

edited

Loading

sai6855 commented Jan 14, 2023 •

edited

Loading

ZeeshanTamboli Feb 6, 2023 •

edited

Loading

oliviertassinari Feb 6, 2023 •

edited

Loading

ZeeshanTamboli Feb 8, 2023 •

edited

Loading

oliviertassinari Feb 8, 2023 •

edited

Loading

michaldudak Feb 9, 2023 •

edited

Loading

waful Feb 9, 2023 •

edited

Loading

honia19 commented Jan 18, 2024 •

edited

Loading