[Autocomplete] Invalid highlighted index when selecting last item with filterSelectedOptions

[jonatanklosko]

Current Behavior 😯

When filterSelectedOptions is enabled, selecting the last item means it disappears from the list, yet the highlight is not updated.

Expected Behavior 🤔

Not entirely sure, I can imagine one of the following:

• clear highlight (i.e. set highlighted index to -1)
• highlight the previous item (edge case: when the list has only 1 element, then the index should be set to -1)

Steps to Reproduce 🕹

Steps:

1. Open this demo (taken directly from the documentation).
2. Select the last item and hit enter.
3. Hit arrow down (to open the popup) and then hit enter again (the highlighted index is still the old end-of-list and thus the selected value is undefined).

Tech	Version
Material-UI	v4.9.2

Notes

One simple solution is to reset the highlighted index when a new value is selected and filterSelectedOptions is used. Adding the following code

if (filterSelectedOptions) {
  setHighlightedIndex(-1);
}

below this: https://github.com/mui-org/material-ui/blob/f567b384f509d36b7e10f691e6b58538e7db8869/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js#L453-L456
does the job, although as mentioned above, it's not necessarily the expected behavior.

[oliviertassinari]

The best fix I could find so far is

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 19cea91d7..e3ee398af 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -464,6 +464,10 @@ export default function useAutocomplete(props) {
       }
     }

+    if (filterSelectedOptions && highlightedIndexRef.current === filteredOptions.length - 1) {
+      setHighlightedIndex(highlightedIndexRef.current - 1);
+    }
+
     resetInputValue(event, newValue);

     handleValue(event, newValue);

However, it's related to #19779 and #19705. I suspect we can use a single solution for these 3.

(I'm not sure that a reset of the index to -1 after selection is an option. It seems quite disruptive for keyboard users with filterSelectedOptions and disableCloseOnSelect).

[jonatanklosko]

Looks like a good solution to me!

Not sure how this relates to #19779, as it has to do with initial scroll position, not highlight. For #19705 I'd say it makes sense to introduce resetHighlightOnSelect that jumps the highlight index to 0 (unless it's empty, otherwise -1), as both behaviors may be useful in some cases.

[oliviertassinari]

Here is a proposed solution for #19637, #19779, and #19637. Let us know if it works for your use case!

diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.test.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.test.js
index bd9dc472f..c71b1b88f 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.test.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.test.js
@@ -757,7 +757,6 @@ describe('<Autocomplete />', () => {
         />,
       );

-      fireEvent.keyDown(document.activeElement, { key: 'ArrowDown' });
       fireEvent.keyDown(document.activeElement, { key: 'ArrowDown' });
       fireEvent.keyDown(document.activeElement, { key: 'Enter' });

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 9957ed76d..2200b20c1 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -396,6 +396,32 @@ export default function useAutocomplete(props) {
     changeHighlightedIndex('reset', 'next');
   }, [inputValue]); // eslint-disable-line react-hooks/exhaustive-deps

+  React.useEffect(() => {
+    if (!open) {
+      return;
+    }
+
+    const valueItem = multiple ? value[0] : value;
+
+    if (filteredOptions.length === 0 || valueItem == null) {
+      changeHighlightedIndex('reset', 'next');
+      return;
+    }
+
+    if (!filterSelectedOptions && valueItem != null) {
+      const itemIndex = findIndex(filteredOptions, optionItem =>
+        getOptionSelected(optionItem, valueItem),
+      );
+
+      setHighlightedIndex(itemIndex);
+      return;
+    }
+
+    if (highlightedIndexRef.current >= filteredOptions.length - 1) {
+      setHighlightedIndex(filteredOptions.length - 1);
+    }
+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [value, open, filterSelectedOptions, multiple]);
+
   const handleOpen = event => {
     if (open) {
       return;

We will probably need to add one test case per issue for this logic (corresponding to each issue).

[jonatanklosko]

Definitely works for me! The following test passes after applying the fix:

describe('prop: filterSelectedOptions', () => {
  it('when the last item is selected, highlights the new last item', () => {
    const { getByRole } = render(
      <Autocomplete
        {...defaultProps}
        filterSelectedOptions
        options={['one', 'two', 'three']}
        renderInput={params => <TextField {...params} autoFocus />}
      />,
    );

    function checkHighlightIs(expected) {
      expect(getByRole('listbox').querySelector('li[data-focus]')).to.have.text(expected);
    }

    fireEvent.keyDown(document.activeElement, { key: 'ArrowUp' });
    checkHighlightIs('three');
    fireEvent.keyDown(document.activeElement, { key: 'Enter' }); // selects the last option
    const input = getByRole('textbox');
    input.blur();
    input.focus(); // opens the listbox again
    checkHighlightIs('two');
  });
});

[captain-yossarian]

@jonatanklosko I want to double check my PR. Could you please checkout on my branch and check if code is working as expected?
Thanks in advance

[jonatanklosko]

Works as expected, thanks!