[Autocomplete] Fix when onHighlightChange is called
#45438
Conversation
ZeeshanTamboli
added
bug 🐛
…d-when-listbox-opens
Netlify deploy previewhttps://deploy-preview-45438--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
|
Can I get a review on this? |
| * @param {string} reason Can be: `"keyboard"`, `"auto"`, `"mouse"`, `"touch"`. | ||
| * @param {string} reason Can be: `"keyboard"`, `"mouse"`, `"touch"`. |
There was a problem hiding this comment.
Why is "auto" being removed? Under which scenarios is "auto" the reason?
There was a problem hiding this comment.
The auto reason is the default value for setHighlightedIndex under which onHighlightChange is triggered. It was introduced in #20691 here and is used when no keyboard, mouse, or touch interaction occurs, like maybe in the cases noted in the bug report and point 2 of defaultValue in the PR description. With the updated logic, reason should now be limited to keyboard, mouse, or touch interactions.
Edit: I think I can remove default auto reason from code as well if it isn't used anywhere else as a reason.
Edit: Removed in 678b49c
| }); | ||
|
|
||
| describe('prop: onHighlightChange', () => { | ||
| it('should trigger event when default value is passed', () => { |
There was a problem hiding this comment.
Should we inverse this test? i.e. make it validate no event is triggered when the default value is passed.
| const { getByRole, setProps } = render( | ||
| <Autocomplete | ||
| onHighlightChange={(event, option) => { |
There was a problem hiding this comment.
Would it make sense to validate that onHighlightChange is not being called?
There was a problem hiding this comment.
Yes, we can. Done.
Fixes #43213
The
onHighlightChangeprop, introduced in #20691, did not correctly determine when it should be triggered.This PR:
onHighlightChangewas incorrectly triggered when the listbox opened.defaultValue: When adefaultValueis provided, the highlighted option remains the same when opening the listbox, soonHighlightChangeshould not be called. The existing test case for this behavior has been removed, as it does not align with the expected logic. Example: https://stackblitz.com/edit/stackblitz-starters-bocrcrmconHighlightChangewhen there is a mouse, keyboard or touch interaction over the options.Notice that the
eventparameter wasundefined, further indicating incorrect handling.I have also modified the test cases.
Before: https://stackblitz.com/edit/react-d4b5ls?file=Demo.tsx
After: https://codesandbox.io/p/sandbox/material-ui-cra-ts-forked-mpm2ft