[Autocomplete] Fix auto highlight when options change but not the length #46489
Conversation
Netlify deploy previewhttps://deploy-preview-46489--material-ui.netlify.app/ Bundle size report
|
[Switch] Add role="switch" (mui#46482) fix again update new tests
yafeng-c
force-pushed
the
autocomplete-45279
branch
from
cdd33f8 to
8744a12
Compare
merge master
| if ( | ||
| highlightedIndexRef.current !== -1 && | ||
| previousProps.filteredOptions && | ||
| previousProps.filteredOptions.length !== filteredOptions.length && |
There was a problem hiding this comment.
strengthen this condition by checking if elements are the same.
zannager
added
the
scope: autocomplete
ZeeshanTamboli
changed the title
merge master
ZeeshanTamboli
left a comment
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
@yafeng-c Thanks for the PR!
This also fixes #45279 — I’ve updated the PR description with before/after repros.
For #33634 (see https://stackblitz.com/edit/github-cfngmona-urwg9d2g), I need to press Arrow Down twice to reach the options.
Also, can we avoid the flash when the listbox updates but the highlight stays on the first option?
Nice catch. I reproduced this issue by first typing, for example 0, then pressing arrow down, it did not hight light the second option although the highlighted value was updated. This was caused by
If I understand the issue correctly, I reproduced it with https://stackblitz.com/edit/github-cfngmona-urwg9d2g. When updating from |
There was a problem hiding this comment.
This looks good to me. But I would like a final review from others in case I missed something. cc @DiegoAndai
| array1 && | ||
| array2 && | ||
| array1.length === array2.length && | ||
| array1.every((prevOption, index) => parser(prevOption) === parser(array2[index])) |
There was a problem hiding this comment.
Is it possible to invert the logic using array.some for better performance?
if array1 is large, says >300 it could introduce performance issue.
There was a problem hiding this comment.
Can I ask what's the specific solution you are suggesting?
if you mean !array1.some((prevOption, index) => parser(prevOption) !== parser(array2[index])), I don't think there will be improvement:
- if two array length are different then line 15 will not execute.
everywill return false and stop iterating immediately once a false returned in callback.- if the two arrays are the same,
somewill still iterate the whole array1.
DiegoAndai
requested review from
siriwatknp
and removed request for
DiegoAndai and
mj12albert
|
@siriwatknp Could you also review this? |
5 participants
Fixes #33634
Before: https://stackblitz.com/edit/github-cfngmona-qpp5b5ev
After: https://stackblitz.com/edit/github-cfngmona-urwg9d2g
Fixes #45279
Before: https://stackblitz.com/edit/github-cfngmona
After: https://stackblitz.com/edit/github-cfngmona-azw9frdq
I think it is due to the dependency filteredOptions.length of syncHighlightedIndex. Since the new options have the same length just elements order change, it will not sync highlight with the new options.
Tracing back this condition, looks like if we simply change it to
filteredOptions, it might reintroduce some other errors,(ref: #21280 and #19923). On the other hand, since options are indeed different, so this PR tries to make useEffect with syncHighlightedIndex listen to the change of options. With the deps of syncHighlightedIndex not changed, old issue should not be brought back.I have added some unit tests and this playground file could also be used to test locally.
After