From f572cdb2930aacc233cb70897e43ab1b331db961 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Thu, 9 Feb 2023 11:27:09 +0100 Subject: [PATCH 1/4] [Autocomplete] Fix tag removal regression --- .../AutocompleteUnstyled/useAutocomplete.js | 7 +- .../src/Autocomplete/Autocomplete.test.tsx | 7 +- .../src/Autocomplete/Autocomplete.test.js | 91 +++++++++++++++---- 3 files changed, 82 insertions(+), 23 deletions(-) diff --git a/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js b/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js index 235c201b880898..1e2fa8b2157ba7 100644 --- a/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js +++ b/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js @@ -470,14 +470,15 @@ export default function useAutocomplete(props) { previousProps.filteredOptions && previousProps.filteredOptions.length !== filteredOptions.length && (multiple - ? previousProps.value.every((val, i) => getOptionLabel(value[i]) === getOptionLabel(val)) - : getOptionLabel(previousProps.value ?? '') === getOptionLabel(value ?? '')) + ? value.length === previousProps.value.length && + previousProps.value.every((val, i) => isOptionEqualToValue(value[i], val)) + : isOptionEqualToValue(previousProps.value ?? '', value ?? '')) ) { const previousHighlightedOption = previousProps.filteredOptions[highlightedIndexRef.current]; if (previousHighlightedOption) { const previousHighlightedOptionExists = filteredOptions.some((option) => { - return getOptionLabel(option) === getOptionLabel(previousHighlightedOption); + return isOptionEqualToValue(option, previousHighlightedOption); }); if (previousHighlightedOptionExists) { diff --git a/packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx b/packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx index 71b8a084920995..5c4ef76665f615 100644 --- a/packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx +++ b/packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx @@ -1325,7 +1325,12 @@ describe('Joy ', () => { it('should keep focus on selected option when options updates and when options are provided as objects', () => { const { setProps } = render( - , + option.label === value.label} + />, ); const textbox = screen.getByRole('combobox'); const listbox = screen.getByRole('listbox'); diff --git a/packages/mui-material/src/Autocomplete/Autocomplete.test.js b/packages/mui-material/src/Autocomplete/Autocomplete.test.js index 64543292803ba1..6f8bc5b498933f 100644 --- a/packages/mui-material/src/Autocomplete/Autocomplete.test.js +++ b/packages/mui-material/src/Autocomplete/Autocomplete.test.js @@ -1667,6 +1667,7 @@ describe('', () => { open options={[{ label: 'one' }, { label: 'two' }]} renderInput={(params) => } + isOptionEqualToValue={(o, v) => o.label === v.label} />, ); const textbox = screen.getByRole('combobox'); @@ -2798,28 +2799,80 @@ describe('', () => { fireEvent.mouseDown(textbox); expect(screen.queryByRole('listbox')).to.equal(null); }); + }); - it('should not be able to delete the tag when multiple=true', () => { - const { container } = render( - } - />, - ); + it('should not be able to delete the tag when multiple=true', () => { + const { container } = render( + } + />, + ); - const chip = container.querySelector(`.${chipClasses.root}`); - expect(chip).not.to.have.class(chipClasses.deletable); + const chip = container.querySelector(`.${chipClasses.root}`); + expect(chip).not.to.have.class(chipClasses.deletable); - const textbox = screen.getByRole('combobox'); - act(() => { - textbox.focus(); - }); - expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(2); - fireEvent.keyDown(textbox, { key: 'Backspace' }); - expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(2); + const textbox = screen.getByRole('combobox'); + act(() => { + textbox.focus(); }); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(2); + fireEvent.keyDown(textbox, { key: 'Backspace' }); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(2); + }); + + // https://github.com/mui/material-ui/issues/36114 + it('should allow deleting a tag immediately after adding it while the listbox is still open', () => { + const { container } = render( + } + />, + ); + + const textbox = screen.getByRole('combobox'); + + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight the first option... + fireEvent.keyDown(textbox, { key: 'Enter' }); // ...and select it + + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight another option + + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(1); + + fireEvent.keyDown(textbox, { key: 'Backspace' }); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(0); + }); + + it('should allow deleting a tag immediately after adding it while the listbox is still open 2', () => { + const { container } = render( + opt.label === val.label} + renderInput={(params) => } + />, + ); + + const textbox = screen.getByRole('combobox'); + + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight the first option... + fireEvent.keyDown(textbox, { key: 'Enter' }); // ...and select it + + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight another option + + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(1); + + fireEvent.keyDown(textbox, { key: 'Backspace' }); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(0); }); }); From f0cbc5fd5a6ecb44748bd289d66c80c8aa1b1097 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Thu, 9 Feb 2023 11:30:49 +0100 Subject: [PATCH 2/4] [Autocomplete] Fix tag removal regression --- .../src/Autocomplete/Autocomplete.test.js | 78 ++++++++++--------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/packages/mui-material/src/Autocomplete/Autocomplete.test.js b/packages/mui-material/src/Autocomplete/Autocomplete.test.js index 6f8bc5b498933f..749c19a089e614 100644 --- a/packages/mui-material/src/Autocomplete/Autocomplete.test.js +++ b/packages/mui-material/src/Autocomplete/Autocomplete.test.js @@ -2825,54 +2825,56 @@ describe('', () => { }); // https://github.com/mui/material-ui/issues/36114 - it('should allow deleting a tag immediately after adding it while the listbox is still open', () => { - const { container } = render( - } - />, - ); + describe('deleting a tag immediately after adding it while the listbox is still open', () => { + it('should allow it, given that options are primitive values', () => { + const { container } = render( + } + />, + ); - const textbox = screen.getByRole('combobox'); + const textbox = screen.getByRole('combobox'); - fireEvent.keyDown(textbox, { key: 'ArrowDown' }); - fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight the first option... - fireEvent.keyDown(textbox, { key: 'Enter' }); // ...and select it + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight the first option... + fireEvent.keyDown(textbox, { key: 'Enter' }); // ...and select it - fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight another option + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight another option - expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(1); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(1); - fireEvent.keyDown(textbox, { key: 'Backspace' }); - expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(0); - }); + fireEvent.keyDown(textbox, { key: 'Backspace' }); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(0); + }); - it('should allow deleting a tag immediately after adding it while the listbox is still open 2', () => { - const { container } = render( - opt.label === val.label} - renderInput={(params) => } - />, - ); + it('should allow it, given that options are objects', () => { + const { container } = render( + opt.label === val.label} + renderInput={(params) => } + />, + ); - const textbox = screen.getByRole('combobox'); + const textbox = screen.getByRole('combobox'); - fireEvent.keyDown(textbox, { key: 'ArrowDown' }); - fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight the first option... - fireEvent.keyDown(textbox, { key: 'Enter' }); // ...and select it + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight the first option... + fireEvent.keyDown(textbox, { key: 'Enter' }); // ...and select it - fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight another option + fireEvent.keyDown(textbox, { key: 'ArrowDown' }); // highlight another option - expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(1); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(1); - fireEvent.keyDown(textbox, { key: 'Backspace' }); - expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(0); + fireEvent.keyDown(textbox, { key: 'Backspace' }); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(0); + }); }); }); From 26229ea30277a8b71b0e9a11095df15a503d8e0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Fri, 10 Feb 2023 10:48:20 +0100 Subject: [PATCH 3/4] Bring back comparison with getOptionLabel --- .../mui-base/src/AutocompleteUnstyled/useAutocomplete.js | 6 +++--- packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx | 7 +------ .../mui-material/src/Autocomplete/Autocomplete.test.js | 2 -- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js b/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js index 1e2fa8b2157ba7..c41218e0648d4b 100644 --- a/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js +++ b/packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.js @@ -471,14 +471,14 @@ export default function useAutocomplete(props) { previousProps.filteredOptions.length !== filteredOptions.length && (multiple ? value.length === previousProps.value.length && - previousProps.value.every((val, i) => isOptionEqualToValue(value[i], val)) - : isOptionEqualToValue(previousProps.value ?? '', value ?? '')) + previousProps.value.every((val, i) => getOptionLabel(value[i]) === getOptionLabel(val)) + : getOptionLabel(previousProps.value ?? '') === getOptionLabel(value ?? '')) ) { const previousHighlightedOption = previousProps.filteredOptions[highlightedIndexRef.current]; if (previousHighlightedOption) { const previousHighlightedOptionExists = filteredOptions.some((option) => { - return isOptionEqualToValue(option, previousHighlightedOption); + return getOptionLabel(option) === getOptionLabel(previousHighlightedOption); }); if (previousHighlightedOptionExists) { diff --git a/packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx b/packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx index 5c4ef76665f615..71b8a084920995 100644 --- a/packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx +++ b/packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx @@ -1325,12 +1325,7 @@ describe('Joy ', () => { it('should keep focus on selected option when options updates and when options are provided as objects', () => { const { setProps } = render( - option.label === value.label} - />, + , ); const textbox = screen.getByRole('combobox'); const listbox = screen.getByRole('listbox'); diff --git a/packages/mui-material/src/Autocomplete/Autocomplete.test.js b/packages/mui-material/src/Autocomplete/Autocomplete.test.js index 749c19a089e614..403a5f2bb12b9d 100644 --- a/packages/mui-material/src/Autocomplete/Autocomplete.test.js +++ b/packages/mui-material/src/Autocomplete/Autocomplete.test.js @@ -1667,7 +1667,6 @@ describe('', () => { open options={[{ label: 'one' }, { label: 'two' }]} renderInput={(params) => } - isOptionEqualToValue={(o, v) => o.label === v.label} />, ); const textbox = screen.getByRole('combobox'); @@ -2858,7 +2857,6 @@ describe('', () => { disableCloseOnSelect filterSelectedOptions options={[{ label: 'one' }, { label: 'two' }, { label: 'three' }]} - isOptionEqualToValue={(opt, val) => opt.label === val.label} renderInput={(params) => } />, ); From 35afa32054496b20d44724e824aea407f88698c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dudak?= Date: Fri, 10 Feb 2023 10:50:06 +0100 Subject: [PATCH 4/4] Restore test nesting --- .../src/Autocomplete/Autocomplete.test.js | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/packages/mui-material/src/Autocomplete/Autocomplete.test.js b/packages/mui-material/src/Autocomplete/Autocomplete.test.js index 403a5f2bb12b9d..8558f47410902c 100644 --- a/packages/mui-material/src/Autocomplete/Autocomplete.test.js +++ b/packages/mui-material/src/Autocomplete/Autocomplete.test.js @@ -2798,29 +2798,29 @@ describe('', () => { fireEvent.mouseDown(textbox); expect(screen.queryByRole('listbox')).to.equal(null); }); - }); - it('should not be able to delete the tag when multiple=true', () => { - const { container } = render( - } - />, - ); + it('should not be able to delete the tag when multiple=true', () => { + const { container } = render( + } + />, + ); - const chip = container.querySelector(`.${chipClasses.root}`); - expect(chip).not.to.have.class(chipClasses.deletable); + const chip = container.querySelector(`.${chipClasses.root}`); + expect(chip).not.to.have.class(chipClasses.deletable); - const textbox = screen.getByRole('combobox'); - act(() => { - textbox.focus(); + const textbox = screen.getByRole('combobox'); + act(() => { + textbox.focus(); + }); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(2); + fireEvent.keyDown(textbox, { key: 'Backspace' }); + expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(2); }); - expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(2); - fireEvent.keyDown(textbox, { key: 'Backspace' }); - expect(container.querySelectorAll(`.${chipClasses.root}`)).to.have.length(2); }); // https://github.com/mui/material-ui/issues/36114