-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Add a higher order component to constrain Tab keyboard navigation. #6987
Conversation
/* eslint-enable jsx-a11y/no-static-element-interactions */ | ||
} | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure a similar mechanism is also implemented elsewhere. Maybe NavigableContainer? Should we consolidate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about naming. withConstrainedTabbing
would probably be my loosely-held preference.
Otherwise, this looks good 👍
Consistency on creation of higher-order components
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
withFocusContain
doesn'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
Popover
component 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.tabbables
implementation 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.tabbables
returns 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