[popups] Fix return focus when reference disconnects#4655
[popups] Fix return focus when reference disconnects#4655atomiks merged 3 commits intomui:masterfrom
Conversation
atomiks
added
component: menu
commit: |
Bundle size
PerformanceTotal duration: 1,476.39 ms 🔺+130.91 ms(+9.7%) | Renders: 53 (+0) | Paint: 2,259.73 ms 🔺+185.20 ms(+8.9%)
...and 7 more. View full report Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
atomiks
requested review from
LukasTy,
colmtuite,
flaviendelangle and
michaldudak
as code owners
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
PR #4655 Review Summary
PR #4655 — [floating-ui] Fix return focus when reference disconnects · 2 files, +63/−3
Files changed:
- packages/react/src/floating-ui-react/components/FloatingFocusManager.tsx — +6 / −3 (logic fix)
- packages/react/src/dialog/root/DialogRoot.test.tsx — +57 / −0 (new Chromium-only regression test)
What it does: Opening a dialog from a Menu.Item that also renders Dialog.Trigger causes the menu to close, which unmounts the dialog's direct trigger. FloatingFocusManager then treats that disconnected reference as the return-focus target and gives up, leaving focus on <body> after the dialog closes. The fix falls back to getPreviouslyFocusedElement() (a WeakRef list that already filters connected elements) when domReference?.isConnected is false. The same isConnected guard is applied to the non-boolean returnFocus branch, fixing the same latent bug there too.
Critical Issues
None.
(The test-analyzer flagged a potential JSX bracket mismatch in the new test as a blocker. I re-fetched the raw diff and verified: <Menu.Root> closes correctly before <Dialog.Root> opens as a sibling inside <React.Fragment>. False alarm — dismiss.)
Important Issues
Test coverage gap on the non-boolean returnFocus branch — code-reviewer · pr-test-analyzer
The diff also changes the non-boolean branch at FloatingFocusManager.tsx:790: const fallback = domReference || … → domReference?.isConnected ? domReference : …. This fixes the same class of latent bug (disconnected domReference used as fallback when returnFocus is a ref/function that resolves nullish), but neither the added Dialog-level test nor the existing FloatingFocusManager tests at lines 1634-1700 or 347-453 exercise it. A small unit test in FloatingFocusManager.test.tsx passing returnFocus={someRef} (with someRef.current = null) and detaching the reference would cover it.
Suggestions
- Combine the split
waitForpair (DialogRoot.test.tsx:1306-1311) into onewaitForthat asserts both menu-closed and dialog-open. Avoids accidental-pass diagnostics if a regression interleaves the transitions. Low priority. —pr-test-analyzer - Commit/PR message could note the bonus fix in the non-boolean branch — it's not just the default-
returnFocus=truecase being fixed; the ref/function case gets the sameisConnectedguarantee. Worth mentioning for reviewers / changelog. —code-reviewer
Strengths
- Surgical, minimally scoped 5-line fix. When
domReferenceis connected (the common path), return value is unchanged — zero risk to happy paths. —code-reviewer getReturnElement()is confirmed to be the single source of truth for return-focus targets inFloatingFocusManager.tsx. Other.focus()call sites (lines 510/519/533 inrestoreFocus, 926/931/946/954 in focus guards) are orthogonal and unaffected. No other unfixed sites of the same class. —code-reviewer- Test uses realistic composition —
Menu.Item render={<Dialog.Trigger handle={dialogHandle} />}— reproducing the production pattern rather than a synthetic unmount. Fidelity matches the reported bug exactly. —pr-test-analyzer - Test is not redundant with the existing FloatingFocusManager unit test at lines 1634-1700: that one protects the internal focus-fallback contract, this one protects the user-facing Menu-triggers-Dialog integration. Complementary coverage. —
pr-test-analyzer - AGENTS.md compliance is clean:
it.skipIf(isJSDOM)correct, no redundantflushMicrotasks, Vitest-only APIs, imports already present, file co-located with source, nouseTimeout/useAnimationFrame/etc. rules triggered by the diff. — both agents expect(menuTrigger).toHaveFocus()is tight enough — it fails for<body>, other elements, or null focus. No silent-pass risk. —pr-test-analyzer
Semantic-equivalence verification (boolean branch)
domReference state |
Pre-PR return | Post-PR return | Same? |
|---|---|---|---|
| connected | domReference |
domReference |
yes |
| defined, disconnected | null |
getPreviouslyFocusedElement() || null |
No — intentional fix |
| null/undefined | getPreviouslyFocusedElement() gated by isConnected |
getPreviouslyFocusedElement() || null |
yes (fallback is already connected-or-undefined via internal clearDisconnectedPreviouslyFocusedElements) |
Only divergence is the intended fix. No unintended drift.
Review scope
- Ran
code-reviewerandpr-test-analyzerin parallel. - Skipped
silent-failure-hunter(no error-handling changes),comment-analyzer(no comments),type-design-analyzer(no new types),code-simplifier(diff is already minimal).
Recommended Action
- Not blocking. The fix is correct, narrowly scoped, and well tested at the Dialog integration layer. It's mergeable as-is.
- Nice-to-have follow-up: add a FloatingFocusManager unit test for the ref/function
returnFocusbranch with a detached reference, so both post-PR branches have explicit regression coverage. Could be a separate small PR.
Local verification commands
pnpm test:chromium DialogRoot --no-watch— runs the new regression test.pnpm test:chromium FloatingFocusManager --no-watch— ensures existing return-focus tests still pass.- Manual smoke: deploy preview — open a menu-triggered dialog and confirm focus returns to the menu trigger on Escape.
atomiks
changed the title
atomiks
added
scope: all components
2 participants
Opening a dialog from a menu item can disconnect the dialog's direct trigger when the menu closes. The focus manager was treating that disconnected reference as the default target and then giving up, so closing the dialog left focus on the body. This falls back to the last connected focus target instead, allowing focus to return to the root menu trigger.
Changes