react-spectrum
react-spectrum copied to clipboard
adobe
Accept tabIndex in the useFocusable hook
Closes https://github.com/adobe/react-spectrum/issues/1384 , cc @ktabors
✅ Pull Request Checklist:
- [x] Included link to corresponding React Spectrum GitHub Issue.
- [x] 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).
- [x] Looked at the Accessibility Practices for this feature - Aria Practices
📝 Test Instructions:
yarn test
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?
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.