-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Autocomplete] prevent highlight index from resetting when options are added #33645
Conversation
…when options are added
@@ -61,6 +61,14 @@ function findIndex(array, comp) { | |||
return -1; | |||
} | |||
|
|||
function usePrevious(value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't 100% on what to do with this.
This exact function exists in TreeView, but isn't exported.
We also have usePreviousProps in the utils package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the usePreviousProps
from the utils package.
// three option is added and autocomplete re-renders, do not reset the highlight | ||
setProps({ options: ['one', 'two', 'three'] }); | ||
checkHighlightIs(listbox, null); | ||
checkHighlightIs(listbox, 'two'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me if this is not a regression. Some might expect that new options = a reset of the highlight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting that we allow the highlight index to reset, but prevent scroll to top?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliviertassinari seeing the number of issues reported about this, the community seems to have a different view on this. It may be confusing to have highlight gone when data is loaded on demand.
@cjtroiano I'm wondering about another scenario that your PR doesn't cover - what if the number of items decreased, but the highlighted item is still among them. IMO we should keep the highlight there as well:
it('should keep focus on selected option and not reset to top option when options updated', () => {
const { setProps } = render(
<Autocomplete
open
options={['one', 'two', 'three']}
renderInput={(params) => <TextField {...params} autoFocus />}
/>,
);
const textbox = screen.getByRole('combobox');
const listbox = screen.getByRole('listbox');
fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // goes to 'one'
fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // goes to 'two'
checkHighlightIs(listbox, 'two');
// three option is added and autocomplete re-renders, do not reset the highlight
setProps({ options: ['two', 'three'] });
checkHighlightIs(listbox, 'two');
});
Any movement on this? I'd be happy to try to help! Maybe instead of always resetting or never resetting, it could reset only if the previously highlighted option no longer exists. If it still exists, it can remain highlighted. If it no longer exists, then we reset. |
I would also be very happy if this got prioritized, let me know if I can help somehow! |
@justintoman, @jimmycallin, feel free to give it a stab (just decide between the two of you who'll do it).
I think this should be the way to go. Let's implement this and see how it behaves live. |
Since there is no activity in this PR, I am closing it in favour of #35735. |
fix #29508