[autocomplete] Fix highlight sync and scroll preservation#48322
[autocomplete] Fix highlight sync and scroll preservation#48322mj12albert wants to merge 1 commit intomui:masterfrom
Conversation
mj12albert
added
type: bug
Bundle size
Deploy previewhttps://deploy-preview-48322--material-ui.netlify.app/ Check out the code infra dashboard for more information about this PR. |
mj12albert
force-pushed
the
autocomplete/fix-highlight-sync-scroll-preservation-3of5
branch
from
4701647 to
c8d6c80
Compare
mj12albert
left a comment
mj12albert
left a comment
There was a problem hiding this comment.
The diff is very noisy due to indentation changes from extracting out functions, so I've commented on the key changes
| if (index === -1) { | ||
| if (!preserveScroll) { | ||
| listboxNode.scrollTop = 0; | ||
| } |
There was a problem hiding this comment.
Highlight 2
Enable resets to index === -1 to opt into preserveScroll
| // Check if the previously highlighted option still exists in the updated filtered options list and if the value and inputValue haven't changed | ||
| // If it exists and the value and the inputValue haven't changed, just update its index, otherwise continue execution | ||
| const previousHighlightedOptionIndex = getPreviousHighlightedOptionIndex(); | ||
| if (previousHighlightedOptionIndex !== -1) { | ||
| // Bypass setHighlightedIndex to preserve the existing highlightReasonRef. | ||
| // Keep the original highlight reason while re-syncing the DOM state. | ||
| // The highlighted option still exists after the filteredOptions array changed | ||
| // (e.g. async fetch returns new options while the user is mid-navigation), | ||
| // so the original interaction reason (keyboard, mouse, etc.) still applies. | ||
| highlightedIndexRef.current = previousHighlightedOptionIndex; | ||
| setHighlightedIndexFromSync({ index: previousHighlightedOptionIndex }); | ||
| return; |
There was a problem hiding this comment.
Highlight 3
Instead of directly changing highlightedIndexRef.current, call the new setHighlightedIndexFromSync() instead. This is the key fix for the stale visual highlight issue
| // Preserve scroll when new options are appended without changing the current filter. | ||
| const isAppendOnly = | ||
| filteredOptionsChanged && | ||
| previousProps.inputValue === inputValue && |
There was a problem hiding this comment.
The important part of the check is previousProps.inputValue === inputValue which differentiates normal filtering from append-only loading
| // Keep the current highlighted index if possible | ||
| if ( | ||
| multiple && | ||
| currentOption && | ||
| value.findIndex((val) => isOptionEqualToValue(currentOption, val)) !== -1 | ||
| ) { | ||
| return; | ||
| if (previousProps.filteredOptions?.length > 0) { | ||
| setHighlightedIndexFromSync({ index: highlightedIndexRef.current }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Highlight 4
“keep current highlighted selected option” now only applies when there were previous filtered options, so reopens don't preserve stale highlights
| const setHighlightedIndexFromSync = useEventCallback(({ index, preserveScroll = false }) => { | ||
| highlightedIndexRef.current = index; | ||
| syncHighlightedIndexToDOM({ | ||
| index, | ||
| reason: highlightReasonRef.current, | ||
| preserveScroll, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Highlight 1
setHighlightedIndex is split into
syncHighlightedIndexToDOMsetHighlightedIndexsetHighlightedIndexFromSync
setHighlightedIndexFromSync() is the important addition, it re-applies .Mui-focused and aria-activedescendant without changing highlightReasonRef and without firing onHighlightChange
mj12albert
requested review from
ZeeshanTamboli,
michelengelen,
silviuaavram and
siriwatknp
1 participant
Before: https://stackblitz.com/edit/oeui5u2z-2ocdxxvp?file=src%2FDemo.tsx
After: https://stackblitz.com/edit/oeui5u2z-r5vp8zge?file=src%2FDemo.tsx
Manual testing steps
1. Highlight mismatch when reopening (#48177)
Before: "two" is highlighted by step 3 and removed by Enter (the active highlight was stuck after step 1 and Esc failed to clear it)
After: "one" is highlighted by step 3 and removed by Enter
2. Adding options resets scroll position (#40250)
Before: when options are added the scroll position resets to the top
After: scroll position preserved when options are added
Without the scroll position fix, it's impossible to make a sane infinite loading UX; example Tanstack Query
useInfiniteQueryintegration with the fix: https://stackblitz.com/edit/rqmnxopa?file=src%2FDemo.tsxFixes #40250
Fixes #48177