[Autocomplete] fix adding items (pagination) causing scroll to go

[kshitij-p]

Closes #30249

Link to a demo demonstrating the fix: DEMO

I have a few questions about the docs demo part-

Since my PR adds two new props:

1. isPaginated - boolean which when set to true, the scroll into view functionality checks if current number of options is >= totalOptions prop to determine whether to scroll to top or not
2. totalOptions - number which holds the total number of options going to be available when all pages are added

Now the question is, do I need to write a demo for the isPaginated prop ? Because then I would have to write a demo for the docs with an implementation of the pagination that complies with keyboard accessibility and other concerns.

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

[mui-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 9bc8fbd

[aakash-lohono]

hi @kshitij-p, thanks for the effort

i'm not sure what the general consensus w.r.t to this issue.
following are my observations over proposed changes:

1. this assumes traditional pagination approach of total and current count.
2. this isn't the case for us and might not be for a lot of people
3. alternate use case which is also quite common is using row_number in sql query for performance reasons ref1 ref2
4. what if the primary key is some kind of string, usually uuid ref1 ref2. (think of requesting 10 records after aaa....ffff)
5. instead, we can do the following at ref with a resetPosition: boolean parameter

if (resetPosition && index === -1) {
  listboxNode.scrollTop = 0;
  return;
}

6. this way users will have to override and can still use whatever flavour of pagination they deem fit.

[siriwatknp]

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

1. pull latest master from upstream to your branch
2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
   2.1 revert the change on those markdown files you did
   2.2 pull latest master from upstream (you should not see conflict)
   2.3 make the changes again at docs/data/material/*
3. run yarn docs:api
   • you might see the changes in docs/pages/material/api/* if your PR touches some of the components
   • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

[alannungaray]

Any news of this?

[kshitij-p]

Sorry for not maintaining any communication, been a bit busy with college work.

Finishing the steps mentioned above by siriwaktnp as of now.

[alveloper]

Hello, will this PR request be done when a maintainer approves?

[siriwatknp]

@kshitij-p Thanks for kicking off the PR. However, I feels like there should be a better way for the autocomplete to take care of this issue without the need for developers to use the props.

For example, can we store the height of the list internally and then use it to compare with the new height (when more options are added), if the height is a lot more than some threshold then don't scroll. This is not 100% perfect but we don't need new props and it is easier to use.

cc @michaldudak any idea?

[michaldudak]

Hi, I also don't think adding new props is a good idea. The issue here is caused by logic that scrolls to the highlighted item whenever options change.

It seems to me that we could remove the check:

@@ -456,7 +460,7 @@ export default function useAutocomplete(props) {
     const valueItem = multiple ? value[0] : value;

     // The popup is empty, reset
-    if (filteredOptions.length === 0 || valueItem == null) {
+    if (filteredOptions.length === 0) {
       changeHighlightedIndex({ diff: 'reset' });
       return;
     }

valueItem == null does not indicate that a popup is empty and unnecessarily scrolls to the top even if the list has items. This should solve the issue when moving the listbox' scrollbar by mouse.

To solve the problem appearing when using keyboard, I'm considering not firing setHighlightedIndex logic when an index to set equals the already highlighted one:

const setHighlightedIndex = useEventCallback(({ event, index, reason = 'auto' }) => {
+    if (index === highlightedIndexRef.current) {
+      return;
+    }
+
     highlightedIndexRef.current = index;

It does, however, break some tests, so it would have to be explored further.

One more thing to note - in the example from the issue it would be better to load more results before the scroll is at the very bottom. This is because highlighting the last item does not move the scroll to the bottom (there is some margin between the last item and the bottom of listbox), so highlighting the last item would not trigger loading additional items.

[mnajdova]

@kshitij-p do you plan to continue the effort? If not, we can link @michaldudak's #30719 (comment) on the issue so that someone else could pick up and start working on it.

[mnajdova]

No response in more than three weeks, I am closing this PR and linking #30719 (comment) on the issue