-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: tablist auto selection #7529
base: main
Are you sure you want to change the base?
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snowystinger That's weird, that works for me 🤷
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.
? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get 8 failures... even on a brand new pull. lets see if CI disagrees with me.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
I see now, thanks 👍
Closes #7526
Decision made was to leave the controlled case up to the controller. They know if they are about to add a key or remove a key, so they can make the decision to change the selected key at the same time.
The root issue was that the collection takes two renders. During the first one, the new key doesn't exist yet, so if you've controlled the selected key to the new one, then our logic would say that you have no valid tab selected, and it would auto select one for you. However, after the second render, the key does exist, but by that time, we've already moved on to and fired a selection change.
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: