Keep focus where the user moved it when a menu finishes closing

[loganj]

🤖

Summary

• Pressing Escape on a menu and then clicking another control right away could lose that click. For example: right-click a channel in the sidebar, press Escape, then click the message box. When the menu finished closing, it pulled focus back to the channel row, so typing did not go into the message box. If the click opened another menu instead, that menu could close again at once.
• The cause: when a menu finished its closing animation, it moved keyboard focus back to the control that opened it, even though you had already moved on. A newly opened menu closes when focus leaves it.
• Now a closing menu, popover or dialog returns focus only if focus is still inside it or nowhere in particular. If you already moved focus somewhere else, focus stays there.

Details

• The popup library (Base UI) already does this check for its default behavior. It skips the check when the app names an exact place to return focus (finalFocus), which the channel row menu, profile menu, search and some dialogs do.
• ui/finalFocus.ts adds the check back for those cases. The shared Menu, Popover and Dialog components use it, so every caller gets the fix. No caller changes.
• When nothing else has focus, Escape still returns focus to the control that opened the popup, as before.
• This also removes a source of flaky browser tests: tests that press Escape and then click something next could fail in the same way.
• New test: tests/browser/menu-dismiss.spec.mjs slows the closing animation so the next click lands before it ends. Without the fix it fails most runs in both Chromium and WebKit.

[wesbillman]

Star Lord — automated source review via Wes’s account

Reviewed head 35dd419c37420c894ab20f4487da1894cd1101c4 against base 1108ab64cf232acfc0c8087db54b74ce73098bfe (PR merge base 3899de3e241f53bb138b46ae291479108ef3986e).

One actionable regression-test finding, inline. I did not identify a concrete production-code defect in the reviewed paths. The shared guard is consistent with the intended focus handoff; I traced the Base UI 1.8.0 return-focus cleanup, ref forwarding, and supported callers. Groot independently reviewed caller/handoff behavior; that lane is complete.

Existing evidence, not tests run by this reviewer

Hosted run 36477449405 succeeded. Browser artifact metadata records merge tree 90bb2bf78e95810fd623ef72a0a8c2e6160af7e1, combining the head/base above, with clean inputs for the new case. The new journey passed once per engine (Chromium 5.544s; WebKit 7.903s), with one Alpha and one Beta message. Native focus/exit-transition behavior justifies a browser case; one scenario was added, none removed. These passes do not establish the required race ordering.

Timing snapshot inspected: 818 functional browser instances passed, plus 7 measurements; longest shard wall times were 465.4s Chromium / 719.1s WebKit, summed test execution 2448.8s / 3828.5s. Slowest journey was a WebKit nested-replies case (59.8s), with that file totaling 243.9s; measurements were led by Chromium cursor paging (83.5s). Vitest: 4,885 passed, 347.3s wall / 579.9s summed execution. No comparable before/after run was established, so these are observations, not a performance-regression verdict.

Limits: source-only review; no local tests, builds, dependency installs, PR-code execution, or app launch. Hosted Linux browser evidence is not native-app acceptance; the documented local-only WebKit cases are outside that CI selection. This is a non-blocking COMMENT, not approval or merge authorization.

[wpfleger96]

I found two issues I think we should address before approval: the closing-popup exception also matches unrelated collapsed content, and the regression test does not guarantee the ordering it is meant to cover. The second finding matches the existing review thread. I checked the source against Base UI 1.8.0 and inspected the passing Chromium/WebKit CI results; I did not run a local browser or mutation check.

[wpfleger96]

I re-reviewed f416250 against my previous findings. Both are addressed: the exception now requires a closing popup role rather than any collapsed ancestor, and the regression test pauses the actual exit transition, moves focus while the menu remains mounted, then releases it and checks focus after removal. The collapsed-container case and ordinary Escape return are both covered, with release in finally.

I also retraced the shared Menu/Popover/Dialog integration and Base UI 1.8.0 focus/animation cleanup; I did not find a new blocking issue in these fixes. This is a source re-review, not an independent browser run. The fail-before/pass-after repetitions are the author's reported evidence in the review replies. CI was still running when I checked, so this approval does not attest that the current checks have all passed.

Nit: the PR description still describes the old slowed-animation test and intermittent failure. It should describe the explicit transition gate and current evidence instead.

[wesbillman]

Star Lord’s automated source review (via Wes’s account)

No new actionable findings in this bounded follow-up. Reviewed head f416250425a11668e3fbc4a3179a3aa13af085ae against API base 1108ab64cf232acfc0c8087db54b74ce73098bfe, comparing the two-file revision since the prior review at 35dd419c37420c894ab20f4487da1894cd1101c4.

• The prior ordering finding is addressed in source. menu-dismiss.spec.mjs:7–47,68–80,103–113 registers the exit observer before dismissal, pauses the real closing transitions, observes the pause, checks focus while the popup is still mounted, releases it, and checks again after removal. Cleanup releases in finally; the ordinary Escape-return assertion remains. It no longer depends on Playwright beating a 400ms delay.
• finalFocus.ts:16–22,59–65 limits the closing-tree exception to popup roles, so an unrelated collapsed disclosure no longer authorizes stealing focus. The existing shared Menu/Popover/Dialog ownership, explicit-target resolution, and boolean/default behavior remain unchanged. The added collapsed-container scenario exercises the narrowed selector.
• Browser testing remains justified by native focus and CSS-transition completion. This revision adds no separate test cases or app startups; it extends the existing case, using one Alpha and one Beta message. The commit records fail-before/pass-after controls (10 runs per engine for each variant); these are author-reported, not independently reproduced here. All three PR commits carry DCO trailers.

Existing hosted evidence, not tests run by this reviewer

One CI snapshot showed JavaScript, Rust/tool integration, all three Chromium shards, browser measurements, security and DCO successful; WebKit shards were still running and Windows native validation was skipped. Available artifacts include a later-completed WebKit 1/3 report, but not the changed test’s WebKit result. No polling or all-green claim.

The Chromium focus journey passed once in 3.172s. Its artifact records clean synthetic merge 6ccb4f926e6b1e7789c2ba7ba5e1364c58d99361, whose parents are this head and newer main 3a19fa43075283423c88a68d4a1362fade28ad3e—not the API base above. All six feature-file blobs match the reviewed head; this remains merge-tree evidence, not a raw-head run.

Available timing reports: Chromium 414/414 cases, longest shard 504.0s wall, 2447.5s summed execution; available WebKit 1/3 140/140, 597.8s wall / 1165.4s execution; measurements 7/7, 148.9s wall / 141.1s execution; Vitest 4936/4936, 356.2s wall / 591.6s execution. Slowest available browser case was Chromium cursor paging (75.4s); slowest functional file aggregate was Chromium message-navigation (141.0s). These are partial-run observations, not a controlled performance comparison.

Validation limits: source-only, with 26 pinned Git-blob/SHA-256-verified source extracts and no dirty worktree inputs. No PR code, tests, builds, installs, app launches or live workflows executed. Current-head native focus behavior, both-engine mutation controls and human acceptance were not independently verified. This is a non-blocking COMMENT, not approval or merge authorization.