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

[stefanprobst]

🐛 Bug Report

When the ref passed to useSelectableCollection is not yet attached to a DOM node in first render - and if autoFocus is set to truthy - a crash with TypeError: element is undefined happens in this useEffect, which triggers focusSafely => focusWithoutScrolling which then tries to call focus() on null.

🤔 Expected Behavior

No errors.

😯 Current Behavior

see above

💁 Possible Solution

🔦 Context

I ran into this issue when moving useListBox up a level in the useSelect docs example, i.e. when the listbox ref is only attached when state.isOpen becomes true.

💻 Code Sample

CSB was acting up, so i put a repro here: https://github.com/stefanprobst/react-aria-issue/blob/main/components/Select.js

EDIT: here's a codesandbox based on the useSelect docs example: https://codesandbox.io/s/react-aria-listbox-ref-r6elr?file=/src/App.js

🌍 Your Environment

Software	Version(s)
react-spectrum	latest
Browser	Firefox 85
Operating System	Ubuntu 20.04

[snowystinger]

Yep, I see the issue in your project. Looks like we can just add a check to that if for the ref.current #1629 I've opened a PR for it so we can see if there's anything else needed

[snowystinger]

Hey, so I was talking to a coworker, and we noticed that with the fix, even though it doesn't crash, we can't select any items from the picker. Any idea what's going on there? I'll try to check tomorrow as well

[stefanprobst]

hmm that's surpsrising - i'll try to dig a bit later today.

[stefanprobst]

updated issue description with CSB based on useSelect docs example.

EDIT: hmm looks like selection is working with the fix from #1629 in the CSB repro? Ah ok, but not with keyboard since the focus stays on the button?

[stefanprobst]

would it make sense to use a callback ref so ref changes can trigger a render?

cf https://twitter.com/sebmarkbage/status/1091808948523581440

[snowystinger]

Thanks for the extra information, we'll try it out soon.

[snowystinger]

Sorry for the delay, I was pulled away to some other work.

I don't think we can use a callback ref, at least, not as described here https://stackoverflow.com/questions/55838351/how-do-we-know-when-a-react-ref-current-value-has-changed
The ref is passed in and not through, and we can't really require everything using this to move over to a state kept one.

My coworker actually noted a great way to deal with this (thanks @LFDanLu) if you pull the ListBoxPopup out of the Select as we have in our docs, then this will work as expected. Since there isn't a good way for us to know when to try to do the autofocus other than after mounting, I think this is the best way to handle it. useSelectableCollection has no idea about modals and we shouldn't be passing something like "isOpen" to it or "isMounted" (which is what the callback ref amounts to)

I don't think we should continue with the PR I created to try this out. I think by not failing, it'll hide the problem that my coworker pointed out.
I'll leave this open in case you have some other ideas, but feel free to close if not.