From 316e25a6cacbc2aec4f359fd08af5d231bada5cc Mon Sep 17 00:00:00 2001
From: xarmian <xarmian@gmail.com>
Date: Wed, 22 Jul 2026 17:32:31 -0400
Subject: [PATCH] fix(web): trap focus in BottomSheet so ESC/Tab hit the sheet,
 not the layer under it (BUG-2130) (#1006)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* 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
---
 .../lib/components/common/BottomSheet.svelte  |  88 ++++++++
 .../common/BottomSheet.svelte.test.ts         | 192 ++++++++++++++++++
 2 files changed, 280 insertions(+)
 create mode 100644 web/src/lib/components/common/BottomSheet.svelte.test.ts

diff --git a/web/src/lib/components/common/BottomSheet.svelte b/web/src/lib/components/common/BottomSheet.svelte
index b3bc449e..6c948a7c 100644
--- a/web/src/lib/components/common/BottomSheet.svelte
+++ b/web/src/lib/components/common/BottomSheet.svelte
@@ -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"
diff --git a/web/src/lib/components/common/BottomSheet.svelte.test.ts b/web/src/lib/components/common/BottomSheet.svelte.test.ts
new file mode 100644
index 00000000..9675eb06
--- /dev/null
+++ b/web/src/lib/components/common/BottomSheet.svelte.test.ts
@@ -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);
+	});
+});