Updating combobox input value when selected item loads or updates #1683

LFDanLu · 2021-03-10T23:12:17Z

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

I think this is probably easier. Also ran into difficulties this was affecting inputValue/defaultSelected key cases and how the combobox actually renders twice due to setMenuWidth

adobe-bot · 2021-03-11T18:34:49Z

Build successful! 🎉

adobe-bot · 2021-03-11T20:26:40Z

Build successful! 🎉

adobe-bot · 2021-03-11T21:13:13Z

Build successful! 🎉

LFDanLu · 2021-03-11T21:41:15Z

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

-    // In this case, it's the user's responsibility to update inputValue in onSelectionChange. In addition, we preserve the defaultInputValue
-    // on initial render, even if it doesn't match the selected item's text.


last bit of this comment was outdated since the reseting is done in the if statement immediately above now

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

LFDanLu · 2021-03-11T21:55:16Z

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

+          (props.selectedKey !== undefined && props.inputValue !== undefined) ||
+          (props.inputValue !== undefined && props.isOpen !== undefined)


Similar to the commitSelection logic, but w/o the selectedKey + isOpen controlled condition here since resetInputValue should work just fine on its own and we don't need to explicitly call onSelectionChange to close the menu IMO. Still mulling this over

Hmm, should we handle this case at all or should we only handle when inputValue is uncontrolled? I feel like maybe it's the user's responsibility to update inputValue when they update items if it is controlled.

What about when someone uses useAsyncList with the ComboBox? Would this be the expectation:

they calculate the new filterValue from the fetched list of items inside the load func that they provide to the useAsyncList hook

they call list.setFilterText or provide the adjusted filterText value through load's return object?

Just trying to feel out the mechanics of how the user would go about doing this. IMO I felt this was quite similar to our commitSelection logic in that we reset the inputValue (or call onSelectionChange instead, depending on what is controlled) for them even if inputValue was controlled

Well, feels like if you're controlling inputValue you wouldn't necessarily want it to change, even when items changes, and therefore it's your responsibility to update it in that case. Basically, whenever you control inputValue you need to update it whenever anything changes that would affect it, and I think that's true in other cases (e.g. in onSelectionChange). So yeah, they'd need to do it in the places you said.

So essentially the logic would become just:

if (props.inputValue === undefined) { resetInputValue(); }

?

I guess this is fine as long as we call this out in the docs (aka if users are controlling items or providing a item map then they should be aware that inputValue and selectedKey text value could drift).

You're mentioning updating the docs, but I don't see that change? Separate PR based on how docs deploy?

…ue reset this covers the scenario where items/defaultItems are both undef and the user provides items to the combobox via map

…ue_1645

adobe-bot · 2021-03-12T23:44:14Z

Build successful! 🎉

only resetting input value when collection changes for uncontrolled inputValue comboboxes now

adobe-bot · 2021-03-15T18:26:40Z

Build successful! 🎉

devongovett · 2021-03-15T19:00:30Z

packages/@react-spectrum/combobox/docs/ComboBox.mdx

-This example uses the [useAsyncList](../react-stately/useAsyncList.html) hook to handle loading the data.
-See the docs for more information.
+This example uses the [useAsyncList](../react-stately/useAsyncList.html) hook to handle loading the data. See the docs for more information.
+**Note:** If you provide a `selectedKey` to the Combobox example below, it is your responsibility to update the input text when the `items` change or load.


Maybe call out specifically that it's because inputValue is controlled as well. Also you need a newline if you want this in a separate paragraph.

Suggested change

**Note:** If you provide a `selectedKey` to the Combobox example below, it is your responsibility to update the input text when the `items` change or load.

**Note:** When both `inputValue` and `selectedKey` are controlled, and the `selectedKey` is set to an initial value before the `items` load, you must update the `inputValue` when the `items` load. This can be done by returning `filterText` from the `useAsyncList` load function.

Also I guess we should implement the ability to return filterText from the load function.

Should we show this in the example as well or would that make things too complex?

Also hmm... we could probably do this automatically in useAsyncList? Like, we have the selectedKey, filterText, and items already. 🤔

I've put a first stab at doing the above in 67b8a93, is this kinda what you were envisioning?
As for adding it to the example, I can go ahead and add it a separate one, feels like a use case some people will have

LFDanLu · 2021-03-16T18:36:44Z

packages/@react-stately/data/src/useAsyncList.ts

@@ -182,6 +187,15 @@ function reducer<T, C>(data: AsyncListState<T, C>, action: Action<T, C>): AsyncL
            items: action.type === 'loading' ? [] : data.items,
            abortController: action.abortController
          };
