Material-ui: [Autocomplete] autoHighlight not working properly for asynchronous requests

[mstykow]

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When typing a query into an asynchronous Autocomplete before the request finishes loading, Autocomplete with autoHighlight set to true does not highlight the first option. Only after pressing another key do first options get highlighted as they should. Everything then works as expected until the user clears the textfield and starts over.

Expected Behavior 🤔

The first option should be selected when the request finishes loading.

Steps to Reproduce 🕹

Steps:

1. Go to https://codesandbox.io/s/material-demo-ycmz7?file=/demo.tsx
2. Click on the textfield and quickly type a letter such as "a" _before_ the request finishes loading
3. You will see that "Portugal" is not selected even though autoHighlight is on
4. Type another letter and you'll see that first options are selected as they should
5. Delete (backspace) the input and type another letter and you see that everything works as it should
6. Leave the textfield by clicking away or hitting escape. When you start at step 2 you'll see the bug again.

Your Environment 🌎

I just upgraded from v4.0.0-alpha.50 where this bug did not yet exist. So something must have changed in the recent merges.

| Tech | Version |
| ----------- | ------- |
| Material-UI Lab | v4.0.0-alpha.53 |
| React | 16.13.1 |
| Browser | Chrome |
| TypeScript | 3.9.2 |

[marcosvega91]

Hi @mstykow waiting core members for a response I propose a solution

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index db79387ae..862209657 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -97,6 +97,7 @@ export default function useAutocomplete(props) {
     onClose,
     onHighlightChange,
     onInputChange,
+    loading = false,
     onOpen,
     open: openProp,
     openOnFocus = false,
@@ -402,13 +403,21 @@ export default function useAutocomplete(props) {
     }
   });

+  React.useEffect(() => {
+    if (!loading && filteredOptions.length > 0) {
+       changeHighlightedIndex('reset', 'next');
+    }
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [loading, changeHighlightedIndex]);
+

[mstykow]

@marcosvega91 Thank you very much for your quick proposal! I guess this could work. Two questions:
1) why do you nest your if blocks instead of using &&
2) this hook will fire every time loading changes (when changes in query text trigger new fetches) but the bug actually only occurs on the first load, that is, when user first enters the textfield. so we'd fire more often than necessary and this indicates to me that we haven't really found the source of the problem yet.

[marcosvega91]

Hey

1. You are totally right
2. I think that is makes sense to update the highlight because on a new fetch you probably are changing data so could be better to update it

[mstykow]

Nice. Maybe one more improvement simplifying your if statement:

if (!loading && filteredOptions.length) {

[oliviertassinari]

Is there a way that does not require leaking the loading state to the hook?

[marcosvega91]

Interesting question, mmm... we can use options in the useEffect. If options change and the length > 0 we can reset the highlight.
The problem here is that because options are passed from user we need to pay attention to avoid unexpected call to useEffect

[oliviertassinari]

It seems that ignoring the filterOptions dependency leads to this bug
https://github.com/mui-org/material-ui/blob/f63646ab4a169081783842b21b70fdb447a47243/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js#L448-L450

@marcosvega91 Yeah, I share the same concern, getOptionSelected and getOptionLabel are, most of the time, not memoized. Regarding the option, adding it as dependency could be less risky.

Maybe?

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index db79387ae..3688d704b 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -448,6 +448,7 @@ export default function useAutocomplete(props) {
     // Ignore filterOptions => options, getOptionSelected, getOptionLabel)
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [
+    filteredOptions.length,
     value,
     popupOpen,
     filterSelectedOptions,

[marcosvega91]

@oliviertassinari Dealing with arrays and objects in the dependences is always a problem. In this case can be worked.

[oliviertassinari]

@marcosvega91 Do you have a preference for the resolution? Do you want to take care of it? :)

[marcosvega91]

I leave the resolution to @mstykow that has discovered the bug, otherwise I can do it.
It's ok for your resolution but I think that in both cases a test case is needed 😄

[mstykow]

Sorry, this is my first issue here. What do you mean by "I leave the resolution to XYZ"?

I personally would prefer you stop ignoring the filteredOptions dependency on the existing useEffect hook if that gets rid of the bug. But didn't the person who decided to ignore the dependency have a good reason for it?

[marcosvega91]

@mstykow I mean that if you have the pleasure you can open a PR and fix the bug 💯

[mstykow]

Done: https://github.com/mui-org/material-ui/pull/21090. I did not add any tests because I did not see any other tests checking that dependency changes fire the effect hook. In fact, I was surprised to see that for a component as complex as this there were less than 10 tests. All of this leaves me feeling a little uneasy about the changes I just made. Who knows what side-effects it'll have no longer ignoring the filteredOptions! @oliviertassinari

[mstykow]

I was looking in the wrong place and now see some tests failing. Checking it out now.