Event API: follow up fixes for FocusScope + context changes #15496
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.0%, gzip: -0.0% Details of bundled changes.Comparing: 1eb2b89...503984a react-dom
react-events
Generated by 🚫 dangerJS |
necolas
approved these changes
Apr 25, 2019
necolas
reviewed
Apr 25, 2019
necolas
reviewed
Apr 26, 2019
There was a problem hiding this comment.
I tried to use FocusScope around the Pressables in my sandbox (using the latest React builds) and it crashed when focus moved into the scope's subtree with the following error.
Uncaught Invariant Violation: addRootEventTypes() found a duplicate root event type of "focus". This might be because the event type exists in the event responder "rootEventTypes" array or because of a previous addRootEventTypes() using this root event type.
| if (props.restoreFocus) { | ||
| state.nodeToRestore = document.activeElement; | ||
| state.nodeToRestore = context.getActiveDocument().activeElement; |
There was a problem hiding this comment.
I think we'll need to do something like check for blur events that have a relatedTarget within the event component subtree, that way we can know the last element focused (blurEvent.target) before focus moved into the FocusScope instance. This should give the expected result in cases where the FocusScope is mounted well before the focus moves into it.
There was a problem hiding this comment.
Sounds good. We should do this later at some point.
| if (props.restoreFocus && state.nodeToRestore !== null) { | ||
| ): void { | ||
| if ( | ||
| props.restoreFocus && |
There was a problem hiding this comment.
Perhaps we could both simplify the responsibilities of this component and make it more flexible for users if we instead had a callback API so we can leave it up to people to shift the focus themselves, in case they need custom behaviour.
const onFocusIn = (event) => {
// if there's a cancel button in the scope, move focus there instead of to the first focusable element
if (cancelRef.current != null) {
focusElement(cancelRef.current);
}
};
const onFocusOut = (event) => {
if (event.target != null) {
// restore focus if menu open-button if it is still open
focusElement(event.target);
} else {
// shift focus to menu open-button if menu auto-collapsed when modal opened
focusElement(fallbackElement);
}
};
<FocusScope onFocusIn={onFocusIn} onFocusOut={onFocusOut}>There was a problem hiding this comment.
Sounds like a feature I was going to add anyway. I just added this to add a simple autofocus for modal cases.
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 PR contains some follow up fixes as per comments in #15487 from @giuseppeg. This also adds a new helper method for getting the current active document
context.getActiveDocument, which removes a bunch of code in places. I also addedcontext.isTargetWithinEventResponderScopeandcontext.requestResponderOwnershipto deal with ownership control of the same responder.Ref #15257