+        case 'update':


Added update case handling now that selectedKeys/setSelectedKeys from useAsyncList is being used instead of a user defined selectedKey tracker

adobe-bot · 2021-04-13T22:27:04Z

Build successful! 🎉

devongovett · 2021-04-15T17:09:15Z

packages/@react-stately/data/src/useAsyncList.ts

+        case 'loadingMore':
+          // If already loading more and another loading more is triggered, abort the new load more.
+          // Do not overwrite the data.abortController
+          action.abortController.abort();


How would this case occur? Don't we prevent it here?

react-spectrum/packages/@react-stately/data/src/useAsyncList.ts

Lines 327 to 329 in 48c95b9

if (data.state === 'loadingMore' || data.state === 'filtering' || data.cursor == null) {

return;

}

That is what I thought at first, but if loadMore is called multiple times in quick succession it is apparently possible that data.state doesn't update in time. I only noticed this in the async combobox story where Virtualizer occasionally called loadMore multiple times when the menu and break cuz loadingMore wasn't a valid action when state was loadingMore

I added a test specifically for this here:

react-spectrum/packages/@react-stately/data/test/useAsyncList.test.js

Lines 746 to 777 in 48c95b9

it('should handle multiple loadMore called in quick succession', async () => {

let load = jest.fn()

.mockImplementationOnce(getItems)

.mockImplementationOnce(getItems2)

.mockImplementationOnce(getItems);

let {result} = renderHook(

() => useAsyncList({load})

);

expect(load).toHaveBeenCalledTimes(1);

expect(result.current.isLoading).toBe(true);

expect(result.current.items).toEqual([]);

await act(async () => {

jest.runAllTimers();

});

expect(load).toHaveBeenCalledTimes(1);

expect(result.current.isLoading).toBe(false);

expect(result.current.items).toEqual(ITEMS);

await act(async () => {

result.current.loadMore();

result.current.loadMore();

jest.runAllTimers();

});

// Only original load more is handled, the other is canceled

expect(load).toHaveBeenCalledTimes(3);

expect(result.current.isLoading).toBe(false);

expect(result.current.items).toEqual(ITEMS.concat(ITEMS2));

});

If you reset this branch to 121f4bc (aka before I made these changes) and reuse the above test with a try catch around the double .loadMore calls you can see the error Invalid action "loadingMore" in state "loadingMore"

oh that makes sense. Ideally we wouldn't start a request only to immediately abort it though. One option would be to track the loading state in a ref so we don't have to wait for render to get the updated value. Another option would be to only trigger the request from within reduce, but this may delay the start for longer than necessary.

kk, implemented this kinda. Added a separate loadingState ref that gets updated in dispatchFetch but preserved data.state. Was the intention here to completely remove data.state and replace it with the loadingState ref?

devongovett · 2021-04-15T17:10:43Z

packages/@react-stately/data/src/useAsyncList.ts

@@ -199,6 +209,10 @@ function reducer<T, C>(data: AsyncListState<T, C>, action: Action<T, C>): AsyncL
            cursor: action.cursor
          };
        case 'error':
