Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
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
132 changes: 130 additions & 2 deletions packages/react-aria-components/test/ComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -857,11 +857,11 @@ describe('ComboBox', () => {
act(() => {getByTestId('form').checkValidity();});
expect(combobox).toHaveAttribute('aria-describedby');
expect(container.querySelector('.react-aria-ComboBox')).toHaveAttribute('data-invalid');

await comboboxTester.open();
let options = comboboxTester.options();
await user.click(options[0]);

act(() => combobox.blur());
expect(combobox).not.toHaveAttribute('required');
expect(combobox.validity.valid).toBe(true);
Expand Down Expand Up @@ -937,4 +937,132 @@ describe('ComboBox', () => {
expect(comboboxTester.combobox).toHaveFocus();
expect(onOpenChange).toHaveBeenCalledTimes(1);
});

it('should re-open the menu when controlled items go from empty to non-empty controlled items', async () => {
let onOpenChange = jest.fn();
let onInputChange = jest.fn().mockReturnValueOnce(true).mockReturnValue(false);

function ControlledComboBox() {
let [items, setItems] = useState([{id: 1, name: 'Luke Skywalker'}]);
return (
<ComboBox
items={items}
onInputChange={() => {
if (onInputChange()) {
setItems([]);
} else {
setItems([{id: 1, name: 'Luke Skywalker'}]);
}
}}
onOpenChange={onOpenChange}>
<Label>SW Characters</Label>
<Input />
<Button>{'<'}</Button>
<Popover>
<ListBox>
{(item) => {
return <ListBoxItem id={item.id}>{item.name}</ListBoxItem>;
}}
</ListBox>
</Popover>
</ComboBox>
);
}

let {container, queryByRole} = render(<ControlledComboBox />);
let comboboxTester = testUtilUser.createTester('ComboBox', {root: container});
await user.tab();
await user.keyboard('{ArrowDown}');
expect(onOpenChange).toHaveBeenCalledTimes(1);
expect(comboboxTester.listbox).toBeVisible();
onOpenChange.mockClear();

await user.keyboard('L');
expect(queryByRole('listbox')).toBeNull();

await user.keyboard('{Backspace}');
expect(comboboxTester.listbox).toBeVisible();
});

it('should still close the menu when uncontrolled items are empty', async () => {
let onOpenChange = jest.fn();
let onInputChange = jest.fn().mockReturnValueOnce(true).mockReturnValue(false);

let items = [{id: 1, name: 'Luke Skywalker'}];
function ControlledComboBox() {
return (
<ComboBox
defaultItems={items}
onOpenChange={onOpenChange}>
<Label>SW Characters</Label>
<Input />
<Button>{'<'}</Button>
<Popover>
<ListBox>
{(item) => {
return <ListBoxItem id={item.id}>{item.name}</ListBoxItem>;
}}
</ListBox>
</Popover>
</ComboBox>
);
}

let {container, queryByRole} = render(<ControlledComboBox />);
let comboboxTester = testUtilUser.createTester('ComboBox', {root: container});
await user.tab();
await user.keyboard('{ArrowDown}');
expect(onOpenChange).toHaveBeenCalledTimes(1);
expect(comboboxTester.listbox).toBeVisible();
onOpenChange.mockClear();

await user.keyboard('Z');
expect(queryByRole('listbox')).toBeNull();
});

it('should not re-open after user dismisses with Escape (revert) controlled items', async () => {
let onOpenChange = jest.fn();
let onInputChange = jest.fn().mockReturnValueOnce(true).mockReturnValue(false);

function ControlledComboBox() {
let [items, setItems] = useState([{id: 1, name: 'Luke Skywalker'}]);
return (
<ComboBox
items={items}
onInputChange={() => {
if (onInputChange()) {
setItems([]);
} else {
setItems([{id: 1, name: 'Luke Skywalker'}]);
}
}}
onOpenChange={onOpenChange}>
<Label>SW Characters</Label>
<Input />
<Button>{'<'}</Button>
<Popover>
<ListBox>
{(item) => {
return <ListBoxItem id={item.id}>{item.name}</ListBoxItem>;
}}
</ListBox>
</Popover>
</ComboBox>
);
}

let {container, queryByRole} = render(<ControlledComboBox />);
let comboboxTester = testUtilUser.createTester('ComboBox', {root: container});
await user.tab();
await user.keyboard('{ArrowDown}');
expect(onOpenChange).toHaveBeenCalledTimes(1);
expect(comboboxTester.listbox).toBeVisible();
onOpenChange.mockClear();

await user.keyboard('L');
expect(queryByRole('listbox')).toBeNull();

await user.keyboard('{Escape}');
expect(queryByRole('listbox')).toBeNull();
});
});
19 changes: 19 additions & 0 deletions packages/react-stately/src/combobox/useComboBoxState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export function useComboBoxState<T extends object, M extends SelectionMode = 'si
let [showAllItems, setShowAllItems] = useState(false);
let [isFocused, setFocusedState] = useState(false);
let [focusStrategy, setFocusStrategy] = useState<FocusStrategy | null>(null);
let closedDueToEmptyControlled = useRef(false);

