Add a higher order component to constrain Tab keyboard navigation.#6987
Merged
Add a higher order component to constrain Tab keyboard navigation.#6987
Conversation
youknowriad
reviewed
May 28, 2018
| /* eslint-enable jsx-a11y/no-static-element-interactions */ | ||
| } | ||
| }; | ||
| }; |
Contributor
There was a problem hiding this comment.
I'm pretty sure a similar mechanism is also implemented elsewhere. Maybe NavigableContainer? Should we consolidate?
Contributor
Author
There was a problem hiding this comment.
Worth also considering the name should clarify this is something different from what other components do, for example NavigableContainer.
🙂 Yes and no. The mechanism in NavigableContainer is used also for elements with tabindex="-1" like menu items.
This was referenced May 30, 2018
Merged
aduth
approved these changes
Jun 26, 2018
Member
aduth
left a comment
There was a problem hiding this comment.
I don't feel strongly about naming. withConstrainedTabbing would probably be my loosely-held preference.
Otherwise, this looks good 👍
Contributor
Author
Consistency on creation of higher-order components
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR borrows code from #6261 special thanks to @xyfi and aims to introduce a new Higher Order Component to constrain keyboard navigation with the Tab key.
Constraining tabbing is necessary for accessibility of various UI, for example modal dialogs, "popovers" and the like, where keyboard users must execute a specific task without navigating away and loosing context. For more details please refer to #5242.
TODO:
1
Consider to rename the component. The current name
withFocusContaindoesn't accurately describes what this component does. Actually, it's not about focus: it's about tabbing. Wondering if a different name would be beneficial, for example:withTabbingContain?
withTabbingContained?
withTabbingConstrained?
Worth also considering the name should clarify this is something different from what other components do, for example
NavigableContainer.2
For now, and for testing purposes, this new HoC wraps the
Popovercomponent when it already has managed focus. Not sure this is a good assumption or if it's preferable to explicitly add this behavior via a specific prop. I guess this can always be done later in a new iteration.3
There's a problem with the
focus.tabbablesimplementation when the tabbables are exclusively radio buttons. This is clearly visible in the "Visibility setting" (see also screenshot below). Radio buttons are technically tabbables but it's actually possible to tab into just one of them. In this case, the implementation fails asfocus.tabbablesreturns a collection of 3 elements but only one of them can be "tabbed" into:Will open a separate issue.
For now, I'd recommend to test the behavior in some important pieces of the UI, most notably:
To test:
Fixes #5242