Accept tabIndex in the useFocusable hook

[Andarist]

Closes #1384 , cc @ktabors

✅ 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:

yarn test

[dannify]

Plus missing tests?! 🤩 Thankyou

[snowystinger]

Whoa, thanks for adding tests!
I think we're waiting to merge this as we are just wrapping up testing for our next release. So tiny code freeze :)

[Andarist]

This shows that we'll have 2 competing props - excludeFromTabOrder and tabIndex which both "control" the returned tabIndex. I would say this is not ideal - maybe excludeFromTabOrder should be deprecated in the future?

[Andarist]

q: what should happen for {tabIndex: 0, isDisabled: true}?

[devongovett]

We'd like to keep tabIndex out of our component APIs generally to avoid improper usage (e.g. tabIndex > 0). That's why we have the excludeFromTabOrder prop instead.

Most of the the components using useFocusable are already focusable by default (e.g. textfields, radios, etc.), so we don't need to add a tabIndex explicitly. In other cases, the tabIndex is already controlled by something else (e.g. to implement roving tab index pattern).

useFocusable also won't know whether you want to make an element tabbable, focusable, or both without more information. This would essentially be the same as passing a tabIndex manually. We could add tabIndex as a supported option to the useFocusable hook itself rather than other public APIs, but I'm not sure I see the need since you could just apply it to the element directly. Perhaps this should be documented though. Thoughts?

[Andarist]

We'd like to keep tabIndex out of our component APIs generally to avoid improper usage (e.g. tabIndex > 0).

That doesn't quite avoid the problem though and you also lose validation opportunity the way it's handled right now.

That's why we have the excludeFromTabOrder prop instead.

That's understandable - but if you plan to proceed with this PR then this creates a tiny conflict between excludeFromTabOrder and tabIndex props

Most of the the components using useFocusable are already focusable by default (e.g. textfields, radios, etc.), so we don't need to add a tabIndex explicitly. In other cases, the tabIndex is already controlled by something else (e.g. to implement roving tab index pattern).

Maybe a good opportunity to add dev-only validation for this in the useEffect? 🤔

We could add tabIndex as a supported option to the useFocusable hook itself rather than other public APIs, but I'm not sure I see the need since you could just apply it to the element directly

The slight problem I have with this is that for a consumer it's not obvious that this is different from other props in other hooks that "could just be applied to the element directly".

Perhaps this should be documented though.

That would definitely be an improvement 👍 I probably don't mind this whole case that much as it feels sort of special - but you have a much broader context to assess if that's actually the case. Docs + validation would be nice though - I believe that this (especially validation) would improve the DX greatly.

[Andarist]

@devongovett any further thoughts on this PR?