Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[Autocomplete] onHighlightChange returns wrong option when options are set asychronously #22170
Comments
oliviertassinari
added
component: Autocomplete
status: incomplete
and removed
status: needs triage
status: incomplete
labels
|
After some investigation it seems to me that we should invoke the How could we solve this? First, we would need to change the dependency list of this callback, to contain the diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index a9d8d48de..f4439ca55 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -464,9 +464,7 @@ export default function useAutocomplete(props) {
// Ignore filteredOptions (and options, getOptionSelected, getOptionLabel) not to break the scroll position
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [
- // Only sync the highlighted index when the option switch between empty and not
- // eslint-disable-next-line react-hooks/exhaustive-deps
- filteredOptions.length === 0,
+ filteredOptions,
// Don't sync the highlighted index with the value when multiple
// eslint-disable-next-line react-hooks/exhaustive-deps
multiple ? false : value,With this change, the @oliviertassinari @herisn23 what do you think about this? |
|
@mnajdova The proposed fix also solves #22073, the issue is that However, the change breaks 2 tests in the regressions suite:
we can observe the change of behavior with: import React from "react";
import TextField from "@material-ui/core/TextField";
import Autocomplete from "@material-ui/lab/Autocomplete";
export default function BrokenAutocomplete() {
const [options, setOptions] = React.useState(['one', 'two']);
React.useEffect(() => {
setTimeout(() => {
setOptions(['one', 'two', 'three']);
}, 4000);
}, []);
return (
<Autocomplete
options={options}
renderInput={(params) => <TextField {...params} autoFocus />}
/>
);
}Arrow Down x2 before the timeout triggers. But we can probably break this test. #21090 was the first step toward relying more on the input of
|
|
For b. maybe we should have a |
|
|
|
@mnajdova Cool, I have added the good to take label. It's a quite a challenging one to get right, but at least we now have a clear direction a understanding of the problem. |
|
Hi there, Sorry if this might mess around the discussion so far, but I'd like to suggest a simpler solution for this. I believe we can fix this issue by replacing Because checking What do you think of this solution, @oliviertassinari @mnajdova? |
|
@TakumaKira diff --git a/packages/material-ui/src/Autocomplete/Autocomplete.test.js b/packages/material-ui/src/Autocomplete/Autocomplete.test.js
index 9a72f608e4..d9027bf14d 100644
--- a/packages/material-ui/src/Autocomplete/Autocomplete.test.js
+++ b/packages/material-ui/src/Autocomplete/Autocomplete.test.js
@@ -1269,7 +1269,7 @@ describe('<Autocomplete />', () => {
// three option is added and autocomplete re-renders, two should still be highlighted
setProps({ options: ['one', 'two', 'three'] });
- checkHighlightIs(listbox, 'two');
+ checkHighlightIs(listbox, null);
});
it('should not select undefined', () => {
diff --git a/packages/material-ui/src/useAutocomplete/useAutocomplete.js b/packages/material-ui/src/useAutocomplete/useAutocomplete.js
index 3fa4975012..d6ffc973d6 100644
--- a/packages/material-ui/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui/src/useAutocomplete/useAutocomplete.js
@@ -468,7 +468,7 @@ export default function useAutocomplete(props) {
}, [
// Only sync the highlighted index when the option switch between empty and not
// eslint-disable-next-line react-hooks/exhaustive-deps
- filteredOptions.length === 0,
+ filteredOptions.length,
// Don't sync the highlighted index with the value when multiple
// eslint-disable-next-line react-hooks/exhaustive-deps
multiple ? false : value, |
|
Also, we can keep #22170 (comment) in the back of our mind in case somebody come with a use case in the future. |
I'd love to! But I realized things around here are getting complicated, and my change will soon affect other fixes. So I want to choose what I am going to do or not carefully here.
- checkHighlightIs(listbox, 'two');
+ checkHighlightIs(listbox, null);I had missed that this test will fail with my proposal. But we don't need this change with the fix below. diff --git a/packages/material-ui/src/useAutocomplete/useAutocomplete.js b/packages/material-ui/src/useAutocomplete/useAutocomplete.js
index 52d559872..82f8c457d 100644
--- a/packages/material-ui/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui/src/useAutocomplete/useAutocomplete.js
@@ -422,7 +422,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;
}I believe resetting highlight when I also found |
@TakumaKira Definitely, that's an approximation taken in #22170 (comment) that is good enough for the provided reproduction of this issue (it solves it). I think that [{}, {}].toString() === '[object Object],[object Object]'
Did you look into the broken test cases when you do such?
What do you mean? This prop is coming from the developers.
We have a load of test cases. I think that they will tell you. |
Sorry, I thought always
I should have run the tests before suggesting removing diff --git a/packages/material-ui/src/Autocomplete/Autocomplete.test.js b/packages/material-ui/src/Autocomplete/Autocomplete.test.js
index f3e610268..8020f3e1c 100644
--- a/packages/material-ui/src/Autocomplete/Autocomplete.test.js
+++ b/packages/material-ui/src/Autocomplete/Autocomplete.test.js
@@ -2016,17 +2016,17 @@ describe('<Autocomplete />', () => {
);
const textbox = screen.getByRole('textbox');
+ fireEvent.keyDown(textbox, { key: 'ArrowDown' });
+ expect(handleHighlightChange.callCount).to.equal(2);
+ expect(handleHighlightChange.args[1][0]).to.not.equal(undefined);
+ expect(handleHighlightChange.args[1][1]).to.equal(options[0]);
+ expect(handleHighlightChange.args[1][2]).to.equal('keyboard');
+
fireEvent.keyDown(textbox, { key: 'ArrowDown' });
expect(handleHighlightChange.callCount).to.equal(3);
expect(handleHighlightChange.args[2][0]).to.not.equal(undefined);
- expect(handleHighlightChange.args[2][1]).to.equal(options[0]);
+ expect(handleHighlightChange.args[2][1]).to.equal(options[1]);
expect(handleHighlightChange.args[2][2]).to.equal('keyboard');
-
- fireEvent.keyDown(textbox, { key: 'ArrowDown' });
- expect(handleHighlightChange.callCount).to.equal(4);
- expect(handleHighlightChange.args[3][0]).to.not.equal(undefined);
- expect(handleHighlightChange.args[3][1]).to.equal(options[1]);
- expect(handleHighlightChange.args[3][2]).to.equal('keyboard');
});
it('should support mouse event', () => {
@@ -2042,10 +2042,10 @@ describe('<Autocomplete />', () => {
);
const firstOption = getAllByRole('option')[0];
fireEvent.mouseOver(firstOption);
- expect(handleHighlightChange.callCount).to.equal(3);
- expect(handleHighlightChange.args[2][0]).to.not.equal(undefined);
- expect(handleHighlightChange.args[2][1]).to.equal(options[0]);
- expect(handleHighlightChange.args[2][2]).to.equal('mouse');
+ expect(handleHighlightChange.callCount).to.equal(2);
+ expect(handleHighlightChange.args[1][0]).to.not.equal(undefined);
+ expect(handleHighlightChange.args[1][1]).to.equal(options[0]);
+ expect(handleHighlightChange.args[1][2]).to.equal('mouse');
});
it('should pass to onHighlightChange the correct value after filtering', () => {
diff --git a/packages/material-ui/src/useAutocomplete/useAutocomplete.js b/packages/material-ui/src/useAutocomplete/useAutocomplete.js
index 52d559872..4ccb17e97 100644
--- a/packages/material-ui/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui/src/useAutocomplete/useAutocomplete.js
@@ -422,7 +422,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;
}
@@ -455,6 +455,11 @@ export default function useAutocomplete(props) {
return;
}
+ if (highlightedIndexRef.current === -1) {
+ changeHighlightedIndex({ diff: 'reset' });
+ return;
+ }
+
// Prevent the highlighted index to leak outside the boundaries.
if (highlightedIndexRef.current >= filteredOptions.length - 1) {
setHighlightedIndex({ index: filteredOptions.length - 1 });
@@ -467,8 +472,7 @@ export default function useAutocomplete(props) {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [
// Only sync the highlighted index when the option switch between empty and not
- // eslint-disable-next-line react-hooks/exhaustive-deps
- filteredOptions.length === 0,
+ filteredOptions,
// Don't sync the highlighted index with the value when multiple
// eslint-disable-next-line react-hooks/exhaustive-deps
multiple ? false : value,I'm suggesting removing
Not checking the change of the option itself would miss changes if the length of the result stays the same. So if |
Current Behavior馃槸
Disabled filtering
filterOptions={x=>x}Use
autoHighlightthen change options asynchronously. CallbackonHighlightChangeprovide option from previous options after options are set instead of new one.Expected Behavior馃
Callback
onHighlightChangeshould provide auto highlighted option from latest provided options instead of previous options.Steps to Reproduce馃暪
1in textfield. Autocomplete display options filtered by input value and callbackonHighlightChangelog correct valueoption 1 12in textfield. Autocomplete display options filtered by input value and callbackonHighlightChangelog wrong valueoption 1 1instead ofoption 2 12Your Environment馃寧