PLAN-2392 phase 3a: viewer unification (parity commit + DR-4b a11y contract) - #1060
Merged
Conversation
New `web/src/lib/a11y/viewerBackdrop.ts` (PLAN-2392 phase 3a): a refcounted, lease-stacked owner of `inert` on `document.body.children` for body-portaled viewer surfaces, plus the modal-arbitration helpers. - Only the FRONTMOST lease's exempt root stays interactive; release RECOMPUTES the desired inert set from the stack rather than undoing its own writes, so out-of-order release is correct. - Records only what it set, so pre-existing `inert` survives. - ONE shared childList observer, disconnected at zero leases, so a body portal arriving mid-lease is inerted too. - Releasing the frontmost lease hands focus to the next viewer's first tabbable descendant; releasing a background lease changes nothing. - `isBlockedByModal` derives from lease state (never DOM `inert`), returns false on an empty stack, and uses a feature-detected `dialog:modal` for the native branch. Nothing consumes it yet — independently green. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
`Lightbox` was a `role="presentation"` fixed div with a local `<svelte:window>`
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 `<body>` DIRECTLY — deliberately not `portalAction.ts`, which
targets the nearest ancestor `<dialog>` when one exists: the opposite of
what a top-most surface needs. `<body>` 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 `<body>` 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
…ce (TASK-2431)
DR-16's allowlist gated the image the user CLICKED. That is not a gate.
The timeline built its ←/→ sibling list from every `img[data-attachment-id]`
in the comment body with no MIME consulted at all, while the markdown renderer
emits an `<img>` for any `image/*` — correct for RENDERING, wrong for OPENING.
So a user could open a safe PNG and press Right onto an `image/svg+xml`. The
whole list is now resolved from the CACHED probe metadata and filtered through
`canOpenInViewer`, on both the mouse and the keyboard path; the index is
derived from the clicked attachment's ID rather than its DOM position, because
filtering reindexes everything after the first refusal. An unresolved MIME
fails safe and is retried on a later probe run — no cold-start regression,
since an unprobed thumbnail renders as a placeholder, not an image.
A refused thumbnail is no longer a dead control either: `role="button"`,
`tabindex` and the "View image" name now track the same predicate, so a
filtered-out SVG is not a focus stop whose activation does nothing. That pass
tracks the RENDERED set, not the fetched one — the pane's Activity / Versions
tabs rebuild every comment card without changing `entries`, and the rebuilt
images were left mouse-openable with no keyboard route at all.
The timeline also had NO A→B viewer reset — `lightbox` was cleared on close and
nowhere else — so a workspace switch under the same ref left a viewer up,
rebuilding URLs for the previous workspace's ids. It now clears on a view
change. Both direct mounts are keyed per open, like the bus host's, so the
viewer's untracked capture of its index can never be reused.
`Lightbox` re-states the rule at the point of USE, and FAILS CLOSED: only a
positively allowlisted `mime_type` is viewable, so a null / unresolved one is
not. It is the last thing between a set and a rendered image — the place where
the benefit of the doubt is worth least — and admitting null let an emitter
hand over `[safe, unresolved]` and the user arrow onto the unresolved one. The
producers lose nothing: the strip always has the MIME from its list row, and
the timeline already excludes unresolved entries. The contract for new
producers is therefore to RESOLVE BEFORE EMITTING. The filter is `$derived`
rather than captured, so a record whose MIME resolves to something unsafe
after open, a set replaced under an open viewer, or an entry removed beneath
the position the user navigated to are all re-answered rather than trusted;
the shown index clamps instead of blanking.
`LightboxImage` gains `mime_type`, `filename` and — nullable — `size_bytes`,
`width`, `height`. The dimensions have no reader yet: they land now so phase
3b's pixel-based loading policy need not reopen the event, the host and every
producer. The component's own `{id, alt}` twin is gone; the channel's
declaration is the only one. The strip threads the full row (it had been
dropping `width`/`height` at `StripAttachment` and `size_bytes` at the mapping)
and both producers now pass the invoking element, so focus returns to the tile
rather than relying on the viewer's held-focus fallback.
NOT changed: `isImageMime`. It decides `<img>` vs chip and governs deferred
share-page surfaces; this phase gates the viewer OPEN, not the render.
Tests: a mixed safe/unsafe/unresolved list driven through mouse, keyboard and a
full ←/→ cycle against the REAL viewer (what an arrow key lands on is the
claim); the set changing UNDER an open viewer — resolved-unsafe-after-open,
removed, replaced, appended; and the payload each producer emits, fed unsafe
and unresolved rows rather than only safe ones, since a stub-based payload test
on safe inputs cannot fail when the gate does. One earlier test asserted that
an unresolved MIME OPENS — it pinned the hole open, and is now the test that
it must not. Every guard was mutation-checked; each kills the tests that
cover it.
… (TASK-2430)
Global keyboard/gesture owners now consult the shared arbitration helper
(`isBlockedByModal`) instead of acting unconditionally, so the
native-`dialog:modal` branch is enforced everywhere rather than only in the
two route files TASK-2429 rewired.
This is modal-contract work, not parity work: route, graph, pane, sidebar and
sheet behaviour deliberately changes while a viewer is frontmost. With NO
viewer and no native modal the helper returns false, so EMPTY-LEASE BEHAVIOUR
IS UNCHANGED FOR EVERY OWNER — each ships an empty-stack regression test, and
forcing any owner's guard to decline unconditionally fails one (verified per
owner). Every guard is also mutation-verified to kill at least one test in the
other direction; there is no guard left that a test cannot fail on.
Two earlier revisions of this commit broke that promise and were REVERTED:
DockedSheet declining an already-`defaultPrevented` Escape, and the overflow
menu's arrow-nav being revived past `svelte-dnd-action`'s role rewrite. Both
fire with no viewer present, both are defensible on their own merits, and both
belong in their own item rather than arriving unannounced inside an attachments
phase. The one remaining empty-lease change is named and intended: `?` no
longer closes the Keyboard Shortcuts modal from inside itself, which falls out
of the native-dialog branch this task exists to enforce (Escape and its close
button still dismiss it).
The seven owners:
1. root app-shell shortcuts (+layout) — were entirely unguarded
2. both route keydown handlers (see the asymmetry below)
3. DockedSheet — an unregistered role="dialog" Escape owner
4. BottomSheet — front layer wins over the sheet-only frontmost check
5. TopBar overflow menu — Escape + Up/Down
6. Sidebar — mobile edge-open swipe and the swipe-to-close
7. ItemGraph — wheel zoom and pan; it co-mounts with the viewer
ESCAPE IS NOT ARBITRATED ON THE TWO PANE ROUTES, deliberately. Those handlers
are the only code that runs `escapeStack`, and the VIEWER's Escape lives there
— an arbitration bail above the dispatch would return first and leave a
frontmost viewer undismissable by keyboard, reintroducing exactly the dead key
TASK-2429 fixed. What DID need arbitrating is the collection route's NAVIGATION
half (j/k, arrows, h/l, Enter, Tab), which would otherwise keep re-targeting
the list under the viewer — so the guard sits below the Escape dispatch and
above the nav switch, and both bounds are asserted. The item route, being
Escape-only, gains no arbitration guard at all (its existing `defaultPrevented`
/ text-entry / `hasForeignEscapeOwner` guards are untouched). Hoisting the
guard, dropping it, and adding one to the item route are all mutation-verified
to fail a test.
`hasForeignEscapeOwner()`'s ARIA branch becomes LEASE-AWARE, because 3b changes
its premise. It used to be right that a sheet open beneath a viewer still owned
Escape — the sheets acted unconditionally. Now they stand down, so reporting
one would leave Escape with NO owner: driver returns, sheet declines, viewer's
stack never runs. The branch now counts only sheets NOT behind the frontmost
viewer, by CONTAINMENT rather than a blanket "a lease exists": a sheet nested
INSIDE the viewer is in front of its content and does still own its Escape.
The native branch is checked first and wins outright, on both the
`dialog:modal` path and the `dialog[open]` fallback: nothing in the app guards
a native `<dialog>`, so unlike a sheet it never stood down and does still own
Escape. Applying the containment rule to it as well was tried and reverted —
the fallback cannot tell a modal from a non-modal dialog, so letting the lease
out-rank it would fire the browser's native `cancel` AND run the stack, closing
two layers on one press. The residual asymmetry that leaves (a NON-modal
`<dialog open>` beside a viewer, on an engine without `:modal`) is documented
at the branch and is unreachable here twice over: `Modal.svelte` is the only
`<dialog>` in the tree and only ever calls `showModal()`, and every engine that
ships `<dialog>` ships `:modal`.
Plus two more global Escape owners found by review sweep: the workspace graph
route and the console shell. Neither can host a viewer and neither drives the
escape stack, but the root layout mounts native dialogs on both, so one press
would cancel the dialog AND mutate the layer underneath.
Captured gestures that straddle the viewer opening are gated at START and on
the captured move/end: the graph has no `lostpointercapture` handler, so its
pan is torn down (capture released) rather than merely skipped; the pane
divider ends its resize; the sheet and sidebar swipes are abandoned. The start
gates are separately load-bearing — a gesture begun under a viewer must not
come alive when the viewer closes — and are tested as such.
Owner arguments are the ACTING SURFACE (a bound element, `e.currentTarget`, or
`null` for the app shell), never `event.target`. The four WINDOW-level call
sites — +layout, TopBar, DockedSheet, BottomSheet — each have a test that
dispatches from inside the viewer, which is the case that distinguishes the two
choices; the element-bound listeners (PaneHost's divider, Sidebar's aside,
ItemGraph's viewport) cannot receive an event originating in the viewer at all,
so there is nothing to distinguish there.
Deliberately NOT guarded: pure pointer-dismissers (clickOutside, the pickers,
board lanes, and TopBar's outside-click), which only tear down lower UI.
DEFERRED, not covered here: `svelte-dnd-action`'s global drag handlers (nine
call sites) and the editor's block-drag action own gestures whose finalize can
persist a reorder if a viewer opens mid-drag. Gating them needs a reactive
lease signal rather than a call-site guard, which is a materially larger change
than this task's contract — flagged for a follow-up item.
ItemGraph's pointerup path carries NO gate: the obvious symmetry with the move
gate is unfalsifiable — teardown is identical either way, so no test can fail on
its removal — and an unkillable guard reads as coverage without being any. The
straddle is covered by the move gate, which releases the pointer capture. The
one sequence neither gate can see (a capture-less press whose release RETARGETS
to the portaled viewer, leaving `maybeDrag` latched) is pre-existing and already
mitigated by the `buttons & 1` abort in `onPointerMove`; a test now pins that
mitigation so it cannot be removed silently.
Also in this commit:
- e2e: target the create-workspace dialog by accessible name, not a bare
`dialog` role
- test infra: `$app/navigation` mock + a localStorage shim for the jsdom
project, without which Sidebar/TopBar/PaneHost/+layout could not be
mounted at all. Both Storages are cleared before every TEST (not per
setup-file load) so the shim is deterministic under any pool config; the
trade-off — in-memory stand-ins cannot reproduce real Storage failures — is
documented at the shim. NOTE: `svelte-dnd-action`'s role rewrite
(`menu`/`menuitem` → `list`/`listitem`) leaves TopBar's roving-focus query
matching nothing in the browser. Pre-existing, left as-is, documented at
both the query and its test.
The native top-layer leg of the precedence rule is not asserted against a real
engine (jsdom has no top layer and throws on `:modal`; the suite emulates it);
end-to-end proof belongs to TASK-2436's Playwright suite.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
The AttachmentImage NodeView's <img> opened the viewer on click and was reachable by no other means: no role, no tabindex, no accessible name. Give it the DR-12 button contract, and route BOTH activation paths through one shared activate(). The MIME gate used to live inside the click handler, which made it a property of the MOUSE rather than of activation — a keyboard path that opened on its own would have bypassed it. One canActivate() predicate now owns the gate and is read by activate() AND by the semantics pass, because 'can be opened' and 'announces itself as openable' are the same question and a dead focus stop is what they look like when they disagree. Keyboard handling: stops propagation before activating (ItemTimeline delegates thumbnail handlers across its whole entry list, and that list contains live CommentEditor instances rendering this very NodeView, so without it one keypress opened two viewers); ignores MODIFIED keys, since Cmd/Ctrl+Enter is CommentEditor's submit binding; and suppresses key REPEATS without re-activating, so a held key opens one viewer rather than one per repeat. Semantics are conditional on the image actually being a control — no uuid, a confirmed deletion, a load-failure placeholder, or a probed non-allowlisted MIME each take role/tabindex/aria-label back off (and blur it first) rather than leave a focus stop that announces itself as a button and does nothing. The MIME clause is a judgment call: the contract names only deleted/missing, but it is the same dead-stop rule and the same thing ItemTimeline does, and its cost is documented in place. The accessible name is alt with a GENERIC fallback: there is no filename on the node's attrs and the HEAD metadata carries none, so the filename form DR-12 sketches has no source here. DECLARED CROSS-FILE FOLD-IN — ItemTimeline.svelte no longer manages images a live editor owns (new isEditorOwnedImage predicate). Making the NodeView image focusable made two of its behaviours reachable that were not before: its accessibility pass stripped role/tabindex from any image whose UUID is not in attMeta — which is every image in a DRAFT comment, since attMeta is probed from SAVED bodies only — and its delegated keydown, having no modifier check, opened a viewer on the Cmd+Enter this node now deliberately lets through. Both are regressions this commit introduces, so both are fixed here. Its own rendered thumbnails are untouched. That wiring is covered by an integration spec that mounts the REAL ItemTimeline with a REAL unstubbed CommentEditor, rather than a copy of the delegation logic: every assertion about a guard fails when that guard is deleted from the component. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
… (TASK-2433) The inline `pad-attachment:` image NodeView opened its own hand-rolled `<dialog>`: `openImageLightbox` appended one to `document.body` and `showModal()`d it. It worked, and everything the modal contract is made of came free from the platform — top layer, inertness, focus trap, focus restore, Escape. This commit deletes it and emits on the viewer channel instead, so the `Lightbox` that `AttachmentViewerHost` mounts is the only viewer on this route, with the lease-stacked backdrop, the escape ordering and the DR-16 filter re-applied over the whole set. Shaped as a pure swap so the deletion is reviewable on its own. THE MIME IS RESOLVED BEFORE ANYTHING IS EMITTED, revising the decomposition's "keep today's positively-known gate". TASK-2431 made `Lightbox` fail closed on an unresolved MIME, so an event carrying `mime_type: null` is not "let the viewer decide" — it is a viewer that mounts and renders no image. Activation now awaits `fetchAttachmentMetadata` (a cache hit in the common case) and emits only on a positively-known allowlisted answer. That also closes a mid-phase bypass: the old gate read `knownMime` only when truthy, so a click landing before the lazy probe resolved opened the original file, and a later unsafe answer did not close it. What it costs, deliberately and temporarily, is that a `transient` probe now opens nothing — the four-branch matrix that makes the gate total is TASK-2434's. AND THE CHANNEL ENFORCES IT, because a rule only one producer follows is a convention, not an invariant — and the failure mode is silent precisely because the viewer fails closed. `notifyViewerOpen` now takes a set whose `mime_type` is non-nullable (`ViewerReadyImage` / `ViewerOpenRequest`), so a forgetful producer is a compile error rather than an image that does not open, and it refuses a set at runtime unless every entry is positively allowlisted, the way it already no-ops on a missing address. The WHOLE emission is dropped rather than the offending entry: `index` and `attachmentId` name a position in the set the producer built, and silently renumbering it would open the viewer on a different image than the one activated. `LightboxImage` is untouched — the consumer side stays nullable, because its records are live and `size_bytes` / `width` / `height` must remain optional for 3b. `events.test.ts` asserted the permissive behaviour; it now asserts the refusal, with a control so a gate that refused everything cannot pass. The await is new, so the fences are too: the gate's premises are re-checked on the far side (teardown, a uuid swap, a deletion, and a host that moved — the comment composer's address is live across an item switch), and one activation at a time. That latch is generation-stamped: it is released by the resolution's own finalizer AND by a uuid swap, which this NodeView outlives, so an unconditional release would let a superseded request unlock the one that replaced it, and a HEAD that never settles would otherwise leave the new image permanently unopenable. The `.attachment-image-lightbox` CSS goes with it; it lived in `app.css`, not the TS file, where a JS-only sweep would have left it. Tests assert the emitted PAYLOAD, not the dialog's absence — an implementation that deleted the dialog and emitted nothing satisfies "no dialog" — and a new spec drives the WHOLE route with nothing stubbed but the network: real NodeView, real bus, real `AttachmentViewerHost`, real `Lightbox`, asserting a viewer in the document, addressed to the right host, showing the un-variant original. Verified by mutation: emit-nothing, emit-unresolved-MIME, drop-the-allowlist-check at either the producer or the channel, gate the set on its first entry only, drop the one-at-a-time latch or release it unconditionally, drop any post-await fence, null the invoker, and append an overlay to `documentElement` each fail at least one test. Two of those needed cases the existing specs could not reach: every prior DR-16 test selected the node first, which pre-resolves the MIME through `canActivate()` and leaves the activation path's own check unexercised — precisely the state a body image is in when it is clicked. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
…434)
TASK-2433 made the inline-image activation path resolve the attachment's
MIME before emitting and refuse anything not positively allowlisted. That
refusal was binary: `ok` + raster opened the viewer and every other result
returned silently, leaving two states where a focused, button-announced
image swallowed the gesture — a `transient` probe, and a resolved MIME the
viewer will not take.
This completes the four-branch matrix, each arm with a destination:
- `ok` + allowlisted raster → the viewer, unchanged.
- `ok` + anything else → the options PANEL (DR-7). A REDIRECT, not a
refusal: an SVG or a PDF referenced as an inline image is a real
attachment with real options, it is just not something to hand a viewer
that would execute it. The image therefore stays a real activation
target and its accessible name names the panel — taking the semantics
off (as the binary gate did) would hide a working control, and would
make the redirect fire exactly once before the recorded MIME closed the
gate on every later tap.
- `missing` (authoritative 404) → the permanent placeholder, latched,
nothing opened.
- `transient` → the RETRYABLE placeholder. Never an open and never a
latch: only a 404 is authoritative (DR-17).
The fence is hardened to the FULL address. The whole address is captured
before the await and compared after, and both emissions stamp the CAPTURED
values — the reader is live (`CommentEditor` is reused across an item
switch) so a re-read can address the wrong host. The continuation also
re-checks `deleted` on its own terms: a delete does not change which
attachment the node points at, so a probe issued before it can resolve `ok`
afterwards with the uuid still current. Check and emit stay adjacent and
synchronous — no timer, no microtask between them.
Also adds the minimal pending contract the await needs: `aria-busy` plus a
wait cursor while the MIME resolves, cleared by the resolution's finalizer
and by a uuid swap. No new chrome.
Seam with PLAN-2411, stated in comments and deliberately not built: the
`deleted` latch is cleared ONLY by an authoritative restore signal on
2411's channel, never by editor undo — DR-17 requires Ctrl-Z to leave an
inert placeholder rather than resurrect a working attachment.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
…426, TASK-2435) The body editor fetched /server/capabilities and wrote the processor's format list onto the extension — `ext.options.supportedFormats = …`. Tiptap's `options` is a getter returning a fresh spread per access, so that write landed on a temporary; the NodeView (which snapshots `this.options` once at construction) never saw it, and rotate/crop sat permanently in the degraded "no image processor" state. Same root cause `$lib/attachments/hostAddress` exists for, so the same shape: `supportedFormats` becomes a READER the host supplies, read at the moment the toolbar gates on it. Editor.svelte closes over its own capability state; the assignment is gone. CommentEditor keeps transforms deliberately OFF — its reader is a constant `[]`, not the server's list. The new spec drives both REAL mount sites: a body toolbar built before capabilities resolve snaps to correct per-format gating afterwards, one built after is correct immediately, an unsupported format (image/tiff) stays refused with the format-specific tooltip, and the comment composer stays empty through the capability fan-out. Reverting the reader to an assignment turns all four red. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
xarmian
force-pushed
the
feat/attachment-viewer-unification
branch
from
August 6, 2026 16:53
c290dd1 to
59289ba
Compare
Phase 3a deleted a native `<dialog>` that `showModal()` was giving five
guarantees for free — top-layer stacking, background inertness, a focus trap,
focus restore and Escape — and hand-wrote each one. jsdom's `<dialog>` polyfill
(`src/test/setup-jsdom.ts`) only toggles attributes, so the phase's unit suites
cannot see ANY of those five: no inertness, no top layer, no `:modal`, no real
Tab traversal, no stacking. DR-9 says this is verified in a real browser or it
is not verified.
Three specs, 32 tests, each written against "what mutation would this catch
that a jsdom-equivalent implementation would survive":
attachment-viewer-modal.spec.ts — portal + MEASURED viewport geometry (a
`transform`/`contain` ancestor changes the rect, not the declaration); focus
entry; background inertness proven by injecting a focusable probe into every
body child AND by the REAL top-bar control, which can only go inert by
cascade; focus restore asserted as an ORDERING (the invoker is verified
UNFOCUSABLE while the viewer is up, so a restore-before-release could not
pass) AND on its DECLINE path, with a detached invoker — the ordinary case,
since the NodeView that opens the viewer is re-rendered on any document
change; the focus trap in BOTH directions, including the backward-wrap branch
(`nextTrapTarget` returns `last` only for Shift+first) and the single-control
viewer where first === last; `showModal()` vs `show()` vs a dialog mounted
closed and shown later, plus a native modal opened OVER the viewer winning
both Escape and Tab outright; paint order hit-tested against a 99999
body-portaled rival, with a raised-z-index control so the measurement is
provably sensitive to stacking (Chromium excludes inert subtrees from hit
testing, which would otherwise make it vacuous); Escape through BOTH real
route guards, asserting which layer closed; two stacked viewers; and the
mobile pane integration, where the pane's nested `inert` writes and the
backdrop's body-child writes are shown to be disjoint at every transition.
attachment-viewer-owners.spec.ts — all seven TASK-2430 owners, each with a
viewer-frontmost case AND an empty-stack regression: the six root shortcuts,
the collection route's navigation half, DockedSheet, BottomSheet, the TopBar
overflow menu, the sidebar edge swipe (including a gesture that STRADDLES the
viewer opening) and the co-mounted item graph.
attachment-viewer-parity.spec.ts — the finite parity matrix, four producers ×
{open, ←/→, Escape, backdrop click, close}; Enter/Space activation of inline
images including explicit `repeat: true` keydowns; Cmd/Ctrl+Enter still being
the comment editor's SUBMIT; hostile/long/bidi accessible names and RTL
geometry; the host lifecycle, driven through CLIENT-SIDE navigation with the
document verifiably still mounted (a `page.goto()` version would prove only
that unloading a document removes its DOM); and two-host isolation.
TWO KNOWN DEFECTS ARE RECORDED AS `test.fail()`, not papered over — BUG-2441.
One Escape over a DockedSheet or a BottomSheet closes BOTH that sheet and the
viewer. The sheets' `isBlockedByModal()` guards are correct; they are READ too
late. Both they and the route's escape driver are `window` keydown listeners,
and Svelte flushes the viewer's teardown synchronously inside the driver's
handler, so a sheet listener running later in the SAME dispatch sees an
already-empty lease stack. Invisible to the unit suites (one component's
handler, nothing releasing a lease mid-dispatch) and invisible to a
click-driven test — closing the same viewer with its Close button leaves the
sheet open, which is how it was isolated. The annotations are applied AFTER
setup, so a login/seed/navigation failure cannot hide behind them. The tests
assert the CONTRACT, so the day it is fixed they go red and the annotations
must come off. The TopBar overflow menu, checked the same way, is unaffected.
Documented gaps, stated rather than papered over: the paint-order rival is a
synthetic overlay at the picker's declared z-index (the real picker cannot be
co-present — opening the viewer by pointer dismisses it) and must be de-inerted
to be hit-testable; `expectBackgroundInert`'s floor is one behaviourally-proven
background child; the pane test's inert-set comparison identifies elements by
tag plus first class; and gestures under a frontmost viewer are dispatched
rather than delivered, since a real wheel or touch cannot reach a covered
element (the graph's baseline leg does use real input).
The shared fixture builds a real 200x150 PNG rather than reusing the 1x1 the
older attachment specs share: that one has a bad IDAT checksum, so thumbnail
decoding skips and the rendered `<img>` has no box — unclickable, and "not
visible" to Playwright. It also has to out-size the editor's image toolbar,
which is absolutely positioned over a small image's whole area.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
xarmian
force-pushed
the
feat/attachment-viewer-unification
branch
from
August 6, 2026 17:03
59289ba to
228f993
Compare
…ne layer (BUG-2441, TASK-2448)
`isBlockedByModal()` answers "is a viewer lease held RIGHT NOW". `Lightbox`'s
escape-stack handler calls `onClose()` synchronously, and Svelte flushes the
teardown — hence `lease.release()` — inside that call. Every `window` keydown
listener later in the SAME dispatch therefore asked against an empty stack, was
told "nothing is in front of you", and closed a second layer: one Escape closed
the viewer AND the `DockedSheet` / `BottomSheet` underneath it. The query was
right; the moment it was read was not.
Consumption is now recorded per EVENT. The viewer marks the dispatch it
consumed (`noteEscapeConsumedByViewer`) before closing, and `isBlockedByModal`
takes an optional event: a marked one blocks every later owner outright,
whatever the lease says by then. `runTopEscape(event)` forwards the driving
event to handlers so the viewer has something to mark; the stack itself never
reads it.
Keyed on the event object, NOT on `defaultPrevented` — that flag says only
"somebody handled this key", is set by controls that are not viewers, and
honouring it would change sheet behaviour with no viewer present. This marker
can only ever be set by a frontmost viewer, so on an empty lease stack it is
unreachable by construction.
DELIBERATE, NAMED BEHAVIOUR CHANGE — the FOURTH named parity exception of
PLAN-2392 phase 3a, alongside the three already recorded. `DockedSheet`,
`BottomSheet` and the `TopBar` overflow menu now decline an Escape a viewer has
already consumed. TASK-2430 shipped `DockedSheet` declining an already-
`defaultPrevented` Escape unannounced and it was reverted; this is approximately
that change made deliberately, with a stated reason, a narrower trigger and
tests. `TopBar` is not known to be broken today — its listener happens to run
before the route driver — but that is mount-order luck, not a guarantee, so it
is closed too.
EMPTY-STACK PARITY, per owner: with no viewer, the marker cannot exist, so each
touched call site reduces to exactly its previous expression. Asserted rather
than argued — `DockedSheet` and `BottomSheet` each gain an unmarked-Escape
regression beside the new blocked case, `TopBar`'s existing owner-5 e2e covers
both directions, and a `viewerBackdrop` unit test states the equivalence
directly (`isBlockedByModal(o, unmarked) === isBlockedByModal(o)`). The reverted
2430 `defaultPrevented` regression test still passes untouched.
The two `test.fail()` cases pinning BUG-2441 are now real assertions, each
extended with a second press proving the sheet keeps its own Escape rather than
going permanently deaf.
MUTATION-VERIFIED, both halves (TASK-2436's precedent):
• drop the viewer's mark → owners 3 and 4 fail: "the sheet is a LOWER layer
and must survive the press / element(s) not found", plus the new Lightbox
jsdom case ("expected spy to not be called at all, but actually been
called 1 times").
• drop the sheets' event argument → the same two e2e cases fail identically.
• drop the driver's `runTopEscape(e)` → the wiring contract fails
("expected … to match /runTopEscape\s*\(\s*e\s*\)/").
Gates: npm run check 0 errors; npm run test 1090 passed; the three viewer e2e
specs 32 passed.
The final full-diff review found BUG-2441 surviving by a second route. The per-event consumption mark cannot cover a HOLD: every auto-repeat keydown is a FRESH event object, and by the second one the viewer's lease is already released — so the event is unmarked and the owner underneath acts. Holding Escape closed the viewer and then the sheet or menu beneath it, from one physical press. The two route guards already rejected `e.repeat` for exactly this reason (with a comment saying so). `DockedSheet`, `BottomSheet` and `TopBar` did not. They do now. Regression tests come in the pair this file already establishes: the BLOCKED case (a repeat must not close) and the EMPTY-STACK REGRESSION (a repeat is ignored, but the next REAL press still closes) — the second is what fails if a guard declines unconditionally, which is how a deference change silently deadens a control. Mutation-verified: removing the guard fails both, restoring it passes both. Also corrects a stale comment in `attachments/events.ts` — `Lightbox` imports `LightboxImage`, it does not re-export it. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 3a of PLAN-2392. Retires the editor's hand-rolled
<dialog>lightbox and unifies every attachment-image surface onto one body-portaled viewer with a real modal contract; makes the raster MIME allowlist gate total on every open path; and makes the app shell's global key and gesture owners defer to a frontmost viewer.Not in this phase: zoom (3b), the action toolbar and DR-20 convergence (3c), touch (3d). Images open the unified viewer; files open the phase-2 panel, exactly as merged.
Why this phase is the risky one
It deletes a working native modal.
showModal()was providing five guarantees for free — top-layer stacking, an inert background, a focus trap, focus restore, and Escape — and this PR is where each stops being free. Nothing downstream catches a gap in them, and jsdom's<dialog>polyfill only toggles attributes, so none are visible to a unit test.z-index: 100000— not equivalentviewerBackdroprefcounted lease stackLightboxTab handler +paneFocusablesrestoreFocus()on teardownescapeStack, sole ownershow()vsshowModal()dialog:modalDecision record (highlights)
<dialog>means re-implementing what it gave for free.role="dialog"+aria-modal, portal to<body>(notportalAction, which targets the nearest ancestor dialog), focus entry to the first tabbable descendant, Tab trap reusingpaneFocus.ts, realinerton every body child,escapeStackas sole Escape owner.Lightboxfails closed: an entry whose MIME is not positively allowlisted is not viewable. The filter is$derived, so a record resolving to unsafe after open is re-filtered.{itemId, hostToken}addressing, so a master and a peeked pane cannot consume each other's events. Permission never travels on the event.item.id+ a dedicated generation), not the route: a collection-only URL change preserves the item and must not close the viewer; a same-resource refresh must not either.Parity exceptions (four, all named)
DockedSheet/BottomSheet/TopBarEscape handling. Every touched owner has an empty-stack regression proving behaviour with no viewer present is byte-identical.TASK-2430's global sweep is modal-contract work, not parity work: it intentionally changes route, graph, pane, sidebar and sheet behaviour while a viewer is frontmost.Defects found and fixed inside this PR
img[data-attachment-id]with no MIME, so opening a safe PNG and pressing Right could reach an SVG.z-index: 1000let the body-portaled emoji picker (99999) paint over the viewer — DR-4c's layering hazard returning via z-index.activate()survived a transient load failure, so a stale event could still open a hidden image.role/tabindexfrom draft-comment images (attMetacovers saved bodies only), and its delegated keydown had no modifier check, soCmd/Ctrl+Enter— the comment editor's submit — also opened a viewer.Test plan — gates that actually ran
make checkgreen on the integrated tip:golangci-lint,go test ./...,govulncheck,npm audit,svelte-check.main).nextTrapTarget's Shift wrap fails the trap test withReceived: null; removing the repeat guard fails both held-Escape tests. Sources restored and rebuilt each time.make test-pgnot required — no Go, no store query.Known pre-existing e2e flakes, not from this branch: BUG-2334 and BUG-2447, both passing in isolation and reproduced on baselines without these specs.
Filed, not fixed
pane-collab-teardownflakes under parallel load (pre-existing).Review note
Nine of twelve tasks first shipped green tests that could not fail on their own hardest property — one asserted the vulnerability it existed to close; one I reviewed and endorsed was false-green. All were caught by a per-task adversarial review round asking "would this fail if the implementation were wrong?", and none by the gates. That question is the reason the defect list above exists.
Draft: do not merge without Dave.
https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC