fix(focus-trap): don't crash when unpause finds no focusable element#3220
Open
s-kthr wants to merge 1 commit into
Open
fix(focus-trap): don't crash when unpause finds no focusable element#3220s-kthr wants to merge 1 commit into
s-kthr wants to merge 1 commit into
Conversation
Nesting overlays (e.g. a popover inside a dialog) share a single trap stack. Deactivating the inner trap calls deactivateTrap(), which automatically calls unpause() on the trap now on top of the stack as internal housekeeping. If that trap's container has no connected focusable element at that instant (a transient DOM state, not a real misconfiguration), getInitialFocusNode() threw unconditionally and crashed the app even though the outer trap was still legitimately active. addListeners()/getInitialFocusNode() now take an isUnpausing flag so this internal auto-refocus path returns false (skip focusing) instead of throwing, mirroring the existing initialFocus: false sentinel. Explicit activate() calls are unaffected and still throw on a genuine "no focusable element" misconfiguration.
🦋 Changeset detectedLatest commit: 08b154c The changes in this PR will be included in the next version bump. This PR includes changesets to release 86 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
When two
FocusTrapinstances share a trap stack (the default), deactivating the inner one (e.g. a popover or menu) automatically callsunpause()on whichever trap is now on top of the stack (e.g. a dialog) as pure internal housekeeping — seeactiveFocusTraps.deactivateTrap():unpause()callsupdateTabbableNodes()and thenaddListeners(), which synchronously callsgetInitialFocusNode()to refocus (whendelayInitialFocus: false, this happens inline; with the defaultdelayInitialFocus: trueit happens a tick later inside thesetTimeout, but the underlying issue is the same). If, at that exact instant, the outer trap's container has no connected focusable element — a transient state that can legitimately happen while a nested overlay is being torn down mid re-render —getInitialFocusNode()throws unconditionally:This happens even though the outer trap (e.g. the dialog) is still fully active and its content is still visible on screen — it's not a real "no focusable element" misconfiguration, just a momentary DOM state during the inner trap's teardown.
Current behavior (updates)
Nesting focus traps (e.g. a
Popover/Menuinside aDialog, all sharing the default trap stack) can throw an uncaught error whenever the inner overlay closes, if the outer trap's container has zero connected focusable elements at that exact moment. This surfaces in real apps as nested Dialog → Popover → Menu UIs (e.g. a template picker with an edit menu inside a popover inside a message-composer dialog) throwing this error every time the innermost overlay closes, even though the outer dialog stays open and fully functional.Minimal repro (see the added test): activate an outer trap configured with
fallbackFocus: () => someElement, activate an inner trap (pausing the outer one via the shared stack), then removesomeElementfrom the DOM before deactivating the inner trap. Deactivating the inner trap throws.New behavior
unpause()'s own automatic re-focus (as opposed to an explicitactivate()call) no longer throws when it can't find a focusable element. Instead, it now returnsfalsefromgetInitialFocusNode()and skips focusing — the same sentinel the code already uses for an explicitinitialFocus: false.activate()is unaffected: calling.activate()on a trap with no focusable element (a genuine misconfiguration) still throws exactly as before.Implementation:
addListeners()andgetInitialFocusNode()take anisUnpausingflag, set only by the call fromunpause(), so only that internal housekeeping path is softened.Is this a breaking change (Yes/No):
No. The only externally observable difference is that
unpause()'s automatic re-focus no longer throws in a situation where it previously crashed;activate()'s validation behavior (the documented/tested "throws when there is no valid focus target" case) is unchanged.Additional Information
packages/utilities/focus-trap/tests/focus-trap.test.ts) that reproduces the exact reported stack trace shape (getInitialFocusNode←addListeners←unpause←deactivateTrap←deactivate) using two traps on the shared trap stack.mainsource (throws with the reported error message) before the fix, and passes after.packages/utilities/focus-traptests (16) still pass, plus the new one (17 total).tsc --noEmitandeslint srcare clean for the package.@zag-js/focus-trap(patch).