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] Fix tag removal regression #36116

Merged
merged 4 commits into from
Feb 13, 2023

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Feb 9, 2023

This PR fixes the regression introduced in #35735 and described in #36114.
It makes sure the options that are compared exist.

Before: https://codesandbox.io/s/recursing-snyder-ybg6m5?file=/demo.tsx
After: https://codesandbox.io/s/hardcore-einstein-yx1xwr?file=/demo.tsx
(see the linked issue for repro steps)

Fixes #36114.

@michaldudak michaldudak added bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! regression A bug, but worse labels Feb 9, 2023
@mui-bot
Copy link

mui-bot commented Feb 9, 2023

Netlify deploy preview

No updates.

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 35afa32

) {
const previousHighlightedOption = previousProps.filteredOptions[highlightedIndexRef.current];

if (previousHighlightedOption) {
const previousHighlightedOptionExists = filteredOptions.some((option) => {
return getOptionLabel(option) === getOptionLabel(previousHighlightedOption);
return isOptionEqualToValue(option, previousHighlightedOption);
Copy link
Member

Choose a reason for hiding this comment

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

The second argument was meant to be called with a value, not an option. It's related to #23708 and #31192. Unclear what is right here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, ok. We could use something similar to optionComparer from Base's useListbox:

optionComparer = defaultOptionComparer,

But then introducing yet another prop looks like an overkill. I'll leave just the bare minimum to fix the issue here, and we can discuss the approach for changing getOptionLabel elsewhere.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Tests looks clean 👍

@michaldudak michaldudak merged commit b1d2e8d into mui:master Feb 13, 2023
@michaldudak michaldudak deleted the iss/36114-autocomplete-crash branch February 13, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] 5.11.7 Introduced a Crashing Bug For This Configuration of Autocomplete
4 participants