-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(combobox): re-open menu when async items arrive after empty response #9823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4bdf53f
e0d2726
5fbcaf1
4493e3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 () { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue here is that async ComboBoxes typically make use of a Collection + LoadMoreItem and thus pass the controlled items to the wrapped ListBox, meaning
props.itemshere will be null always. I suppose we could ask users to passitemsto both places, but that feels a bit gross...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example of this in the storybook:
react-spectrum/packages/react-aria-components/stories/ComboBox.stories.tsx
Lines 304 to 325 in 93df536
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its possible that we could pass something to useComboBoxState to inform it of this specific configuration and use the
collectionprovided by the props instead of.itemsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — I hadn't considered the Collection + LoadMoreItem pattern where items go to ListBox's Collection rather than to ComboBox directly.
I traced through the code for all three patterns (items on ComboBox, Collection+LoadMoreItem, and allowsEmptyCollection) and I think the simplest fix is to remove the
props.items != nullguard entirely — just setclosedDueToEmptyunconditionally when the menu auto-closes due to empty collection.This is safe because:
filteredCollectiononly changes wheninputValuechanges, which already triggers re-open via theinputValue !== lastValuecheck above. The flag becomes redundant but harmless.filteredCollectiongoes from 0 → N without input changing — exactly the scenario this flag is designed for.!allowsEmptyCollectionis false), so the flag is never set. No change.I'll also rename
closedDueToEmptyControlled→closedDueToEmptysince it's no longer specific to controlled items, and add a test covering the Collection-based async pattern.