fix: refactor and fix controlled combobox (#17527)

* Refactor code related to controlled combobox for simplicity.
* Update ComboBox when selectedItem prop changes externally.
* Call onChange when combobox is cleared or updated.
* Unit tests for these cases.
* Storybook example for fully controlled component.

.all-contributorsrc

@@ -1625,6 +1625,16 @@
         "doc"
       ]
     },
+    {
+      "login": "Neues",
+      "name": "Noah Sgorbati",
+      "avatar_url": "https://avatars.githubusercontent.com/u/9409245?v=4",
+      "profile": "https://github.com/Neues",
+      "contributions": [
+        "code",
+        "doc"
+      ]
+    },
     {
       "login": "divya-281",
       "name": "Divya G",

README.md

@@ -306,6 +306,7 @@ check out our [Contributing Guide](/.github/CONTRIBUTING.md) and our
   <tr>
     <td align="center"><a href="https://github.com/NabeelAyubee"><img src="https://avatars.githubusercontent.com/u/64087875?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Md Nabeel Ayubee</b></sub></a><br /><a href="https://github.com/carbon-design-system/carbon/commits?author=NabeelAyubee" title="Code">💻</a></td>
     <td align="center"><a href="https://github.com/pamrulla"><img src="https://avatars.githubusercontent.com/u/4942741?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Patan Amrulla Khan</b></sub></a><br /><a href="https://github.com/carbon-design-system/carbon/commits?author=pamrulla" title="Code">💻</a> <a href="https://github.com/carbon-design-system/carbon/commits?author=pamrulla" title="Documentation">📖</a></td>
+    <td align="center"><a href="https://github.com/Neues"><img src="https://avatars.githubusercontent.com/u/9409245?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Noah Sgorbati</b></sub></a><br /><a href="https://github.com/carbon-design-system/carbon/commits?author=Neues" title="Code">💻</a> <a href="https://github.com/carbon-design-system/carbon/commits?author=Neues" title="Documentation">📖</a></td>
     <td align="center"><a href="https://github.com/divya-281"><img src="https://avatars.githubusercontent.com/u/72991264?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Divya G</b></sub></a><br /><a href="https://github.com/carbon-design-system/carbon/commits?author=divya-281" title="Code">💻</a></td>
   </tr>
 </table>

packages/react/src/components/ComboBox/ComboBox-test.js

@@ -64,6 +64,18 @@ describe('ComboBox', () => {
     }
   });
 
