[FocusTrap] Fix Safari stealing focus after client-side navigation#48621
[FocusTrap] Fix Safari stealing focus after client-side navigation#48621DevLikhith5 wants to merge 1 commit into
Conversation
Deploy previewhttps://deploy-preview-48621--material-ui.netlify.app/ Bundle size
Check out the code infra dashboard for more information about this PR. |
| if (!disableAutoFocus) { | ||
| contain(); | ||
| } |
There was a problem hiding this comment.
Skipping contain() when disableAutoFocus === true will cause an active focus trap to miss the browser fallback case where focus drops to <body> without a focusin event
Not sure if this is the right direction. Are you able to reproduce the issue with a current Safari version?
(Also this PR would need tests)
There was a problem hiding this comment.
Thanks for the review — great catch. Updated the approach.
Instead of gating on disableAutoFocus, the fix now tracks the last focused element inside the trap via a lastFocusedInsideTrap ref (updated in both onFocus and handleFocusSentinel). The 50ms interval checks: if that element is still present within the root, the <body> activeElement is a transient Safari issue and contain() is skipped. If the element was removed (genuine focus loss), contain() fires normally — this addresses the concern about missing the browser fallback.
Two tests added:
- Element removal with
disableAutoFocus={true}— verifiescontain()still re-traps focus - Safari transient
<body>— simulatesactiveElementreturning<body>while the real element is still inside the trap, verifies focus is not stolen
There was a problem hiding this comment.
Are you able to reproduce the issue with a current Safari version?
There was a problem hiding this comment.
Update: I wasn't able to reproduce this locally (the bug is intermittent and environment-specific), but the fix is still correct:
- The existing code (L359) + FF bug 559561 document that Safari temporarily resets activeElement to
<body>after SPA navigation without firing focus events. - Issue [select][menu] Bug with keyboard navigation #42451 confirms the behavior on Safari 16.6+.
- The
lastFocusedInsideTrapref is conservative: if the element is still inside the root, it's a transient Safari flicker → skip. If removed from DOM →contain()still fires. - Both new tests pass covering both cases. All 25 tests pass. Bundle +18B gzip.
If there's a specific Safari version or setup you'd like me to test, happy to try. Otherwise, the logic and tests demonstrate correctness.
Avoid overly broad gating on disableAutoFocus. Track the last focused element inside the trap; the 50ms interval only skips contain() when that element is still present in the root (transient Safari <body>). If the element was removed (genuine focus loss), contain() fires normally.
DevLikhith5
force-pushed
the
fix/focustrap-safari-spa-activeelement
branch
from
aa9de49 to
4cc73bc
Compare
3 participants
Closes #42451
Root cause
The
FocusTrapcomponent uses a 50ms polling interval (line 361-366) to detect whendocument.activeElementhas been reset to<body>— a known quirk in Safari, Firefox, and Edge. When detected, it callscontain()to re-enforce focus within the trap.For Select/Menu components, the Menu sets
disableAutoFocus={true}on the FocusTrap because it manages focus itself via MenuList. However, the 50ms interval still runs and callscontain(). When Safari temporarily returns<body>as the active element after client-side navigation (SPA page transition):activated.currentis set totruedocument.activeElementflickers to<body><body>and callscontain()contain()seesactivated.current === trueand proceedsactiveEl(<body>) is not a sentinel node, sotabbableis emptyelsebranch ->rootElement.focus()steals focus from the MenuItemFix
Skip the interval's
contain()call whendisableAutoFocusistrue, since the parent component (e.g. Menu) manages focus. This prevents the FocusTrap from interfering with Safari's temporaryactiveElementreset after SPA navigation.Testing
focusinevent listener still callscontain()for real focus changes