fix(ComboBox): avoid extra onSelectionChange calls (#9729)

Co-authored-by: Robert Snow <rsnow@adobe.com>

Author: reidbarber

packages/@react-aria/combobox/src/useComboBox.ts

@@ -156,7 +156,9 @@ export function useComboBox<T, M extends SelectionMode = 'single'>(props: AriaCo
             break;
           }
         }
-        state.commit();
+        if (e.key === 'Enter' || state.isOpen) {
+          state.commit();
+        }
         break;
       case 'Escape':
         if (

packages/@react-aria/combobox/test/useComboBox.test.js

@@ -119,6 +119,23 @@ describe('useComboBox', function () {
     expect(preventDefault).toHaveBeenCalledTimes(1);
   });
 
+  it('should only call commit on Tab when the menu is open', function () {
+    let commitSpy = jest.fn();
+    let {result: state} = renderHook((props) => useComboBoxState(props), {initialProps: props});
+    let closedState = {...state.current, isOpen: false, commit: commitSpy};
+    let {result: closedResult} = renderHook((props) => useComboBox(props, closedState), {initialProps: props});
+    act(() => {
+      closedResult.current.inputProps.onKeyDown(event({key: 'Tab'}));
+    });
+    expect(commitSpy).toHaveBeenCalledTimes(0);
+    let openState = {...state.current, isOpen: true, commit: commitSpy};
+    let {result: openResult} = renderHook((props) => useComboBox(props, openState), {initialProps: props});
+    act(() => {
+      openResult.current.inputProps.onKeyDown(event({key: 'Tab'}));
+    });
+    expect(commitSpy).toHaveBeenCalledTimes(1);
+  });
+
   it('calls open and toggle with the expected parameters when arrow down/up/trigger button is pressed', function () {
     let {result: state} = renderHook((props) => useComboBoxState(props), {initialProps: props});
     state.current.open = openSpy;

packages/@react-stately/combobox/src/useComboBoxState.ts

@@ -369,14 +369,21 @@ export function useComboBoxState<T extends object, M extends SelectionMode = 'si
     closeMenu();
   };
 
-  let commitSelection = () => {
-    // If multiple things are controlled, call onSelectionChange
+  let commitSelection = (shouldForceSelectionChange = false) => {
+    // If multiple things are controlled, call onSelectionChange only when selecting the focused item,
+    // or when inputValue needs to be synced back to the selected item on commit/blur.
     if (value !== undefined && props.inputValue !== undefined) {
-      props.onSelectionChange?.(selectedKey);
-      props.onChange?.(displayValue as ChangeValueType<M>);
+      let itemText = selectedKey != null ? collection.getItem(selectedKey)?.textValue ?? '' : '';
+      if (
+        shouldForceSelectionChange ||
+        selectionMode === 'multiple' ||
+        inputValue !== itemText
+      ) {
+        props.onSelectionChange?.(selectedKey);
+        props.onChange?.(displayValue as ChangeValueType<M>);
+      }
 
       // Stop menu from reopening from useEffect
-      let itemText = selectedKey != null ? collection.getItem(selectedKey)?.textValue ?? '' : '';
       setLastValue(itemText);
       closeMenu();
     } else {
@@ -401,7 +408,7 @@ export function useComboBoxState<T extends object, M extends SelectionMode = 'si
       // Reset inputValue and close menu here if the selected key is already the focused key. Otherwise
       // fire onSelectionChange to allow the application to control the closing.
       if (selectionManager.isSelected(selectionManager.focusedKey) && selectionMode === 'single') {
-        commitSelection();
+        commitSelection(true);
       } else {
         selectionManager.select(selectionManager.focusedKey);
       }

packages/@react-stately/combobox/test/useComboBoxState.test.js

@@ -209,6 +209,23 @@ describe('useComboBoxState tests', function () {
       expect(onSelectionChange).toHaveBeenCalledWith('1');
     });
 
+    it('does not fire onSelectionChange on blur when fully controlled values are already synced', function () {
+      let initialProps = {...defaultProps, selectedKey: '1', inputValue: 'onomatopoeia'};
+      let {result} = renderHook((props) => useComboBoxState(props), {initialProps});
+
+      act(() => {result.current.setFocused(false);});
+      expect(onSelectionChange).not.toHaveBeenCalled();
+    });
+
+    it('fires onSelectionChange on blur when fully controlled inputValue is out of sync', function () {
+      let initialProps = {...defaultProps, selectedKey: '1', inputValue: 'onom'};
+      let {result} = renderHook((props) => useComboBoxState(props), {initialProps});
+
+      act(() => {result.current.setFocused(false);});
+      expect(onSelectionChange).toHaveBeenCalledTimes(1);
+      expect(onSelectionChange).toHaveBeenCalledWith('1');
+    });
+
     it('won\'t update the returned collection if the combobox is closed (uncontrolled items)', function () {
       let filter = renderHook((props) => useFilter(props), {sensitivity: 'base'});
       let initialProps = {...defaultProps, items: null, defaultItems: [{id: '0', name: 'one'}, {id: '1', name: 'onomatopoeia'}], defaultInputValue: '', defaultFilter: filter.result.current.contains};

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

@@ -263,6 +263,63 @@ describe('ComboBox', () => {
     expect(document.querySelector('input[type=hidden]')).toBeNull();
   });
 
+  it.each(['click', 'tab'])('should not fire extra onSelectionChange calls after focus moves away in fully controlled mode via %s', async (focusMove) => {
+    let onSelectionChange = jest.fn();
+    let keyToText = {
+      '1': 'Cat',
+      '2': 'Dog',
+      '3': 'Kangaroo'
+    };
+
+    function ControlledComboBox() {
+      let [selectedKey, setSelectedKey] = useState(null);
+      let [inputValue, setInputValue] = useState('');
+
+      return (
+        <>
+          <ComboBox
+            value={selectedKey}
+            inputValue={inputValue}
+            onChange={(key) => {
+              onSelectionChange(key);
+              setSelectedKey(key);
+              setInputValue(key != null ? keyToText[key] : '');
+            }}
+            onInputChange={setInputValue}>
+            <Label>Favorite Animal</Label>
+            <Input />
+            <Button />
+            <Popover>
+              <ListBox>
+                <ListBoxItem id="1">Cat</ListBoxItem>
+                <ListBoxItem id="2">Dog</ListBoxItem>
+                <ListBoxItem id="3">Kangaroo</ListBoxItem>
+              </ListBox>
+            </Popover>
+          </ComboBox>
+          <button type="button">Next</button>
+        </>
+      );
+    }
+
+    let tree = render(<ControlledComboBox />);
+    let comboboxTester = testUtilUser.createTester('ComboBox', {root: tree.container});
+
+    await comboboxTester.selectOption({option: 'Dog'});
+    expect(onSelectionChange).toHaveBeenCalledTimes(1);
+
+    if (focusMove === 'click') {
+      await user.click(tree.getByRole('button', {name: 'Next'}));
+    } else {
+      act(() => {
+        comboboxTester.combobox.focus();
+      });
+      await user.tab();
+    }
+
+    expect(onSelectionChange).toHaveBeenCalledTimes(1);
+  });
+
   it('should support form reset', async () => {
     const tree = render(
       <form>