wip: fix: tablist auto selection #7529
Conversation
| ]); | ||
|
|
||
| // Use functional update to ensure you're working with the most recent state | ||
| setSelectedTabId((prevSelectedTabId) => tabId); |
There was a problem hiding this comment.
Why isn't prevSelectedTabId not being used a lint error?
Does the most recent state matter if we're setting the state to the newly created tabId?
| await user.tab(); | ||
| onSelectionChange.mockClear(); | ||
| await user.keyboard('{Enter}'); | ||
| expect(onSelectionChange).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Should we confirm which tab is selected?
| @@ -56,7 +56,7 @@ export function useTabListState<T extends object>(props: TabListStateOptions<T>) | |||
| selectionManager.setFocusedKey(selectedKey); | |||
| } | |||
| lastSelectedKey.current = selectedKey; | |||
| }); | |||
| }, [collection]); | |||
There was a problem hiding this comment.
Don't we need this to also update when selectedKey changes, or if selectionManager.isEmpty or a bunch of other conditions?
There was a problem hiding this comment.
yeah, I'd like to get more at the root cause
I'd hoped to change the first if in the effect to include
lastCollection.current !== collection
and then leave off the dependency array, but it's not working for some reason. probably should have opened as a draft, but I forgot
There was a problem hiding this comment.
@snowystinger That's weird, that works for me 🤷
There was a problem hiding this comment.
? tests all pass?
have I messed up my own logic?
let lastSelectedKey = useRef(currentSelectedKey);
let lastCollection = useRef(collection);
useEffect(() => {
// Ensure a tab is always selected (in case no selected key was specified or if selected item was deleted from collection)
let selectedKey = currentSelectedKey;
if (lastCollection.current !== collection && (selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey))) {
...
lastSelectedKey.current = selectedKey;
lastCollection.current = collection;
});
There was a problem hiding this comment.
yes, all tests pass exactly like this 😅 Also did a clean install and cleared the parcel cache just to make sure...
There was a problem hiding this comment.
I get 8 failures... even on a brand new pull. lets see if CI disagrees with me.
There was a problem hiding this comment.
I see, I misunderstood what you meant by all tests 😓
Running the whole test pipeline i also get 8 failures in Tabs.test.js. I was running this test file in isolation, so it appears to be some sort of cascading failure because the DOM isn't cleared.
Can you check whether running just Tabs.test.js also passes for you?
There was a problem hiding this comment.
I don't think it's a cascade failure, we have more behavior tests in React Spectrum, those are the ones failing, for the reasons outlined by @devongovett
The crux of the issue is that the selectedkey is set at the same time as the item with that key is added.
When the selectedkey changes (regardless of the collection changing), we need to check to see if we should set a forced default since "no tab selected" is not an option.
At this point though, the collection isn't updated, that takes one more render pass.
As a result, the new key doesn't exist when we go to check if we need to set a default, so we set a default which triggers onSelectionChange.
Then the collection is ready, but it's too late.
snowystinger
changed the title
Closes #7526
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: