-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[TrapFocus] Capture nodeToRestore via relatedTarget #26696
Conversation
Details of bundle changes (experimental) @material-ui/lab: parsed: -0.08% 😍, gzip: -0.10% 😍 |
Was first suspicion but breaks React 17
2574ef7
to
9241209
Compare
@@ -338,7 +316,7 @@ function Unstable_TrapFocus(props) { | |||
}; | |||
|
|||
const handleFocusSentinel = (event) => { | |||
if (!activated.current) { | |||
if (nodeToRestore.current === null) { |
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.
This handler wasn't covered by tests. The behavior of this handler is now covered in test/e2e/fixtures/Unstable_TrapFocus/DefaultOpenLazyTrapFocus.tsx
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.
Previous implementation would break with React's planned Offscreen API (or weaker: breaks with Strict Effects in React 18) since we capture the nodeToRestore during render.
@eps1lon The changes are definitely better from the complexity and bundle size perspectives.
React 18, I fail to follow the causality. Won't a trap focus always be onscreen when active? If we had a reproduction, this would be great.
cc @mui-org/x it solve our cross-document issue: https://github.com/mui-org/material-ui-x/blob/9dd5cf94061d1733566b7b66982fae554ebcf242/packages/grid/_modules_/grid/components/panel/GridPanelWrapper.tsx#L28
Yeah, I haven't looked into if we'll ever be affected by it. But I don't think it's useful to think about it since React will have these semantics going forward. Considering we keep existing behavior and add React 18 compat by removing code, I'm happy with this PR. Even if the change is only required for tests. |
Previous implementation would break with React's planned Offscreen API (or weaker: breaks with Strict Effects in React 18) since we capture the
nodeToRestore
during render. However, effects might run and cleanup without render running again e.g. when no props/state/context changes but the component is hidden and revealed again.We captured it during render because in effects other compoents might've already moved focus into the TrapFocus tree i.e. effects were too late.
What we can do instead (now that React 17 implements
event.relatedTarget
in IE 11) is capture thenodeToRestore
when focus moves into theTrapFocus
tree. We already did this but only sometimes. Probably because we already hit issues with capturing the nodeToRestore during render.