[react-interactions] Ensure onBeforeBlur fires for hideInstance #18064
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
facebook-github-bot
added
CLA Signed
React Core Team
trueadm
commented
Feb 18, 2020
| function instanceContainsElem(instance: Instance, element: HTMLElement) { | ||
| let fiber = getClosestInstanceFromNode(element); | ||
| while (fiber !== null) { | ||
| if (fiber.tag === HostComponent && fiber.stateNode === element) { |
There was a problem hiding this comment.
It might be a bit strange that we expose Fiber details in the host config, but we do this in other places.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Details of bundled changes.Comparing: 9def56e...c13297b react-dom
ReactDOM: size: 0.0%, gzip: 🔺+0.3% Size changes (experimental) |
gaearon
reviewed
Feb 18, 2020
There was a problem hiding this comment.
I'm concerned we're adding more to an already deprecated codepath — which might make it harder to get rid of it later because it would lead to regressions. I'm also not sure if this is a hot path or not. I guess not because it only happens for the receded state?
Details of bundled changes.Comparing: 9def56e...c13297b react-dom
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
trueadm
force-pushed
the
onbeforeblur-hideinstance
branch
from
48ed570 to
a393d91
Compare
necolas
reviewed
Feb 18, 2020
| if (enableDeprecatedFlareAPI && selectionInformation) { | ||
| const focusedElem = selectionInformation.focusedElem; | ||
| if (focusedElem !== null && instanceContainsElem(instance, focusedElem)) { | ||
| dispatchBeforeDetachedBlur(((focusedElem: any): HTMLElement)); |
There was a problem hiding this comment.
Do we want to dispatch this event for the new event system too?
There was a problem hiding this comment.
We will eventually.
| @@ -336,6 +338,68 @@ describe.each(table)('FocusWithin responder', hasPointerEvents => { | |||
| expect.objectContaining({isTargetAttached: false}), | |||
| ); | |||
| }); | |||
|
|
|||
| it.experimental( | |||
There was a problem hiding this comment.
These kinds of unit tests that involve the scheduler don't feel like they belong in the responders to me. Doesn't really matter right now since all this is going away eventually. But it makes me think about how we decide where the boundaries lie, as did your PR that rearranged the DOM event plugin system and its tests for the better
There was a problem hiding this comment.
Yeah, I thought that. It's best if this code moved out of Flare soon.
necolas
approved these changes
Feb 18, 2020
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.
As part of the React Flare system, we have the notion of a React specific event called
onBeforeBlur. This is meant to triggered just before a managed node of React gets removed or hidden from the UI. We already support the case where this fires before we removed the node, but we didn't have a codepath or support for when this can occur during Suspense. During Suspense, wedisplay: nonethe content when showing the fallback, which means that the React Flare focus systems fail to know of the fact thatbluris about to be triggered, which is required to properly handle focus management logic. This PR adds that code path in, soonBeforeBlurcorrectly fires during Suspense too.On a separate node, we should move this logic and other existing
onBeforeBlurlogic out of the Flare flag sometime and make it its own core feature at some point.