leaving fully controlled ComboBox (with allowsCustomValue) by Tab/Enter clears selection

[GammaWolf]

🐛 Bug Report

If you have a fully controlled ComboBox with added property allowsCustomValue and a selection is made from the list, then the selection and input is cleared when you leave the ComboBox with Tab or Enter. onSelectionChange is called with value null on Tab\Enter (the event with value null also happens with an uncontrolled ComboBox, which shouldn't). When losing focus by other means (like clicking outside the ComboBox), the value is retained correctly.

Expected Behaviour

Pressing Tab or Enter in a fully controlled ComboBox with property allowsCustomValue with a selected value retains the selection.

Actual Behaviour

The Combobox is cleared.

Reproduce Scenario (including but not limited to)

Steps to Reproduce

Select item via mouse (click on item in opened popup) or keyboard (press return on some item in opened popup). Now the Combobox is closed and the text in the box is of the selected item. Tab out of the ComboBox (or press Return).

Platform and Version

react-spectrum 3.14.1 browser: chromium 93.0.4577.63 on linux

Sample Code that illustrates the problem

https://codesandbox.io/s/intelligent-cache-y6tui?file=/src/App.tsx

Notes

call stack excerpt for the onSelectionChange with value null: useComboBox.ts::onKeyDown() calls state.commit() if 'Enter' or 'Tab' is pressed useComboBoxState.ts::commit() always calls commitCustomValue() if allowsCustomValue is true. commitCustomValue() calls setSelectedKey(null) which eventually leads the onSelectionChange with value null

Maybe relevant: Because losing focus by clicking out of the ComboBox after selection retains the value, I noticed that there seems to be an additional check there for that in (useComboBoxState.ts::setFocused):

  let itemText = collection.getItem(selectedKey)?.textValue ?? '';
  if (allowsCustomValue && inputValue !== itemText) {
    commitCustomValue();
  } else {
    commitSelection();
  }

[LFDanLu]

Perhaps the same inputValue !== itemText check should be in the else if case https://github.com/adobe/react-spectrum/blob/1543be1d9ff156188aa9c9f6e07006800cd8e9ff/packages/%40react-stately/combobox/src/useComboBoxState.ts#L278. Will need to test to make sure this works and doesn't break any of the other cases.

[davidspiess]

Experiencing the same bug

[BDesta]

@LFDanLu created a PR per the suggestion. Had a hard time adding unit tests as the tests are spread between useComboBoxState.test.js and ComboBox.test.js.

I also noticed in the mean time the example can be modified to workaround the bug as the example given exposes the bug

I am thinking something like

import * as React from "react";
import { FC } from "react";
import { ComboBox, Item } from "@adobe/react-spectrum";
import { useTreeData } from "@react-stately/data";

const App: FC = () => {
  let options = [
    { id: 1, name: "Aerospace" },
    { id: 2, name: "Mechanical" },
    { id: 3, name: "Civil" },
    { id: 4, name: "Biomedical" },
    { id: 5, name: "Nuclear" },
    { id: 6, name: "Industrial" },
    { id: 7, name: "Chemical" },
    { id: 8, name: "Agricultural" },
    { id: 9, name: "Electrical" }
  ];

  let [selectedKey, setSelectedKey] = React.useState('');
  let [inputValue, setInputValue] = React.useState('');

  let list = useTreeData({
    initialItems: options
  });

  let onSelectionChange = (key) => {
    console.log('onSelectionChange ' + key);
    setSelectedKey(key);

    if(key) {
      setInputValue(list.getItem(key).value.name)
    }
  };

  let onInputChange = (value) => {
    console.log('onInputChange ' + value);
    setInputValue(value);
  };

  return (
    <>
      <p>Current selected major id: {selectedKey}</p>
      <p>Current input text: {inputValue}</p>
      <ComboBox
        label="Pick a engineering major"
        defaultItems={list.items}
        selectedKey={selectedKey}
        inputValue={inputValue}
        onSelectionChange={onSelectionChange}
        onInputChange={onInputChange}
        allowsCustomValue
      >
        {(item) => <Item>{item.value.name}</Item>}
      </ComboBox>
    </>
  );
};

export default App;

[LFDanLu]

@BDesta One thing I noticed in your workaround example is that clearing the input field doesn't clear the selected key like the original sandbox does (that behavior mimics the behavior of an uncontrolled combobox).