Skip to content

Fix Crash in useSelectableCollection when ref not attached to dom node in first render #1629

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix Crash in useSelectableCollection when ref not attached to dom node in first render #1629

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
caf6144
Fix Crash in useSelectableCollection when ref not attached to dom nod…
snowystinger Feb 23, 2021
f569d8e
Merge branch 'main' into snowystinger-patch-1
snowystinger Mar 24, 2021
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
4 changes: 2 additions & 2 deletions packages/@react-aria/selection/src/useSelectableCollection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,8 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S
manager.setFocused(true);
manager.setFocusedKey(focusedKey);

// If no default focus key is selected, focus the collection itself.
if (focusedKey == null && !shouldUseVirtualFocus) {
// If no default focus key is selected and the collection is mounted, focus the collection itself.
if (focusedKey == null && !shouldUseVirtualFocus && ref.current) {
Comment on lines +349 to +350
Copy link
Member

@LFDanLu LFDanLu Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, this makes sense for stopping the error but there might be an issue since then focus never gets set on the ListBox itself (this useEffect only happens on the initial render). That would mean the focus remains on the select button itself and pressing up/down would open and close the select menu instead of navigating through the items in the list.

Perhaps this should be extracted to it's own useEffect? Maybe useListBox shouldn't be pulled out into the Select component? Maybe the useEffect should fire if ref.current updates? Not sure

Copy link
Member Author

@snowystinger snowystinger Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So I had some similar reservations, however, in practice it seems to work, though the example code didn't have any of the focus/selection code running yet.
I updated the example to match our code on our documentation page so that I could try out the concern that focus would remain on the button, basically a copy paste from https://react-spectrum.adobe.com/react-aria/useSelect.html#example and now it would appear that this check is unnecessary. It behaves with or without it.

I think the outcome of this is

  1. this person can update their code to be more like our documentation Edit, they were trying to move it on purpose, so we'll still need to do Component css packages #2
  2. we should still perform this check somewhere, but maybe some more thought should be put into it, i'll try some other configurations later hopefully this week
  3. figure out why their selection code isn't working

focusSafely(ref.current);
}
}
Expand Down