From 22dea8009096774f3a125aa2be7db0b69be2604f Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Fri, 21 Jun 2024 08:43:35 -0500 Subject: [PATCH] fix(combobox): correct behavior of selection onChange with filtered items (#16835) * fix(combobox): onChange returns the proper item with shouldFilterItem * WIP * fix(combobox): use filterItems for items * fix(combobox): correct selection when filtered items --- .../src/components/ComboBox/ComboBox-test.js | 68 ++++++++++++++++++- .../components/ComboBox/ComboBox.stories.js | 8 ++- .../src/components/ComboBox/ComboBox.tsx | 8 ++- .../src/components/ListBox/test-helpers.js | 49 ++++++++++++- 4 files changed, 127 insertions(+), 6 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox-test.js b/packages/react/src/components/ComboBox/ComboBox-test.js index f2bc3c3ccfeb..c2d07c34703e 100644 --- a/packages/react/src/components/ComboBox/ComboBox-test.js +++ b/packages/react/src/components/ComboBox/ComboBox-test.js @@ -14,6 +14,7 @@ import { assertMenuClosed, generateItems, generateGenericItem, + cognateItems, waitForPosition, } from '../ListBox/test-helpers'; import ComboBox from '../ComboBox'; @@ -63,7 +64,71 @@ describe('ComboBox', () => { } }); - it('capture filter text events', async () => { + it('should call `onChange` with the proper item when `shouldFilterItem` is provided', async () => { + const filterItems = (menu) => { + return menu?.item?.label + ?.toLowerCase() + .includes(menu?.inputValue?.toLowerCase()); + }; + const onInputChange = jest.fn(); + + render( + + ); + + await userEvent.type(findInputNode(), 'Item 2'); + expect(onInputChange).toHaveBeenCalledWith('Item 2'); + + await userEvent.click(screen.getAllByRole('option')[0]); + expect(mockProps.onChange).toHaveBeenCalledTimes(1); + expect(mockProps.onChange).toHaveBeenCalledWith({ + selectedItem: mockProps.items[2], + }); + }); + + it('should select the correct item from the filtered list after text input on click', async () => { + const user = userEvent.setup(); + + render(); + + await user.type(findInputNode(), 'struct'); + + await user.click(screen.getAllByRole('option')[1]); + + expect(mockProps.onChange).toHaveBeenCalledTimes(1); + expect(mockProps.onChange).toHaveBeenCalledWith({ + selectedItem: { + id: 'construct', + text: 'Construct', + }, + }); + }); + + it('should select the correct item from the filtered list after text input on [Enter]', async () => { + const user = userEvent.setup(); + + render(); + + await user.type(findInputNode(), 'struct'); + + await userEvent.keyboard('{ArrowDown}'); + await userEvent.keyboard('{ArrowDown}'); + await userEvent.keyboard('[Enter]'); + + expect(mockProps.onChange).toHaveBeenCalledTimes(1); + expect(mockProps.onChange).toHaveBeenCalledWith({ + selectedItem: { + id: 'construct', + text: 'Construct', + }, + }); + }); + + it('capture filter text event onInputChange', async () => { const onInputChange = jest.fn(); render(); @@ -103,6 +168,7 @@ describe('ComboBox', () => { expect(mockProps.onChange).toHaveBeenCalledWith({ selectedItem: mockProps.items[1], }); + expect(screen.getByRole('combobox')).toHaveDisplayValue('Item 1'); }); it('should not let the user select an option by clicking on the disabled option node', async () => { diff --git a/packages/react/src/components/ComboBox/ComboBox.stories.js b/packages/react/src/components/ComboBox/ComboBox.stories.js index d43b231671a0..916f7777baae 100644 --- a/packages/react/src/components/ComboBox/ComboBox.stories.js +++ b/packages/react/src/components/ComboBox/ComboBox.stories.js @@ -74,7 +74,7 @@ export const Default = () => ( ); -export const AllowCustomValue = () => { +export const AllowCustomValue = (args) => { const filterItems = (menu) => { return menu?.item?.toLowerCase().includes(menu?.inputValue?.toLowerCase()); }; @@ -83,7 +83,7 @@ export const AllowCustomValue = () => { {}} + onChange={args.onChange} id="carbon-combobox" items={['Apple', 'Orange', 'Banana', 'Pineapple', 'Raspberry', 'Lime']} titleText="ComboBox title" @@ -108,6 +108,10 @@ export const ExperimentalAutoAlign = () => ( ); +AllowCustomValue.argTypes = { + onChange: { action: 'onChange' }, +}; + export const _WithLayer = () => ( {(layer) => ( diff --git a/packages/react/src/components/ComboBox/ComboBox.tsx b/packages/react/src/components/ComboBox/ComboBox.tsx index 089435a974f6..3fa05acf5ae3 100644 --- a/packages/react/src/components/ComboBox/ComboBox.tsx +++ b/packages/react/src/components/ComboBox/ComboBox.tsx @@ -590,7 +590,7 @@ const ComboBox = forwardRef( setHighlightedIndex, } = useCombobox({ ...downshiftProps, - items, + items: filterItems(items, itemToString, inputValue), inputValue: inputValue, itemToString: (item) => { return itemToString(item); @@ -708,7 +708,11 @@ const ComboBox = forwardRef( toggleMenu(); if (highlightedIndex !== -1) { - selectItem(items[highlightedIndex]); + selectItem( + filterItems(items, itemToString, inputValue)[ + highlightedIndex + ] + ); } event.preventDownshiftDefault = true; diff --git a/packages/react/src/components/ListBox/test-helpers.js b/packages/react/src/components/ListBox/test-helpers.js index f9958e7f3e32..e0c55700399e 100644 --- a/packages/react/src/components/ListBox/test-helpers.js +++ b/packages/react/src/components/ListBox/test-helpers.js @@ -107,4 +107,51 @@ export const generateItems = (amount, generator) => export const customItemToString = ({ field }) => field; -export const waitForPosition = () => act(async () => {}); // Flush microtasks. Position state is ready by this line. +/** + * This object contains two sets of three items that share the same root + * word in different portions of the string (beginning, middle, end): + * + * - 'struct' + * - 'port' + * + * Separated by a disabled item, these derivative words are helpful when + * testing fuzzy search functions and components that do substring filtering. + */ +export const cognateItems = [ + { + id: 'structure', + text: 'Structure', + }, + { + id: 'construct', + text: 'Construct', + }, + { + id: 'instruction', + text: 'Instruction', + }, + { + id: 'disabled-item', + text: 'A disabled item', + disabled: true, + }, + { + id: 'transport', + text: 'Transport', + }, + { + id: 'portable', + text: 'Portable', + }, + { + id: 'import', + text: 'Import', + }, +]; + +/** + * Flushes microtasks to ensure element position state is settled + * From https://floating-ui.com/docs/react#testing + * More context here: https://github.com/floating-ui/react-popper/issues/368#issuecomment-1340413010 + */ +export const waitForPosition = () => act(async () => {});