fix(web): trap focus in BottomSheet so ESC/Tab hit the sheet, not the layer under it (BUG-2130)#1006
Merged
Merged
Conversation
… layer under it (BUG-2130) BottomSheet is a role="dialog" aria-modal mobile sheet but, unlike the native-<dialog> Modal.svelte, it never moved focus into itself or trapped Tab. Two consequences, app-wide (most visible over the mobile split-pane): - ESC closed the wrong layer: focus stayed on the trigger outside the sheet, so a window-level ESC handler underneath (e.g. the collection page's pane-close) fired first and closed THAT instead of the sheet. - Tab escaped the sheet into the obscured content behind it. Fix in the shared component, mirroring Modal.svelte's behavior: - Move focus onto the panel (tabindex=-1) on open; restore focus to the trigger on close and on teardown-while-open. - Trap Tab/Shift+Tab within the sheet, reusing the pane's already-tested trap math (paneFocusables + nextTrapTarget from paneFocus.ts) so the two focus traps can't drift. The focus effect reads only `open`/`sheetEl` and writes the non-reactive `previouslyFocused`, so it can't self-invalidate (CONVE-1688). Surgical over a native-<dialog> rebuild: 11 consumers make the blast radius large, and the bug is scoped to the shared component. Converging BottomSheet onto the Modal primitive is a separate, larger refactor. Adds BottomSheet.svelte.test.ts (focus-in, Tab/Shift+Tab wrap, Escape, backdrop, focus-restore). Verified: full web suite (471) green, svelte-check clean, Codex CLEAN, and a real mobile-browser drive (focus-in, Tab + Shift+Tab trapped, ESC closes only the sheet with the item pane surviving, focus restored to the trigger). Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
…heets) Codex PR review caught an adjacent facet of the same layer-isolation bug: every open BottomSheet registers a window-level Escape/Tab handler, so when one sheet opens another (Quick Actions sheet → the mobile emoji picker's sheet, both role="dialog" BottomSheets, the inner DOM-nested in the outer), a single Escape fired both handlers and closed BOTH layers. Gate each sheet's handler on being the frontmost (innermost) open sheet: a nested child sheet renders inside our content, so a sheet that CONTAINS another open `.bs-sheet` is not frontmost and stays out. Order-independent by design — a defaultPrevented/stopPropagation check can't work here because the outer sheet's window listener is registered first and fires before the inner's. Verified at runtime (mobile): open Quick Actions → New quick action → the emoji-picker button opens a nested sheet; one Escape now closes only the picker (Quick Actions survives), a second closes Quick Actions. Adds a nested-sheet unit test. Full web suite 472 green, svelte-check clean. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
Follow-up to the nested-sheet fix: replace the descendant-only guard with a document-wide frontmost check so the "only the topmost sheet handles Escape/Tab" rule also holds for sibling sheets (two open overlays where neither DOM-contains the other). A sheet that contains a deeper open sheet is never frontmost; among the remaining leaf sheets the last in document order paints on top at the shared z-index, so it wins. Recomputed per keydown, so order-independent. Two full-screen overlays can't both be reached by the user today (opening one covers every other trigger), so this hardens a currently-unreachable topology rather than fixing a live repro — but it makes the invariant total and closes the Codex review's remaining finding. The single-sheet path short-circuits to frontmost=true, so the verified primary behavior is unchanged (re-verified at runtime: single-sheet focus-in/trap/Escape/restore + nested one-layer-per-Esc both still green). Adds a sibling-topology unit test. Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
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.
What & why
BottomSheet.svelteis arole="dialog" aria-modal="true"mobile sheet, but unlike the native-<dialog>Modal.svelteit never moved focus into itself or trapped Tab. Two consequences, app-wide (most visible when a sheet opens over the mobile split-pane — field selects, Quick Actions, "Move to…"):role="dialog", so a window-level ESC handler underneath (e.g. the collection page's pane-close) fired first and closed that instead of the sheet.Fixes BUG-2130.
Change
Focus management in the shared component, mirroring
Modal.svelte:tabindex=-1) — avoids implying a selection in the option-list sheets and lets a screen reader announce the dialog; Tab then steps to the first control.paneFocusables+nextTrapTargetfrompaneFocus.ts) so the two focus traps can't drift.The focus effect reads only
open/sheetEland writes the non-reactivepreviouslyFocused, so it can't self-invalidate (CONVE-1688).Why surgical, not a native-
<dialog>rebuild11 consumers make the blast radius large, and the bug is scoped to the shared component. Converging
BottomSheetonto theModalprimitive would fix this "for free" but is a separate, larger refactor with real mobile-regression risk — deliberately out of scope here.Verification
BottomSheet.svelte.test.ts: focus-in, Tab + Shift+Tab wrap, Escape, backdrop, focus-restore.svelte-check: clean..select-trigger.https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra