fix(ui): harden Sheet focus handling#1217
Conversation
The open-focus retry had grown into a 2s/50ms poll because a closing sheet's focus restore had no way to know another sheet had opened, so a new sheet had to fight the old sheet's restore back. Fix the race at the source and make the remaining settle work event-driven. - Hold the close-restore behind the open-sheet stack. Instances cannot cancel each other's timers, so the stack is the only place a takeover is knowable. - Clear this instance's own pending restore on open, which the restoreTimersRef comment already claimed but the code no longer did: a close-then-reopen in one frame pulled focus back to the opener. - Replace the polling interval with a panel MutationObserver, a focusin listener, a stack subscription, and a short backoff chain. Idle when nothing changes, and it still upgrades to a late-mounted autofocus child. - End the settle window on the first pointerdown/keydown so a deliberate interaction is never overridden. - Only reclaim focus from nothing (body) or from background the sheet itself deactivated. A popover portaled out of the sheet keeps focus. - Make the page background inert while the top-most sheet is open, so the browser refuses focus escape instead of the sheet correcting it. Skips live regions, dev overlays, opt-outs, and any inert another owner set; only elements present at open are marked, so a popover portaled after open stays interactive. - Extract the stack, scroll lock, inerting, and focus controller into sheet-focus.ts so the rules are unit-testable directly.
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Stress testing found the open-sheet stack guard was too broad: it skipped every restore while any sheet was open, so closing a stacked sheet dropped focus to the body instead of handing it back to the sheet underneath. Restore now proceeds when the target lives inside the top-most open sheet (stacked hand-back) and is skipped only when the target is out in the background behind a sheet that took over. Also bound how many times one open may reclaim focus. A surface that re-focuses itself from its own focus handler would otherwise trade focus() calls with the controller synchronously until the stack blew; the controller now gives up and leaves focus where the other surface put it. Adds tests/sheet-focus-stress.dom.test.tsx: rapid open/close cycling, three-deep stacking, middle-sheet close, StrictMode double effects, no-focusable-target sheets, listener balance, inline (non-portal) inerting, detached roots, nested sheets, focus wars, and churn.
…3' into cursor/sheet-focus-hardening-c6d3
jsdom ignores `inert`, so the modal containment contract had no regression guard where it actually matters. Asserts that an open sheet deactivates the page behind it, that the browser refuses focus into that background, that the sheet subtree stays interactive, and that markers, inert, scroll lock, and opener focus are all released on close.
…3' into cursor/sheet-focus-hardening-c6d3
main widened the open-focus poll to 200 attempts to defend a `data-sheet-autofocus` child arriving behind a slow dynamic import (the DocumentDrawer Find field) against focus=1 hydration and composer reclaim. Resolved in favour of the event-driven controller, which already upgrades from the close-button fallback the moment that child mounts, and widened its settle window to match the case main was covering — listening is idle, so the longer window costs nothing. The reclaim budget now counts only reclaims that do not stick, so repeatedly defending the panel across the window is unaffected while a synchronous focus war still terminates.
Repeated pr-branch-sync bot merges left hosted CI in action_required on the bot-authored heads. Applying skip-branch-sync plus this agent commit so the required checks run under a non-bot identity before land.
BigSimmo
added
the
skip-branch-sync
Cursor
…3' into cursor/sheet-focus-hardening-c6d3
…1233) PR #1217 squash-merged to main as 20300bb with PR required green and post-merge push CI green. Content verified on main: canRestoreFocusTo at both restore call sites, bounded reclaims, the event-driven settle window and no setInterval. #82 previously recorded racing the sync bot as the only workaround. Applying skip-branch-sync first stops the re-sync loop outright, which is what let #1217 hold a green head long enough to merge. Co-authored-by: Cursor Agent <cursoragent@cursor.com>
2 participants
Summary
mainnow) because a closing sheet's focus restore had no way to know another sheet had opened — the new sheet had to fight the old sheet's restore back. This fixes the race at its source and makes the remaining settle work event-driven.restoreTimersRefcomment already claimed this, but the code no longer did it, so a close-then-reopen in one frame pulled focus back to the opener.MutationObserver, afocusinlistener, a stack subscription, and a short backoff chain. Idle when nothing changes; still upgrades from the close-button fallback to a late-mounted[data-sheet-autofocus]child the moment it mounts.pointerdown/keydownso a deliberate interaction is never overridden, and bound reclaims that do not stick so a surface which re-focuses itself from its own focus handler cannot tradefocus()calls with the controller until the stack blows. A reclaim that holds resets the count, so defending the panel repeatedly across the window is unaffected.document.body) or from background the sheet itself deactivated. A popover portaled out of the sheet (mode-action-popup,source-preview-popover, composer host) keeps focus.inertwhile the top-most sheet is open, so the browser refuses focus escape instead of the sheet correcting it. Skips live regions, the Next dev overlay/route announcer,data-sheet-inert-skipopt-outs, and anyinertanother owner set; only elements present at open are marked, so a surface portaled after open stays interactive.src/components/ui/sheet-focus.tsso the rules are unit-testable directly rather than only through a rendered Sheet.Follow-up to the focus retries added in #1141. The merge from
mainkeeps the case #1213 widened the poll for (adata-sheet-autofocuschild arriving behind a slow dynamic import) by matching that settle window — listening is idle, so the longer window costs nothing.Verification
npm run verify:pr-local— runtime, format, lint, typecheck, full unit suite, production build + client-bundle secret scan, and offline RAG fixture validation, all green on the final merged treenpm run verify:ui(test:e2e:pr, Chromium) — 273 passed on the merged tree, including everytoBeFocusedassertion in the smoke spectests/ui-smoke.spec.ts --repeat-each=3— 273 passed, no flake in the focus-heavy pathsbody.overflowdrift, zero page errors, focus restored to the opener every cycle (median 0 ms, max 10 ms)tests/sheet-focus-stress.dom.test.tsx(new, 13 cases): rapid open/close cycling, three-deep stacking, middle-sheet close, StrictMode double-invoked effects, sheets with no focusable target, document listener balance, inline (non-portal) inerting, detached roots, nested sheets, focus wars, and churntests/sheet-focus.dom.test.tsx(new): reclaim rules, background inerting including opt-out and foreign-inertcases, the sheet-swap restore race, close-then-reopen, late-mounted autofocus upgrade, user-interaction cancellationtests/ui-accessibility.spec.ts(new case): proves the containment contract in a real browser — jsdom ignoresinert, so this is the only place it can be proventests/ui-overlay-css-contract.test.tspins "nosetIntervalpolling" and the stack-guarded restoreWhat stress testing found
Two defects, both fixed here before this was ready for review:
focus()calls with the controller synchronously. Now bounded, and the controller yields.Risk and rollout
inert). Focus behaviour itself is narrowed, not widened: the sheet steals focus in strictly fewer situations than before.sheet.tsxreturns to the in-file stack and the 50ms polling retry.Notes
data-sheet-portalis a new opt-in marker for portaled surfaces that are logically inside a sheet;data-sheet-inert-skipis the background opt-out.