From 849798dcfe759cd06ee818f32f5b72d85385c110 Mon Sep 17 00:00:00 2001 From: Nicholas Sollazzo Date: Thu, 2 Jul 2026 19:06:11 +0200 Subject: [PATCH 1/2] refactor(ui): extract shared modal-dialog controller for Dialog + Sheet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dialog and Sheet carried byte-identical native-`` wiring: the showModal()/close() effect, the teardown focus-restore effect, and the single-dismissal handleClose() (save/restore previouslyFocused, sync the `open` binding, notify onclose). Two copies of subtle, a11y-critical focus logic risk silent divergence on any future fix. Consolidated into useModalDialog() in src/lib/actions/modalDialog.svelte.ts, following the repo's getter-param convention (cf. useOverlayTheme(invert)). Logic moved verbatim — behavior is unchanged. Internal helper (not exported from index.ts). Net -37 lines in the components (-53/+16). Gates green: svelte-check (0), prettier+eslint, knip, 300/300 unit tests (incl. the backdrop double-fire regression guards), build + svelte-package + are-the-types-wrong. Co-Authored-By: Paperclip --- src/lib/actions/modalDialog.svelte.ts | 56 +++++++++++++++++++++++++++ src/lib/components/Dialog.svelte | 38 ++++-------------- src/lib/components/Sheet.svelte | 31 ++++----------- 3 files changed, 72 insertions(+), 53 deletions(-) create mode 100644 src/lib/actions/modalDialog.svelte.ts diff --git a/src/lib/actions/modalDialog.svelte.ts b/src/lib/actions/modalDialog.svelte.ts new file mode 100644 index 0000000..83ddf31 --- /dev/null +++ b/src/lib/actions/modalDialog.svelte.ts @@ -0,0 +1,56 @@ +/** + * Shared controller for the native modal `` overlays (Dialog, Sheet). + * + * Drives a `` imperatively from a two-way `open` binding: `showModal()` + * reflects it into the top layer with a focus trap + inert background, and every + * dismissal path (Escape, backdrop click, `open=false`) routes through the native + * `close` event so `handleClose` fires exactly once. Also saves the previously + * focused element on open and restores it on close or on teardown-while-open + * (where the native `close` event never fires). + * + * Params are getters so the caller stays the single source of truth (mirrors the + * `useOverlayTheme(invert)` convention). Call during component init. + */ +export function useModalDialog(opts: { + /** The bound `` element, or null before mount. */ + dialog: () => HTMLDialogElement | null; + /** Current open state. */ + open: () => boolean; + /** Sync the two-way `open` binding back to the caller. */ + setOpen: (value: boolean) => void; + /** Optional close callback, read lazily so a late-bound prop is honored. */ + onclose: () => (() => void) | undefined; +}): { handleClose: () => void } { + let previouslyFocused: HTMLElement | null = null; + + // Never set the `open` attribute in markup; showModal()/close() are the only + // drivers so the native `close` event stays the single dismissal path. + $effect(() => { + const d = opts.dialog(); + if (!d) return; + if (opts.open() && !d.open) { + previouslyFocused = document.activeElement as HTMLElement | null; + d.showModal(); + } else if (!opts.open() && d.open) { + d.close(); + } + }); + + // If unmounted while still open, the native `close` event never fires, so + // handleClose() never runs — restore focus on teardown so it isn't stranded + // on a now-removed element. + $effect(() => () => { + if (previouslyFocused?.isConnected) previouslyFocused.focus({ preventScroll: true }); + }); + + // The single source of truth for closing is the native `close` event; sync the + // binding, notify, and restore focus. + function handleClose() { + opts.setOpen(false); + opts.onclose()?.(); + if (previouslyFocused?.isConnected) previouslyFocused.focus({ preventScroll: true }); + previouslyFocused = null; + } + + return { handleClose }; +} diff --git a/src/lib/components/Dialog.svelte b/src/lib/components/Dialog.svelte index 9ff7b58..f8531e7 100644 --- a/src/lib/components/Dialog.svelte +++ b/src/lib/components/Dialog.svelte @@ -1,6 +1,7 @@ import type { Snippet } from 'svelte'; import { useOverlayTheme } from '../theme/context.js'; + import { useModalDialog } from '../actions/modalDialog.svelte.js'; interface Props { /** Open state (two-way bindable). */ @@ -37,31 +38,15 @@ const titleId = $props.id(); let dialog = $state(null); - let previouslyFocused: HTMLElement | null = null; - $effect(() => { - const d = dialog; - if (!d) return; - if (open && !d.open) { - previouslyFocused = document.activeElement as HTMLElement | null; - d.showModal(); - } else if (!open && d.open) { - d.close(); - } - }); - - // Restore focus on teardown if unmounted while open (no native `close` fires). - $effect(() => () => { - if (previouslyFocused?.isConnected) previouslyFocused.focus({ preventScroll: true }); + // Native modal wiring (showModal/close, single-dismissal `close` event, focus + // save/restore) is shared with — see useModalDialog. + const { handleClose } = useModalDialog({ + dialog: () => dialog, + open: () => open, + setOpen: (v) => (open = v), + onclose: () => onclose }); - - // Closing always routes through the native `close` event so this fires once. - function handleClose() { - open = false; - onclose?.(); - if (previouslyFocused?.isConnected) previouslyFocused.focus({ preventScroll: true }); - previouslyFocused = null; - } Date: Thu, 2 Jul 2026 19:20:27 +0200 Subject: [PATCH 2/2] test(ui): pin focus save/restore + teardown-while-open for useModalDialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The centralized modal-dialog controller now owns the a11y-critical focus logic for both Dialog and Sheet, but no test pinned it — a regression could land silently. Add two tests to Dialog's suite (both components delegate to the same helper): - restores focus to the opener when closed - restores focus on teardown while still open (native `close` never fires, so restore comes from the $effect cleanup branch — verified by mutation: removing that branch fails this test) Test-only. Four gates green (lint, check, test:coverage). Closes POS-75. Co-Authored-By: Paperclip --- src/lib/components/Dialog.svelte.test.ts | 46 ++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/lib/components/Dialog.svelte.test.ts b/src/lib/components/Dialog.svelte.test.ts index 30942a1..ac63da1 100644 --- a/src/lib/components/Dialog.svelte.test.ts +++ b/src/lib/components/Dialog.svelte.test.ts @@ -97,6 +97,52 @@ describe('Dialog', () => { expect(document.querySelector('dialog.pn-dialog')!.getAttribute('data-theme')).toBeNull(); }); + // --- Focus save/restore (owned by useModalDialog, shared with Sheet) --- + // Pinned here per POS-75: the a11y-critical focus logic was centralized so a + // future fix can't land in one component and not the other — that guarantee + // only holds if the centralized behavior is test-pinned. Render closed and open + // via rerender so the controller captures a real opener as document.activeElement + // (rendering open from the start would capture , not an opener). + + test('restores focus to the opener when closed', async () => { + const opener = document.createElement('button'); + document.body.appendChild(opener); + opener.focus(); + expect(document.activeElement).toBe(opener); + + const screen = render(Dialog, { open: false, title: 'T', children: body }); + await screen.rerender({ open: true, title: 'T', children: body }); + + const el = document.querySelector('dialog.pn-dialog') as HTMLDialogElement; + await expect.poll(() => el.open).toBe(true); + // showModal() pulls focus into the top layer, off the opener… + expect(document.activeElement).not.toBe(opener); + + el.close(); // native close — the single dismissal path + // …and the close handler returns focus to whoever opened the overlay. + await expect.poll(() => document.activeElement).toBe(opener); + opener.remove(); + }); + + test('restores focus on teardown while still open (native close never fires)', async () => { + const opener = document.createElement('button'); + document.body.appendChild(opener); + opener.focus(); + + const screen = render(Dialog, { open: false, title: 'T', children: body }); + await screen.rerender({ open: true, title: 'T', children: body }); + + const el = document.querySelector('dialog.pn-dialog') as HTMLDialogElement; + await expect.poll(() => el.open).toBe(true); + expect(document.activeElement).not.toBe(opener); + + // Unmounting an open never fires the native `close` event, so focus + // restore must come from the $effect cleanup branch — assert it still runs. + await screen.unmount(); + await expect.poll(() => document.activeElement).toBe(opener); + opener.remove(); + }); + // Polarity inversion alone is the dialog's figure-ground cue — no neon glow, // even when the inverted surface is Machine-themed. test('no glow in either polarity', () => {