Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/@react-aria/combobox/src/useComboBox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,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 (
Expand Down
17 changes: 17 additions & 0 deletions packages/@react-aria/combobox/test/useComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,23 @@ describe('useComboBox', function () {
expect(preventDefault).toHaveBeenCalledTimes(1);
});

it('should only call commit on Tab when the menu is open', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still true if it allows a custom value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think useComboBoxState will still clear the selected key on field blur via

still

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;
Expand Down
19 changes: 13 additions & 6 deletions packages/@react-stately/combobox/src/useComboBoxState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +373 to +374
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to post this for others walking this logic for this change:
commitSelection is only called in these cases:

  • when revert is called aka pressing Esc, which should revert the inputValue and make sure the selectedKey doesn't change
  • when inputValue === itemText in the allowCustomValue case of commitValue of which results in a no-op except for multiple selection mode which we don't really support anyways
  • conversely, used to reset the inputValue and close the menu in commitValue if allowCustomValue is false. This is only actually called as .close or if the menu is already closed and we are committing the value
  • as a part of commit if we are selecting an already selected key

of those cases, we only really need to fire the onSelectionChange and onChange for the last two cases which seems to check out with new conditions here. The only one I'm not entirely sure is the selectionMode === 'multiple' part of the check, do we really need that? It might not hurt to have it I guess since it would simply call it for an extra case where inputValue === itemText but that is only when inputValue = '' since selectedKey is always null for multiple selection

if (value !== undefined && props.inputValue !== undefined) {
props.onSelectionChange?.(selectedKey);
props.onChange?.(displayValue);
let itemText = selectedKey != null ? collection.getItem(selectedKey)?.textValue ?? '' : '';
if (
shouldForceSelectionChange ||
selectionMode === 'multiple' ||
inputValue !== itemText
) {
props.onSelectionChange?.(selectedKey);
props.onChange?.(displayValue);
}

// Stop menu from reopening from useEffect
let itemText = selectedKey != null ? collection.getItem(selectedKey)?.textValue ?? '' : '';
setLastValue(itemText);
closeMenu();
} else {
Expand All @@ -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);
}
Expand Down
17 changes: 17 additions & 0 deletions packages/@react-stately/combobox/test/useComboBoxState.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
57 changes: 57 additions & 0 deletions packages/react-aria-components/test/ComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down