+          if (action.abortController !== data.abortController) {


What was this case for?

Paired with the added case below handling loadingMore if loadingMore is already happening. The change below will abort the newer (and extraneous) loadingMore actions which as a result will dispatch an error for the newer loadingMore actions. This case will then be triggered and preserve the current data state (i.e. we don't want to change the state to 'error' just cuz we canceled some extraneous loadMore calls)

adobe-bot · 2021-04-19T18:23:09Z

Build successful! 🎉

devongovett · 2021-04-19T22:32:07Z

packages/@react-spectrum/combobox/docs/ComboBox.mdx

+
+```tsx example
+function AsyncLoadingExample() {
+  let [isFocused, setFocused] = React.useState(false);


This doesn't affect rendering so could be a ref to avoid extra re-renders

devongovett · 2021-04-19T22:40:17Z

packages/@react-spectrum/combobox/docs/ComboBox.mdx

+    <ComboBox
+      label="Star Wars Character Lookup"
+      onFocus={() => setFocused(true)}
+      onBlur={() => setFocused(false)}


onFocusChange might be a bit simpler

devongovett · 2021-04-19T22:41:46Z

packages/@react-spectrum/combobox/docs/ComboBox.mdx

+
+      // If selectedKey exists and combobox isn't focused, update the input value with the selected key text
+      // This allows the input value to be up to date when items load for the first time or the selected key text is updated server side.
+      if (!isFocused && selectedKey) {


A little unfortunate that the user needs to track isFocused themselves for this. Would it update on blur though? I guess not because it's controlled? Should it perhaps be based on the loadingState (so loadingState === 'loading') instead? If the user typed in the input before the initial load completed, the initial load would get aborted anyway.

Well, it does update on blur in this case since onSelectionChange is called on blur in useComboBoxState since this is a multiple controlled prop combobox. Since onSelectionChange in this example is:

let onSelectionChange = (key) => { let itemText = list.getItem(key)?.name; list.setSelectedKeys(new Set([key])); list.setFilterText(itemText); };

the combobox value will be updated to "Luke Skywalker" on blur. I can try changing it to be based on loadingState, I guess I'll add it to one of the params load accepts in useAsyncList

Do you think that makes sense? I guess the only case where it wouldn't work would be if the user focused the input before the items loaded but didn't type anything? But maybe that's ok? Just trying to think which is easier to explain/document.

adobe-bot · 2021-04-20T18:41:16Z

Build successful! 🎉

adobe-bot · 2021-04-20T19:22:31Z

Build successful! 🎉

LFDanLu · 2021-04-20T19:26:39Z

packages/@react-spectrum/combobox/stories/ComboBox.stories.tsx

+    name: string,
+    url: string
+  }
+  let isFocused = useRef(false);


Will use this story or revert to using the loadingState in load depending on what we feel is the most appropriate

LFDanLu · 2021-04-20T19:29:47Z

packages/@react-stately/data/src/useAsyncList.ts

+      if (action.type !== 'success' && action.type !== 'update') {
+        loadingState.current = action.type;
+      } else {
+        loadingState.current = 'idle';


opinions on this? Not sure a case exists where dispatchFetch is called w/ action=success/update but I needed a state to be set if there is hence the idle but that doesn't feel quite right... I could also modify dispatch in such a way that loadingState ref is modified in the reducer.

Hmm I don't think a case exists where action.type would be success or update calling dispatchFetch.

yeah, figured as much. Maybe I'll just delete the else part

adobe-bot · 2021-04-20T21:19:55Z

Build successful! 🎉

adobe-bot · 2021-04-21T00:59:28Z

Build successful! 🎉

devongovett · 2021-04-21T01:29:51Z

packages/@react-stately/data/src/useAsyncList.ts

+      // Fetch a new filtered list if filterText is updated via `load` response func rather than list.setFilterText
+      // Only do this if not aborted (e.g. user triggers another filter action before load completes)
+      if (filterText && (filterText !== previousFilterText) && !abortController.signal.aborted) {
+        loadingState.current = 'filtering';


Should be set inside dispatchFetch already right?

bleh, was too broad with my search on "dispatch", good catch

adobe-bot · 2021-04-21T01:40:49Z

Build successful! 🎉

adobe-bot · 2021-04-21T01:47:10Z

Build successful! 🎉

…obe#1683)

making combobox input value update when items change

2818ab2

LFDanLu requested review from dannify, devongovett and snowystinger as code owners March 10, 2021 23:12

changing from initial render check to props.item change check

c3f556f

I think this is probably easier. Also ran into difficulties this was affecting inputValue/defaultSelected key cases and how the combobox actually renders twice due to setMenuWidth

adding tests

b1af8b7

Merge branch 'main' into issue_1645

31506f6

LFDanLu changed the title ~~(WIP) Updating combobox input value when selected item loads or updates~~ Updating combobox input value when selected item loads or updates Mar 11, 2021

LFDanLu commented Mar 11, 2021

View reviewed changes

packages/@react-stately/combobox/src/useComboBoxState.ts Outdated Show resolved Hide resolved

LFDanLu commented Mar 11, 2021

View reviewed changes

LFDanLu added 3 commits March 12, 2021 10:59

using collection as comparator instead of items for ComboBox inputVal…

93d0fc1

…ue reset this covers the scenario where items/defaultItems are both undef and the user provides items to the combobox via map

Merge branch 'issue_1645' of github.com:adobe/react-spectrum into iss…

9026228

…ue_1645

fixing lint

2727ce1

addressing review comments

ad777d2

only resetting input value when collection changes for uncontrolled inputValue comboboxes now

devongovett reviewed Mar 15, 2021

View reviewed changes

LFDanLu added 2 commits March 15, 2021 17:36

updating copy

6e14591

first stab at having useAsyncList handle input value reset

67b8a93

LFDanLu commented Mar 16, 2021

View reviewed changes

LFDanLu changed the title ~~Updating combobox input value when selected item loads or updates~~ (WIP) Updating combobox input value when selected item loads or updates Mar 16, 2021

LFDanLu added 3 commits March 18, 2021 11:28

cleanup and updating reset input logic in combobox

21482f3

updating combobox docs

576ad28

tweaking comments

3e1e043

devongovett added the waiting Waiting on Issue Author label Mar 18, 2021

Merge branch 'main' of github.com:adobe/react-spectrum into issue_1645

121f4bc

devongovett self-assigned this Apr 15, 2021

devongovett reviewed Apr 15, 2021

View reviewed changes

devongovett removed their assignment Apr 15, 2021

Merge branch 'main' of github.com:adobe/react-spectrum into issue_1645

619b725

devongovett reviewed Apr 19, 2021

View reviewed changes

LFDanLu added 2 commits April 19, 2021 17:15

Merge branch 'main' of github.com:adobe/react-spectrum into issue_1645

ba4d810

propagating loadingState to useAysncList load fn

6f3d4b9

REVERT THIS: example with using focus ref

407cdd7

LFDanLu commented Apr 20, 2021

View reviewed changes

LFDanLu mentioned this pull request Apr 20, 2021

Updating ComboBox docs with input text update on async list fetch #1811

Merged

5 tasks

pulling docs out

783f5ee

making progress circle appear in combobox for initial async loading

e5c4441

Merge branch 'main' of github.com:adobe/react-spectrum into issue_1645

794a3b1

LFDanLu force-pushed the issue_1645 branch from 612190f to 794a3b1 Compare April 21, 2021 01:27

devongovett reviewed Apr 21, 2021

View reviewed changes

removing some extraneous lines

138800d

devongovett approved these changes Apr 21, 2021

View reviewed changes

devongovett merged commit b7aa7f5 into main Apr 21, 2021

devongovett deleted the issue_1645 branch April 21, 2021 02:24

LFDanLu mentioned this pull request May 18, 2021

useAsyncList doesn't update selectedKeys properly if set via load return statement and by list.setSelectedKeys #1923

Open

majornista pushed a commit to majornista/react-spectrum-v3 that referenced this pull request May 19, 2021

Updating combobox input value when selected item loads or updates (ad…

8d431d8

…obe#1683)

		// In this case, it's the user's responsibility to update inputValue in onSelectionChange. In addition, we preserve the defaultInputValue
		// on initial render, even if it doesn't match the selected item's text.

		(props.selectedKey !== undefined && props.inputValue !== undefined) \|\|
		(props.inputValue !== undefined && props.isOpen !== undefined)

	Note: If you provide a `selectedKey` to the Combobox example below, it is your responsibility to update the input text when the `items` change or load.

	Note: When both `inputValue` and `selectedKey` are controlled, and the `selectedKey` is set to an initial value before the `items` load, you must update the `inputValue` when the `items` load. This can be done by returning `filterText` from the `useAsyncList` load function.

	if (data.state === 'loadingMore' \|\| data.state === 'filtering' \|\| data.cursor == null) {
	return;
	}

	it('should handle multiple loadMore called in quick succession', async () => {
	let load = jest.fn()
	.mockImplementationOnce(getItems)
	.mockImplementationOnce(getItems2)
	.mockImplementationOnce(getItems);
	let {result} = renderHook(
	() => useAsyncList({load})
	);

	expect(load).toHaveBeenCalledTimes(1);
	expect(result.current.isLoading).toBe(true);
	expect(result.current.items).toEqual([]);

	await act(async () => {
	jest.runAllTimers();
	});

	expect(load).toHaveBeenCalledTimes(1);
	expect(result.current.isLoading).toBe(false);
	expect(result.current.items).toEqual(ITEMS);

	await act(async () => {
	result.current.loadMore();
	result.current.loadMore();
	jest.runAllTimers();
	});

	// Only original load more is handled, the other is canceled
	expect(load).toHaveBeenCalledTimes(3);
	expect(result.current.isLoading).toBe(false);
	expect(result.current.items).toEqual(ITEMS.concat(ITEMS2));
	});

Updating combobox input value when selected item loads or updates #1683

Updating combobox input value when selected item loads or updates #1683

Conversation

LFDanLu commented Mar 10, 2021

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

adobe-bot commented Mar 11, 2021

adobe-bot commented Mar 11, 2021

adobe-bot commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Mar 12, 2021

adobe-bot commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LFDanLu Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Apr 20, 2021

adobe-bot commented Apr 20, 2021

Choose a reason for hiding this comment

LFDanLu Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Apr 20, 2021

adobe-bot commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Apr 21, 2021

adobe-bot commented Apr 21, 2021

LFDanLu Mar 16, 2021 •

edited

Loading

LFDanLu Apr 20, 2021 •

edited

Loading