+  it('should call `onChange` when selection is cleared', async () => {
+    render(<ComboBox {...mockProps} />);
+    expect(mockProps.onChange).not.toHaveBeenCalled();
+    await openMenu();
+    await userEvent.click(screen.getAllByRole('option')[0]);
+    expect(mockProps.onChange).toHaveBeenCalledTimes(1);
+    await userEvent.click(
+      screen.getByRole('button', { name: 'Clear selected item' })
+    );
+    expect(mockProps.onChange).toHaveBeenCalledTimes(2);
+  });
+
   it('should call `onChange` with the proper item when `shouldFilterItem` is provided', async () => {
     const filterItems = (menu) => {
       return menu?.item?.label
@@ -90,12 +102,6 @@ describe('ComboBox', () => {
     });
   });
 
-  it('should call `onChange` on a fully controlled component', async () => {
-    render(<ComboBox {...mockProps} selectedItem={mockProps.items[0]} />);
-    await userEvent.click(screen.getAllByRole('button')[0]);
-    expect(mockProps.onChange).toHaveBeenCalled();
-  });
-
   it('should select the correct item from the filtered list after text input on click', async () => {
     const user = userEvent.setup();
 
@@ -270,14 +276,14 @@ describe('ComboBox', () => {
     });
   });
 
-  describe('should display selected item found in `selectedItem`', () => {
-    it('using an object type for the `selectedItem` prop', async () => {
+  describe('provided `selectedItem`', () => {
+    it('should display selected item using an object type for the `selectedItem` prop', async () => {
       render(<ComboBox {...mockProps} selectedItem={mockProps.items[0]} />);
       await waitForPosition();
       expect(findInputNode()).toHaveDisplayValue(mockProps.items[0].label);
     });
 
-    it('using a string type for the `selectedItem` prop', async () => {
+    it('should display selected item using a string type for the `selectedItem` prop', async () => {
       // Replace the 'items' property in mockProps with a list of strings
       mockProps = {
         ...mockProps,
@@ -288,6 +294,45 @@ describe('ComboBox', () => {
       await waitForPosition();
       expect(findInputNode()).toHaveDisplayValue(mockProps.items[1]);
     });
+    it('should update and call `onChange` when selection is updated from the combobox', async () => {
+      render(<ComboBox {...mockProps} selectedItem={mockProps.items[0]} />);
+      expect(mockProps.onChange).not.toHaveBeenCalled();
+      await openMenu();
+      await userEvent.click(screen.getByRole('option', { name: 'Item 2' }));
+      expect(mockProps.onChange).toHaveBeenCalledTimes(1);
+      expect(
+        screen.getByRole('combobox', { value: 'Item 2' })
+      ).toBeInTheDocument();
+    });
+    it('should update and call `onChange` when selection is updated externally', async () => {
+      const { rerender } = render(
+        <ComboBox {...mockProps} selectedItem={mockProps.items[0]} />
+      );
+      expect(findInputNode()).toHaveDisplayValue(mockProps.items[0].label);
+      rerender(<ComboBox {...mockProps} selectedItem={mockProps.items[1]} />);
+      expect(findInputNode()).toHaveDisplayValue(mockProps.items[1].label);
+      expect(mockProps.onChange).toHaveBeenCalledTimes(1);
+    });
+    it('should clear selected item and call `onChange` when selection is cleared from the combobox', async () => {
+      render(<ComboBox {...mockProps} selectedItem={mockProps.items[1]} />);
+      expect(mockProps.onChange).not.toHaveBeenCalled();
+      await userEvent.click(
+        screen.getByRole('button', { name: 'Clear selected item' })
+      );
+      expect(mockProps.onChange).toHaveBeenCalled();
+      expect(findInputNode()).toHaveDisplayValue('');
+    });
+    it('should clear selected item when `selectedItem` is updated to `null` externally', async () => {
+      const { rerender } = render(
+        <ComboBox {...mockProps} selectedItem={mockProps.items[1]} />
+      );
+      await waitForPosition();
+      expect(findInputNode()).toHaveDisplayValue(mockProps.items[1].label);
+      rerender(<ComboBox {...mockProps} selectedItem={null} />);
+      await waitForPosition();
+      expect(findInputNode()).toHaveDisplayValue('');
+      expect(mockProps.onChange).toHaveBeenCalled();
+    });
   });
 
   describe('when disabled', () => {

packages/react/src/components/ComboBox/ComboBox.mdx

@@ -19,6 +19,7 @@ import ComboBox from '../ComboBox';
 - [downshiftProps](#combobox-downshiftprops)
 - [Placeholder and labeling](#placeholders-and-labeling)
 - [initialSelectedItem](#initialselecteditem)
+- [selectedItem](#selecteditem)
 - [itemToElement](#itemtoelement)
 - [itemToString](#itemtostring)
 - [shouldFilterItem](#shouldfilteritem)
@@ -126,6 +127,60 @@ const items = ['Option 1', 'Option 2', 'Option 3']
 <Combobox initialSelectedItem={items[2]} />
 ```
 
+## `selectedItem`
+
+You can use `selectedItem` for a fully controlled component.
+
+<Canvas of={ComboBoxStories._fullyControlled} />
+
+```jsx
+
+const options = [
+  {
+    id: 'option-1',
+    text: 'Option 1',
+  },
+  {
+    id: 'option-2',
+    text: 'Option 2',
+  },
+  {
+    id: 'option-3',
+    text: 'Option 3',
+  },
+];
+const [value, setValue] = useState(options[0]);
+
+const onChange = ({ selectedItem }) => {
+  setValue(selectedItem);
+};
+
+return (
+  <div>
+    <ComboBox
+      onChange={onChange}
+      id="carbon-combobox"
+      items={options}
+      selectedItem={value}
+      itemToString={(item) => (item ? item.text : '')}
+      titleText="Fully Controlled ComboBox title"
+      helperText="Combobox helper text"
+    />
+    <div style={{
+      display: 'flex',
+      alignItems: 'center',
+      justifyContent: 'space-between'
+    }}>
+      <Button onClick={() => setValue(null)}>Reset</Button>
+      <Button onClick={() => setValue(options[0])}>Option 1</Button>
+      <Button onClick={() => setValue(options[1])}>Option 2</Button>
+      <Button onClick={() => setValue(options[2])}>Option 3</Button>
+    </div>
+  </div>
+);
+
+```
+
 ## `itemToElement`
 
 The Combobox takes in an `items` array and can then be formatted by

packages/react/src/components/ComboBox/ComboBox.stories.js

@@ -5,7 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import React, { useRef } from 'react';
+import React, { useState, useRef } from 'react';
 
 import { WithLayer } from '../../../.storybook/templates/WithLayer';
 
@@ -214,6 +214,52 @@ export const withAILabel = () => (
   </div>
 );
 
+export const _fullyControlled = () => {
+  const options = [
+    {
+      id: 'option-1',
+      text: 'Option 1',
+    },
+    {
+      id: 'option-2',
+      text: 'Option 2',
+    },
+    {
+      id: 'option-3',
+      text: 'Option 3',
+    },
+  ];
+  const [value, setValue] = useState(options[0]);
+  const onChange = ({ selectedItem }) => {
+    setValue(selectedItem);
+  };
+
+  return (
+    <div>
+      <ComboBox
+        onChange={onChange}
+        id="carbon-combobox"
+        items={options}
+        selectedItem={value}
+        itemToString={(item) => (item ? item.text : '')}
+        titleText="Fully Controlled ComboBox title"
+        helperText="Combobox helper text"
+      />
+      <div
+        style={{
+          display: 'flex',
+          alignItems: 'center',
+          justifyContent: 'space-between',
+        }}>
+        <Button onClick={() => setValue(null)}>Reset</Button>
+        <Button onClick={() => setValue(options[0])}>Option 1</Button>
+        <Button onClick={() => setValue(options[1])}>Option 2</Button>
+        <Button onClick={() => setValue(options[2])}>Option 3</Button>
+      </div>
+    </div>
+  );
+};
+
 export const Playground = (args) => (
   <div style={{ width: 300 }}>
     <ComboBox

packages/react/src/components/ComboBox/ComboBox.tsx

@@ -57,7 +57,6 @@ const {
   InputKeyDownArrowUp,
   InputKeyDownArrowDown,
   MenuMouseLeave,
-  FunctionSelectItem,
 } = useCombobox.stateChangeTypes;
 
 const defaultItemToString = <ItemType,>(item: ItemType | null) => {
@@ -85,11 +84,13 @@ const getInputValue = <ItemType,>({
   inputValue,
   itemToString,
   selectedItem,
+  prevSelectedItem,
 }: {
   initialSelectedItem?: ItemType | null;
   inputValue: string;
   itemToString: ItemToStringHandler<ItemType>;
   selectedItem?: ItemType | null;
+  prevSelectedItem?: ItemType | null;
 }) => {
   if (selectedItem) {
     return itemToString(selectedItem);
@@ -99,6 +100,10 @@ const getInputValue = <ItemType,>({
     return itemToString(initialSelectedItem);
   }
 
+  if (!selectedItem && prevSelectedItem) {
+    return '';
+  }
+
   return inputValue || '';
 };
 
@@ -426,23 +431,29 @@ const ComboBox = forwardRef(
       })
     );
     const [isFocused, setIsFocused] = useState(false);
-    const [prevSelectedItem, setPrevSelectedItem] = useState<ItemType | null>();
-    const [doneInitialSelectedItem, setDoneInitialSelectedItem] =
-      useState(false);
     const savedOnInputChange = useRef(onInputChange);
+    const prevSelectedItemProp = useRef<ItemType | null | undefined>(
+      selectedItemProp
+    );
 
-    if (!doneInitialSelectedItem || prevSelectedItem !== selectedItemProp) {
-      setDoneInitialSelectedItem(true);
-      setPrevSelectedItem(selectedItemProp);
-      setInputValue(
-        getInputValue({
+    // fully controlled combobox: handle changes to selectedItemProp
+    useEffect(() => {
+      if (prevSelectedItemProp.current !== selectedItemProp) {
+        const currentInputValue = getInputValue({
           initialSelectedItem,
           inputValue,
           itemToString,
           selectedItem: selectedItemProp,
-        })
-      );
-    }
+          prevSelectedItem: prevSelectedItemProp.current,
+        });
+        setInputValue(currentInputValue);
+        onChange({
+          selectedItem: selectedItemProp,
+          inputValue: currentInputValue,
+        });
+        prevSelectedItemProp.current = selectedItemProp;
+      }
+    }, [selectedItemProp]);
 
     const filterItems = (
       items: ItemType[],
@@ -523,15 +534,6 @@ const ComboBox = forwardRef(
             return changes;
           }
 
-          case FunctionSelectItem:
-            if (onChange) {
-              onChange({
-                selectedItem: changes.selectedItem,
-                inputValue: changes.inputValue,
-              });
-            }
-            return changes;
-
           case InputKeyDownEnter:
             if (allowCustomValue) {
               setInputValue(inputValue);
@@ -670,7 +672,12 @@ const ComboBox = forwardRef(
         return itemToString(item);
       },
       onInputValueChange({ inputValue }) {
-        setInputValue(inputValue || '');
+        const newInputValue = inputValue || '';
+        setInputValue(newInputValue);
+        if (selectedItemProp && !inputValue) {
+          // ensure onChange is called when selectedItem is cleared
+          onChange({ selectedItem, inputValue: newInputValue });
+        }
         setHighlightedIndex(indexToHighlight(inputValue));
       },
       onSelectedItemChange({ selectedItem }) {