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 @@ + +