Event API: Add FocusScope surface
#15487
Merged
Conversation
This file contains 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
|
ReactDOM: size: -0.1%, gzip: -0.0% Details of bundled changes.Comparing: 3f058de...410a786 react
react-dom
react-test-renderer
react-reconciler
react-events
Generated by 🚫 dangerJS |
necolas
approved these changes
Apr 24, 2019
giuseppeg
reviewed
Apr 25, 2019
| type === 'textarea' || | ||
| type === 'input' || | ||
| type === 'object' || | ||
| type === 'select' |
There was a problem hiding this comment.
The list is missing a few elements: iframe, embed and contenteditable.
Should this component skip subtrees that have the inert attribute set on the root?
There was a problem hiding this comment.
We can deal with inert in a future PR. It's a bit more involved as what if a parent tree has inert and its sub-trees have the FocusScope?
There was a problem hiding this comment.
Agreed that is not straightforward, I guess that FocusScope should noop when an ancestor has the inert attribute i.e. (force) disable trap and restore.
I am afraid that, because of portals, React needs to polyfill inert and can't rely on the native implementation.
There was a problem hiding this comment.
It would be pretty great if React polyfilled inert anyway, as it is only supported in Chrome behind a flag.
There was a problem hiding this comment.
WICG have a polyfill https://github.com/WICG/inert. There doesn't seem to be vendor agreement on whether to proceed with this attribute or use a different approach. I don't think we need to be too concerned about it here at this stage
| state: FocusScopeState, | ||
| ) { | ||
| if (props.restoreFocus) { | ||
| state.nodeToRestore = document.activeElement; |
There was a problem hiding this comment.
For this component to work inside of iframes this should be the ownerDocument of the current node.
There was a problem hiding this comment.
Thanks for the feedback. I addressed these points in #15496 :)
|
Work around the Events API seems to be even more exciting for me as a developer than Concurrent Mode etc, and I'm stoked about the latter 🔥 . Great work! |
|
@trueadm Will the focusscope remain only a react-dom thing or will it be expanded also over react native eventually? I would love that! Thanks for making this happening to everyone contributing! |
|
@mattiamanzati it’s very early to say but that’s definitely our long term ambition with this new event API. |
2 tasks
|
@trueadm FWIW back then I forgot to mention that in order for elements to be focusable they should be visible. At work I do this: export function getFocusableElements(node: HTMLElement): Array<HTMLElement> {
return Array.prototype.filter.call(
node.querySelectorAll(FOCUSABLE_ELEMENTS_SELECTOR),
child =>
!(child.inert || child.hasAttribute('inert') || child.getAttribute('tabindex') === '-1') &&
!!(child.offsetWidth || child.offsetHeight || child.getClientRects().length),
)
} |
|
We don’t want to use query selectors. They don’t work with suspense and portals correctly. |
|
@trueadm oh yea definitely, that part is not relevant. The filter function filters out non visible elements this way |
|
sorry, noob question, I see this is merged to master, does that mean it is this something we can expect to be in react 17 but won't be in any 16.x version? |
|
@ryankshaw No, this won't be in React 17. It's not even in React's master branch now and has since been removed as it was only a short experiment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This adds a new
FocusScopeevent API surface/responder. For now,FocusScopeis a very basic implementation that only supportsautoFocus,restoreFocusandtrap. When you tab in a trappedFocusScope, you're limited to the scope of the event component. Pressing tab on the last focusable element of aFocusScopethat is trapped will result in the focus moving to the first focusable element of theFocusScope. Furthermore, elements that aren't focusable (i.e. withtabIndexset to-1) will be skipped in all cases and elements nested in portals will traverse as expected via the React fiber tree rather than the DOM tree.Example of
FocusScopewith focus trapping and a few other props:Ref #15257