-
-
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 reset scroll position when new options are added #35735
Changes from all commits
506224b
28975c8
2733b10
7ba6ecc
80221d6
2e2a772
f3bf7d3
429c2c1
3c36115
54cb9c3
c95c288
14ccfd9
33c970d
47233c9
fb77c56
c75f72e
e6f5294
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,6 +5,7 @@ import { | |||||
unstable_useEventCallback as useEventCallback, | ||||||
unstable_useControlled as useControlled, | ||||||
unstable_useId as useId, | ||||||
usePreviousProps, | ||||||
} from '@mui/utils'; | ||||||
|
||||||
// https://stackoverflow.com/questions/990904/remove-accents-diacritics-in-a-string-in-javascript | ||||||
|
@@ -193,24 +194,6 @@ export default function useAutocomplete(props) { | |||||
[getOptionLabel, inputValue, multiple, onInputChange, setInputValueState, clearOnBlur, value], | ||||||
); | ||||||
|
||||||
const prevValue = React.useRef(); | ||||||
|
||||||
React.useEffect(() => { | ||||||
const valueChange = value !== prevValue.current; | ||||||
prevValue.current = value; | ||||||
|
||||||
if (focused && !valueChange) { | ||||||
return; | ||||||
} | ||||||
|
||||||
// Only reset the input's value when freeSolo if the component's value changes. | ||||||
if (freeSolo && !valueChange) { | ||||||
return; | ||||||
} | ||||||
|
||||||
resetInputValue(null, value); | ||||||
}, [value, resetInputValue, focused, prevValue, freeSolo]); | ||||||
|
||||||
const [open, setOpenState] = useControlled({ | ||||||
controlled: openProp, | ||||||
default: false, | ||||||
|
@@ -247,6 +230,26 @@ export default function useAutocomplete(props) { | |||||
) | ||||||
: []; | ||||||
|
||||||
const previousProps = usePreviousProps({ | ||||||
filteredOptions, | ||||||
value, | ||||||
}); | ||||||
|
||||||
React.useEffect(() => { | ||||||
const valueChange = value !== previousProps.value; | ||||||
|
||||||
if (focused && !valueChange) { | ||||||
return; | ||||||
} | ||||||
|
||||||
// Only reset the input's value when freeSolo if the component's value changes. | ||||||
if (freeSolo && !valueChange) { | ||||||
return; | ||||||
} | ||||||
|
||||||
resetInputValue(null, value); | ||||||
}, [value, resetInputValue, focused, previousProps.value, freeSolo]); | ||||||
|
||||||
const listboxAvailable = open && filteredOptions.length > 0 && !readOnly; | ||||||
|
||||||
if (process.env.NODE_ENV !== 'production') { | ||||||
|
@@ -461,11 +464,41 @@ export default function useAutocomplete(props) { | |||||
}, | ||||||
); | ||||||
|
||||||
const checkHighlightedOptionExists = () => { | ||||||
if ( | ||||||
highlightedIndexRef.current !== -1 && | ||||||
previousProps.filteredOptions && | ||||||
previousProps.filteredOptions.length !== filteredOptions.length && | ||||||
(multiple | ||||||
? previousProps.value.every((val, i) => getOptionLabel(value[i]) === getOptionLabel(val)) | ||||||
: getOptionLabel(previousProps.value ?? '') === getOptionLabel(value ?? '')) | ||||||
) { | ||||||
sai6855 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
const previousHighlightedOption = previousProps.filteredOptions[highlightedIndexRef.current]; | ||||||
|
||||||
if (previousHighlightedOption) { | ||||||
const previousHighlightedOptionExists = filteredOptions.some((option) => { | ||||||
return getOptionLabel(option) === getOptionLabel(previousHighlightedOption); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure about this comparison? I would expect the scroll to reset if no options are equal to be enough.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But in the case when options are objects, it would always reset, with the strict equality always returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why? I would expect developers to fetch new options, append them, and keep the same reference to the existing options. At least, the bug reproduction we have seem to be compatible with this idea. If developers re-fetch all the options which leads to creating new references, then maybe they would also expect a scroll reset? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you're saying that, consider in the test, this is incorrect because developers re-fetch all the options (including a new one), so we should expect to reset? But shouldn't React developers treat objects/arrays as immutable when storing in state ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, in this test, these look like entirely new options. I think that a scroll reset can make sense. #26492 makes the case that developers expect support for duplicated option labels. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, we could use @oliviertassinari resetting the highlight when a reference to an option changes but its value/label does not could lead to a worse DX, for example when a new set of options is received from a server, and it includes the currently highlighted one. I would expect the highlight to remain where it was. |
||||||
}); | ||||||
|
||||||
if (previousHighlightedOptionExists) { | ||||||
return true; | ||||||
} | ||||||
} | ||||||
} | ||||||
return false; | ||||||
}; | ||||||
|
||||||
const syncHighlightedIndex = React.useCallback(() => { | ||||||
if (!popupOpen) { | ||||||
return; | ||||||
} | ||||||
|
||||||
// Check if the previously highlighted option still exists in the updated filtered options list and if the value hasn't changed | ||||||
// If it exists and the value hasn't changed, return, otherwise continue execution | ||||||
if (checkHighlightedOptionExists()) { | ||||||
return; | ||||||
} | ||||||
|
||||||
const valueItem = multiple ? value[0] : value; | ||||||
|
||||||
// The popup is empty, reset | ||||||
|
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.
This PR introduces a runtime error
TypeError: Cannot read properties of undefined (reading 'label')
atgetOptionLabel(value[i])
when under these conditions:multiple
,disableCloseOnSelect
, andfilterSelectedOptions
flagsWhen you delete the option,
value
is now[]
and sovalue[i]
is undefined. When fed togetOptionLabel
, it crashes because it expects option to not be undefined.Line 100:
getOptionLabel: getOptionLabelProp = (option) => option.label ?? option
EDIT: issue filed with more details and replication sandbox: #36114