mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 13:28:57 +00:00
fix(web): trap focus in BottomSheet so ESC/Tab hit the sheet, not the layer under it (BUG-2130) (#1006)
* fix(web): trap focus in BottomSheet so ESC/Tab hit the sheet, not the 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 * fix(web): only the frontmost BottomSheet handles Escape/Tab (nested sheets) 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 * fix(web): generalize BottomSheet frontmost gate to sibling sheets too 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 commit is contained in:
xarmian
@@ -18,6 +18,7 @@
|
||||
-->
|
||||
<script lang="ts">
|
||||
import type { Snippet } from 'svelte';
|
||||
import { paneFocusables, nextTrapTarget } from '$lib/collections/paneFocus';
|
||||
|
||||
interface Props {
|
||||
open: boolean;
|
||||
@@ -35,11 +36,97 @@
|
||||
const uid = $props.id();
|
||||
const headingId = `bottom-sheet-heading-${uid}`;
|
||||
|
||||
// bind:this the sheet panel so the open/close effect can move focus INTO it
|
||||
// and the Tab handler can cycle focus WITHIN it. `$state` so the effect
|
||||
// re-runs once the `{#if open}` block mounts the element (mirrors
|
||||
// Modal.svelte's `dialogEl`).
|
||||
let sheetEl = $state<HTMLElement>();
|
||||
|
||||
// Plain `let` (NOT $state, per CONVE-1688): focus bookkeeping read/written
|
||||
// only inside the effect + teardown, never in reactive position.
|
||||
let previouslyFocused: HTMLElement | null = null;
|
||||
|
||||
function restoreFocus() {
|
||||
if (previouslyFocused && document.contains(previouslyFocused)) {
|
||||
previouslyFocused.focus();
|
||||
}
|
||||
previouslyFocused = null;
|
||||
}
|
||||
|
||||
// Move focus INTO the sheet when it opens, and restore it to the trigger on
|
||||
// close (BUG-2130). Without this the sheet is a `role="dialog"` that never
|
||||
// takes focus: ESC reaches the trigger's layer underneath (closing THAT),
|
||||
// and Tab escapes the sheet. Reads `open` (prop) + `sheetEl` ($state); writes
|
||||
// only the plain `previouslyFocused`, so no $state is both read and written
|
||||
// here and the effect can't self-invalidate (mirrors Modal.svelte).
|
||||
$effect(() => {
|
||||
const el = sheetEl;
|
||||
if (open && el) {
|
||||
if (previouslyFocused === null) {
|
||||
previouslyFocused = (document.activeElement as HTMLElement | null) ?? null;
|
||||
}
|
||||
// Focus the panel itself (tabindex=-1) rather than a control inside —
|
||||
// avoids implying a selection in the option-list sheets, and lets a
|
||||
// screen reader announce the dialog. Tab then steps to the first
|
||||
// control. Guarded so a benign effect re-run can't yank focus back off
|
||||
// a control the user has already tabbed to.
|
||||
if (!el.contains(document.activeElement)) {
|
||||
el.focus({ preventScroll: true });
|
||||
}
|
||||
} else if (!open) {
|
||||
restoreFocus();
|
||||
}
|
||||
});
|
||||
|
||||
// If the component is torn down while open (e.g. a consumer that only mounts
|
||||
// the sheet on mobile), still return focus to the trigger.
|
||||
$effect(() => () => restoreFocus());
|
||||
|
||||
// Is this the FRONTMOST open sheet — the only one that should act on
|
||||
// Escape/Tab? Every open sheet listens on `window`, so without a gate a
|
||||
// single Escape closes every open layer at once (BUG-2130 layer isolation).
|
||||
// The realistic multi-sheet case is nesting (a control inside a sheet opens
|
||||
// another — e.g. the emoji picker inside Quick Actions), where the child
|
||||
// renders DOM-INSIDE our content; a sheet that contains a deeper open sheet
|
||||
// is never frontmost. Among the remaining leaf sheets (the theoretical
|
||||
// sibling case — two full-screen overlays can't both be reached by the user,
|
||||
// but stay robust anyway) the last in document order paints on top at the
|
||||
// shared z-index, so it's the frontmost. Recomputed per keydown, so it's
|
||||
// order-independent (a `defaultPrevented` check can't work: the outer sheet's
|
||||
// window listener is registered first and fires before the inner's). The
|
||||
// single-sheet path short-circuits to `true` — no behavior change there.
|
||||
function isFrontmostSheet(): boolean {
|
||||
if (!sheetEl) return false;
|
||||
const open = Array.from(document.querySelectorAll<HTMLElement>('.bs-sheet'));
|
||||
if (open.length <= 1) return true;
|
||||
if (sheetEl.querySelector('.bs-sheet')) return false; // we contain a deeper sheet
|
||||
const leaves = open.filter((s) => !s.querySelector('.bs-sheet'));
|
||||
return leaves[leaves.length - 1] === sheetEl;
|
||||
}
|
||||
|
||||
function handleKeydown(e: KeyboardEvent) {
|
||||
if (!open) return;
|
||||
if (!isFrontmostSheet()) return;
|
||||
if (e.key === 'Escape') {
|
||||
e.preventDefault();
|
||||
onclose();
|
||||
return;
|
||||
}
|
||||
// Trap Tab within the sheet: without this, Tab past the last control
|
||||
// escapes into the obscured content behind it (BUG-2130). Reuses the
|
||||
// pane's tested trap math (paneFocus.ts) so the two focus traps can't
|
||||
// drift.
|
||||
if (e.key === 'Tab' && sheetEl) {
|
||||
const target = nextTrapTarget(
|
||||
paneFocusables(sheetEl),
|
||||
document.activeElement,
|
||||
e.shiftKey,
|
||||
sheetEl
|
||||
);
|
||||
if (target) {
|
||||
e.preventDefault();
|
||||
target.focus({ preventScroll: true });
|
||||
}
|
||||
}
|
||||
}
|
||||
</script>
|
||||
@@ -51,6 +138,7 @@
|
||||
<!-- svelte-ignore a11y_no_static_element_interactions -->
|
||||
<div class="bs-overlay" onclick={onclose}>
|
||||
<div
|
||||
bind:this={sheetEl}
|
||||
class="bs-sheet"
|
||||
role="dialog"
|
||||
aria-modal="true"
|
||||
|
||||
@@ -0,0 +1,192 @@
|
||||
// Runs in the jsdom vitest project (filename ends `.svelte.test.ts`). Covers
|
||||
// BottomSheet.svelte's focus behavior (BUG-2130): move focus INTO the sheet on
|
||||
// open, trap Tab within it, and restore focus to the trigger on close. The
|
||||
// Tab-cycle *math* lives in — and is exhaustively tested by — paneFocus.ts
|
||||
// (`nextTrapTarget` / `paneFocusables`); here we assert the component wires it
|
||||
// up and the open/close focus bookkeeping.
|
||||
import { describe, it, expect, vi, afterEach } from 'vitest';
|
||||
import { render, cleanup } from '@testing-library/svelte';
|
||||
import { createRawSnippet, tick, flushSync } from 'svelte';
|
||||
import BottomSheet from './BottomSheet.svelte';
|
||||
|
||||
// Two focusable controls so the Tab-trap wrap has a first/last to cycle between.
|
||||
const bodySnippet = createRawSnippet(() => ({
|
||||
render: () =>
|
||||
`<div><button id="first-btn" type="button">First</button><button id="last-btn" type="button">Last</button></div>`
|
||||
}));
|
||||
|
||||
function baseProps(overrides: Record<string, unknown> = {}) {
|
||||
return {
|
||||
open: true,
|
||||
onclose: vi.fn(),
|
||||
title: 'Sheet',
|
||||
children: bodySnippet,
|
||||
...overrides
|
||||
};
|
||||
}
|
||||
|
||||
function getSheet(): HTMLElement {
|
||||
const el = document.querySelector('.bs-sheet');
|
||||
if (!el) throw new Error('.bs-sheet not found');
|
||||
return el as HTMLElement;
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
vi.restoreAllMocks();
|
||||
document.body.innerHTML = '';
|
||||
});
|
||||
|
||||
describe('BottomSheet.svelte', () => {
|
||||
it('renders a labelled role="dialog" and moves focus onto the panel on open', async () => {
|
||||
render(BottomSheet, { props: baseProps({ open: true }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
|
||||
const sheet = getSheet();
|
||||
expect(sheet.getAttribute('role')).toBe('dialog');
|
||||
expect(sheet.getAttribute('aria-modal')).toBe('true');
|
||||
// Focus moved into the sheet (onto the tabindex=-1 panel) rather than
|
||||
// staying on whatever triggered it.
|
||||
expect(document.activeElement).toBe(sheet);
|
||||
});
|
||||
|
||||
it('is not rendered while closed', async () => {
|
||||
render(BottomSheet, { props: baseProps({ open: false }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
expect(document.querySelector('.bs-sheet')).toBeNull();
|
||||
});
|
||||
|
||||
it('fires onclose on Escape', async () => {
|
||||
const onclose = vi.fn();
|
||||
render(BottomSheet, { props: baseProps({ open: true, onclose }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
|
||||
window.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }));
|
||||
expect(onclose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('fires onclose on a backdrop (overlay) click', async () => {
|
||||
const onclose = vi.fn();
|
||||
render(BottomSheet, { props: baseProps({ open: true, onclose }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
|
||||
const overlay = document.querySelector('.bs-overlay') as HTMLElement;
|
||||
overlay.dispatchEvent(new MouseEvent('click', { bubbles: true }));
|
||||
expect(onclose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('traps Tab: forward Tab off the last control wraps to the first', async () => {
|
||||
// jsdom has no layout, so paneFocusables' default visibility check
|
||||
// (offsetParent / getClientRects) would filter everything out. Make the
|
||||
// controls report as on-screen for this assertion.
|
||||
vi.spyOn(HTMLElement.prototype, 'getClientRects').mockReturnValue([
|
||||
{ width: 1, height: 1 } as DOMRect
|
||||
] as unknown as DOMRectList);
|
||||
|
||||
// No title → no header close button, so the two body buttons are the
|
||||
// only focusables and are unambiguously first/last.
|
||||
render(BottomSheet, { props: baseProps({ open: true, title: undefined }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
|
||||
const first = document.getElementById('first-btn') as HTMLButtonElement;
|
||||
const last = document.getElementById('last-btn') as HTMLButtonElement;
|
||||
last.focus();
|
||||
expect(document.activeElement).toBe(last);
|
||||
|
||||
const evt = new KeyboardEvent('keydown', { key: 'Tab', cancelable: true });
|
||||
window.dispatchEvent(evt);
|
||||
|
||||
expect(document.activeElement).toBe(first);
|
||||
expect(evt.defaultPrevented).toBe(true);
|
||||
});
|
||||
|
||||
it('traps Tab: Shift+Tab off the first control wraps to the last', async () => {
|
||||
vi.spyOn(HTMLElement.prototype, 'getClientRects').mockReturnValue([
|
||||
{ width: 1, height: 1 } as DOMRect
|
||||
] as unknown as DOMRectList);
|
||||
|
||||
render(BottomSheet, { props: baseProps({ open: true, title: undefined }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
|
||||
const first = document.getElementById('first-btn') as HTMLButtonElement;
|
||||
const last = document.getElementById('last-btn') as HTMLButtonElement;
|
||||
first.focus();
|
||||
|
||||
const evt = new KeyboardEvent('keydown', { key: 'Tab', shiftKey: true, cancelable: true });
|
||||
window.dispatchEvent(evt);
|
||||
|
||||
expect(document.activeElement).toBe(last);
|
||||
expect(evt.defaultPrevented).toBe(true);
|
||||
});
|
||||
|
||||
it('a sheet containing a nested open sheet stays out of Escape (only the inner closes)', async () => {
|
||||
// Reproduces the nested case (Quick Actions sheet → emoji-picker sheet):
|
||||
// the inner sheet renders DOM-nested inside the outer's content. Both
|
||||
// listen on window, so a naive handler would close BOTH on one Escape.
|
||||
// Here the outer's children include a nested `.bs-sheet`, so the outer
|
||||
// must NOT fire its onclose — the innermost sheet owns Escape.
|
||||
const nested = createRawSnippet(() => ({
|
||||
render: () =>
|
||||
`<div><div class="bs-overlay"><div class="bs-sheet" role="dialog" aria-modal="true" tabindex="-1"><button type="button">Inner</button></div></div></div>`
|
||||
}));
|
||||
const onclose = vi.fn();
|
||||
render(BottomSheet, { props: baseProps({ open: true, onclose, children: nested }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
|
||||
window.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }));
|
||||
// Outer sheet stayed out: its onclose did not fire (the inner sheet, were
|
||||
// it a real BottomSheet, would have closed via its own window listener).
|
||||
expect(onclose).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('only the topmost of sibling sheets handles Escape (last in document order)', async () => {
|
||||
// Two open sheets that do NOT contain each other (sibling topology). Both
|
||||
// listen on window; only the one painted on top (last in document order at
|
||||
// the shared z-index) should act on Escape.
|
||||
const onclose = vi.fn();
|
||||
render(BottomSheet, { props: baseProps({ open: true, onclose }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
|
||||
// A later sibling sheet appended after this one → this one is no longer
|
||||
// frontmost and must stay out of Escape.
|
||||
const sibling = document.createElement('div');
|
||||
sibling.className = 'bs-sheet';
|
||||
document.body.appendChild(sibling);
|
||||
|
||||
window.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }));
|
||||
expect(onclose).not.toHaveBeenCalled();
|
||||
|
||||
// Remove the sibling → this sheet is frontmost again and handles Escape.
|
||||
sibling.remove();
|
||||
window.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }));
|
||||
expect(onclose).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('restores focus to the previously-focused trigger on close', async () => {
|
||||
const trigger = document.createElement('button');
|
||||
trigger.type = 'button';
|
||||
document.body.appendChild(trigger);
|
||||
trigger.focus();
|
||||
expect(document.activeElement).toBe(trigger);
|
||||
|
||||
const { rerender } = render(BottomSheet, { props: baseProps({ open: true }) });
|
||||
await tick();
|
||||
flushSync();
|
||||
// Focus is now inside the sheet.
|
||||
expect(document.activeElement).toBe(getSheet());
|
||||
|
||||
await rerender(baseProps({ open: false }));
|
||||
await tick();
|
||||
flushSync();
|
||||
|
||||
expect(document.activeElement).toBe(trigger);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user