Skip to content
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

Merged
merged 17 commits into from
Jan 31, 2023
69 changes: 51 additions & 18 deletions packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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') {
Expand Down Expand Up @@ -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))
Copy link

@waful waful Feb 9, 2023

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') at getOptionLabel(value[i]) when under these conditions:

  1. Autocomplete is using the multiple, disableCloseOnSelect, and filterSelectedOptions flags
  2. There is currently one selected option
  3. You delete the existing option while the dropdown is open

When you delete the option, value is now [] and so value[i] is undefined. When fed to getOptionLabel, 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

: 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);
Copy link
Member

Choose a reason for hiding this comment

The 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
return getOptionLabel(option) === getOptionLabel(previousHighlightedOption);
return option === previousHighlightedOption;

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Feb 6, 2023

Choose a reason for hiding this comment

The 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 false because they point to different object instances.

Copy link
Member

@oliviertassinari oliviertassinari Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would always reset

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?

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Feb 8, 2023

Choose a reason for hiding this comment

The 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 (options prop)? Then it would be a new reference?

Copy link
Member

@oliviertassinari oliviertassinari Feb 8, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Member

@michaldudak michaldudak Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we could use isOptionEqualToValue to determine if two options are the same instead of relying on labels. This should also fix #36114. I created a PR with this approach: #36116

@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
Expand Down
85 changes: 84 additions & 1 deletion packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1267,8 +1267,91 @@ describe('Joy <Autocomplete />', () => {

checkHighlightIs(listbox, 'two');

// three option is added and autocomplete re-renders, reset the highlight
// three option is added and autocomplete re-renders, restore the highlight
setProps({ options: ['one', 'two', 'three'] });
checkHighlightIs(listbox, 'two');
});

it('should keep focus when multiple options are selected and not reset to top option when options updated', () => {
const { setProps } = render(
<Autocomplete
open
multiple
defaultValue={['one', 'two']}
options={['one', 'two', 'three']}
autoFocus
/>,
);
const textbox = screen.getByRole('combobox');
const listbox = screen.getByRole('listbox');

fireEvent.keyDown(textbox, { key: 'ArrowDown' });
fireEvent.keyDown(textbox, { key: 'ArrowDown' });

checkHighlightIs(listbox, 'three');

// fourth option is added and autocomplete re-renders, restore the highlight
setProps({ options: ['one', 'two', 'three', 'four'] });
checkHighlightIs(listbox, 'three');
});

it('should keep focus when multiple options are selected by not resetting to the top option when options are updated and when options are provided as objects', () => {
const value = [{ label: 'one' }];
const options = [{ label: 'one' }, { label: 'two' }, { label: 'three' }];
const { setProps } = render(
<Autocomplete
multiple
options={options}
value={value}
isOptionEqualToValue={(option, val) => option.label === val.label}
autoFocus
open
/>,
);
const textbox = screen.getByRole('combobox');
const listbox = screen.getByRole('listbox');

fireEvent.keyDown(textbox, { key: 'ArrowDown' });
fireEvent.keyDown(textbox, { key: 'ArrowDown' });

checkHighlightIs(listbox, 'three');

// fourth option is added and autocomplete re-renders, restore the highlight
setProps({
options: [{ label: 'one' }, { label: 'two' }, { label: 'three' }, { label: 'four' }],
});
checkHighlightIs(listbox, 'three');
});

it('should keep focus on selected option when options updates and when options are provided as objects', () => {
const { setProps } = render(
<Autocomplete open options={[{ label: 'one' }, { label: 'two' }]} 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, restore the highlight
setProps({ options: [{ label: 'one' }, { label: 'two' }, { label: 'three' }] });
checkHighlightIs(listbox, 'two');
});

it("should reset the highlight when previously highlighted option doesn't exists in new options", () => {
const { setProps } = render(<Autocomplete open options={['one', 'two']} 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');

// Options are updated and autocomplete re-renders; reset the highlight since two doesn't exist in the new options.
setProps({ options: ['one', 'three', 'four'] });
checkHighlightIs(listbox, null);
});

Expand Down
93 changes: 92 additions & 1 deletion packages/mui-material/src/Autocomplete/Autocomplete.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1607,8 +1607,99 @@ describe('<Autocomplete />', () => {

checkHighlightIs(listbox, 'two');

// three option is added and autocomplete re-renders, reset the highlight
// three option is added and autocomplete re-renders, restore the highlight
setProps({ options: ['one', 'two', 'three'] });
checkHighlightIs(listbox, 'two');
});

it('should keep focus when multiple options are selected and not reset to top option when options updated', () => {
const { setProps } = render(
<Autocomplete
open
multiple
defaultValue={['one', 'two']}
options={['one', 'two', 'three']}
renderInput={(params) => <TextField {...params} autoFocus />}
/>,
);
const textbox = screen.getByRole('combobox');
const listbox = screen.getByRole('listbox');

fireEvent.keyDown(textbox, { key: 'ArrowDown' });
fireEvent.keyDown(textbox, { key: 'ArrowDown' });

checkHighlightIs(listbox, 'three');

// fourth option is added and autocomplete re-renders, restore the highlight
setProps({ options: ['one', 'two', 'three', 'four'] });
checkHighlightIs(listbox, 'three');
});

it('should keep focus when multiple options are selected by not resetting to the top option when options are updated and when options are provided as objects', () => {
const { setProps } = render(
<Autocomplete
open
multiple
defaultValue={[{ label: 'one' }]}
isOptionEqualToValue={(option, value) => option.label === value.label}
options={[{ label: 'one' }, { label: 'two' }, { label: 'three' }]}
renderInput={(params) => <TextField {...params} autoFocus />}
/>,
);
const textbox = screen.getByRole('combobox');
const listbox = screen.getByRole('listbox');

fireEvent.keyDown(textbox, { key: 'ArrowDown' });
fireEvent.keyDown(textbox, { key: 'ArrowDown' });

checkHighlightIs(listbox, 'three');

// fourth option is added and autocomplete re-renders, restore the highlight
setProps({
options: [{ label: 'one' }, { label: 'two' }, { label: 'three' }, { label: 'four' }],
});
checkHighlightIs(listbox, 'three');
});

it('should keep focus on selected option when options updates and when options are provided as objects', () => {
const { setProps } = render(
<Autocomplete
open
options={[{ label: 'one' }, { label: 'two' }]}
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, restore the highlight
setProps({ options: [{ label: 'one' }, { label: 'two' }, { label: 'three' }] });
checkHighlightIs(listbox, 'two');
});

it("should reset the highlight when previously highlighted option doesn't exists in new options", () => {
ZeeshanTamboli marked this conversation as resolved.
Show resolved Hide resolved
ZeeshanTamboli marked this conversation as resolved.
Show resolved Hide resolved
const { setProps } = render(
<Autocomplete
open
options={['one', 'two']}
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');

// Options are updated and autocomplete re-renders; reset the highlight since two doesn't exist in the new options.
setProps({ options: ['one', 'three', 'four'] });
checkHighlightIs(listbox, null);
});

Expand Down