PLAN-2392 phase 3b: desktop zoom for the attachment viewer - #1064
Draft
xarmian wants to merge 5 commits into
Draft
PLAN-2392 phase 3b: desktop zoom for the attachment viewer#1064xarmian wants to merge 5 commits into
xarmian wants to merge 5 commits into
Conversation
The DOM-free half of PLAN-2392 phase 3b's desktop zoom: scale/translate state, anchored zoom, pan clamping and the fit<->actual toggle, with no imports at all so the math is testable without layout. jsdom reports all-zero rects, so anything that reads geometry is unprovable there — keeping the numbers in a pure module is what makes them assertable. Scale is relative to fit, so FIT is the constant 1 rather than a derived quantity: the CSS (92vw/92vh + object-fit: contain) stays the fit engine and the transform multiplies from there, which keeps the module and the stylesheet in ONE coordinate system. clampState composes scale-then-pan in that order because a resize can lower MAX_SCALE beneath a currently valid scale, and clamping pan first would bound it against a scale that is about to change. Reviewed adversarially in three rounds. Round 3 took the numerical robustness angle and found two real defects, both fixed here and both pinned by tests that fail against the pre-fix code: - extent * scale could overflow to Infinity at extreme finite geometry, making the pan bound infinite and silently disabling the clamp — the one failure mode that lets blank stage show past an image edge. - a subnormal fitted extent divided to an infinite actualScale, which zoomTo's clampScale then rejected as invalid and replaced with fit, turning the double-click toggle into exactly the silent no-op that TOGGLE_SMALL_SCALE exists to prevent. The fit-epsilon boundary is asymmetric (FIT + EPSILON reads as at-fit, FIT - EPSILON does not, a few ulps of binary rounding). Left as is and documented rather than papered over: every emitted scale has been through clampScale, whose floor is FIT, so a sub-fit state cannot arise. Both legs are pinned so a future change to the floor cannot make it matter silently. Also deliberately generalises the spec's "when actualScale === 1" rule to "within FIT_EPSILON of 1". At fractional devicePixelRatio a 1:1 image measures 1.0000000004, and strict equality makes the toggle a no-op in precisely the case the rule exists to prevent. 88 unit tests; 23 hand-built mutants killed. npm run check clean; full web suite 1180 tests green.
Wire TASK-2454's pure zoom/pan math into the attachment viewer: - Stage + transform wrapper around the <img> per the module's coordinate system (92vw x 92vh stage, object-fit: contain image carrying translate/scale). pointer-events: none on the stage lets letterbox clicks reach the backdrop; the image re-enables them; controls sit above via z-index (kept < the viewer's 100000 sweep bound). - +/- zoom about the stage centre, 0 resets, all INSIDE the existing onKeydown gates (defaultPrevented / isViewerFrontmost / isBlockedByModal). Ctrl/Cmd/Alt are left untouched (no act, no preventDefault) so browser page-zoom and OS shortcuts keep working; bare =, shift, and numpad forms accepted. - Transform resets when the shown image changes (arrow nav, or the set shrinking under current), keyed on image id via a plain-let sentinel so no $effect reads a $state it also writes (CONVE-1688). - ResizeObserver on the stage re-clamps SCALE first, then pan (clampState), since enlarging the viewport lowers the geometry-dependent MAX_SCALE. - Global inert ResizeObserver shim in setup-jsdom.ts; reduced-motion suppresses the transition only. Adds zoom-key, arbitration, reset-on-navigate, resize-clamp and centre-anchor tests to Lightbox.svelte.test.ts.
…TASK-2456)
The attachment viewer claims aria-modal but dropped focus to <body> behind
its own inerted app when a focused, conditionally-rendered control unmounted:
when a MIME resolves unsafe and the set shrinks to one, the focused 'Next
image' button is removed and nothing moved focus (the Tab trap only repairs on
a later Tab; the restore only runs at teardown). A pre-existing 3a defect that
every control 3b adds inherits.
- New exported helper paneFocus.ts::handoffFocus keeps focus inside a modal
surface when a focused control leaves it. Two shapes: reactive (no departing
arg — repair after a Svelte {#if} drops the control, within the same flush)
and imperative (pass the departing control — blur it and hand off BEFORE the
caller removes/disables it, the house pattern from attachment-image.ts). The
departing control is excluded from fallback candidates so it is never
re-selected and then dropped on disable; the fallback (first tabbable, else
the tabindex=-1 container) is verified to have taken focus, else the container
backstops it. Shaped for TASK-2459 retry / TASK-2460 tap-to-load reuse.
- Lightbox wires it via an $effect keyed on the nav-visibility signal, guarded
to the frontmost, non-blocked viewer so a background viewer can't steal focus.
Reads only derived/element state and mutates DOM focus (no $state) — CONVE-1688 safe.
Tests: handoffFocus unit tests (reactive/imperative/disable/only-tabbable/
inert-refusal/no-op) with an explicit without-handoff control leg; Lightbox
same-instance shrink handoff, background-viewer non-theft, and Tab-cycles-at-
max-zoom.
…iewer input (TASK-2457)
Wheel zoom for the attachment viewer plus a scroll-restoration guard so the
viewer's own input can't strand a page restore underneath it.
Lightbox:
- Plain AND ctrl/cmd wheel zoom (DR-4) via TASK-2454's zoomTo, anchored at the
cursor (stage-local coords; the stage is untransformed). Registered
imperatively on the viewer ROOT with { passive: false } so preventDefault
works — the inert page must not scroll and ctrl/cmd+wheel must not trigger the
browser's page zoom — and so a wheel over the backdrop is consumed too. Also
stopPropagation (belt to the restoration guard). Same frontmost /
blocked-by-modal gates as onKeydown. A horizontal-only wheel (deltaY 0) is
consumed but does not zoom.
Scroll restoration (shared route infra, scroll/restore.svelte.ts):
- Its passive wheel/touchmove/keydown listeners aborted a pending restore on ANY
such event — so the viewer's own arrow-nav (shipped 3a) stranded a restore
today, and wheel-zoom would too. New exported isModalViewerScrollInput ignores
events that are defaultPrevented OR originate inside the frontmost viewer;
generalized across wheel/key/touch (touch stays native until 3d, so it is not
defaultPrevented and only the origin check catches it). Purely additive to the
user-input branch — BUG-1425's anchor handling / one-shot / budget untouched.
A genuine non-viewer scroll still aborts, as before.
Tests: wheel anchor asserted against the module (nonzero stage offset), direct
preventDefault + stopPropagation, frontmost/blocked no-op each with a positive
control, horizontal-wheel no-op (seeded so a spurious zoom-out is observable);
restore integration tests drive a real restore and assert a viewer-originated
wheel/key/touch does NOT abort it while a genuine event does, plus
isModalViewerScrollInput unit tests. New fixture RestoreHarness.svelte.
Desktop single-pointer gestures for the attachment viewer (3d keeps two-pointer pinch, double-tap and touch semantics), modelled on the captured-drag house pattern in graph/ItemGraph.svelte. - Double-click toggles fit <-> actual size, anchored at the pointer (toggleFitOrActual). Excludes presses/double-clicks on the close/nav controls, and stands down while a pan's click is being suppressed. - Primary-button pointer drag pans while zoomed, clamped by TASK-2454. Arm on pointerdown (capture only once the 4px threshold is crossed, so dblclick survives); the pan is origin + total delta, rebased whenever an external zoom (wheel / +/-/0 keys / resize) moves the transform mid-gesture so it never snaps. draggable=false + user-select:none stop native image-drag / text selection; the transform transition is dropped while dragging so the image tracks the pointer. - Drag-vs-click: a below-threshold press still closes on the backdrop; a past-threshold drag suppresses its synthesized click (owned single timer); a plain backdrop click still closes. - Arbitration: every pointer entry point (drag start, double-click, backdrop click) carries onKeydown's isViewerFrontmost / !isBlockedByModal gates, and the WHOLE gesture re-checks on every move — a drag that straddles a stacked-viewer or native-modal transition aborts (releases capture, leaves the transform). The gesture is owned by its pointerId, so a touch / second pointer can't engage, hijack, or terminate a live mouse drag; state fully resets so no gesture leaks into the next pointerdown. Tests cover the positive-pan-then-clamp acceptance, two anchored double-clicks returning (realistic click/click/dblclick on the image), all three disambiguation legs, stacked-viewer AND native-modal aborts mid-capture, and the gesture-hygiene edges (wheel/keyboard-mid-drag rebase, buttons-released teardown, lostpointercapture, stale-arm, pointer ownership, transition-off-while-dragging).
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 3b of [[PLAN-2392]] — desktop zoom on the unified attachment viewer that phase 3a shipped (PR #1060).
Scale/translate state, wheel + ctrl/⌘-wheel + keyboard + double-click, desktop drag-to-pan, and DR-5b's thumb-then-original loading policy. Touch (two-pointer pinch, double-tap,
touch-action) stays in phase 3d.Tasks
zoom.ts)Lightbox.svelte; keyboard zoom; reset on navigate; resizeDecisions worth knowing when reading this
The CSS stays the fit engine. The existing
92vw/92vh+object-fit: containalready does the fit math, soscaleis defined relative to fit —FITis the constant 1, not a derived quantity. Atscale = 1the rendering is identical to today's. Deriving fit independently in the module would have put the stylesheet and the math in two coordinate systems, which is how anchoring and pan bounds come out inconsistent.Desktop drag-pan moved here from phase 3d. The plan's phase map assigns drag-pan and clamping to 3d (touch). Zoom you cannot pan only ever shows the centre of the image, and with Pointer Events the mouse path is the same code. 3d keeps genuine multi-touch. Critically,
touch-action: nonedoes not ship in 3b — it would leave phone users with no zoom at all until 3d's pinch handler exists.The
thumb-mdfallback is not harmless, and the fix needs no server change. WebP/AVIF have no server-derived thumbnail (the pure-Go processor decodes gif/jpeg/png/bmp/tiff only) and derivation is asynchronous, so?variant=thumb-mdcan return the original. A cell that requests the thumb and then prefetches the original therefore decodes it twice — ~400 MiB for a 50 MP image. Rather than mirroring the server's decoder list client-side (a mirror that rots silently), the client uses the fact thatthumb-mdis bounded at a 1024 px long edge by definition: a decoded bitmap longer than that means the fallback served the original, so the prefetch is skipped. Works with null dimensions.One pre-existing
aria-modalviolation is folded in. Onmaintoday, when a MIME resolves unsafe and the viewer's set shrinks to one, the focused "Next image" button is removed and nothing catches focus — it lands on<body>, behind a surface that has inerted everything else. TASK-2456 fixes it; 3b would otherwise have multiplied it, since the tap-to-load affordance is replaced by the image it loads and retry vanishes on success.Review process
The decomposition went through 19 adversarial Codex rounds before any code was written, finding 11 P1s and 22 P2s. Two fresh-angle rounds returned CLEAN before it was declared converged. Rounds 17-19 were a pure coherence sweep and found nine self-contradictions the earlier revisions had accumulated — including "fit scale" silently naming two different quantities.
Each task additionally gets its own adversarial round against the branch tip, and every test carries the phase-3a bar: would this fail if the implementation were wrong? Phase 3a found nine of twelve tasks first shipped tests that could not fail on their own hardest property, and the gates caught none of them.
Test plan
Per task: the web vitest suite plus
npm run check. TASK-2461 adds the browser proof on desktop-chromium and the mobile project, mutation-checked against the binary Playwright'swebServeractually launches. No Go changes, somake test-pgis not required — the plan's DR-13 records why.Discovered and routed out: BUG-2453 (the editor's block-drag owner has no modal arbitration — a gap in the contract 3a shipped).