let defaultValue = useMemo(() => {
return props.defaultValue !== undefined ? props.defaultValue : (selectionMode === 'single' ? props.defaultSelectedKey ?? null : []) as ValueType<M>;
Expand Down Expand Up @@ -359,9 +360,25 @@ export function useComboBoxState<T extends object, M extends SelectionMode = 'si
triggerState.isOpen &&
filteredCollection.size === 0
) {
if (props.items != null) {
closedDueToEmptyControlled.current = true;
}
closeMenu();
}

// Re-open the menu when controlled items become non-empty after being auto-closed due to
// an empty collection (e.g. async load completed with results after a previous empty response).
if (
isFocused &&
closedDueToEmptyControlled.current &&
filteredCollection.size > 0 &&
!triggerState.isOpen &&
menuTrigger !== 'manual'
) {
closedDueToEmptyControlled.current = false;
open(null, 'input');
}

// Close when an item is selected.
if (
displayValue != null &&
Expand Down Expand Up @@ -418,6 +435,7 @@ export function useComboBoxState<T extends object, M extends SelectionMode = 'si

// Revert input value and close menu
let revert = () => {
closedDueToEmptyControlled.current = false;
if (allowsCustomValue && selectedKey == null) {
commitCustomValue();
} else {
Expand Down Expand Up @@ -457,6 +475,7 @@ export function useComboBoxState<T extends object, M extends SelectionMode = 'si
};

const commitValue = () => {
closedDueToEmptyControlled.current = false;
if (allowsCustomValue) {
const itemText = selectedKey != null ? collection.getItem(selectedKey)?.textValue ?? '' : '';
(inputValue === itemText) ? commitSelection() : commitCustomValue();
Expand Down
80 changes: 80 additions & 0 deletions packages/react-stately/test/combobox/useComboBoxState.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -305,4 +305,84 @@ describe('useComboBoxState tests', function () {
expect(result.current.collection.size).toEqual(2);
});
});

describe('controlled items (async loading)', function () {
it('should re-open the menu when controlled items go from empty to non-empty', function () {
let onOpenChange = jest.fn();
let initialProps = {
items: [{id: 1, name: 'Luke Skywalker'}],
children: (props) => <Item>{props.name}</Item>,
onOpenChange
};

let {result, rerender} = renderHook((props) => useComboBoxState(props), {initialProps});

// Focus and open the menu by setting input value
act(() => {result.current.setFocused(true);});
act(() => {result.current.open(null, 'input');});
expect(result.current.isOpen).toBe(true);

// Simulate async load returning empty results (e.g. user typed "luka")
rerender({...initialProps, items: []});
// Menu closes on empty collection
expect(result.current.isOpen).toBe(false);

// Simulate async load returning results again (e.g. user backspaced to "luk")
rerender({...initialProps, items: [{id: 1, name: 'Luke Skywalker'}]});
// Menu should re-open because items were controlled and the close was due to empty collection
expect(result.current.isOpen).toBe(true);
expect(result.current.collection.size).toEqual(1);
expect(onOpenChange).toHaveBeenLastCalledWith(true, 'input');
});

it('should not re-open after user dismisses with Escape (revert)', function () {
Copy link
Member

Choose a reason for hiding this comment

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

This test and the one below appear to have been passing before the change, where did they come from?

They also passed in the component level, which I've pushed up here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, both of those pass without the change too. The original thinking was they'd serve as guard rails for the re-open logic boundaries (Escape clearing the flag, uncontrolled items being excluded), but since they assert the default state (menu stays closed) they pass vacuously whether or not the re-open behavior exists.

Happy to remove them. The component-level tests you added cover the same scenarios, and the useAsyncList integration test is a much better representation of the actual user flow this fix is targeting.

Want me to drop those two hook-level tests and keep just the one that actually demonstrates the fix (controlled items going from empty to non-empty)?

let onOpenChange = jest.fn();
let initialProps = {
items: [{id: 1, name: 'Luke Skywalker'}],
children: (props) => <Item>{props.name}</Item>,
onOpenChange
};

let {result, rerender} = renderHook((props) => useComboBoxState(props), {initialProps});

// Focus and open
act(() => {result.current.setFocused(true);});
act(() => {result.current.open(null, 'input');});
expect(result.current.isOpen).toBe(true);

// Async returns empty, menu auto-closes
rerender({...initialProps, items: []});
expect(result.current.isOpen).toBe(false);

// User presses Escape (revert) while menu is closed
act(() => {result.current.revert();});

// Async returns items — menu should NOT re-open because user explicitly dismissed
rerender({...initialProps, items: [{id: 1, name: 'Luke Skywalker'}]});
expect(result.current.isOpen).toBe(false);
});

it('should still close the menu when uncontrolled items are empty', function () {
let onOpenChange = jest.fn();
let contains = (a, b) => a.toLowerCase().includes(b.toLowerCase());
let initialProps = {
defaultItems: [{id: 1, name: 'Luke Skywalker'}],
children: (props) => <Item>{props.name}</Item>,
onOpenChange,
defaultFilter: contains
};

let {result} = renderHook((props) => useComboBoxState(props), {initialProps});

// Focus and open
act(() => {result.current.setFocused(true);});
act(() => {result.current.open(null, 'input');});
expect(result.current.isOpen).toBe(true);

// Type something that filters to zero results
act(() => {result.current.setInputValue('zzz');});
// Menu should close because items are uncontrolled and filtered to empty
expect(result.current.isOpen).toBe(false);
});
});
});
Loading