From 13852439e13fc34bddbd5869ccd328e361d64688 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 5 Aug 2026 12:42:40 +0000 Subject: [PATCH] feat(a11y): give the attachment viewer a real modal contract (TASK-2429) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Lightbox` was a `role="presentation"` fixed div with a local `` keydown handler and no focus management at all. 3a's later tasks delete the editor's hand-rolled `showModal()` dialog and route inline body images here, so everything that dialog was getting from the platform for free has to exist here first (DR-4b). The contract: - `role="dialog"` + `aria-modal="true"` + an accessible name (the image alt, else "Attachment viewer"), tracking the image CURRENTLY shown. The controls get real `aria-label`s too — their text is "✕" / "‹" / "›", and `title` does not win over element content for the accessible name. - Portaled to `` DIRECTLY — deliberately not `portalAction.ts`, which targets the nearest ancestor `` when one exists: the opposite of what a top-most surface needs. `` is also the parent the backdrop manager's inert bookkeeping requires, and the only one with no ancestor that could establish a containing block and silently trap a `position: fixed` overlay. - `wsSlug` CAPTURED at open, not read live: the pane switches workspace without remounting what is above it, so a live read could rebuild already-captured attachment ids against a different workspace. - Focus entry to the first TABBABLE DESCENDANT via `paneFocusables` (the root is `tabindex="-1"` only as the no-controls-yet fallback); restore on close to an OPTIONAL `invoker` prop, verified still connected AND focusable — else focus is parked on `` deliberately. With no invoker threaded it falls back to whatever held focus at open, so the producers that thread one only in TASK-2431 don't come out of this commit worse than they went in. The restore declines when something else already owns focus. - Background inertness via TASK-2427's manager (`acquire(exemptRoot)`), never a hand-rolled `inert`. Released BEFORE the focus restore, and the returned `stackEmpty` decides: with a viewer still open the manager has already handed focus into it and this one stands down. - Tab trap through `paneFocusables` / `nextTrapTarget` — the pane's tested trap math, not a second implementation. - ONLY the frontmost viewer (`isViewerFrontmost`) traps Tab, handles ←/→ and consumes Escape. Handlers are global and `nextTrapTarget` deliberately redirects out-of-container focus INWARD, so a background viewer would otherwise drag focus out of the viewer in front of it. And the viewer stands down entirely (`isBlockedByModal`) while a `showModal()` dialog is open over it: the top layer is above any body-portaled surface, so the frontmost LEASE is not always the frontmost SURFACE, and the manager keeps such a dialog operable on purpose. - Escape: the local branch is DELETED, not gated — it ignored `defaultPrevented`, so alongside the stack it gave Escape two owners and let one press collapse two layers. `escapeStack` is now the sole owner, at a new `viewer` priority (50) above `menu` (40). `AttachmentViewerHost` stops restoring focus itself and threads `invoker` down instead. Its own restore ran while the viewer still held the backdrop lease — i.e. while the invoker sat inside an `inert` body child, where it is not focusable at all — so it would have silently become a no-op the moment this commit landed. The only correct moment is after the lease is released, which is inside the viewer's teardown. Atomic with the route guards, because the stack is unreachable without them: both `[collection]` and `[collection]/[slug]` bailed out of the ESC chain on `document.querySelector('dialog[open], [role="dialog"]:not(.item-pane)')`, which the viewer now matches — Escape would have been dead. Both call the shared `hasForeignEscapeOwner()` instead. It KEEPS the ARIA branch (`BottomSheet` / `DockedSheet` are shipped `role="dialog"` Escape owners with no stack registration; dropping it would regress them) and narrows only the NATIVE branch to a feature-detected `dialog:modal`, falling back to today's `dialog[open]` where the pseudo-class is unsupported. The backdrop's `z-index` goes to 100000. At 1000 it was under the desktop emoji picker's body-portaled dropdown (99999) — and the app shell wrapper is `display: contents`, so every fixed overlay in the tree competes in the ROOT stacking context; being a body child is not protection. A surface that paints over a viewer that has inerted it is visible-but-untouchable, the worst of both. The full sweep, and the rule that a new overlay above this value is a bug, are recorded at the declaration. The one thing legitimately above it is a native `showModal()` dialog, which gets there via the top layer and no z-index at all. Collision audit for `[role="dialog"]`-as-foreign-modal consumers: - `app.css` `@media print` — the viewer matches, and should: a transient overlay must not print. Documented in place (a JS-only grep misses this). - `paneFocus.ts` `PANE_EXEMPT_SURFACE_SELECTOR` and `PaneHost`'s mobile trap + focus-follows classifier — the viewer matches, and must: it runs its own trap and key handling. - `web/e2e/**` incl. `e2e/lib/*.ts` — no shared dialog selector constants; the only bare `getByRole('dialog')` is `workspace-bundle-roundtrip.spec.ts:204`, claimed by TASK-2430 by name. Tests: 36 cases for the modal contract, 10 for `hasForeignEscapeOwner`, plus the fallout of portaling (host tests can no longer scope by container — ownership is proven by distinguishable payloads and destroying a known host; the strip's Escape case now drives the stack). Comments state what jsdom cannot prove — real inertness, layout/stacking, real Tab traversal — which is TASK-2436's browser suite, and mark the one assertion whose guard is genuinely indistinguishable from its neighbour in jsdom. Verified by mutation: 26 mutations run, all caught except that documented one. `escapeGuardWiring.svelte.test.ts` is a deliberately narrow addition: the Lightbox tests drive a route-SHAPED driver they define themselves, so they prove the shape and not that either route still calls it. This asserts against the real source of both route files that `hasForeignEscapeOwner()` is imported, called as an early return, and called BEFORE `runTopEscape()` — catching deletion, re-inlining of the old selector, and reordering, which is the actual regression risk. Mounting a route under vitest to prove it behaviourally is not worth its cost; the behavioural proof is TASK-2436's browser suite. Two things stop it being a grep that lies: comments are stripped first (every one of these strings now appears in prose in those files, so a whole-file search could be satisfied by a comment), and the assertions are scoped to the handler that actually calls `runTopEscape` rather than to the file. Verified against a commented-out guard, a guard moved to an unrelated helper, and a reverted selector under different quoting — while a prettier-style reflow of the guard still passes. TASK-2429 --- web/src/app.css | 7 +- .../lib/a11y/escapeGuardWiring.svelte.test.ts | 115 +++ .../lib/a11y/viewerBackdrop.svelte.test.ts | 115 +++ web/src/lib/a11y/viewerBackdrop.ts | 52 ++ web/src/lib/collections/paneFocus.ts | 8 + .../attachments/AttachmentViewerHost.svelte | 40 +- .../AttachmentViewerHost.svelte.test.ts | 79 +- .../AttachmentViewerHostClose.svelte.test.ts | 22 +- .../attachments/fixtures/LightboxStub.svelte | 5 +- .../attachments/fixtures/lightboxStub.ts | 2 + .../components/collections/PaneHost.svelte | 3 + web/src/lib/components/common/Lightbox.svelte | 277 ++++++- .../components/common/Lightbox.svelte.test.ts | 753 ++++++++++++++++++ .../items/ItemAttachmentStrip.svelte.test.ts | 13 + web/src/lib/stores/escapeStack.ts | 7 + .../[workspace]/[collection]/+page.svelte | 11 +- .../[collection]/[slug]/+page.svelte | 12 +- 17 files changed, 1442 insertions(+), 79 deletions(-) create mode 100644 web/src/lib/a11y/escapeGuardWiring.svelte.test.ts create mode 100644 web/src/lib/components/common/Lightbox.svelte.test.ts diff --git a/web/src/app.css b/web/src/app.css index 2312f090..c422e7ce 100644 --- a/web/src/app.css +++ b/web/src/app.css @@ -854,7 +854,12 @@ dialog.attachment-image-lightbox .attachment-image-lightbox-close:hover { is covered by the `dialog[open]` selector above. `:not(.item-pane)` keeps the detail pane printable: on mobile the pane carries role="dialog" (TASK-2131) but IS the content being printed, not a - transient overlay to strip. */ + transient overlay to strip. + The body-portaled attachment viewer (`.lightbox-backdrop.attachment-viewer`, + TASK-2429) DOES match here, and should: it is a transient full-screen + overlay, and printing with one open must print the item underneath, not a + black rectangle. Audited deliberately — a JS-only grep for `role="dialog"` + consumers misses this rule entirely. */ [role="dialog"]:not(.item-pane), [role="tooltip"], [role="menu"] { diff --git a/web/src/lib/a11y/escapeGuardWiring.svelte.test.ts b/web/src/lib/a11y/escapeGuardWiring.svelte.test.ts new file mode 100644 index 00000000..12191920 --- /dev/null +++ b/web/src/lib/a11y/escapeGuardWiring.svelte.test.ts @@ -0,0 +1,115 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +/** + * WIRING CONTRACT for the two route ESC guards (TASK-2429). + * + * The viewer's Escape has exactly one owner — `escapeStack` — and the only code + * that runs it is the keydown handler in each of these two route files. Both + * used to bail on an inline + * `document.querySelector('dialog[open], [role="dialog"]:not(.item-pane)')`, + * which the body-portaled viewer now MATCHES; without the shared + * `hasForeignEscapeOwner()` (which excludes it) the guard returns early, the + * stack never runs, and Escape closes nothing at all. That is a silent, + * app-wide dead key. + * + * WHY A SOURCE ASSERTION. The unit tests around `Lightbox` drive a + * ROUTE-SHAPED driver they define themselves, so they prove the shape works — + * not that either route still calls it. Mounting a SvelteKit route under vitest + * to prove the real thing costs far more than it is worth (a 2,900-line + * component, its stores, its data loaders), while the actual regression risk is + * mundane: someone deletes or reverts the call, or re-inlines the old selector + * during a merge. A grep-shaped contract catches exactly that, and nothing else + * pretends to be covered here. + * + * TWO THINGS KEEP IT FROM BEING A GREP THAT LIES: + * • COMMENTS ARE STRIPPED FIRST. Every one of these strings appears in prose + * in these files (this commit added several), so a whole-file search could + * be satisfied by a comment while the handler ran unguarded. + * • ASSERTIONS ARE SCOPED TO THE HANDLER that actually calls `runTopEscape`, + * not to the file. A guard in some unrelated helper is not this contract. + * + * The BEHAVIOURAL proof — a real Escape press, in a real browser, closing + * exactly the viewer and not the pane beneath it — belongs to TASK-2436's + * Playwright suite (DR-9), together with the inertness and stacking guarantees + * jsdom cannot see either. + */ + +const ROUTES = [ + '../../routes/[username]/[workspace]/[collection]/+page.svelte', + '../../routes/[username]/[workspace]/[collection]/[slug]/+page.svelte', +] as const; + +/** The guard call, tolerant of formatting (prettier reflow, added braces). */ +const GUARD = /if\s*\(\s*hasForeignEscapeOwner\(\)\s*\)\s*\{?\s*return\s*;/; +/** Any `querySelector` whose selector string reaches for a `role="dialog"`. */ +const RAW_DIALOG_QUERY = /querySelector\w*\(\s*(['"`])[^'"`]*\[\s*role\s*=\s*\\?["']?dialog/; + +/** + * Source with comments removed. Line comments are matched only when the `//` + * is not preceded by `:`, so URLs (`https://…`) survive — the point is to drop + * PROSE, not to be a parser. + */ +function stripComments(source: string): string { + return source + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(//g, '') + .replace(/(^|[^:])\/\/[^\n]*/g, '$1'); +} + +/** + * The body of the function that calls `runTopEscape()`, from its `function` + * keyword up to that call — i.e. exactly the region the guard must sit in. + * Returns null when there is no such call at all, which is itself a failure. + */ +function escapeHandlerPrologue(code: string): string | null { + const call = code.search(/runTopEscape\s*\(/); + if (call === -1) return null; + const start = code.lastIndexOf('function ', call); + if (start === -1) return null; + return code.slice(start, call); +} + +describe.each(ROUTES)('ESC guard wiring in %s', (relative) => { + const source = readFileSync(fileURLToPath(new URL(relative, import.meta.url)), 'utf8'); + const code = stripComments(source); + + it('imports the shared guard from the a11y module', () => { + expect(code).toMatch( + /import\s*\{[^}]*\bhasForeignEscapeOwner\b[^}]*\}\s*from\s*'\$lib\/a11y\/viewerBackdrop'/ + ); + }); + + it('calls it as an early return INSIDE the handler that runs the stack', () => { + // Scoped, not file-wide: this is the assertion that would otherwise be + // satisfiable by prose or by an unrelated helper. `return` (not + // `preventDefault`) is the point — a foreign modal owns the key outright, + // so the stack must not run at all. + const prologue = escapeHandlerPrologue(code); + expect(prologue).not.toBeNull(); + expect(prologue!).toMatch(GUARD); + }); + + it('does not reach the stack without passing the guard first', () => { + // The ordering IS the contract: running the stack first would close the + // viewer out from under a modal that owns the key, and guarding after the + // fact would do nothing. Falls out of the scoping above — the prologue + // ends AT the call — so this re-states it against the whole file, where a + // second, unguarded `runTopEscape` would also show up. + const occurrences = code.match(/runTopEscape\s*\(/g) ?? []; + expect(occurrences).toHaveLength(1); + const guardAt = code.search(GUARD); + const stackAt = code.search(/runTopEscape\s*\(/); + expect(guardAt).toBeGreaterThan(-1); + expect(guardAt).toBeLessThan(stackAt); + }); + + it('no longer bails on a raw role="dialog" query', () => { + // The pre-TASK-2429 inline check, re-introduced by hand or by a bad merge + // resolution, is the regression this file exists to catch: it matches the + // viewer's own `role="dialog"` root and kills its Escape. Matched by shape + // rather than by one exact quoting, so a reformatted revert still trips it. + expect(code).not.toMatch(RAW_DIALOG_QUERY); + }); +}); diff --git a/web/src/lib/a11y/viewerBackdrop.svelte.test.ts b/web/src/lib/a11y/viewerBackdrop.svelte.test.ts index 09dfa254..0c230d37 100644 --- a/web/src/lib/a11y/viewerBackdrop.svelte.test.ts +++ b/web/src/lib/a11y/viewerBackdrop.svelte.test.ts @@ -3,6 +3,8 @@ import { acquire, isBlockedByModal, isViewerFrontmost, + hasForeignEscapeOwner, + VIEWER_ROOT_CLASS, __resetViewerBackdropForTests, } from './viewerBackdrop'; @@ -701,3 +703,116 @@ describe('isBlockedByModal', () => { }); }); }); + +describe('hasForeignEscapeOwner', () => { + // The shared form of the existence check the two route ESC guards used to + // hand-roll (TASK-2429). It answers a NARROWER question than + // `isBlockedByModal`: not "is something in front of me" but "does a surface + // that owns Escape ITSELF exist" — so a driver of the escape stack knows + // whether to stand down entirely. + + /** The shape a portaled viewer root has, including the marker class. */ + function viewerRoot(): HTMLElement { + const el = bodyChild('viewer'); + el.setAttribute('role', 'dialog'); + el.classList.add(VIEWER_ROOT_CLASS); + return el; + } + + /** + * Emulate an engine where `dialog:modal` IS supported (jsdom throws on it — + * probed, not assumed), with `modals` as the open modal dialogs. Only the + * combined selector the module builds is intercepted. + */ + function mockModalSupport(modals: Element[]): void { + const real = document.querySelector.bind(document); + vi.spyOn(document, 'querySelector').mockImplementation((selector: string) => { + if (!selector.startsWith('dialog:modal')) return real(selector); + const rest = selector.slice(selector.indexOf(',') + 1).trim(); + return modals[0] ?? real(rest); + }); + } + + it('is false with nothing open, so today’s guards are unchanged', () => { + bodyChild('app', ''); + expect(hasForeignEscapeOwner()).toBe(false); + }); + + it('is true for an ARIA sheet — the shipped BottomSheet / DockedSheet', () => { + // The regression fence for the branch that must NOT be dropped: both are + // `role="dialog"` Escape owners with no escape-stack registration, so a + // guard that stopped seeing them would let one press close two layers. + bodyChild('sheet').setAttribute('role', 'dialog'); + expect(hasForeignEscapeOwner()).toBe(true); + }); + + it('is false for the pane’s own mobile overlay', () => { + const pane = bodyChild('pane'); + pane.setAttribute('role', 'dialog'); + pane.classList.add('item-pane'); + expect(hasForeignEscapeOwner()).toBe(false); + }); + + it('is false for the attachment viewer: its Escape is on the stack', () => { + // The whole reason this helper exists. A guard that treated the viewer as + // a foreign modal would return before running the stack, and Escape would + // close nothing at all. + viewerRoot(); + expect(hasForeignEscapeOwner()).toBe(false); + }); + + it('still sees a sheet opened WHILE a viewer is up', () => { + // The exclusion is targeted at the viewer, not a blanket "a viewer is + // open, so nothing else counts". + viewerRoot(); + bodyChild('sheet').setAttribute('role', 'dialog'); + expect(hasForeignEscapeOwner()).toBe(true); + }); + + it('falls back to `dialog[open]` where `:modal` is unsupported', () => { + // jsdom is that engine (it throws on the pseudo-class), so this is the + // path every other test here runs on. The fallback is deliberately the + // PRE-TASK-2429 selector: where the narrower question can't be asked, the + // answer is exactly today's behaviour, never something wider. + const dialog = openModal(); + dialog.setAttribute('open', ''); + expect(hasForeignEscapeOwner()).toBe(true); + }); + + it('does not count a CLOSED native in the fallback path', () => { + // `Modal.svelte` keeps its native mounted at all times and drives + // it with showModal()/close(), so a fallback that dropped the `[open]` + // qualifier would report a foreign Escape owner on every page that merely + // HAS a Modal — swallowing the viewer's Escape everywhere. + openModal(); + expect(hasForeignEscapeOwner()).toBe(false); + }); + + it('asks for `dialog:modal` before falling back', () => { + // Guards against an implementation that simply hard-codes `[open]`. + const seen: string[] = []; + const real = document.querySelector.bind(document); + vi.spyOn(document, 'querySelector').mockImplementation((selector: string) => { + seen.push(selector); + if (selector.startsWith('dialog:modal')) throw new SyntaxError('unsupported'); + return real(selector); + }); + hasForeignEscapeOwner(); + expect(seen.some((s) => s.startsWith('dialog:modal'))).toBe(true); + expect(seen.some((s) => s.includes('dialog[open]'))).toBe(true); + }); + + it('on a supporting engine, an open NON-modal does not count', () => { + // The one behaviour change from the hand-rolled `dialog[open]` string: a + // `show()` / declarative-open dialog never owned Escape, and + // `Modal.svelte` keeps a native mounted at all times. + const dialog = openModal(); + dialog.setAttribute('open', ''); + mockModalSupport([]); + expect(hasForeignEscapeOwner()).toBe(false); + + // ...while a real `showModal()` one does. + mockModalSupport([dialog]); + expect(hasForeignEscapeOwner()).toBe(true); + }); +}); diff --git a/web/src/lib/a11y/viewerBackdrop.ts b/web/src/lib/a11y/viewerBackdrop.ts index 2d9da79e..a9c50354 100644 --- a/web/src/lib/a11y/viewerBackdrop.ts +++ b/web/src/lib/a11y/viewerBackdrop.ts @@ -342,6 +342,58 @@ export function isBlockedByModal(owner?: Element | null): boolean { return false; } +/** + * Class carried by every body-portaled VIEWER root (TASK-2429). The viewer is a + * `role="dialog"`, so without a marker it is indistinguishable from the foreign + * modals the app's ESC guards stand down for — and standing down for it would + * leave Escape with NO owner, since the viewer's own Escape lives on + * `escapeStack`. Exported (rather than typed twice) so the markup and the + * selector below cannot drift apart. + */ +export const VIEWER_ROOT_CLASS = 'attachment-viewer'; + +/** + * Is a modal surface open that owns Escape ITSELF, so an escape-stack driver + * must stand down entirely? + * + * This is the shared form of the existence check the two route keydown handlers + * hand-rolled as `document.querySelector('dialog[open], [role="dialog"]:not(.item-pane)')`. + * Two deliberate differences from that string: + * + * • The NATIVE branch is feature-detected `dialog:modal`, not `dialog[open]`. + * A non-modal `show()` / declarative `` never owned Escape, and + * `Modal.svelte` is always mounted — so `[open]` was over-broad. Where the + * pseudo-class is unsupported (jsdom, legacy engines) it falls back to + * `dialog[open]`, i.e. exactly today's behaviour, never to something wider. + * • The ARIA branch additionally excludes {@link VIEWER_ROOT_CLASS}. It is + * otherwise UNCHANGED and deliberately kept: `BottomSheet` and `DockedSheet` + * are shipped `role="dialog"` Escape owners with no stack registration, and + * dropping the branch would regress both. `.item-pane` stays excluded for + * the reason it always was (TASK-2131) — it is on the stack too. + * + * Existence-based, not target-based, for the reason recorded at the call sites: + * a sheet that doesn't move focus into itself leaves `document.activeElement` + * on the trigger underneath, so a `closest()` test would miss it. + * + * TASK-2430 folds this into {@link isBlockedByModal}'s three-way precedence + * across all seven global Escape/key owners; 3a needs only the two route + * guards to stop swallowing the viewer's Escape. + */ +export function hasForeignEscapeOwner(): boolean { + if (!hasDocument()) return false; + const aria = `[role="dialog"]:not(.item-pane):not(.${VIEWER_ROOT_CLASS})`; + if (modalSelectorSupported !== false) { + try { + const found = !!document.querySelector(`dialog:modal, ${aria}`); + modalSelectorSupported = true; + return found; + } catch { + modalSelectorSupported = false; + } + } + return !!document.querySelector(`dialog[open], ${aria}`); +} + /** Test seam: drop all leases and observers without running focus handoff. */ export function __resetViewerBackdropForTests(): void { stack.length = 0; diff --git a/web/src/lib/collections/paneFocus.ts b/web/src/lib/collections/paneFocus.ts index f447169f..ca33a7b4 100644 --- a/web/src/lib/collections/paneFocus.ts +++ b/web/src/lib/collections/paneFocus.ts @@ -64,6 +64,14 @@ export function paneFocusables( * the `:not`, `closest()` from any in-pane element would match the pane and * wrongly mark the whole pane exempt (killing the mobile Tab trap and confusing * the classifier). A genuinely nested dialog opened FROM the pane still matches. + * + * The attachment viewer (TASK-2429) matches the ARIA branch, and MUST: it is a + * body-portaled `role="dialog"` that runs its own Tab trap and key handling, so + * both consumers have to leave it alone — exactly the case this set exists for. + * That is the opposite of the route ESC guards, which have to look PAST it + * (`hasForeignEscapeOwner`, `$lib/a11y/viewerBackdrop`) because its Escape is on + * the shared stack. Same attribute, two different questions; audited as part of + * TASK-2429's collision sweep. */ export const PANE_EXEMPT_SURFACE_SELECTOR = 'dialog, [role="dialog"]:not(.item-pane), [role="menu"], [role="listbox"], .block-context-menu'; diff --git a/web/src/lib/components/attachments/AttachmentViewerHost.svelte b/web/src/lib/components/attachments/AttachmentViewerHost.svelte index ae3c9d1c..e6548a7a 100644 --- a/web/src/lib/components/attachments/AttachmentViewerHost.svelte +++ b/web/src/lib/components/attachments/AttachmentViewerHost.svelte @@ -81,23 +81,26 @@ return () => { if (target && request !== target) return; request = null; - // Focus returns to the element that opened the viewer — the only use - // this host makes of `invoker`. `Lightbox` manages no focus of its - // own, so without this a close leaves focus on and the - // keyboard user restarts from the top of the document. + // FOCUS RESTORE IS NOT DONE HERE (TASK-2429). It used to be — this + // host focused `target.invoker` itself, because `Lightbox` managed no + // focus at all. Now the viewer holds a backdrop lease that makes every + // other body child `inert`, and the invoker is inside one of them: an + // inert element is NOT FOCUSABLE, so a focus() from here — which runs + // while the viewer is still mounted and the lease still held — would + // silently do nothing and leave the keyboard user on . // - // `isConnected` because the opener can be gone by now: an editor - // NodeView is re-rendered on any document change, and focusing a - // detached node silently does nothing on some engines and moves - // focus to on others. - const invoker = target?.invoker; - if (invoker?.isConnected) invoker.focus(); - // Deliberately only on a USER close. The lifecycle teardown below - // does not return focus: it fires because the item under the viewer - // is being replaced, so the invoker is part of a subtree on its way - // out — putting focus back into it would land the user inside - // content that is about to disappear. Where focus goes after a - // switch is the pane's business, not the viewer's. + // The only correct moment is AFTER the lease is released, which is + // inside the viewer's own teardown. So the invoker is threaded down as + // a prop instead (see the markup below) and `Lightbox` owns the whole + // restore, including the still-connected / still-focusable check that + // an editor NodeView re-render makes necessary. + // + // This does mean the restore now also runs on the LIFECYCLE teardown + // (an item switch), which this host previously refused on the grounds + // that the invoker is in a subtree on its way out. That refusal is no + // longer worth a special case: the viewer verifies the invoker still + // takes focus, and where it doesn't, focus lands on — which is + // exactly where the old no-op left it. }; } @@ -156,7 +159,9 @@ through `untrack`, so prop-sync does not work — opening a second image while the first viewer is up must produce a NEW component instance. - `wsSlug` comes off the request, per the note above. + `wsSlug` comes off the request, per the note above. So does `invoker`: the + viewer restores focus itself, after releasing the inert lease — see + `closeRequest` for why this host must not do it. --> {#key request} {#if request} @@ -164,6 +169,7 @@ images={[...request.images]} index={request.index} wsSlug={request.workspaceSlug} + invoker={request.invoker} onClose={closeRequest(request)} /> {/if} diff --git a/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts b/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts index 1aea27cc..04dfb779 100644 --- a/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts +++ b/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { flushSync, mount, unmount } from 'svelte'; +import { __resetViewerBackdropForTests } from '$lib/a11y/viewerBackdrop'; +import { _resetEscapeStackForTests } from '$lib/stores/escapeStack'; // TASK-2428. The viewer is exercised THROUGH its host, because the host is // where the two rules that matter live: an event is consumed only when both @@ -71,12 +73,16 @@ function openEvent(over: Partial = {}): ViewerEvent { } /** - * The viewer is a fixed-position overlay rendered into its host's own mount - * container, which is what lets a two-host test say WHICH host opened rather - * than only how many viewers exist. + * Every open viewer in the document. + * + * DOM SCOPE CANNOT ANSWER "WHICH HOST" ANY MORE (TASK-2429): the viewer now + * portals to ``, so both hosts' viewers are siblings there regardless of + * where their host is mounted. A two-host test proves ownership by DESTROYING + * one host and watching which viewer goes with it, plus the two-direction count + * below — not by querying inside a container. */ -function viewers(scope: ParentNode = document): HTMLElement[] { - return Array.from(scope.querySelectorAll('.lightbox-backdrop')); +function viewers(): HTMLElement[] { + return Array.from(document.body.querySelectorAll('.lightbox-backdrop')); } function viewer(): HTMLElement | null { @@ -116,6 +122,11 @@ describe('AttachmentViewerHost', () => { while (mounted.length) unmount(mounted.pop()!); target.remove(); document.querySelectorAll('.viewer-host-target').forEach((el) => el.remove()); + // The viewer holds a backdrop lease and an ESC-stack registration + // (TASK-2429). Both are module-global, so a case that ends with one open + // would otherwise leak `inert` writes and a handler into the next test. + __resetViewerBackdropForTests(); + _resetEscapeStackForTests(); }); /** Mounts a host in its OWN container, and hands that container back. */ @@ -148,24 +159,37 @@ describe('AttachmentViewerHost', () => { }); it('ignores an event addressed to the OTHER host, with both mounted', () => { - const containerA = mountHost(propsA); - const containerB = mountHost(propsB); + mountHost(propsA); + const hostA = mounted[mounted.length - 1]; + mountHost(propsB); - // Same item, other host token: exactly one viewer may open, and it must - // be the ADDRESSED one — counting viewers alone would pass if the wrong - // host opened. Matching on itemId alone would open two. + // DISTINGUISHABLE PAYLOADS, because counting alone cannot see a + // CROSS-SWAP — A answering host-2's event while B answers host-1's + // produces exactly the same counts as the correct routing. Each event + // carries its own image id, so the surviving viewer's `src` says which + // EVENT it came from, and destroying a known host says which HOST held it. notifyViewerOpen(openEvent({ hostToken: 'host-2' })); flushSync(); - + // Exactly one viewer may open. Matching on itemId alone would open two. expect(viewers()).toHaveLength(1); - expect(viewers(containerB)).toHaveLength(1); - expect(viewers(containerA)).toHaveLength(0); // ...and the reverse direction, so neither host is simply inert. - notifyViewerOpen(openEvent({ hostToken: 'host-1' })); + notifyViewerOpen( + openEvent({ hostToken: 'host-1', attachmentId: ATT_ID_2, images: [image({ id: ATT_ID_2 })] }) + ); flushSync(); - expect(viewers(containerA)).toHaveLength(1); expect(viewers()).toHaveLength(2); + + // Destroy host A: the viewer that goes with it must be the one opened by + // A's OWN token (the ATT_ID_2 event), leaving B's ATT_ID one behind. A + // cross-swap fails here — it would leave the ATT_ID_2 viewer standing. + unmount(mounted.splice(mounted.indexOf(hostA), 1)[0]); + flushSync(); + const survivors = viewers(); + expect(survivors).toHaveLength(1); + expect( + survivors[0].querySelector('.lightbox-image')?.getAttribute('src') + ).toContain(ATT_ID); }); it('ignores an event for a different item on its own token', () => { @@ -380,34 +404,35 @@ describe('AttachmentViewerHost', () => { // runs and its viewer stays on screen. Exercised open → mutate → // teardown → RE-open → mutate, because a hazard that no-ops on its // first write only shows once the state has actually moved. - const first = mountHost(propsA); - const neighbour = mountHost(propsA); + // + // Counted across the whole document rather than per container (the + // viewer portals to now): both hosts answer the same address, so + // a stranded teardown shows up as a count that fails to reach 0. + mountHost(propsA); + mountHost(propsA); notifyViewerOpen(openEvent()); flushSync(); - expect(viewers(first)).toHaveLength(1); - expect(viewers(neighbour)).toHaveLength(1); + expect(viewers()).toHaveLength(2); propsA.resourceGen = 2; flushSync(); - expect(viewers(first)).toHaveLength(0); - expect(viewers(neighbour)).toHaveLength(0); + expect(viewers()).toHaveLength(0); // Re-open on the same (now current) resource and drive it again. notifyViewerOpen(openEvent()); flushSync(); - expect(viewers(neighbour)).toHaveLength(1); + expect(viewers()).toHaveLength(2); propsA.itemId = 'item-b'; flushSync(); - expect(viewers(first)).toHaveLength(0); - expect(viewers(neighbour)).toHaveLength(0); + expect(viewers()).toHaveLength(0); - // And the neighbour is still LIVE, not merely emptied: it answers the - // new address, which it could not do if its effects had been stranded. + // And both are still LIVE, not merely emptied: they answer the new + // address, which they could not do if their effects had been stranded. notifyViewerOpen(openEvent({ itemId: 'item-b' })); flushSync(); - expect(viewers(neighbour)).toHaveLength(1); + expect(viewers()).toHaveLength(2); }); // The bound-close invariant is NOT tested here: driving it needs the close diff --git a/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts b/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts index 947d7e32..9ff42129 100644 --- a/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts +++ b/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts @@ -129,26 +129,22 @@ describe('AttachmentViewerHost — bound close', () => { expect(stub()).not.toBeNull(); }); - it('returns focus to the invoker only on the bound close', () => { + it('threads the invoker down and never moves focus itself', () => { + // TASK-2429 moved the restore INTO the viewer. It has to happen after the + // backdrop lease is released — while the lease is held, the invoker is + // inside an `inert` body child and simply will not take focus — and the + // only code that runs at that moment is the viewer's own teardown. So the + // host's job is reduced to handing the invoker over, and its close handler + // must leave `document.activeElement` exactly where it was. const invoker = target.appendChild(document.createElement('button')); const other = target.appendChild(document.createElement('button')); other.focus(); notifyViewerOpen(openEvent({ invoker })); flushSync(); - lightboxStubCalls[0].onClose(); - flushSync(); - expect(document.activeElement).toBe(invoker); + expect(lightboxStubCalls[0].invoker).toBe(invoker); - // A stale close returns nothing: it did not close anything, so moving - // the user's focus would be a jump out of whatever they are now in. - notifyViewerOpen(openEvent({ invoker })); - flushSync(); - const staleClose = lightboxStubCalls[1].onClose; - notifyViewerOpen(openEvent({ attachmentId: ATT_ID_2, invoker: null })); - flushSync(); - other.focus(); - staleClose(); + lightboxStubCalls[0].onClose(); flushSync(); expect(document.activeElement).toBe(other); }); diff --git a/web/src/lib/components/attachments/fixtures/LightboxStub.svelte b/web/src/lib/components/attachments/fixtures/LightboxStub.svelte index 51776ead..3af4eccc 100644 --- a/web/src/lib/components/attachments/fixtures/LightboxStub.svelte +++ b/web/src/lib/components/attachments/fixtures/LightboxStub.svelte @@ -12,15 +12,16 @@ images: { id: string }[]; index?: number; wsSlug: string; + invoker?: HTMLElement | null; onClose: () => void; } - let { images, index = 0, wsSlug, onClose }: Props = $props(); + let { images, index = 0, wsSlug, invoker = null, onClose }: Props = $props(); // Captured ONCE at mount, `untrack`ed like the real component's index seed: // the host remounts per open, so a recorded call belongs to exactly one // viewer instance — which is the whole point of the recording. - const call: LightboxStubCall = untrack(() => ({ images, index, wsSlug, onClose })); + const call: LightboxStubCall = untrack(() => ({ images, index, wsSlug, invoker, onClose })); lightboxStubCalls.push(call); diff --git a/web/src/lib/components/attachments/fixtures/lightboxStub.ts b/web/src/lib/components/attachments/fixtures/lightboxStub.ts index f8ad9c33..05efc2ed 100644 --- a/web/src/lib/components/attachments/fixtures/lightboxStub.ts +++ b/web/src/lib/components/attachments/fixtures/lightboxStub.ts @@ -10,6 +10,8 @@ export interface LightboxStubCall { images: { id: string }[]; index: number; wsSlug: string; + /** Threaded down by the host since TASK-2429; the viewer owns the restore. */ + invoker: HTMLElement | null; onClose: () => void; } diff --git a/web/src/lib/components/collections/PaneHost.svelte b/web/src/lib/components/collections/PaneHost.svelte index d7849e4f..88e851d7 100644 --- a/web/src/lib/components/collections/PaneHost.svelte +++ b/web/src/lib/components/collections/PaneHost.svelte @@ -350,6 +350,9 @@ // menu). Recognised by role/tag/class so pane-owned popups that portal out // to are all covered — the shared `inExemptSurface` set (paneFocus.ts) // is the SAME one the host's focus-follows classifier reuses (PLAN-2179). + // The attachment viewer (TASK-2429) is one of those portaled dialogs and is + // exempt for the same reason: it runs its own trap, so this one must not + // pull focus back out of it. // (Focus already inside `.item-pane` is handled by the `region.contains` // check at each call site.) function onTrapKeydown(e: KeyboardEvent) { diff --git a/web/src/lib/components/common/Lightbox.svelte b/web/src/lib/components/common/Lightbox.svelte index 3e0080f6..e6900e38 100644 --- a/web/src/lib/components/common/Lightbox.svelte +++ b/web/src/lib/components/common/Lightbox.svelte @@ -6,11 +6,28 @@ * (un-variant) blob so the expanded view is full resolution regardless * of the thumbnail variant shown inline. * - * Keyboard: Esc closes, ←/→ navigate when multiple images were passed. - * Backdrop click closes; clicking the image itself does not. + * MODAL CONTRACT (PLAN-2392 phase 3a / TASK-2429, DR-4b). This is a real + * modal now: `role="dialog"` + `aria-modal`, portaled to ``, focus + * entry + restore, a Tab trap, background inertness through the shared + * viewer-backdrop manager, and Escape via the shared `escapeStack`. It is + * the contract the editor's hand-rolled `showModal()` dialog was getting + * from the platform for free, written out by hand — because 3a's later + * tasks delete that dialog and route the inline body images here. + * + * Keyboard: Esc closes (through `escapeStack`, NOT a local listener), + * ←/→ navigate when multiple images were passed, Tab cycles within the + * viewer. Backdrop click closes; clicking the image itself does not. */ import { untrack } from 'svelte'; import { attachmentDownloadUrl } from '$lib/markdown/attachments'; + import { paneFocusables, nextTrapTarget } from '$lib/collections/paneFocus'; + import { + acquire, + isBlockedByModal, + isViewerFrontmost, + VIEWER_ROOT_CLASS, + } from '$lib/a11y/viewerBackdrop'; + import { pushEscapeHandler, ESCAPE_PRIORITY } from '$lib/stores/escapeStack'; export interface LightboxImage { id: string; @@ -23,9 +40,16 @@ index?: number; wsSlug: string; onClose: () => void; + /** + * The control that opened the viewer — focus goes back to it on close. + * OPTIONAL: the strip, the timeline and the NodeView thread real values in + * TASK-2431 / TASK-2433. Until then it falls back to whatever held focus + * at open (see below), which is the same element in every current path. + */ + invoker?: HTMLElement | null; } - let { images, index = 0, wsSlug, onClose }: Props = $props(); + let { images, index = 0, wsSlug, onClose, invoker = null }: Props = $props(); // Seeded once at mount — the host remounts (null → set) on each open, so // no prop-sync effect is needed. untrack makes the initial-value capture @@ -34,9 +58,39 @@ untrack(() => Math.min(Math.max(index, 0), Math.max(images.length - 1, 0))) ); + // CAPTURED AT OPEN, never read live (TASK-2429). The pane switches workspace + // without remounting whatever is above it, so a live read could rebuild the + // URLs of already-captured attachment ids against a DIFFERENT workspace — + // serving a 404, or worse, another workspace's attachment at the same id. + // `invoker` is captured for the same reason: the value that opened this + // viewer is the one to return focus to. + const openWsSlug = untrack(() => wsSlug); + // The invoker falls back to WHATEVER HELD FOCUS AT OPEN (the pattern + // `BottomSheet` uses), not to nothing. Focus entry is about to move focus + // into the viewer, so without this the producers that don't thread an + // invoker yet — the strip and the timeline, until TASK-2431 — would be + // strictly worse off than before this component managed focus at all: they + // keep focus on the clicked tile today, and a null invoker would drop it to + // `` on close. Captured at init, BEFORE the entry focus runs. + const openInvoker = untrack(() => { + if (invoker) return invoker; + if (typeof document === 'undefined') return null; + const active = document.activeElement; + return active && active !== document.body ? (active as HTMLElement) : null; + }); + let hasMultiple = $derived(images.length > 1); let img = $derived(images[current]); - let src = $derived(img ? attachmentDownloadUrl(wsSlug, img.id) : ''); + let src = $derived(img ? attachmentDownloadUrl(openWsSlug, img.id) : ''); + // The accessible name: the image's own alt where there is one, else a + // generic label. Never empty — an unnamed `role="dialog"` is announced as + // nothing at all. + let dialogLabel = $derived(img?.alt || 'Attachment viewer'); + + // The portaled root. `$state` so the effect below re-runs once `bind:this` + // lands; read-only inside every effect, so nothing here can self-invalidate + // a flush (CONVE-1688). + let rootEl = $state(null); function prev() { current = (current - 1 + images.length) % images.length; @@ -45,14 +99,144 @@ current = (current + 1) % images.length; } - function onKeydown(e: KeyboardEvent) { - if (e.key === 'Escape') { + /** + * Return focus where it came from, on close. + * + * Declines when focus has ALREADY moved somewhere outside this viewer — + * a surface opened over the viewer owns focus outright, and so does a + * producer that moves focus from its own close handler (which runs BEFORE + * this teardown). `AttachmentViewerHost` used to be exactly that producer; + * TASK-2429 moved the restore here precisely because its version ran while + * the invoker was still inert, so this guard is now about the surfaces the + * viewer does not control rather than about that host. Focus resting on + * `` / nowhere is the adrift state a teardown leaves behind, and + * counts as ours to move. + * + * The invoker is verified rather than trusted: it can have been detached + * (an editor NodeView is re-rendered on any document change), hidden, or + * inerted since it was captured, and `focus()` on such an element silently + * does nothing on some engines and drops focus to `` on others. So we + * focus it and CHECK, falling back to parking focus on `` — which is + * where the browser would have put it anyway, but deterministically, and + * without leaving focus inside a subtree that is about to be removed. + */ + function restoreFocus(root: HTMLElement): void { + const active = document.activeElement; + if (active !== null && active !== document.body && !root.contains(active)) return; + + if (openInvoker?.isConnected) { + openInvoker.focus?.({ preventScroll: true }); + if (document.activeElement === openInvoker) return; + } + (document.activeElement as HTMLElement | null)?.blur?.(); + } + + // ONE effect owns the whole modal contract, because the steps are ordered + // with respect to each other and to teardown: portal → lease → focus entry → + // Escape registration, unwound in reverse. + $effect(() => { + const el = rootEl; + if (!el) return; + + // PORTAL TO DIRECTLY — deliberately NOT `portalAction.ts`, which + // targets the nearest ancestor `` when there is one: exactly the + // wrong target for a surface that must sit ABOVE everything. A `position: + // fixed` overlay is only viewport-fixed while no ancestor establishes a + // containing block, and `transform` / `filter` / `contain: layout` on any + // ancestor silently does (see the container-query foot-gun). `` is + // the only parent with no such ancestor, and it is what the backdrop + // manager's inert bookkeeping requires: it writes `inert` on BODY CHILDREN + // and exempts this one. + document.body.appendChild(el); + + // Background inertness is the manager's, not ours — no hand-rolled + // `inert`, no `aria-hidden` sweep. It refcounts, so two viewers stacked + // (the strip's and a NodeView's) can't clobber each other's writes. + const lease = acquire(el); + + // Focus ENTRY goes to the first tabbable DESCENDANT (the close button), + // not the root: a screen-reader user landing on the container has to + // discover the controls, and the trap below cycles from wherever focus is. + // The root is `tabindex="-1"` purely as the fallback for a viewer with no + // tabbable control yet (single image, still loading) — focus must not stay + // on whatever is behind the backdrop. + const entry = paneFocusables(el)[0] ?? el; + entry.focus({ preventScroll: true }); + + // Escape has ONE owner: the shared stack. The local `` + // Escape branch this component used to carry was DELETED rather than + // gated — it ignored `defaultPrevented`, so with the stack also running, + // a single press closed the viewer AND the layer beneath it. + // + // Registered ABOVE `menu` (40) so the viewer is the innermost layer. + // Declines (returns false) unless this viewer is the FRONTMOST lease, so + // with two viewers open one press closes exactly the top one and the + // stack falls through to it rather than to an unrelated layer. + const unregisterEscape = pushEscapeHandler(() => { + if (!isViewerFrontmost(el)) return false; onClose(); - } else if (e.key === 'ArrowLeft' && hasMultiple) { + return true; + }, ESCAPE_PRIORITY.viewer); + + return () => { + unregisterEscape(); + // Release BEFORE restoring focus, and let the RESULT decide: when a + // viewer remains beneath this one the manager has already handed focus + // into it, and restoring our own invoker would yank focus out of a + // viewer the user is still looking at. Only the last one out restores. + const { stackEmpty } = lease.release(); + if (stackEmpty) restoreFocus(el); + // Svelte removes the node itself, but it is reparented out of its + // anchor — remove it explicitly so the DOM can never be left with a + // stranded backdrop, and so the manager's next reconcile sees the + // body child list it expects. + el.remove(); + }; + }); + + function onKeydown(e: KeyboardEvent) { + // A control that already handled this key owns it. + if (e.defaultPrevented) return; + const el = rootEl; + // Every mounted viewer listens on `window`, so ONLY the frontmost may act. + // This matters more than the usual layer-isolation argument: `nextTrapTarget` + // deliberately pulls out-of-container focus back INWARD, so a background + // viewer running the trap would drag focus into itself, out of the viewer + // in front of it. + if (!el || !isViewerFrontmost(el)) return; + // ...and a `showModal()` dialog opened OVER the viewer owns the top layer, + // above any body-portaled surface, so the frontmost LEASE is not + // necessarily the frontmost SURFACE. Without this the viewer's trap would + // fight a native modal for Tab and pull focus back out of it — the exact + // inward-redirect hazard the frontmost gate exists to prevent, one layer + // up. The manager already keeps such a dialog out of the inert set + // (`keepInteractiveAsDialog`), so it is reachable and this is real. + if (isBlockedByModal(el)) return; + + if (e.key === 'Tab') { + // Reuses the pane's tested trap math (paneFocus.ts) — one trap + // implementation for the whole app, not a second one here. + const target = nextTrapTarget( + paneFocusables(el), + document.activeElement, + e.shiftKey, + el + ); + if (target) { + e.preventDefault(); + target.focus({ preventScroll: true }); + } + return; + } + + if (e.key === 'ArrowLeft' && hasMultiple) { + e.preventDefault(); prev(); } else if (e.key === 'ArrowRight' && hasMultiple) { + e.preventDefault(); next(); } + // NO Escape branch. See the registration above. } // Close only on a click of the backdrop itself — clicks on the image or @@ -65,20 +249,55 @@ + +