From 27de6221c45d59d916844adb1311daf2eb93a5a1 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 7 Aug 2026 00:09:28 +0000 Subject: [PATCH 1/9] feat(attachments): add pure zoom/pan math module (TASK-2454) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- web/src/lib/attachments/zoom.test.ts | 836 +++++++++++++++++++++++++++ web/src/lib/attachments/zoom.ts | 403 +++++++++++++ 2 files changed, 1239 insertions(+) create mode 100644 web/src/lib/attachments/zoom.test.ts create mode 100644 web/src/lib/attachments/zoom.ts diff --git a/web/src/lib/attachments/zoom.test.ts b/web/src/lib/attachments/zoom.test.ts new file mode 100644 index 00000000..4f2c6ac8 --- /dev/null +++ b/web/src/lib/attachments/zoom.test.ts @@ -0,0 +1,836 @@ +import { describe, it, expect } from 'vitest'; +import { + FIT, + FIT_EPSILON, + MAX_SCALE_FACTOR, + TOGGLE_SMALL_SCALE, + ZOOM_STEP, + actualScale, + clampPan, + clampScale, + clampState, + isAtFit, + maxScale, + reset, + stageCenter, + toggleFitOrActual, + zoomTo, + type Geometry, + type Point, + type ZoomState, +} from './zoom'; + +// --------------------------------------------------------------------------- +// Helpers +// +// Geometry is built the way the BROWSER builds it — `object-fit: contain`, +// which scales down to fit and never up — so the fixtures cannot drift from the +// coordinate system the module documents. Re-typing fitted sizes by hand is how +// a test ends up asserting against a geometry the CSS never produces. +// --------------------------------------------------------------------------- + +function containScale(stageW: number, stageH: number, natW: number, natH: number): number { + return Math.min(stageW / natW, stageH / natH, 1); +} + +function geom(stageW: number, stageH: number, natW: number, natH: number): Geometry { + const k = containScale(stageW, stageH, natW, natH); + return { stageW, stageH, fittedW: natW * k, fittedH: natH * k, naturalW: natW, naturalH: natH }; +} + +/** + * The image-space point currently painted under `anchor`, measured in UNSCALED + * fitted-box px from the image's centre. This is the quantity an anchored zoom + * must leave unchanged; computing it from the documented transform rather than + * from the implementation is what makes the invariance assertions independent. + */ +function imagePointUnder(state: ZoomState, g: Geometry, anchor: Point): Point { + return { + x: (anchor.x - g.stageW / 2 - state.x) / state.scale, + y: (anchor.y - g.stageH / 2 - state.y) / state.scale, + }; +} + +/** Where an image-space point paints, in stage-local px. Inverse of the above. */ +function stagePointOf(state: ZoomState, g: Geometry, d: Point): Point { + return { + x: g.stageW / 2 + state.x + state.scale * d.x, + y: g.stageH / 2 + state.y + state.scale * d.y, + }; +} + +function expectClose(actual: number, expected: number, tol = 1e-9): void { + expect(Math.abs(actual - expected)).toBeLessThanOrEqual(tol); +} + +/** The pan bound the module promises for one axis, recomputed independently. */ +function bound(stageExtent: number, fittedExtent: number, scale: number): number { + return Math.max(0, (fittedExtent * scale - stageExtent) / 2); +} + +function expectWithinBounds(state: ZoomState, g: Geometry, tol = 1e-9): void { + const bx = bound(g.stageW, g.fittedW, state.scale); + const by = bound(g.stageH, g.fittedH, state.scale); + expect(state.x).toBeLessThanOrEqual(bx + tol); + expect(state.x).toBeGreaterThanOrEqual(-bx - tol); + expect(state.y).toBeLessThanOrEqual(by + tol); + expect(state.y).toBeGreaterThanOrEqual(-by - tol); + // The centring rule: an axis that does not overflow is pinned to 0, not + // merely "within a zero-width bound" — assert it as its own property so a + // bound-only implementation that leaves 1e-16 drift is still caught. + if (bx === 0) expect(state.x).toBe(0); + if (by === 0) expect(state.y).toBe(0); +} + +// A square-ish image whose fitted box exactly fills the stage: the only shape +// for which a CORNER-anchored zoom lands exactly on the pan bound rather than +// outside it, so anchor invariance is testable there without the bounds +// legitimately overriding it. +const EXACT = geom(1000, 800, 2000, 1600); +// Landscape image in a landscape stage: fills the width, letterboxed vertically. +const WIDE = geom(1000, 800, 4000, 1000); +// Portrait image in a landscape stage: fills the height, pillarboxed. +const TALL = geom(1000, 800, 1000, 4000); +// Landscape image in a PORTRAIT stage. +const WIDE_IN_PORTRAIT = geom(800, 1000, 4000, 1000); +// An image smaller than the stage on both axes: `contain` does not upscale, so +// fit already is 1:1. +const SMALL = geom(1000, 800, 400, 300); + +describe('geometry fixtures', () => { + it('are built by the same contain rule the CSS applies', () => { + expect(EXACT.fittedW).toBe(1000); + expect(EXACT.fittedH).toBe(800); + expect(WIDE.fittedW).toBe(1000); + expect(WIDE.fittedH).toBe(250); + expect(TALL.fittedW).toBe(200); + expect(TALL.fittedH).toBe(800); + expect(SMALL.fittedW).toBe(400); + expect(SMALL.fittedH).toBe(300); + }); +}); + +describe('the contract constants', () => { + // THE ONE PLACE a literal from the spec is written down. Every other test + // references the exported constant, so behaviour tests cannot drift into + // re-typed magic numbers — but without this, renumbering MAX_SCALE_FACTOR to + // 5 or TOGGLE_SMALL_SCALE to 3 would silently change the product and keep the + // whole suite green, because every assertion would move with it. + it('carry the values PLAN-2392 specifies', () => { + expect(FIT).toBe(1); + expect(MAX_SCALE_FACTOR).toBe(4); + expect(TOGGLE_SMALL_SCALE).toBe(2); + expect(FIT_EPSILON).toBe(0.001); + expect(ZOOM_STEP).toBe(1.25); + }); +}); + +describe('reset', () => { + it('is fit, centred', () => { + expect(reset()).toEqual({ scale: FIT, x: 0, y: 0 }); + }); + + it('returns a fresh object each call so callers cannot alias state', () => { + const a = reset(); + a.x = 99; + expect(reset().x).toBe(0); + }); +}); + +describe('actualScale', () => { + it('is the painted bitmap over the fitted box', () => { + expect(actualScale(WIDE)).toBe(4000 / 1000); + expect(actualScale(TALL)).toBe(1000 / 200); + }); + + it('is exactly fit for an image smaller than the stage — contain never upscales', () => { + expect(actualScale(SMALL)).toBe(FIT); + }); + + it('is the WIDTH pair, not the height pair', () => { + // `contain` preserves the aspect ratio, so on every realistic fixture the + // two ratios agree and a height-based implementation is indistinguishable. + // Pin the axis the contract names with a deliberately non-uniform + // geometry — measurements DO diverge slightly per-axis once layout is + // snapped to a fractional device-pixel grid. + const nonUniform: Geometry = { + stageW: 1000, + stageH: 800, + fittedW: 1000, + fittedH: 100, + naturalW: 3000, + naturalH: 900, + }; + expect(actualScale(nonUniform)).toBe(3); + expect(actualScale(nonUniform)).not.toBe(nonUniform.naturalH / nonUniform.fittedH); + }); + + it('falls back to fit rather than dividing by a degenerate measurement', () => { + expect(actualScale({ ...WIDE, fittedW: 0 })).toBe(FIT); + expect(actualScale({ ...WIDE, fittedW: Number.NaN })).toBe(FIT); + expect(actualScale({ ...WIDE, naturalW: 0 })).toBe(FIT); + }); + + it('grows when a higher-resolution bitmap swaps in under the same layout', () => { + // The thumb-then-original path (TASK-2459): same fitted box, bigger + // bitmap, so 1:1 — and with it the zoom ceiling — moves. + const thumb = { ...WIDE, naturalW: 1024, naturalH: 256 }; + const original = { ...WIDE, naturalW: 4000, naturalH: 1000 }; + expect(maxScale(original)).toBeGreaterThan(maxScale(thumb)); + }); +}); + +describe('maxScale', () => { + it('is MAX_SCALE_FACTOR times 1:1', () => { + expect(maxScale(WIDE)).toBe(actualScale(WIDE) * MAX_SCALE_FACTOR); + }); + + it('stays finite when a nonsensical bitmap size overflows the multiplication', () => { + // A finite ratio can still overflow: an infinite ceiling would disable the + // scale clamp altogether rather than merely raising it. + // fittedW must be 1 for the RATIO itself to reach MAX_VALUE — with a + // larger fitted box the ratio is small enough that x4 stays finite and the + // guard is never exercised. + const absurd: Geometry = { + stageW: 1, + stageH: 1, + fittedW: 1, + fittedH: 1, + naturalW: Number.MAX_VALUE, + naturalH: Number.MAX_VALUE, + }; + expect(actualScale(absurd)).toBe(Number.MAX_VALUE); + expect(Number.isFinite(actualScale(absurd))).toBe(true); + expect(Number.isFinite(maxScale(absurd))).toBe(true); + expect(clampScale(Number.MAX_VALUE, absurd)).toBeLessThanOrEqual(maxScale(absurd)); + }); + + it('floors at fit so a small image still has a zoom range', () => { + expect(maxScale(SMALL)).toBe(MAX_SCALE_FACTOR); + // A nonsensical sub-1 actual scale must not invert the bounds. + const upscaled: Geometry = { ...SMALL, fittedW: 800, fittedH: 600 }; + expect(actualScale(upscaled)).toBeLessThan(FIT); + expect(maxScale(upscaled)).toBe(MAX_SCALE_FACTOR); + }); +}); + +describe('clampScale', () => { + it('clamps to the fit floor and the maxScale ceiling', () => { + expect(clampScale(0.25, WIDE)).toBe(FIT); + expect(clampScale(1e6, WIDE)).toBe(maxScale(WIDE)); + expect(clampScale(2.5, WIDE)).toBe(2.5); + }); + + it('coerces a non-finite scale to fit rather than emitting NaN into a CSS string', () => { + // Infinity falls back rather than saturating: it only ever arrives from a + // caller-side arithmetic bug, and snapping it to maximum zoom would look + // like a deliberate gesture. + expect(clampScale(Number.NaN, WIDE)).toBe(FIT); + expect(clampScale(Number.POSITIVE_INFINITY, WIDE)).toBe(FIT); + expect(clampScale(Number.NEGATIVE_INFINITY, WIDE)).toBe(FIT); + }); + + it('normalises negative zero away — a centred axis is exactly +0', () => { + expect(Object.is(clampPan({ scale: 2, x: -5, y: -5 }, WIDE).y, 0)).toBe(true); + expect(Object.is(clampPan({ scale: 2, x: -0, y: -0 }, WIDE).x, 0)).toBe(true); + }); +}); + +describe('clampPan', () => { + it('centres an axis whose scaled extent fits inside the stage', () => { + // WIDE at 2x: the width overflows, the height still does not. + const clamped = clampPan({ scale: 2, x: 300, y: 300 }, WIDE); + expect(clamped.y).toBe(0); + expect(clamped.x).toBe(300); + }); + + it('centres BOTH axes at fit — the parity property', () => { + for (const g of [EXACT, WIDE, TALL, WIDE_IN_PORTRAIT, SMALL]) { + expect(clampPan({ scale: FIT, x: 500, y: -500 }, g)).toEqual({ scale: FIT, x: 0, y: 0 }); + } + }); + + it('bounds all four edges of a wider-than-stage image', () => { + const g = WIDE; + const s = 2; + const bx = bound(g.stageW, g.fittedW, s); // 500 + expect(bx).toBe(500); + expect(clampPan({ scale: s, x: 10_000, y: 0 }, g).x).toBe(bx); + expect(clampPan({ scale: s, x: -10_000, y: 0 }, g).x).toBe(-bx); + // The vertical axis is the non-overflowing one here: both directions pin to 0. + expect(clampPan({ scale: s, x: 0, y: 10_000 }, g).y).toBe(0); + expect(clampPan({ scale: s, x: 0, y: -10_000 }, g).y).toBe(0); + }); + + it('bounds all four edges of a taller-than-stage image', () => { + const g = TALL; + const s = 3; + const bx = bound(g.stageW, g.fittedW, s); // 200*3=600 > 1000? no -> 0 + const by = bound(g.stageH, g.fittedH, s); // 800*3=2400 -> 800 + expect(bx).toBe(0); + expect(by).toBe(800); + expect(clampPan({ scale: s, x: 10_000, y: 0 }, g).x).toBe(0); + expect(clampPan({ scale: s, x: -10_000, y: 0 }, g).x).toBe(0); + expect(clampPan({ scale: s, x: 0, y: 10_000 }, g).y).toBe(by); + expect(clampPan({ scale: s, x: 0, y: -10_000 }, g).y).toBe(-by); + }); + + it('bounds a corner — both axes at once, in both orientations', () => { + for (const g of [EXACT, WIDE_IN_PORTRAIT]) { + const s = 4; + const bx = bound(g.stageW, g.fittedW, s); + const by = bound(g.stageH, g.fittedH, s); + for (const [sx, sy] of [ + [1, 1], + [1, -1], + [-1, 1], + [-1, -1], + ]) { + const c = clampPan({ scale: s, x: sx * 10_000, y: sy * 10_000 }, g); + // `|| 0` only normalises the -0 a zero bound produces on the test + // side; a real bound is never zero, so no expectation is weakened. + expect(c.x).toBe(sx * bx || 0); + expect(c.y).toBe(sy * by || 0); + } + } + }); + + it('leaves an in-bounds offset untouched', () => { + expect(clampPan({ scale: 2, x: 123.5, y: 0 }, WIDE)).toEqual({ scale: 2, x: 123.5, y: 0 }); + }); + + it('does not mutate its argument', () => { + const state = { scale: 2, x: 10_000, y: 10_000 }; + clampPan(state, WIDE); + expect(state).toEqual({ scale: 2, x: 10_000, y: 10_000 }); + }); + + it('passes scale through — clamping it is clampScale/clampState work', () => { + expect(clampPan({ scale: 99, x: 0, y: 0 }, WIDE).scale).toBe(99); + expect(clampPan({ scale: Number.NaN, x: 0, y: 0 }, WIDE).scale).toBe(FIT); + }); +}); + +describe('clampState (the resize path)', () => { + it('clamps scale FIRST, then bounds pan against the NEW scale', () => { + // Enlarging the window lowers actualScale and with it maxScale, stranding + // a scale above the new ceiling (TASK-2455). + const before = geom(1000, 800, 4000, 1000); // actual 4 -> max 16 + const after = geom(2000, 1600, 4000, 1000); // fitted 2000x500, actual 2 -> max 8 + const zoomed = { scale: 16, x: bound(before.stageW, before.fittedW, 16), y: 0 }; + const next = clampState(zoomed, after); + expect(next.scale).toBe(maxScale(after)); + expectWithinBounds(next, after); + // Pan-first would have kept the OLD scale's much larger bound and left x + // far outside the new one; assert the value, not just the invariant. + expect(next.x).toBe(bound(after.stageW, after.fittedW, maxScale(after))); + }); + + it('preserves a scale that is still inside the new bounds', () => { + const after = geom(2000, 1600, 4000, 1000); + expect(clampState({ scale: 3, x: 0, y: 0 }, after).scale).toBe(3); + }); +}); + +describe('zoomTo anchor invariance', () => { + const anchors: Array<[string, Point]> = [ + ['stage centre', { x: 500, y: 400 }], + ['an off-centre interior point', { x: 250, y: 600 }], + ['the top-left corner', { x: 0, y: 0 }], + ]; + + for (const [label, anchor] of anchors) { + it(`keeps the image point under ${label} under it`, () => { + const from: ZoomState = { scale: 1, x: 0, y: 0 }; + const before = imagePointUnder(from, EXACT, anchor); + const after = zoomTo(from, 2, anchor, EXACT); + const painted = stagePointOf(after, EXACT, before); + expectClose(painted.x, anchor.x); + expectClose(painted.y, anchor.y); + }); + } + + it('holds across a chain of steps from an already-panned state', () => { + const anchor = { x: 812.5, y: 137.25 }; + let state: ZoomState = clampPan({ scale: 2, x: -200, y: -150 }, EXACT); + for (let i = 0; i < 6; i++) { + const before = imagePointUnder(state, EXACT, anchor); + const next = zoomTo(state, state.scale * ZOOM_STEP, anchor, EXACT); + // Once the chain reaches the ceiling the scale stops changing, and an + // unchanged scale trivially preserves the anchor — assert only while + // the step is real. + if (next.scale !== state.scale) { + const painted = stagePointOf(next, EXACT, before); + expectClose(painted.x, anchor.x, 1e-8); + expectClose(painted.y, anchor.y, 1e-8); + } + state = next; + } + expect(state.scale).toBeGreaterThan(2); + }); + + it('is not a no-op dressed up as invariance — the transform actually moves', () => { + // The invariance assertions above all pass for an implementation that + // refuses to zoom at all. This is the control leg. + const out = zoomTo({ scale: 1, x: 0, y: 0 }, 2, { x: 0, y: 0 }, EXACT); + expect(out.scale).toBe(2); + expect(out.x).not.toBe(0); + expect(out.y).not.toBe(0); + }); + + it('lets the pan bounds win over the anchor when they conflict', () => { + // A corner anchor on a letterboxed image would need an offset outside the + // bound; showing blank stage past an image edge is worse than losing the + // anchor, so the bound wins — and the non-overflowing axis stays centred. + const out = zoomTo({ scale: 1, x: 0, y: 0 }, 2, { x: 0, y: 0 }, WIDE); + expectWithinBounds(out, WIDE); + expect(out.x).toBe(bound(WIDE.stageW, WIDE.fittedW, 2)); + expect(out.y).toBe(0); + }); + + it('clamps the target scale into [FIT, maxScale]', () => { + const g = WIDE; + expect(zoomTo({ scale: 2, x: 0, y: 0 }, 0.1, { x: 0, y: 0 }, g).scale).toBe(FIT); + expect(zoomTo({ scale: 2, x: 0, y: 0 }, 1e6, { x: 0, y: 0 }, g).scale).toBe(maxScale(g)); + }); + + it('zooming back out to fit re-centres both axes', () => { + const zoomed = zoomTo({ scale: 1, x: 0, y: 0 }, 3, { x: 0, y: 0 }, EXACT); + const back = zoomTo(zoomed, FIT, { x: 0, y: 0 }, EXACT); + expect(back).toEqual({ scale: FIT, x: 0, y: 0 }); + }); + + it('falls back to the stage centre for a non-finite anchor', () => { + const centred = zoomTo({ scale: 1, x: 0, y: 0 }, 2, stageCenter(EXACT), EXACT); + const nan = zoomTo({ scale: 1, x: 0, y: 0 }, 2, { x: Number.NaN, y: Number.NaN }, EXACT); + expect(nan).toEqual(centred); + }); + + it('does not mutate its argument', () => { + const state = { scale: 1, x: 0, y: 0 }; + zoomTo(state, 3, { x: 0, y: 0 }, EXACT); + expect(state).toEqual({ scale: 1, x: 0, y: 0 }); + }); + + it('normalises an already-invalid incoming state before anchoring', () => { + // The shape a consumer hits after a resize lowers the ceiling under a state + // it has not re-clamped yet. Clamping the incoming SCALE while trusting its + // PAN would apply a pan belonging to the old scale, so the invariant must + // be stated against the normalised state — assert it exactly that way. + const g = WIDE; + // Pan out of bounds for its own scale, which is what a shrinking resize + // leaves behind. Bound at scale 2 is 500, so 700 is 200px past it. + const stale: ZoomState = { scale: 2, x: 700, y: 0 }; + const normalised = clampState(stale, g); + expect(normalised.x).toBe(500); + + const out = zoomTo(stale, 2.2, stageCenter(g), g); + expect(out).toEqual(zoomTo(normalised, 2.2, stageCenter(g), g)); + expectWithinBounds(out, g); + + // Not vacuous: the rejected alternative — clamp the incoming SCALE, trust + // the incoming PAN — is written out here and lands somewhere else, so the + // assertion above has something to catch. A centre anchor scales the pan + // by k, and 700 * 1.1 = 770 exceeds the new bound while 500 * 1.1 does not. + expect(out.x).toBe(550); + expect(clampPan({ scale: 2.2, x: 700 * 1.1, y: 0 }, g).x).toBe(600); + }); +}); + +describe('the drag contract (TASK-2458 consumes this)', () => { + // A drag MUST be computed from the state captured at pointer-down plus the + // total pointer delta. These two legs pin why: clamping is lossy, so feeding + // each move delta into the previous clamped result silently changes the + // gesture's feel at an edge. + const g = WIDE; + const scale = 2; + const bx = bound(g.stageW, g.fittedW, scale); // 500 + + it('takes up the slack when the drag is computed from the pointer-down baseline', () => { + const start: ZoomState = { scale, x: 0, y: 0 }; + const overshoot = clampPan({ ...start, x: bx + 150 }, g); + expect(overshoot.x).toBe(bx); + // Pointer comes back 30px: total delta is +bx+120, still inside the bound. + const back = clampPan({ ...start, x: bx + 150 - 30 }, g); + expect(back.x).toBe(bx); + }); + + it('moves immediately — the wrong feel — if deltas are accumulated instead', () => { + // The control leg. Same pointer path, incremental composition: the image + // starts moving the instant the pointer reverses, 30px early. + const wrong = clampPan({ scale, x: clampPan({ scale, x: bx + 150, y: 0 }, g).x - 30, y: 0 }, g); + expect(wrong.x).toBe(bx - 30); + expect(wrong.x).not.toBe(bx); + }); + + it('an in-bounds drag moves by exactly the pointer delta', () => { + // The positive leg: "pan is clamped" alone passes with pan disabled. + const moved = clampPan({ scale, x: 0 + 123.75, y: 0 }, g); + expect(moved.x).toBe(123.75); + }); +}); + +describe('stageCenter', () => { + it('is half the stage box', () => { + expect(stageCenter(WIDE)).toEqual({ x: 500, y: 400 }); + }); +}); + +describe('isAtFit / toggleFitOrActual', () => { + it('at fit, goes to actual size anchored at the pointer', () => { + const anchor = { x: 700, y: 300 }; + const before = imagePointUnder(reset(), WIDE, anchor); + const out = toggleFitOrActual(reset(), anchor, WIDE); + expect(out.scale).toBe(actualScale(WIDE)); + // Anchored, to the extent the bounds allow: the overflowing axis keeps the + // point, the centred axis cannot. + const painted = stagePointOf(out, WIDE, before); + expectClose(painted.x, anchor.x); + expectWithinBounds(out, WIDE); + }); + + it('away from fit, goes back to fit CENTRED regardless of the anchor', () => { + const zoomed = zoomTo(reset(), 3, { x: 0, y: 0 }, WIDE); + expect(toggleFitOrActual(zoomed, { x: 900, y: 700 }, WIDE)).toEqual(reset()); + }); + + it('returns on a second toggle — the gesture is two-way', () => { + const anchor = { x: 700, y: 300 }; + const once = toggleFitOrActual(reset(), anchor, WIDE); + const twice = toggleFitOrActual(once, anchor, WIDE); + expect(once.scale).toBeGreaterThan(FIT); + expect(twice).toEqual(reset()); + }); + + it('treats the epsilon boundary as at-fit, and just past it as not', () => { + const inside: ZoomState = { scale: FIT + FIT_EPSILON, x: 0, y: 0 }; + const outside: ZoomState = { scale: FIT + FIT_EPSILON * 2, x: 0, y: 0 }; + expect(isAtFit(inside)).toBe(true); + expect(isAtFit(outside)).toBe(false); + expect(toggleFitOrActual(inside, { x: 500, y: 400 }, WIDE).scale).toBe(actualScale(WIDE)); + expect(toggleFitOrActual(outside, { x: 500, y: 400 }, WIDE)).toEqual(reset()); + }); + + it('is never a no-op on an image whose actual size IS fit', () => { + const out = toggleFitOrActual(reset(), { x: 500, y: 400 }, SMALL); + expect(actualScale(SMALL)).toBe(FIT); + expect(out.scale).toBe(TOGGLE_SMALL_SCALE); + // And it still toggles back. + expect(toggleFitOrActual(out, { x: 500, y: 400 }, SMALL)).toEqual(reset()); + }); + + it('is never a no-op when fractional geometry puts actual size a hair above fit', () => { + // A 1:1 image measured at dpr 2.75: the ratio is 1.0000000004, not 1, so an + // `=== 1` test would toggle to a scale the user cannot see. + const hair: Geometry = { ...SMALL, naturalW: 400.0000001, naturalH: 300.0000001 }; + expect(actualScale(hair)).toBeGreaterThan(FIT); + expect(toggleFitOrActual(reset(), { x: 500, y: 400 }, hair).scale).toBe(TOGGLE_SMALL_SCALE); + }); +}); + +// --------------------------------------------------------------------------- +// Fractional geometry / non-integer devicePixelRatio. +// +// The browser lays out on DEVICE-pixel boundaries, which are not CSS-pixel +// boundaries at dpr 1.25 / 1.5 / 2.75 — so every measurement the viewer reads +// is fractional there. An implementation that rounds looks correct on the +// integer fixtures above and drifts the anchored point visibly across a +// sequence of steps. +// --------------------------------------------------------------------------- + +describe('fractional geometry and non-integer devicePixelRatio', () => { + function snap(css: number, dpr: number): number { + return Math.round(css * dpr) / dpr; + } + + const dprs = [1.25, 1.5, 1.75, 2.5, 2.75]; + + for (const dpr of dprs) { + describe(`dpr ${dpr}`, () => { + // 92vw x 92vh of an odd viewport, snapped to the device grid. + const stageW = snap(0.92 * 1367, dpr); + const stageH = snap(0.92 * 769, dpr); + const g = geom(stageW, stageH, 3021, 2013); + + it('produces non-integer geometry (otherwise the case proves nothing)', () => { + expect(Number.isInteger(g.fittedW) && Number.isInteger(g.fittedH)).toBe(false); + }); + + it('keeps the anchored point under the cursor to well under one device pixel', () => { + const tol = 1 / dpr / 1000; + const anchor = { x: stageW * 0.317, y: stageH * 0.733 }; + let state = reset(); + for (let i = 0; i < 8; i++) { + const before = imagePointUnder(state, g, anchor); + const next = zoomTo(state, state.scale * ZOOM_STEP, anchor, g); + const bx = bound(g.stageW, g.fittedW, next.scale); + const by = bound(g.stageH, g.fittedH, next.scale); + const painted = stagePointOf(next, g, before); + // Only where the bounds did not legitimately override the anchor. + if (Math.abs(next.x) < bx - 1 && next.scale !== state.scale) { + expectClose(painted.x, anchor.x, tol); + } + if (Math.abs(next.y) < by - 1 && next.scale !== state.scale) { + expectClose(painted.y, anchor.y, tol); + } + expectWithinBounds(next, g); + state = next; + } + }); + + it('does not round the transform to whole CSS pixels', () => { + const out = zoomTo(reset(), 2.5, { x: stageW * 0.13, y: stageH * 0.87 }, g); + expect(Number.isInteger(out.x) && Number.isInteger(out.y)).toBe(false); + }); + + it('bounds a fractional geometry exactly, with no gap or overshoot', () => { + const s = maxScale(g); + const far = clampPan({ scale: s, x: 1e9, y: 1e9 }, g); + expectClose(far.x, bound(g.stageW, g.fittedW, s), 0); + expectClose(far.y, bound(g.stageH, g.fittedH, s), 0); + }); + }); + } +}); + +// --------------------------------------------------------------------------- +// RTL. +// +// The module's coordinates are STAGE-LOCAL — measured from the stage box's +// physical left edge, which is what `getBoundingClientRect()` reports in either +// direction. `direction: rtl` mirrors the LAYOUT, not that measurement, so the +// math must be exactly direction-symmetric: mirror the anchor and the pan, and +// the result must be the mirror of the original. Any place the implementation +// leaked a signed horizontal direction (an inline-start assumption, a +// `stageW - x`) breaks this and nothing else does. +// --------------------------------------------------------------------------- + +describe('RTL / horizontal direction symmetry', () => { + function mirrorPoint(p: Point, g: Geometry): Point { + return { x: g.stageW - p.x, y: p.y }; + } + function mirrorState(s: ZoomState): ZoomState { + return { scale: s.scale, x: -s.x, y: s.y }; + } + + const cases: Array<[string, Geometry]> = [ + ['exact fill', EXACT], + ['landscape in landscape', WIDE], + ['portrait in landscape', TALL], + ['landscape in portrait', WIDE_IN_PORTRAIT], + ]; + + for (const [label, g] of cases) { + it(`zoomTo is mirror-symmetric (${label})`, () => { + const anchor = { x: g.stageW * 0.19, y: g.stageH * 0.62 }; + const start: ZoomState = clampPan({ scale: 2, x: 37.5, y: -12.25 }, g); + const out = zoomTo(start, 3.5, anchor, g); + const mirrored = zoomTo(mirrorState(start), 3.5, mirrorPoint(anchor, g), g); + expect(mirrored.scale).toBe(out.scale); + expectClose(mirrored.x, -out.x, 1e-9); + expectClose(mirrored.y, out.y, 1e-9); + }); + + it(`clampPan is mirror-symmetric (${label})`, () => { + const s = 3; + const state: ZoomState = { scale: s, x: 1e9, y: 1e9 }; + const a = clampPan(state, g); + const b = clampPan(mirrorState(state), g); + expectClose(b.x, -a.x, 0); + }); + } + + it('the symmetry assertion can fail — a direction-flipped anchor breaks it', () => { + // Control leg: proves the mirror test is load-bearing rather than + // tautological. Mirroring the anchor WITHOUT mirroring the pan is exactly + // the bug shape the test exists to catch. + const g = EXACT; + const anchor = { x: g.stageW * 0.19, y: g.stageH * 0.62 }; + const start: ZoomState = { scale: 2, x: 37.5, y: 0 }; + const out = zoomTo(start, 3.5, anchor, g); + const halfMirrored = zoomTo(start, 3.5, mirrorPoint(anchor, g), g); + expect(Math.abs(halfMirrored.x - -out.x)).toBeGreaterThan(1); + }); +}); + +// --------------------------------------------------------------------------- +// The hard bound: NO sequence of operations escapes [FIT, maxScale] or the pan +// bounds. Individually-correct operations can still compose into an escape — +// this is the property the phase's review round called binding. +// --------------------------------------------------------------------------- + +describe('bounds no sequence of operations can escape', () => { + function lcg(seed: number): () => number { + let s = seed >>> 0; + return () => { + s = (Math.imul(s, 1664525) + 1013904223) >>> 0; + return s / 0x1_0000_0000; + }; + } + + const fixtures: Array<[string, Geometry]> = [ + ['exact fill', EXACT], + ['landscape in landscape', WIDE], + ['portrait in landscape', TALL], + ['landscape in portrait', WIDE_IN_PORTRAIT], + ['smaller than the stage', SMALL], + ['fractional', geom(1257.6, 707.2, 3021, 2013)], + ]; + + for (const [label, g] of fixtures) { + it(`holds over 2000 pseudo-random operations (${label})`, () => { + const rand = lcg(0xc0ffee); + let state = reset(); + let sawCeiling = false; + let sawFloor = false; + let sawBoundedPan = false; + + for (let i = 0; i < 2000; i++) { + const anchor = { x: rand() * g.stageW * 1.4 - g.stageW * 0.2, y: rand() * g.stageH * 1.4 - g.stageH * 0.2 }; + const roll = rand(); + if (roll < 0.35) { + state = zoomTo(state, state.scale * ZOOM_STEP, anchor, g); + } else if (roll < 0.6) { + state = zoomTo(state, state.scale / ZOOM_STEP, anchor, g); + } else if (roll < 0.7) { + // A wild target, as a wheel burst or a hostile caller would give. + state = zoomTo(state, (rand() - 0.5) * 1e4, anchor, g); + } else if (roll < 0.85) { + // A drag: an unclamped offset handed straight to clampPan. + state = clampPan( + { ...state, x: state.x + (rand() - 0.5) * 4000, y: state.y + (rand() - 0.5) * 4000 }, + g, + ); + } else if (roll < 0.95) { + state = toggleFitOrActual(state, anchor, g); + } else { + state = clampState(state, g); + } + + expect(Number.isFinite(state.scale)).toBe(true); + expect(Number.isFinite(state.x)).toBe(true); + expect(Number.isFinite(state.y)).toBe(true); + expect(state.scale).toBeGreaterThanOrEqual(FIT); + expect(state.scale).toBeLessThanOrEqual(maxScale(g)); + expectWithinBounds(state, g); + + if (state.scale === maxScale(g)) sawCeiling = true; + if (state.scale === FIT) sawFloor = true; + if ( + state.scale > FIT && + Math.abs(state.x) === bound(g.stageW, g.fittedW, state.scale) && + state.x !== 0 + ) { + sawBoundedPan = true; + } + } + + // Coverage guards: without these the invariants above could pass on a + // walk that never approached a bound. + expect(sawCeiling).toBe(true); + expect(sawFloor).toBe(true); + expect(sawBoundedPan).toBe(true); + }); + } +}); + +describe('degenerate geometry', () => { + const zero: Geometry = { stageW: 0, stageH: 0, fittedW: 0, fittedH: 0, naturalW: 0, naturalH: 0 }; + + it('never emits NaN into the transform', () => { + for (const out of [ + zoomTo(reset(), 3, { x: 10, y: 10 }, zero), + clampPan({ scale: 3, x: 10, y: 10 }, zero), + toggleFitOrActual(reset(), { x: 10, y: 10 }, zero), + clampState({ scale: 99, x: 10, y: 10 }, zero), + ]) { + expect(Number.isFinite(out.scale)).toBe(true); + expect(Number.isFinite(out.x)).toBe(true); + expect(Number.isFinite(out.y)).toBe(true); + } + }); + + it('centres everything when there is no geometry to pan within', () => { + expect(clampPan({ scale: 3, x: 10, y: 10 }, zero)).toEqual({ scale: 3, x: 0, y: 0 }); + }); +}); + +/** + * Round 3 of the per-task Codex review took the numerical-robustness angle and + * found three P2s. Each is pinned here by a test that FAILS against the code as + * it stood before the fix — the point of the exercise, since a test that passes + * either way would have let all three back in. + */ +describe('extreme finite geometry (round 3)', () => { + it('keeps the pan bound finite when extent * scale would overflow', () => { + // Before the fix `fittedW * scale` was Infinity, so the bound was + // Infinity, so clampSymmetric left ANY offset untouched — the clamp + // silently stopped clamping and blank stage could show past the image. + const huge: Geometry = { + stageW: Number.MAX_VALUE, + stageH: 1000, + fittedW: Number.MAX_VALUE * 0.6, + fittedH: 800, + naturalW: Number.MAX_VALUE * 0.6, + naturalH: 800, + }; + const out = clampPan({ scale: 2, x: Number.MAX_VALUE, y: 0 }, huge); + expect(Number.isFinite(out.x)).toBe(true); + expect(out.x).toBeLessThan(Number.MAX_VALUE); + }); + + it('does not let a subnormal fitted extent turn actualScale infinite', () => { + const subnormal: Geometry = { + stageW: 1000, + stageH: 1000, + fittedW: Number.MIN_VALUE, + fittedH: Number.MIN_VALUE, + naturalW: 4000, + naturalH: 4000, + }; + expect(Number.isFinite(actualScale(subnormal))).toBe(true); + }); + + it('leaves the double-click toggle doing something visible on that geometry', () => { + // The regression this guards: an infinite actualScale reached zoomTo, + // clampScale rejected it as non-finite and fell back to fit, and the + // toggle became the silent no-op TOGGLE_SMALL_SCALE exists to prevent. + const subnormal: Geometry = { + stageW: 1000, + stageH: 1000, + fittedW: Number.MIN_VALUE, + fittedH: Number.MIN_VALUE, + naturalW: 4000, + naturalH: 4000, + }; + const out = toggleFitOrActual(reset(), { x: 500, y: 500 }, subnormal); + expect(out.scale).toBeGreaterThan(FIT + FIT_EPSILON); + }); +}); + +describe('the fit epsilon boundary is asymmetric, and cannot matter (round 3)', () => { + it('reads FIT + FIT_EPSILON as at-fit but FIT - FIT_EPSILON as not', () => { + // Neither value is exactly representable in binary; the subtraction lands + // a few ulps outside the window. Pinned rather than papered over. + expect(isAtFit({ scale: FIT + FIT_EPSILON, x: 0, y: 0 })).toBe(true); + expect(isAtFit({ scale: FIT - FIT_EPSILON, x: 0, y: 0 })).toBe(false); + }); + + it('cannot arise, because no emitted scale is ever below FIT', () => { + // This is what makes the asymmetry unobservable in practice. If the floor + // ever stops being FIT, this fails and the asymmetry becomes real. + const g: Geometry = { + stageW: 1000, + stageH: 800, + fittedW: 900, + fittedH: 700, + naturalW: 3600, + naturalH: 2800, + }; + for (const attempt of [-99, 0, FIT - FIT_EPSILON, 0.5, Number.MIN_VALUE]) { + expect(clampScale(attempt, g)).toBeGreaterThanOrEqual(FIT); + expect(clampState({ scale: attempt, x: 0, y: 0 }, g).scale).toBeGreaterThanOrEqual(FIT); + expect(zoomTo(reset(), attempt, { x: 0, y: 0 }, g).scale).toBeGreaterThanOrEqual(FIT); + } + }); +}); diff --git a/web/src/lib/attachments/zoom.ts b/web/src/lib/attachments/zoom.ts new file mode 100644 index 00000000..8085ff57 --- /dev/null +++ b/web/src/lib/attachments/zoom.ts @@ -0,0 +1,403 @@ +/** + * Zoom / pan math for the attachment viewer — DOM-free, framework-free, and + * the single owner of every number the viewer's transform is built from + * (PLAN-2392 phase 3b / TASK-2454). + * + * WHY IT IS A SEPARATE MODULE. jsdom reports all-zero rects, so nothing that + * reads layout can be unit-tested there. Keeping the arithmetic here means the + * hard properties — anchor invariance, the pan bounds, the scale floor and + * ceiling — are provable without a browser, and the component (TASK-2455) is + * left with measuring, wiring and clamping on resize. + * + * TWO WORDS, ONE MEANING EACH. + * + * - The **fitted box** is the rendered size `object-fit: contain` gives the + * image inside the stage. The browser computes it; this module never + * re-derives it from natural-image pixels. + * - **fit** is `scale === 1`, i.e. {@link FIT}. There is no third quantity + * called a "fit scale" — conflating the two is what put the CSS and the + * module in different coordinate systems in an early revision of the design. + * + * THE COORDINATE SYSTEM. The stage is a box sized as the old bare `` was + * (92vw x 92vh, centred), with the `` inside it at `max-width: 100%; + * max-height: 100%; object-fit: contain`. The transform is + * + * translate(px, px) scale() transform-origin: center + * + * so a point sitting `d` CSS px from the image's centre (measured in UNSCALED + * fitted-box px) paints at + * + * stageCentre + (x, y) + scale * d + * + * At `scale === 1, x === 0, y === 0` that is byte-identical to today's + * rendering — the parity property the phase is built around. + * + * All exported functions are pure: none mutates its arguments, and every one + * that returns a {@link ZoomState} returns a fresh object. + * + * TWO PROPERTIES CONSUMERS MUST NOT ASSUME, both consequences of clamping every + * result rather than tracking an unclamped shadow state: + * + * - **A gesture is not associative.** Ten small {@link zoomTo} steps do not + * equal one big one once a pan bound is reached — the intermediate results + * were clamped and the excess is gone. So a DRAG must be computed from the + * state captured at pointer-down plus the total pointer delta, never by + * feeding each `pointermove` delta into the previous clamped result: + * overshooting a bound and coming back would otherwise move the image + * immediately instead of taking up the slack (TASK-2458). + * - **Clamping is lossy.** Pan discarded because the stage shrank does not come + * back when it grows again. A consumer that wants that must keep its own + * unclamped intent; the viewer deliberately does not (TASK-2455). + * + * NOTHING HERE ROUNDS. Geometry arrives fractional on any non-integer + * `devicePixelRatio` (the browser lays elements out on device-pixel boundaries, + * which are not CSS-pixel boundaries at dpr 1.25 / 1.5 / 2.75), and rounding + * intermediate values there drifts the anchored point visibly across a sequence + * of zoom steps. + */ + +/** + * The measured geometry of one painted image, in CSS px. + * + * Every field is expected finite and positive — a caller with no decoded bitmap + * has nothing to zoom and does not call in at all (TASK-2460 keeps the + * transform inert until one exists). The functions below are nevertheless + * defensive about zeros and non-finite values, because geometry is read from + * live layout and a mid-teardown measurement can produce either; a degenerate + * geometry must still yield a finite, in-bounds transform — never `NaN` or + * `Infinity` in a CSS string. + * + * MEASURING `fittedW/H` (TASK-2455). They are the image's UNSCALED box, so they + * cannot come from `getBoundingClientRect()` on the transformed `` — that + * returns the box AFTER `scale()`, and feeding it back in would make the bounds + * grow with the zoom. Read the untransformed layout box (`offsetWidth` / + * `offsetHeight`, which transforms do not affect), or measure once at fit. + */ +export interface Geometry { + /** Stage box width. */ + stageW: number; + /** Stage box height. */ + stageH: number; + /** Rendered image width at `scale === 1`. Never exceeds `stageW`. */ + fittedW: number; + /** Rendered image height at `scale === 1`. Never exceeds `stageH`. */ + fittedH: number; + /** + * The CURRENTLY PAINTED bitmap's intrinsic width — not the original's. + * While only a `thumb-md` variant has decoded, "actual size" means 1:1 with + * the thumb, and {@link maxScale} grows when the original swaps in + * (TASK-2459). NOTE: this is not the mobile fetch trigger; that trigger is + * `scale > 1`, i.e. past fit (TASK-2460). + */ + naturalW: number; + /** The currently painted bitmap's intrinsic height. See {@link Geometry.naturalW}. */ + naturalH: number; +} + +/** A point in stage-local CSS px, measured from the stage's top-left corner. */ +export interface Point { + x: number; + y: number; +} + +/** + * The viewer's transform state. + * + * `scale` is relative to fit, which is what makes {@link FIT} a constant rather + * than a derived quantity. `x` / `y` are the image centre's offset from the + * stage centre in CSS px. + */ +export interface ZoomState { + scale: number; + x: number; + y: number; +} + +/** + * `scale` at fit — the floor {@link clampScale} and {@link clampState} enforce. + * + * Not enforced by {@link clampPan}, which passes any finite scale through so it + * stays orthogonal to the scale bounds; use {@link clampState} when both matter. + */ +export const FIT = 1; + +/** {@link maxScale} is this multiple of 1:1 (or of fit, whichever is larger). */ +export const MAX_SCALE_FACTOR = 4; + +/** + * Where {@link toggleFitOrActual} goes when the image is already at 1:1 at fit + * (an image smaller than the stage: `object-fit: contain` never upscales, so + * fit IS actual size). Without this, double-click would be a no-op on exactly + * the images where a user is most likely to try it. + */ +export const TOGGLE_SMALL_SCALE = 2; + +/** + * How close to {@link FIT} still counts as "at fit" for {@link isAtFit} and + * therefore for {@link toggleFitOrActual}'s direction. + * + * A tolerance rather than an equality test because a wheel or pinch gesture + * lands on 1.0000000002 routinely, and a double-click there must zoom IN, not + * perform a visually-invisible reset. + */ +export const FIT_EPSILON = 0.001; + +/** + * The multiplicative step for one discrete zoom command — the `+` / `-` keys + * (TASK-2455). Exported so no call site re-types the literal. + */ +export const ZOOM_STEP = 1.25; + +/** + * Coerce a possibly-NaN/Infinite measurement to something usable. + * + * Infinity takes the fallback rather than saturating at a bound: an infinite + * scale or offset is always a caller-side arithmetic bug (a pinch handler + * dividing by a zero pointer distance is the likely one), and quietly snapping + * it to maximum zoom hides the bug behind plausible-looking behaviour, where + * falling back to the neutral value shows it. + */ +function finiteOr(value: number, fallback: number): number { + return Number.isFinite(value) ? value : fallback; +} + +/** + * Clamp to `[-bound, bound]`, normalising negative zero away. + * + * `Math.max(-0, -5)` is `-0`, so a centred axis would otherwise emit + * `translate(-0px)` and, more importantly, make `Object.is(x, 0)` false for a + * value this module documents as exactly zero. + * + * It deliberately does NOT re-guard a negative bound: {@link panBound} is the + * single owner of the centring rule, and a second copy of it here would make + * that one untestable — breaking the real rule would look correct because this + * function silently repaired it. + */ +function clampSymmetric(value: number, bound: number): number { + const clamped = Math.min(bound, Math.max(-bound, value)); + return clamped === 0 ? 0 : clamped; +} + +/** True only for a measurement that can be divided by. */ +function usable(value: number): boolean { + return Number.isFinite(value) && value > 0; +} + +/** + * The largest extent any axis is allowed to claim, in CSS px. + * + * Ten million px is four orders of magnitude past any layout a browser will + * produce (the widest real stage is a few thousand), and its purpose is purely + * arithmetic: it makes `extent * scale` unable to overflow to `Infinity`. + * + * That overflow is not cosmetic. {@link panBound} subtracts the stage from the + * scaled extent, and `Infinity - stage` is `Infinity`, so the pan bound becomes + * infinite and the clamp silently stops clamping — the one failure mode that + * lets blank stage show past an image edge. Saturating the inputs instead keeps + * every bound finite and ordered. + */ +const MAX_EXTENT = 1e7; + +/** Saturate a measurement into `[0, MAX_EXTENT]` so products cannot overflow. */ +function saneExtent(value: number): number { + const v = finiteOr(value, 0); + if (v <= 0) return 0; + return v > MAX_EXTENT ? MAX_EXTENT : v; +} + +/** The stage's centre, in stage-local px. The anchor for centre-origin zooms. */ +export function stageCenter(g: Geometry): Point { + return { + x: finiteOr(g.stageW, 0) / 2, + y: finiteOr(g.stageH, 0) / 2, + }; +} + +/** + * The `scale` at which the painted bitmap is displayed 1:1 with its own pixels. + * + * Measured on the WIDTH axis, as the contract specifies. `object-fit: contain` + * preserves the aspect ratio, so for any geometry the browser actually produces + * the height ratio is the same number; a hand-built non-uniform `Geometry` is + * the only way to make them disagree, and the width pair wins there. + * + * `object-fit: contain` never upscales, so an image smaller than the stage has + * `fittedW === naturalW` and this returns exactly 1 — fit already IS actual + * size for it. + */ +export function actualScale(g: Geometry): number { + if (!usable(g.fittedW) || !usable(g.naturalW)) return FIT; + const ratio = g.naturalW / g.fittedW; + // A subnormal `fittedW` divides to `Infinity` even though both inputs passed + // `usable`. Returning it would not merely be untidy: `toggleFitOrActual` + // hands this value straight to `zoomTo`, whose `clampScale` treats a + // non-finite target as invalid and falls back to fit — turning the toggle + // into the silent no-op the small-image rule exists to prevent. Falling back + // to FIT here instead routes that geometry to TOGGLE_SMALL_SCALE, so the + // gesture still does something visible. + return Number.isFinite(ratio) ? ratio : FIT; +} + +/** + * The upper bound on `scale`, i.e. the contract's `MAX_SCALE(g)`. + * + * `max(actualScale, 1)` rather than `actualScale` so a small image — whose + * actual scale is 1 — still gets a real zoom range instead of being pinned at + * fit, and so a nonsensical sub-1 actual scale can never invert the bounds. + */ +export function maxScale(g: Geometry): number { + const ceiling = Math.max(actualScale(g), FIT) * MAX_SCALE_FACTOR; + // A finite ratio can still overflow when multiplied: a bitmap reported at + // Number.MAX_VALUE px gives a finite actual scale and an infinite ceiling, + // which would silently disable the scale clamp entirely. + return Number.isFinite(ceiling) ? ceiling : Number.MAX_VALUE; +} + +/** Clamp a scale into `[FIT, maxScale(g)]`. */ +export function clampScale(scale: number, g: Geometry): number { + const max = maxScale(g); + const s = finiteOr(scale, FIT); + if (s < FIT) return FIT; + if (s > max) return max; + return s; +} + +/** The largest `|offset|` an axis may take before a stage edge shows past the image. */ +function panBound(stageExtent: number, fittedExtent: number, scale: number): number { + // Saturated, not merely finiteness-checked: the product is what overflows, + // and a finite `fittedExtent` near Number.MAX_VALUE times a finite scale is + // still `Infinity`, which would make the bound infinite and disable the + // clamp entirely. + const stage = saneExtent(stageExtent); + const scaled = saneExtent(saneExtent(fittedExtent) * finiteOr(scale, FIT)); + // An axis that does not overflow the stage is CENTRED, not free to roam. + if (scaled <= stage) return 0; + return (scaled - stage) / 2; +} + +/** + * Clamp the pan offsets for the state's own scale. + * + * Per axis: if the scaled extent is at most the stage, the offset is forced to + * 0 — a smaller-than-stage axis is CENTRED, never draggable. Otherwise the + * offset is bounded so no stage edge can show past the image edge. + * + * `scale` is passed through untouched (only sanitised for finiteness) — clamping + * it is {@link clampScale}'s job, and {@link clampState} composes the two in the + * order a resize needs. + */ +export function clampPan(state: ZoomState, g: Geometry): ZoomState { + const scale = finiteOr(state.scale, FIT); + const bx = panBound(g.stageW, g.fittedW, scale); + const by = panBound(g.stageH, g.fittedH, scale); + const x = finiteOr(state.x, 0); + const y = finiteOr(state.y, 0); + return { + scale, + x: clampSymmetric(x, bx), + y: clampSymmetric(y, by), + }; +} + +/** + * Clamp scale FIRST, then pan — the order a geometry change requires. + * + * `maxScale` is geometry-dependent and geometry is viewport-dependent, so + * ENLARGING the window lowers `actualScale` and with it `maxScale`, stranding a + * previously-valid scale above the new ceiling. Clamping pan first would bound + * it against a scale that is about to change (TASK-2455). + */ +export function clampState(state: ZoomState, g: Geometry): ZoomState { + return clampPan({ ...state, scale: clampScale(state.scale, g) }, g); +} + +/** + * Zoom to `targetScale` keeping the image point currently under `anchor` under + * `anchor` afterwards. + * + * The invariant, with `u` the anchor relative to the stage centre and + * `k = newScale / oldScale`: + * + * t' = u - k * (u - t) + * + * Anchor invariance holds exactly UNLESS the result has to be pan-clamped — + * the bounds win, because letting the anchor survive would mean showing blank + * stage past an image edge. A non-finite anchor falls back to the stage centre. + * + * The incoming state is normalised through {@link clampState} first, so the + * invariant is stated against a VALID starting state. Clamping the incoming + * scale while trusting its pan would mix a pan belonging to one scale with + * another, and the invariant would then be false for the one caller shape most + * likely to hit it — a geometry change that lowered the ceiling under a state + * the consumer has not re-clamped yet. + */ +export function zoomTo( + state: ZoomState, + targetScale: number, + anchor: Point, + g: Geometry, +): ZoomState { + const current = clampState(state, g); + const from = current.scale; + const to = clampScale(targetScale, g); + const centre = stageCenter(g); + const ux = finiteOr(anchor?.x, centre.x) - centre.x; + const uy = finiteOr(anchor?.y, centre.y) - centre.y; + // `from` came out of clampScale, so it is at least FIT — never a zero divide. + const k = to / from; + const x = ux - k * (ux - current.x); + const y = uy - k * (uy - current.y); + return clampPan({ scale: to, x, y }, g); +} + +/** + * Whether `state` is at fit, within {@link FIT_EPSILON}. + * + * ASYMMETRIC AT THE BOUNDARY, AND DELIBERATELY LEFT SO. `FIT + FIT_EPSILON` + * reads as at-fit while `FIT - FIT_EPSILON` does not, because neither value is + * exactly representable in binary and the subtraction lands a few ulps outside + * the window. Widening the comparison to hide that would be papering over an + * unreachable case: every scale this module emits has been through + * {@link clampScale}, whose floor is FIT, so a sub-fit state cannot arise from + * any sequence of operations on it. The asymmetry is only observable on a + * hand-constructed state, and both legs are pinned by tests so a future change + * to the floor cannot make it matter silently. + */ +export function isAtFit(state: ZoomState): boolean { + return Math.abs(finiteOr(state.scale, FIT) - FIT) <= FIT_EPSILON; +} + +/** + * The double-click / double-tap toggle: fit <-> actual size. + * + * At fit (within {@link FIT_EPSILON}) it goes to {@link actualScale} anchored at + * `anchor`; anywhere else it goes back to fit, centred. When actual size IS fit + * — an image smaller than the stage — it goes to {@link TOGGLE_SMALL_SCALE} + * instead, so the gesture always does something visible. + * + * DELIBERATE GENERALISATION OF THE CONTRACT. The written rule says "when + * `actualScale === 1`, go to {@link TOGGLE_SMALL_SCALE}"; this uses + * `actualScale <= FIT + FIT_EPSILON` instead. Exact equality serves the rule's + * PURPOSE only on integer geometry — at a fractional `devicePixelRatio` a 1:1 + * image measures 1.0000000004 and at a rounded layout it can measure 1.0005, + * and a toggle to either is a zoom of at most 0.05%: invisible, i.e. exactly + * the no-op the rule exists to prevent. The epsilon is the same one + * {@link isAtFit} uses, so "already at fit" and "actual size is fit" cannot + * disagree about the same image. + */ +export function toggleFitOrActual(state: ZoomState, anchor: Point, g: Geometry): ZoomState { + if (!isAtFit(state)) return reset(); + const actual = actualScale(g); + const target = actual > FIT + FIT_EPSILON ? actual : TOGGLE_SMALL_SCALE; + return zoomTo(state, target, anchor, g); +} + +/** + * The identity transform: fit, centred. + * + * Takes no geometry, which is precisely what makes it safe to call before a + * bitmap exists — on open, on image change, and on close (TASK-2455). + */ +export function reset(): ZoomState { + return { scale: FIT, x: 0, y: 0 }; +} From e334e8634006b36d07514632a0f6793dcc4d74ae Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 7 Aug 2026 15:12:44 +0000 Subject: [PATCH 2/9] feat(attachments): wire zoom transform into Lightbox (TASK-2455) Wire TASK-2454's pure zoom/pan math into the attachment viewer: - Stage + transform wrapper around the 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. --- web/src/lib/components/common/Lightbox.svelte | 189 +++++++++- .../components/common/Lightbox.svelte.test.ts | 346 ++++++++++++++++++ web/src/test/setup-jsdom.ts | 23 ++ 3 files changed, 551 insertions(+), 7 deletions(-) diff --git a/web/src/lib/components/common/Lightbox.svelte b/web/src/lib/components/common/Lightbox.svelte index 4ffa4ebf..ea9108a0 100644 --- a/web/src/lib/components/common/Lightbox.svelte +++ b/web/src/lib/components/common/Lightbox.svelte @@ -16,7 +16,9 @@ * * 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. + * viewer, `+`/`-` zoom about the stage centre and `0` resets (PLAN-2392 + * phase 3b / TASK-2455). Backdrop click closes; clicking the image itself + * does not. */ import { untrack } from 'svelte'; import { attachmentDownloadUrl } from '$lib/markdown/attachments'; @@ -30,6 +32,15 @@ } from '$lib/a11y/viewerBackdrop'; import { pushEscapeHandler, ESCAPE_PRIORITY } from '$lib/stores/escapeStack'; import { canOpenInViewer } from '$lib/attachments/display'; + import { + reset as resetZoom, + clampState, + zoomTo, + stageCenter, + ZOOM_STEP, + type Geometry, + type ZoomState, + } from '$lib/attachments/zoom'; /** * ONE definition of what an image in this viewer is (PLAN-2392 / TASK-2431). * @@ -173,6 +184,52 @@ // a flush (CONVE-1688). let rootEl = $state(null); + // ── Zoom / pan (PLAN-2392 phase 3b / TASK-2455) ────────────────────────── + // + // The transform is `translate(x,y) scale(scale)` on the , about the + // stage centre. Every number comes from `$lib/attachments/zoom` (TASK-2454), + // which owns the arithmetic and its bounds; this component only MEASURES the + // rendered geometry, wires the keys, and re-clamps on resize. + let zoom = $state(resetZoom()); + // The stage is the 92vw×92vh box the bare used to be; the sits + // inside it, `object-fit: contain`. Both are read live for geometry — never + // through `getBoundingClientRect()` on the transformed image, which returns + // the POST-scale box and would make the pan bounds grow with the zoom. + let stageEl = $state(null); + let imgEl = $state(null); + + /** + * The measured geometry, or null before there is anything to measure. + * + * `offsetWidth` / `offsetHeight` are the UNSCALED layout box — transforms do + * not touch them, which is the whole reason they, not `getBoundingClientRect`, + * are the source here. A not-yet-decoded bitmap reads back all zeros; the zoom + * module is defensive about that and still returns an in-bounds transform, so + * no guard is needed here. + */ + function readGeometry(): Geometry | null { + const stage = stageEl; + const image = imgEl; + if (!stage || !image) return null; + return { + stageW: stage.clientWidth, + stageH: stage.clientHeight, + fittedW: image.offsetWidth, + fittedH: image.offsetHeight, + naturalW: image.naturalWidth, + naturalH: image.naturalHeight, + }; + } + + // `+` / `-` zoom about the stage centre. Reading and writing `zoom` from an + // EVENT handler is fine — the CONVE-1688 rule is about `$effect`s that read + // the state they write, not about handlers. + function stepZoom(factor: number) { + const g = readGeometry(); + if (!g) return; + zoom = zoomTo(zoom, zoom.scale * factor, stageCenter(g), g); + } + // Stepped from `shownIndex`, not from `current`: after the set shrinks they // differ, and moving from the raw value would jump relative to a position // the user was never on. Both are no-ops on an empty set — reachable only @@ -289,6 +346,44 @@ }; }); + // Reset the transform whenever the SHOWN image changes — arrow nav, or the + // set shrinking under `current` so a different member is shown (TASK-2455). + // Close needs no handling: every producer keys the mount, so closing + // unmounts this instance and the next open starts from `resetZoom()`. + // + // `lastResetForId` is a PLAIN let, not `$state`: an effect that read and wrote + // the same `$state` would self-depend and abort its own flush (CONVE-1688), + // stranding unrelated reactivity nearby. This effect reads `img?.id` (tracked) + // and writes `zoom` (which it never reads) plus this sentinel (a plain let, so + // never tracked) — nothing it writes is anything it reads. Seeded to the + // current id so the mount does not fire a redundant reset. + let lastResetForId: string | undefined = untrack(() => img?.id); + $effect(() => { + const id = img?.id; + if (id === lastResetForId) return; + lastResetForId = id; + zoom = resetZoom(); + }); + + // Re-clamp on stage resize. `maxScale` is geometry-dependent and geometry is + // viewport-dependent, so ENLARGING the window lowers the ceiling and can + // strand a previously-valid scale above it. `clampState` re-clamps SCALE + // first, then pan — the order a geometry change needs; clamping pan first + // would bound it against a scale that is about to change. The callback runs + // outside any tracked scope, so its `zoom` read/write is not a self-write. + // Guarded for environments without `ResizeObserver` (SSR); the jsdom test + // project ships a global stand-in (`src/test/setup-jsdom.ts`). + $effect(() => { + const stage = stageEl; + if (!stage || typeof ResizeObserver === 'undefined') return; + const ro = new ResizeObserver(() => { + const g = readGeometry(); + if (g) zoom = clampState(zoom, g); + }); + ro.observe(stage); + return () => ro.disconnect(); + }); + function onKeydown(e: KeyboardEvent) { // A control that already handled this key owns it. if (e.defaultPrevented) return; @@ -327,9 +422,38 @@ if (e.key === 'ArrowLeft' && hasMultiple) { e.preventDefault(); prev(); - } else if (e.key === 'ArrowRight' && hasMultiple) { + return; + } + if (e.key === 'ArrowRight' && hasMultiple) { e.preventDefault(); next(); + return; + } + + // Zoom: `+` / `-` about the stage centre, `0` resets. INSIDE the gates + // above by construction — placed after `defaultPrevented`, + // `isViewerFrontmost` and `isBlockedByModal` have each had their say, so a + // zoom key can never steal a press another owner or a layer above is due. + // + // The modifier rule is a CONTRACT, not a nicety: this listens on `window`, + // so acting on Ctrl/Cmd/Alt-`-`/`0` would swallow the browser's own + // page-zoom (and OS) shortcuts from every surface while a viewer is open. + // Leave those keys entirely — do not act, and do NOT `preventDefault`, or + // the native shortcut is cancelled even though we declined to handle it. + // `shiftKey` is fine (on most layouts `+` IS Shift+`=`), so it is absent + // from the guard. + if (e.ctrlKey || e.metaKey || e.altKey) return; + if (e.key === '+' || e.key === '=') { + // `+` — including the numpad, whose `.key` is also `'+'` — and bare `=`. + e.preventDefault(); + stepZoom(ZOOM_STEP); + } else if (e.key === '-') { + // `-`, including the numpad, whose `.key` is also `'-'`. + e.preventDefault(); + stepZoom(1 / ZOOM_STEP); + } else if (e.key === '0') { + e.preventDefault(); + zoom = resetZoom(); } // NO Escape branch. See the registration above. } @@ -397,9 +521,26 @@ {/if} - {#if img} - {img.alt - {/if} + +
+ {#if img} + {img.alt + {/if} +
{#if hasMultiple}
{#if img} + {img.alt {/if} @@ -658,6 +938,11 @@ background: rgba(0, 0, 0, 0.82); backdrop-filter: blur(2px); cursor: zoom-out; + /* A drag-to-pan surface: no text should get selected mid-drag (the counter + is the only text, and it is not meant to be selectable). draggable=false + on the handles the image; this handles selection (TASK-2458). */ + user-select: none; + -webkit-user-select: none; } .lightbox-stage { @@ -690,6 +975,13 @@ transition: transform 0.15s ease-out; } + /* While DRAGGING, the image must track the pointer 1:1 — the easing transition + would make it lag behind the cursor (TASK-2458). Discrete zoom (keys / wheel / + double-click) keeps the transition. */ + .lightbox-image.panning { + transition: none; + } + /* Reduced motion suppresses the TRANSITION only — the zoom still works (Modal.svelte's precedent). */ @media (prefers-reduced-motion: reduce) { diff --git a/web/src/lib/components/common/Lightbox.svelte.test.ts b/web/src/lib/components/common/Lightbox.svelte.test.ts index c699324a..fb1ea076 100644 --- a/web/src/lib/components/common/Lightbox.svelte.test.ts +++ b/web/src/lib/components/common/Lightbox.svelte.test.ts @@ -15,7 +15,13 @@ import { ESCAPE_PRIORITY, _resetEscapeStackForTests, } from '$lib/stores/escapeStack'; -import { reset as resetZoom, zoomTo, ZOOM_STEP, type Geometry } from '$lib/attachments/zoom'; +import { + reset as resetZoom, + zoomTo, + toggleFitOrActual, + ZOOM_STEP, + type Geometry, +} from '$lib/attachments/zoom'; // TASK-2429 — the DR-4b modal contract on the attachment viewer. // @@ -1670,3 +1676,592 @@ describe('Lightbox — wheel zoom (TASK-2457)', () => { expect(scaleOf()).toBeCloseTo(1.25); }); }); + +// TASK-2458 — drag-to-pan + double-click toggle (probe). +const REAL_PC = { + setPointerCapture: Element.prototype.setPointerCapture, + releasePointerCapture: Element.prototype.releasePointerCapture, +}; +let captured: number[] = []; +let released: number[] = []; + +function pointerEvent( + type: string, + x: number, + y: number, + opts: { buttons?: number; pointerType?: string; button?: number; pointerId?: number } = {} +): Event { + const e = new MouseEvent(type, { bubbles: true, cancelable: true, clientX: x, clientY: y }); + Object.defineProperty(e, 'pointerId', { value: opts.pointerId ?? 1 }); + Object.defineProperty(e, 'buttons', { value: opts.buttons ?? 1 }); + Object.defineProperty(e, 'button', { value: opts.button ?? 0 }); + Object.defineProperty(e, 'pointerType', { value: opts.pointerType ?? 'mouse' }); + return e; +} + +function panX(scope: HTMLElement = root()): number { + const m = /translate\(([-\d.]+)px,/.exec(transformOf(scope)); + return m ? Number(m[1]) : NaN; +} + +const OVERFLOW_G: Geometry = { + stageW: 1000, + stageH: 1000, + fittedW: 900, + fittedH: 900, + naturalW: 2000, + naturalH: 2000, +}; + +/** dblclick at the stage centre → actual size, so drags have pan room. */ +function zoomToActual(scope: HTMLElement = root()): void { + scope.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 500, clientY: 500 })); + flushSync(); +} +/** The pan bound at `actualScale` for OVERFLOW_G: (900·(2000/900) − 1000)/2. */ +const ACTUAL_BOUND = (OVERFLOW_G.fittedW * (OVERFLOW_G.naturalW / OVERFLOW_G.fittedW) - OVERFLOW_G.stageW) / 2; + +function mockOpenModals(modals: Element[]): void { + const realQueryAll = document.querySelectorAll.bind(document); + vi.spyOn(document, 'querySelectorAll').mockImplementation((selector: string) => { + if (selector !== 'dialog:modal') return realQueryAll(selector); + return Array.from(realQueryAll('dialog')).filter((d) => + modals.includes(d) + ) as unknown as NodeListOf; + }); + const realMatches = Element.prototype.matches; + vi.spyOn(Element.prototype, 'matches').mockImplementation(function (this: Element, selector: string) { + if (selector !== 'dialog:modal') return realMatches.call(this, selector); + return realMatches.call(this, 'dialog') && modals.includes(this); + }); +} + +describe('Lightbox — drag-to-pan and double-click (TASK-2458)', () => { + beforeEach(() => { + captured = []; + released = []; + (Element.prototype as unknown as Record).setPointerCapture = function (id: number) { + captured.push(id); + }; + (Element.prototype as unknown as Record).releasePointerCapture = function (id: number) { + released.push(id); + }; + }); + afterEach(() => { + if (REAL_PC.setPointerCapture === undefined) + delete (Element.prototype as unknown as Record).setPointerCapture; + else Element.prototype.setPointerCapture = REAL_PC.setPointerCapture; + if (REAL_PC.releasePointerCapture === undefined) + delete (Element.prototype as unknown as Record).releasePointerCapture; + else Element.prototype.releasePointerCapture = REAL_PC.releasePointerCapture; + }); + + // ── pan, positively AND clamped (the anti-false-green acceptance) ── + it('pans in-bounds by the drag delta, then clamps at the edge and moves NO FURTHER', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); // scale ~2.22, pan bound ±ACTUAL_BOUND (500), pan still 0 + expect(scaleOf()).toBeCloseTo(2000 / 900); + + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + // A small in-bounds drag MOVES the transform by exactly the delta. + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(captured).toContain(1); // a real drag engaged + captured + expect(panX()).toBeCloseTo(100); + + // Over-drag to the same edge: it clamps at the bound... + root().dispatchEvent(pointerEvent('pointermove', 500 + ACTUAL_BOUND + 300, 500)); + flushSync(); + expect(panX()).toBeCloseTo(ACTUAL_BOUND); + // ...and dragging further still moves it no further (the clamp, not the delta). + root().dispatchEvent(pointerEvent('pointermove', 500 + ACTUAL_BOUND + 900, 500)); + flushSync(); + expect(panX()).toBeCloseTo(ACTUAL_BOUND); + + root().dispatchEvent(pointerEvent('pointerup', 500 + ACTUAL_BOUND + 900, 500, { buttons: 0 })); + expect(released).toContain(1); + }); + + // ── double-click toggle, anchored, RETURNS ── + /** The realistic sequence a browser fires for a double-click on `el`. */ + function doubleClick(el: Element, clientX: number, clientY: number): void { + el.dispatchEvent(new MouseEvent('click', { bubbles: true, clientX, clientY })); + el.dispatchEvent(new MouseEvent('click', { bubbles: true, clientX, clientY })); + el.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX, clientY })); + flushSync(); + } + + it('two successive anchored double-clicks ON THE IMAGE toggle fit → actual → fit, never closing', () => { + const onClose = vi.fn(); + mountViewer({ onClose }); + mockGeometry(root(), OVERFLOW_G); + const image = root().querySelector('.lightbox-image')!; + expect(scaleOf()).toBe(1); + + // The full click/click/dblclick sequence on the IMAGE (target !== backdrop), + // off-centre so the toggle-to-actual is genuinely anchored. The constituent + // clicks must NOT dismiss. + doubleClick(image, 800, 500); + expect(onClose).not.toHaveBeenCalled(); + const expected = toggleFitOrActual(resetZoom(), { x: 800, y: 500 }, OVERFLOW_G); + expect(transformOf()).toBe( + `translate(${expected.x}px, ${expected.y}px) scale(${expected.scale})` + ); + expect(expected.scale).toBeGreaterThan(1); + expect(expected.x).not.toBe(0); + + // The SECOND double-click returns to fit, centred — the toggle is two-way. + doubleClick(image, 800, 500); + expect(onClose).not.toHaveBeenCalled(); + expect(transformOf()).toBe('translate(0px, 0px) scale(1)'); + }); + + it('a double-click while a pan is still being suppressed does NOT toggle (drag XOR toggle)', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + const image = root().querySelector('.lightbox-image')!; + // Engage and release a pan — its click is now being swallowed (suppressClick). + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + root().dispatchEvent(pointerEvent('pointerup', 600, 500, { buttons: 0 })); + flushSync(); + // A double-click landing while that suppression is still live must stand + // down — a gesture is a drag OR a toggle, never both (same swallow the + // backdrop click consults). + image.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 800, clientY: 500 })); + flushSync(); + expect(scaleOf()).toBe(1); + }); + + it('a double-click does NOT toggle while a native modal is above it; it resumes after', () => { + const dialog = document.body.appendChild(document.createElement('dialog')); + mockOpenModals([dialog]); + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + const image = root().querySelector('.lightbox-image')!; + + image.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 500, clientY: 500 })); + flushSync(); + expect(scaleOf()).toBe(1); // gated out by isBlockedByModal + + mockOpenModals([]); + image.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 500, clientY: 500 })); + flushSync(); + expect(scaleOf()).toBeCloseTo(2000 / 900); // resumes + }); + + // ── drag-vs-click disambiguation ── + it('a below-threshold press released over the backdrop still CLOSES', () => { + const onClose = vi.fn(); + mountViewer({ onClose }); + root().dispatchEvent(pointerEvent('pointerdown', 100, 100)); + root().dispatchEvent(pointerEvent('pointermove', 102, 101)); // < 4px → still a click + root().dispatchEvent(pointerEvent('pointerup', 102, 101, { buttons: 0 })); + flushSync(); + // The click the press synthesizes, on the backdrop itself. + root().dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(onClose).toHaveBeenCalledTimes(1); + expect(captured).toEqual([]); // never engaged a drag + }); + + it('a past-threshold DRAG released over the backdrop does NOT close', () => { + const onClose = vi.fn(); + mountViewer({ onClose }); + root().dispatchEvent(pointerEvent('pointerdown', 100, 100)); + root().dispatchEvent(pointerEvent('pointermove', 200, 200)); // > 4px → a pan + root().dispatchEvent(pointerEvent('pointerup', 200, 200, { buttons: 0 })); + flushSync(); + // The click a pan synthesizes is suppressed (cleared only on the next tick, + // which this synchronous dispatch beats). + root().dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(onClose).not.toHaveBeenCalled(); + expect(captured).toContain(1); + }); + + it('CONTROL: a plain backdrop click (no gesture) still closes', () => { + const onClose = vi.fn(); + mountViewer({ onClose }); + root().dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + // ── arbitration: the START gate ── + it('does not START a pan when the viewer is not frontmost; the front one does', () => { + mountViewer(); + const back = root(); + mockGeometry(back, OVERFLOW_G); + mountViewer(); + const front = root(); + mockGeometry(front, OVERFLOW_G); + zoomToActual(front); + + // Drag on the BACKGROUND viewer: no capture, no pan. + back.dispatchEvent(pointerEvent('pointerdown', 500, 500)); + back.dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(captured).toEqual([]); + + // Positive control: the FRONT viewer pans. + front.dispatchEvent(pointerEvent('pointerdown', 500, 500)); + front.dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(captured).toContain(1); + expect(panX(front)).toBeCloseTo(100); + }); + + // ── arbitration: whole-gesture, a STACKED-VIEWER transition mid-drag ── + it('ABORTS a captured drag when a SECOND viewer becomes frontmost mid-gesture', () => { + mountViewer(); + const first = root(); + mockGeometry(first, OVERFLOW_G); + zoomToActual(first); + + first.dispatchEvent(pointerEvent('pointerdown', 500, 500)); + first.dispatchEvent(pointerEvent('pointermove', 600, 500)); // positive control: pans to 100 + flushSync(); + expect(captured).toContain(1); + expect(panX(first)).toBeCloseTo(100); + const frozen = panX(first); + + // A second viewer opens — `first` is no longer frontmost. + mountViewer(); + + // A further move on `first` (still captured there) must ABORT: release the + // capture, leave the transform where it was. + first.dispatchEvent(pointerEvent('pointermove', 900, 500)); + flushSync(); + expect(released).toContain(1); + expect(panX(first)).toBeCloseTo(frozen); + }); + + // ── arbitration: whole-gesture, a NATIVE-MODAL transition mid-drag ── + it('ABORTS a captured drag when a native modal opens above it mid-gesture', () => { + const dialog = document.body.appendChild(document.createElement('dialog')); + // Establish `:modal` support with NO modal first (jsdom throws on the + // pseudo-class, and the manager caches support on its first probe), so the + // drag can start; then add the dialog mid-gesture. + mockOpenModals([]); + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); // positive control: pans + flushSync(); + expect(captured).toContain(1); + expect(panX()).toBeCloseTo(100); + const frozen = panX(); + + mockOpenModals([dialog]); // a showModal() dialog is now above the viewer + + root().dispatchEvent(pointerEvent('pointermove', 900, 500)); + flushSync(); + expect(released).toContain(1); + expect(panX()).toBeCloseTo(frozen); + }); + + // ── integration details ── + it('marks the image non-draggable so a native image drag cannot pre-empt the pan', () => { + mountViewer(); + expect(root().querySelector('.lightbox-image')?.getAttribute('draggable')).toBe('false'); + }); + + it('ignores a TOUCH pointer (native until 3d); a mouse pointer pans', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + + root().dispatchEvent(pointerEvent('pointerdown', 500, 500, { pointerType: 'touch' })); + root().dispatchEvent(pointerEvent('pointermove', 600, 500, { pointerType: 'touch' })); + flushSync(); + expect(captured).toEqual([]); + + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(captured).toContain(1); + expect(panX()).toBeCloseTo(100); + }); + + it('drops the transform transition while dragging (image tracks the pointer), restoring it after', () => { + // jsdom applies no transitions, so assert the mechanism: the `panning` class + // (which sets transition:none) is present only for the duration of the drag. + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + const image = root().querySelector('.lightbox-image')!; + expect(image.classList.contains('panning')).toBe(false); + + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(image.classList.contains('panning')).toBe(true); + + root().dispatchEvent(pointerEvent('pointerup', 600, 500, { buttons: 0 })); + flushSync(); + expect(image.classList.contains('panning')).toBe(false); + }); + + it('a TOUCH or second pointer cannot move or END an active mouse drag', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + // Mouse drag engaged (pointerId 1). + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(panX()).toBeCloseTo(100); + + // A touch move with a DIFFERENT pointerId (not captured) must not pan... + root().dispatchEvent( + pointerEvent('pointermove', 900, 500, { pointerId: 2, pointerType: 'touch' }) + ); + flushSync(); + expect(panX()).toBeCloseTo(100); + + // ...and a touch pointerup must not END the mouse drag. + root().dispatchEvent( + pointerEvent('pointerup', 900, 500, { pointerId: 2, pointerType: 'touch', buttons: 0 }) + ); + flushSync(); + expect(released).not.toContain(2); + + // The mouse (pointerId 1) is still driving: its move pans further. + root().dispatchEvent(pointerEvent('pointermove', 700, 500)); + flushSync(); + expect(panX()).toBeCloseTo(200); + }); + + it('a second primary pointerdown mid-drag does NOT hijack the gesture', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); // mouse id 1 + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); // captured, x = 100 + flushSync(); + expect(panX()).toBeCloseTo(100); + + // A second primary press (a pen, id 2) lands mid-drag — it must be ignored, + // not re-arm the gesture onto itself. + root().dispatchEvent( + pointerEvent('pointerdown', 200, 200, { pointerId: 2, pointerType: 'pen' }) + ); + // The ORIGINAL pointer (id 1) still drives the pan. + root().dispatchEvent(pointerEvent('pointermove', 700, 500)); + flushSync(); + expect(panX()).toBeCloseTo(200); + // ...and the interloper's own move does nothing. + root().dispatchEvent( + pointerEvent('pointermove', 900, 900, { pointerId: 2, pointerType: 'pen' }) + ); + flushSync(); + expect(panX()).toBeCloseTo(200); + }); + + it('releases the capture on pointercancel', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(captured).toContain(1); + root().dispatchEvent(pointerEvent('pointercancel', 600, 500)); + expect(released).toContain(1); + }); + + // ── gesture-state hygiene (round 1) ── + it('a wheel DURING a captured drag does not snap the pan back (rebases the baseline)', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 700, 500)); // captured, x = 200 + flushSync(); + expect(panX()).toBeCloseTo(200); + + // A wheel arrives mid-drag, anchored at the stage CENTRE (500) — distinct + // from the drag position — so it moves the pan to a value the pre-wheel + // origin+delta does NOT reproduce. The baseline must rebase to the wheel's + // pointer position (500) and post-wheel pan, so a following pointermove to + // that same point (zero net delta) leaves the transform where the wheel put + // it. Without the rebase, that move snaps the pan back to the pre-wheel + // origin (0) — a visible jump. + wheel(root(), { clientX: 500, clientY: 500, deltaY: -1 }); + const afterWheel = transformOf(); + expect(panX()).not.toBeCloseTo(200); // the wheel genuinely moved the pan + root().dispatchEvent(pointerEvent('pointermove', 500, 500)); + flushSync(); + expect(transformOf()).toBe(afterWheel); + }); + + it('a keyboard zoom (+) DURING a captured drag does not snap the pan back', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 700, 500)); // captured, x = 200 + flushSync(); + expect(panX()).toBeCloseTo(200); + + // `+` zooms about the stage centre mid-drag, moving the pan; the drag + // baseline must rebase to the last pointer position (700), so a move back to + // that point is a zero net delta and holds. Without the rebase it snaps to + // the pre-key origin + total delta. + expect(press('+')).toBe(true); + const afterKey = transformOf(); + expect(panX()).not.toBeCloseTo(200); + root().dispatchEvent(pointerEvent('pointermove', 700, 500)); + flushSync(); + expect(transformOf()).toBe(afterKey); + }); + + it('an external zoom while ARMED (before the drag threshold) rebases, so engaging does not jump', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + // Arm the gesture (pointerdown) but stay below the 4px threshold. + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + // A wheel zooms while merely ARMED (not yet dragging), anchored off-centre so + // it moves the pan. + wheel(root(), { clientX: 800, clientY: 500, deltaY: -1 }); + const afterWheelPan = panX(); + expect(afterWheelPan).not.toBeCloseTo(0); + + // Cross the threshold with a small move from the WHEEL pointer position: the + // engage must continue from the post-wheel pan (armed rebase), NOT snap to + // origin(0) + the full delta from the original press point. + root().dispatchEvent(pointerEvent('pointermove', 810, 500)); + flushSync(); + expect(captured).toContain(1); + expect(panX()).toBeCloseTo(afterWheelPan + 10); + }); + + it('a stale armed gesture (missed pointerup) does not resume as a phantom drag on a later control press', () => { + mountViewer({ images: [image(IMG_A, 'a'), image(IMG_B, 'b', 'image/jpeg')] }); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + // Arm a gesture whose pointerup never arrives (the pointer left the root + // before the drag threshold, so there was no capture to deliver it). + root().dispatchEvent(pointerEvent('pointerdown', 100, 100)); + // A later press lands on a control — excluded from starting a pan, and must + // also clear the stale arm so the following move can't engage from a dead + // baseline. + const close = closeButton(); + close.dispatchEvent(pointerEvent('pointerdown', 20, 20)); + close.dispatchEvent(pointerEvent('pointermove', 300, 300)); // far past threshold + flushSync(); + expect(captured).toEqual([]); + }); + + it('a double-click on a nav control does NOT also toggle zoom', () => { + mountViewer({ images: [image(IMG_A, 'a'), image(IMG_B, 'b', 'image/jpeg')] }); + mockGeometry(root(), OVERFLOW_G); + const next = root().querySelector('.lightbox-nav.next')!; + expect(scaleOf()).toBe(1); + next.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 20, clientY: 20 })); + flushSync(); + expect(scaleOf()).toBe(1); // the toggle stood down for the control + }); + + it('a mid-capture ABORT leaves the SAME viewer able to start a clean next gesture', () => { + const dialog = document.body.appendChild(document.createElement('dialog')); + mockOpenModals([]); // establish :modal support with nothing open + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(captured).toContain(1); + + mockOpenModals([dialog]); // blocked → the next move aborts + root().dispatchEvent(pointerEvent('pointermove', 900, 500)); + flushSync(); + expect(released).toContain(1); + mockOpenModals([]); // unblocked; same viewer frontmost again + + // A fresh gesture engages cleanly — no stuck `dragging` / capture from abort. + captured.length = 0; + released.length = 0; + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 560, 500)); + flushSync(); + expect(captured).toContain(1); + root().dispatchEvent(pointerEvent('pointerup', 560, 500, { buttons: 0 })); + expect(released).toContain(1); + }); + + it('a buttons-released move after a drag releases capture, not leaking into the next gesture', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(captured).toContain(1); + + // A move with the primary button no longer held — a pointerup we never saw. + root().dispatchEvent(pointerEvent('pointermove', 650, 500, { buttons: 0 })); + flushSync(); + expect(released).toContain(1); + + captured.length = 0; + released.length = 0; + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 560, 500)); + flushSync(); + expect(captured).toContain(1); + }); + + it('lostpointercapture ends the gesture; the next one starts clean', () => { + mountViewer(); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(captured).toContain(1); + + root().dispatchEvent(pointerEvent('lostpointercapture', 600, 500)); + flushSync(); + + captured.length = 0; + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 560, 500)); + flushSync(); + expect(captured).toContain(1); + }); + + it('a press ON a control (close / nav) never starts a pan', () => { + mountViewer({ images: [image(IMG_A, 'a'), image(IMG_B, 'b', 'image/jpeg')] }); + mockGeometry(root(), OVERFLOW_G); + zoomToActual(); + const close = closeButton(); + // Press on the control, then move well past the threshold: a drag OFF a + // button must not engage a pan (its own click stays intact). + close.dispatchEvent(pointerEvent('pointerdown', 20, 20)); + close.dispatchEvent(pointerEvent('pointermove', 200, 200)); + flushSync(); + expect(captured).toEqual([]); + }); + + // ── the pointer entry points carry the same gates ── + it('double-click does NOT toggle when the viewer is not frontmost; the front one does', () => { + mountViewer(); + const back = root(); + mockGeometry(back, OVERFLOW_G); + mountViewer(); + const front = root(); + mockGeometry(front, OVERFLOW_G); + + back.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 500, clientY: 500 })); + flushSync(); + expect(scaleOf(back)).toBe(1); // background viewer did not toggle + + front.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 500, clientY: 500 })); + flushSync(); + expect(scaleOf(front)).toBeCloseTo(2000 / 900); // frontmost did + }); +}); From 782686672870f35a52f5f19b6aee83e676051d21 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 8 Aug 2026 03:05:38 +0000 Subject: [PATCH 6/9] feat(attachments): DR-5b classification and desktop thumb-then-original loading (TASK-2459) Add the memory-safety half of DR-5b: a pure pixel classifier (viewerLoading.ts) and a loader lifecycle (viewerImageLoader.svelte.ts) that paints a bounded thumb-md first and background-upgrades to the original on desktop, while a phone auto-pulls nothing (TASK-2460 tap). Classification is on pixels never bytes; null width OR height is a third 'unknown' class, not an alias for large. The fallback detector keys off the decoded long edge exceeding THUMB_LONG_EDGE (a thumb-md is bounded at 1024 by definition), so a thumb request silently served the original (fresh upload, WebP/AVIF) is caught without mirroring the server decoder set; the unknown-dims desktop cell requests the original directly since a bounded fallback would defeat the detector. Wire it into Lightbox: the is keyed on a per-request loadToken (a fresh element per load/retry; reused across the thumb->original upgrade), DR-10 spinner + retryable error with an explicit pointer-events:auto retry that hands off focus before it disappears, and DR-16 as a loading gate at the request chokepoint (no request for unsafe/unresolved MIME). Staleness is fenced by a generation captured as a frozen data-gen DOM attribute plus the src, so a detached element's late load/error in an A->B->A navigation (same URL) can't clobber the live image. The load key includes dimensions so an async metadata fill re-runs the policy. Thread the width/height toUploadedAttachment used to drop. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu --- web/src/lib/attachments/events.test.ts | 28 ++ web/src/lib/attachments/events.ts | 20 +- .../viewerImageLoader.svelte.test.ts | 248 ++++++++++++++++++ .../attachments/viewerImageLoader.svelte.ts | 235 +++++++++++++++++ web/src/lib/attachments/viewerLoading.test.ts | 93 +++++++ web/src/lib/attachments/viewerLoading.ts | 152 +++++++++++ web/src/lib/components/common/Lightbox.svelte | 219 ++++++++++++++-- .../components/common/Lightbox.svelte.test.ts | 233 ++++++++++++++++ .../attachmentImageViewerHost.svelte.test.ts | 10 +- .../items/ItemAttachmentStrip.svelte.test.ts | 2 + 10 files changed, 1211 insertions(+), 29 deletions(-) create mode 100644 web/src/lib/attachments/viewerImageLoader.svelte.test.ts create mode 100644 web/src/lib/attachments/viewerImageLoader.svelte.ts create mode 100644 web/src/lib/attachments/viewerLoading.test.ts create mode 100644 web/src/lib/attachments/viewerLoading.ts diff --git a/web/src/lib/attachments/events.test.ts b/web/src/lib/attachments/events.test.ts index b48f1420..81d82c2a 100644 --- a/web/src/lib/attachments/events.test.ts +++ b/web/src/lib/attachments/events.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest'; +import type { AttachmentUploadResult } from '$lib/types'; import { createAttachmentHostToken, isAttachmentPanelEventForHost, @@ -7,6 +8,7 @@ import { notifyViewerOpen, registerAttachmentPanelListener, registerAttachmentViewerListener, + toUploadedAttachment, type AttachmentPanelOpenEvent, type AttachmentViewerOpenEvent, type LightboxImage, @@ -524,3 +526,29 @@ describe('viewer open channel', () => { expect(viewerSeen[0].attachmentId).toBe('att-1'); }); }); + +describe('toUploadedAttachment (TASK-2459)', () => { + it('threads the dimensions the narrowing used to drop', () => { + const out = toUploadedAttachment({ + id: 'a1', + filename: 'big.png', + mime: 'image/png', + size: 4096, + width: 4000, + height: 3000, + } as AttachmentUploadResult); + expect(out.width).toBe(4000); + expect(out.height).toBe(3000); + }); + + it('leaves dimensions null when the response omits them', () => { + const out = toUploadedAttachment({ + id: 'a1', + filename: 'f.bin', + mime: 'application/octet-stream', + size: 10, + } as AttachmentUploadResult); + expect(out.width).toBeNull(); + expect(out.height).toBeNull(); + }); +}); diff --git a/web/src/lib/attachments/events.ts b/web/src/lib/attachments/events.ts index eae88316..7d947cdf 100644 --- a/web/src/lib/attachments/events.ts +++ b/web/src/lib/attachments/events.ts @@ -84,12 +84,21 @@ export interface UploadedAttachment { filename: string; mime_type: string; size_bytes: number; + /** + * Pixel dimensions, when the server returned them (nullable — a non-image, or + * an image whose dimensions it couldn't read). Carried so a freshly uploaded + * image opened in the viewer can classify for the DR-5b loading policy + * (TASK-2459) instead of falling to `unknown` and pulling the original + * outright; the upload response has them, this narrowing used to DROP them. + */ + width: number | null; + height: number | null; } /** * Narrow an upload response to what subscribers need. Both upload paths (body - * editor, comment composer) were hand-mapping the same four fields, which is - * how the two drift apart. + * editor, comment composer) were hand-mapping the same fields, which is how the + * two drift apart. */ export function toUploadedAttachment(result: AttachmentUploadResult): UploadedAttachment { return { @@ -97,6 +106,8 @@ export function toUploadedAttachment(result: AttachmentUploadResult): UploadedAt filename: result.filename, mime_type: result.mime, size_bytes: result.size, + width: result.width ?? null, + height: result.height ?? null, }; } @@ -282,8 +293,9 @@ export interface LightboxImage { * Metadata the viewer may caption with, all NULLABLE for the same reason * the panel's three are: an emitter knows only what its own surface gives * it, and an inline image's HEAD probe may not have completed or may have - * failed, while an upload event carries only four fields - * (`UploadedAttachment`). + * failed, while an upload event carries only the `UploadedAttachment` fields + * (which now include the pixel dimensions, threaded for the DR-5b policy — + * TASK-2459). * * `mime_type` is not decoration: it is what lets a CONSUMER re-state the * DR-16 open gate over a whole set rather than trusting the one element diff --git a/web/src/lib/attachments/viewerImageLoader.svelte.test.ts b/web/src/lib/attachments/viewerImageLoader.svelte.test.ts new file mode 100644 index 00000000..7a8f53a9 --- /dev/null +++ b/web/src/lib/attachments/viewerImageLoader.svelte.test.ts @@ -0,0 +1,248 @@ +import { describe, it, expect } from 'vitest'; +import { createViewerImageLoader } from './viewerImageLoader.svelte'; +import type { LightboxImage } from './events'; + +// TASK-2459 — the DR-5b loader. `displaySrc` IS the request: the canonical +// attachment URL the viewer's loads natively (a `?variant=thumb-md` first, +// the plain original second). The acceptance is phrased in requests — a DR-16 +// gate is "no request issued", the fallback detector is "no SECOND request", the +// upgrade is "the second request is the original" — and each is a `displaySrc` +// transition here. +// +// `decoded`/`errored` carry the `gen` (the `loadToken` the reporting element was +// mounted under); the current generation is `loader.loadToken`, so a live decode +// passes `loader.loadToken` and a DETACHED element's stale decode passes the +// token captured when it loaded. + +function image(id: string, over: Partial = {}): LightboxImage { + return { + id, + alt: id, + filename: null, + mime_type: 'image/png', + size_bytes: null, + width: null, + height: null, + ...over, + }; +} + +const THUMB = (id: string) => `/api/v1/workspaces/ws/attachments/${id}?variant=thumb-md`; +const ORIGINAL = (id: string) => `/api/v1/workspaces/ws/attachments/${id}`; + +describe('viewerImageLoader — the decision table as requests (TASK-2459)', () => { + it('small, long edge <= 1024: ONE request, the original directly (no variant)', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 800, height: 600 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(ORIGINAL('A')); + expect(loader.phase).toBe('loading'); + loader.decoded(800, 600, loader.displaySrc, loader.loadToken); + // No upgrade — it IS the original. + expect(loader.displaySrc).toBe(ORIGINAL('A')); + expect(loader.phase).toBe('ready'); + }); + + it('unknown dims on desktop: the original directly (one request)', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: null, height: 900 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(ORIGINAL('A')); + loader.decoded(600, 900, loader.displaySrc, loader.loadToken); + expect(loader.displaySrc).toBe(ORIGINAL('A')); + }); + + it('large / unknown on mobile: NO request (idle — the tap affordance is TASK-2460)', () => { + const large = createViewerImageLoader(); + large.load(image('A', { width: 5000, height: 5000 }), 'ws', 'mobile'); + expect(large.displaySrc).toBe(''); + expect(large.phase).toBe('idle'); + + const unknown = createViewerImageLoader(); + unknown.load(image('B', { width: null, height: 900 }), 'ws', 'mobile'); + expect(unknown.displaySrc).toBe(''); + expect(unknown.phase).toBe('idle'); + }); +}); + +describe('viewerImageLoader — thumb then original, the four bound ways (TASK-2459)', () => { + it('large on desktop: first request bounded, second the original, the bitmap CHANGES', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'desktop'); + // (1) first response bounded. + expect(loader.displaySrc).toBe(THUMB('A')); + + // The thumb decoded at a bounded size → the background upgrade fires. + loader.decoded(1024, 768, loader.displaySrc, loader.loadToken); + // (2) second request is explicitly the original (canonical, no variant). + expect(loader.displaySrc).toBe(ORIGINAL('A')); + // (3) the displayed bitmap visibly CHANGES (thumb URL → original URL). + expect(loader.displaySrc).not.toBe(THUMB('A')); + + loader.decoded(5000, 5000, loader.displaySrc, loader.loadToken); + expect(loader.phase).toBe('ready'); + }); + + it('small, long edge > 1024 on desktop: thumb-md then upgrade; on mobile: thumb-md, no upgrade', () => { + const desktop = createViewerImageLoader(); + desktop.load(image('A', { width: 2000, height: 100 }), 'ws', 'desktop'); + expect(desktop.displaySrc).toBe(THUMB('A')); + desktop.decoded(1024, 51, desktop.displaySrc, desktop.loadToken); + expect(desktop.displaySrc).toBe(ORIGINAL('A')); + + const mobile = createViewerImageLoader(); + mobile.load(image('A', { width: 2000, height: 100 }), 'ws', 'mobile'); + expect(mobile.displaySrc).toBe(THUMB('A')); + mobile.decoded(1024, 51, mobile.displaySrc, mobile.loadToken); + expect(mobile.displaySrc).toBe(THUMB('A')); // NO auto upgrade on mobile + }); + + it('(4) the FALLBACK case issues NO second request: a thumb-md served the original', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(THUMB('A')); + // The "thumb" decoded ABOVE the thumbnail bound → it WAS the original. + loader.decoded(5000, 5000, loader.displaySrc, loader.loadToken); + expect(loader.displaySrc).toBe(THUMB('A')); // never upgraded — no double decode + expect(loader.phase).toBe('ready'); + }); +}); + +describe('viewerImageLoader — DR-16 as a LOADING gate (TASK-2459)', () => { + it('issues NO request for an unsafe MIME', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { mime_type: 'image/svg+xml', width: 800, height: 600 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(''); + expect(loader.phase).toBe('idle'); + }); + + it('issues NO request for an unresolved MIME', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { mime_type: null, width: 800, height: 600 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(''); + }); + + it('issues NO request for no image', () => { + const loader = createViewerImageLoader(); + loader.load(undefined, 'ws', 'desktop'); + expect(loader.displaySrc).toBe(''); + }); +}); + +describe('viewerImageLoader — staleness / abort on navigate + shrink (TASK-2459)', () => { + it('repointing DROPS the old URL immediately (abort by src reassignment)', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(THUMB('A')); + // Navigate to B before A finishes: the old URL is gone at once. + loader.load(image('B', { width: 800, height: 600 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(ORIGINAL('B')); + expect(loader.displaySrc).not.toContain('/A'); + }); + + it('a LATE decode for a navigated-away image does NOT drive the new image', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'desktop'); + const staleSrc = loader.displaySrc; // A's thumb + const staleGen = loader.loadToken; // A's generation + loader.load(image('B', { width: 800, height: 600 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(ORIGINAL('B')); + + // A's thumbnail finishes decoding LATE, at a fallback size (>1024). Without + // the src fence this would flip B into an unexpected upgrade / phase. + loader.decoded(5000, 5000, staleSrc, staleGen); + expect(loader.displaySrc).toBe(ORIGINAL('B')); // untouched + }); + + it('the GENERATION fence rejects an A→B→A same-URL stale decode', () => { + // The URL fence alone is insufficient: navigating A→B→A reuses A's exact + // URL, so the detached first A element's late decode has the SAME src as the + // live third request. Only the captured generation tells them apart. + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'desktop'); + const firstA = loader.loadToken; // the detached A element's generation + loader.load(image('B', { width: 5000, height: 5000 }), 'ws', 'desktop'); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(THUMB('A')); // the live third request + + // The FIRST A element decodes late at a fallback size (>1024). If accepted it + // would call `servedOriginal` true and SUPPRESS the live A's upgrade. + loader.decoded(5000, 5000, THUMB('A'), firstA); + // Live A is untouched — its own decode still drives the upgrade. + loader.decoded(1024, 768, loader.displaySrc, loader.loadToken); + expect(loader.displaySrc).toBe(ORIGINAL('A')); // upgraded, not suppressed + }); + + it('the GENERATION fence rejects an A→B→A same-URL stale ERROR', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 800, height: 600 }), 'ws', 'desktop'); + const firstA = loader.loadToken; + loader.load(image('B', { width: 800, height: 600 }), 'ws', 'desktop'); + loader.load(image('A', { width: 800, height: 600 }), 'ws', 'desktop'); + // The live third A decodes successfully. + loader.decoded(800, 600, loader.displaySrc, loader.loadToken); + expect(loader.phase).toBe('ready'); + // The detached first A element errors LATE at the same URL — it must NOT + // flip the live, ready image into 'error'. + loader.errored(ORIGINAL('A'), firstA); + expect(loader.phase).toBe('ready'); + }); + + it('dispose (close / shrink to empty) drops the load', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 800, height: 600 }), 'ws', 'desktop'); + expect(loader.displaySrc).not.toBe(''); + loader.dispose(); + expect(loader.displaySrc).toBe(''); + expect(loader.phase).toBe('idle'); + // A stale decode after dispose is inert. + loader.decoded(800, 600, ORIGINAL('A'), loader.loadToken); + expect(loader.phase).toBe('idle'); + }); +}); + +describe('viewerImageLoader — error + retry (TASK-2459)', () => { + it('a load failure shows a retryable error; retry RE-REQUESTS (never replays)', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 800, height: 600 }), 'ws', 'desktop'); + const url = loader.displaySrc; + loader.errored(url, loader.loadToken); + expect(loader.phase).toBe('error'); + + loader.retry(); + // Re-issued: the src is reset then set again (a real re-request, not a + // replay of the failed one). + expect(loader.displaySrc).toBe(url); + expect(loader.phase).toBe('loading'); + loader.decoded(800, 600, loader.displaySrc, loader.loadToken); + expect(loader.phase).toBe('ready'); + }); + + it('retry bumps the load token so the viewer re-requests a same-URL failure', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 800, height: 600 }), 'ws', 'desktop'); + const t1 = loader.loadToken; + loader.errored(loader.displaySrc, loader.loadToken); + loader.retry(); + // Same URL, but a NEW token — the viewer re-mounts the and re-fetches. + expect(loader.displaySrc).toBe(ORIGINAL('A')); + expect(loader.loadToken).toBeGreaterThan(t1); + }); + + it('the thumb→original UPGRADE does NOT bump the load token (element reused)', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'desktop'); + const t1 = loader.loadToken; + loader.decoded(1024, 768, loader.displaySrc, loader.loadToken); // upgrade + expect(loader.displaySrc).toBe(ORIGINAL('A')); + expect(loader.loadToken).toBe(t1); // unchanged — same element, no flash + }); + + it('ignores an error for a stale (already-navigated-away) src', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 800, height: 600 }), 'ws', 'desktop'); + const staleSrc = loader.displaySrc; + const staleGen = loader.loadToken; + loader.load(image('B', { width: 800, height: 600 }), 'ws', 'desktop'); + loader.errored(staleSrc, staleGen); + expect(loader.phase).toBe('loading'); // B is unaffected + }); +}); diff --git a/web/src/lib/attachments/viewerImageLoader.svelte.ts b/web/src/lib/attachments/viewerImageLoader.svelte.ts new file mode 100644 index 00000000..cc132ae6 --- /dev/null +++ b/web/src/lib/attachments/viewerImageLoader.svelte.ts @@ -0,0 +1,235 @@ +/** + * The attachment viewer's image LOADER (PLAN-2392 / TASK-2459) — the desktop + * thumb-then-original path with the DR-5b memory policy from + * {@link ./viewerLoading}. + * + * MODEL. The viewer's `` is loaded NATIVELY — `displaySrc` is the canonical + * attachment URL (a `?variant=thumb-md` first, the plain original second), and + * the browser fetches, decodes, caches and (on `src` reassignment) CANCELS each + * request. That reassignment is the abort: repointing the loader immediately + * drops the URL the user navigated away from, and no cross-image reference — + * off-DOM `Image`, object URL, fetch controller — is ever retained, so arrowing + * through twenty images can leak nothing. The bytes stay under the browser's own + * cache, not a hand-rolled blob store. + * + * STALENESS (the no-`{#key}` switch-safety class). The `` persists across + * navigation, so a bitmap that finishes decoding LATE could report the wrong + * image's pixels. Every `decoded()` therefore carries the src that decoded and + * is ignored unless it still equals `displaySrc` — the fence that keeps a stale + * completion from driving the fallback detector / upgrade for the image the user + * has already left. The workspace slug is the CAPTURED one the caller passes + * (the pane can switch workspace under a mounted viewer). + * + * The FALLBACK DETECTOR ({@link servedOriginal}) is the load-bearing piece: when + * a `thumb-md` request is served the original (fresh upload mid-derivation, or a + * WebP/AVIF the server can't derive), the decoded long edge exceeds the + * thumbnail bound and the background original request is SKIPPED — the double + * decode the whole policy exists to prevent. + */ +import { attachmentDownloadUrl } from '$lib/markdown/attachments'; +import { canOpenInViewer } from '$lib/attachments/display'; +import { decideFirstRequest, servedOriginal, type Platform } from './viewerLoading'; +import type { LightboxImage } from './events'; + +export type LoadPhase = + /** Nothing to load — no image, an unsafe/unresolved entry (DR-16), or a + * mobile large/unknown cell whose original arrives on tap (TASK-2460). */ + | 'idle' + /** A request is in flight (first paint, or the background upgrade). */ + | 'loading' + /** A bitmap is displayed. The background upgrade may still be running. */ + | 'ready' + /** The image failed to load; retryable. */ + | 'error'; + +export interface ViewerImageLoader { + /** The URL the viewer's `` should show (canonical attachment URL), or `''`. */ + readonly displaySrc: string; + readonly phase: LoadPhase; + /** + * A token that changes on every fresh REQUEST — a load or a retry, but NOT the + * thumb→original upgrade. The viewer keys its `` on this so a retry (whose + * URL is often unchanged) actually re-mounts and re-requests, and so a nav + * gets a fresh element (tearing down the previous one's stale load listener), + * while the upgrade reuses the element and does not flash. + */ + readonly loadToken: number; + /** + * Point the loader at `img` in workspace `wsSlug` on `platform`, superseding + * any prior load (its in-flight request is dropped by the `src` change). An + * `idle` no-op for no image, an unsafe/unresolved MIME (DR-16), or a cell that + * loads nothing now. + */ + load(img: LightboxImage | undefined, wsSlug: string, platform: Platform): void; + /** + * The `` decoded `decodedSrc` at `naturalWidth x naturalHeight`. `gen` is + * the `loadToken` the reporting element was mounted under. Ignored unless BOTH + * `gen` is the current `loadToken` (the generation fence) AND `decodedSrc` is + * still the current `displaySrc`. The generation fence is load-bearing where + * the URL fence is not: an A→B→A navigation reuses A's exact URL, so a detached + * first element's late completion has the same `decodedSrc` as the live request + * — only the per-mount `gen` tells them apart (the no-`{#key}` switch-safety + * class). Drives the fallback detector and the thumb→original upgrade. + */ + decoded(naturalWidth: number, naturalHeight: number, decodedSrc: string, gen: number): void; + /** + * The `` failed to load `erroredSrc`. `gen` is the reporting element's + * mount `loadToken`. Ignored unless it is the current generation AND src — + * without the generation fence a detached same-URL element's late error would + * clobber the live element's success (A→B→A). + */ + errored(erroredSrc: string, gen: number): void; + /** Retry a failed load — RE-REQUESTS from scratch, never replays a failure. */ + retry(): void; + /** Drop the current load (on close / set-shrink to empty). */ + dispose(): void; +} + +interface ActiveLoad { + img: LightboxImage; + wsSlug: string; + platform: Platform; + /** Whether a background original upgrade is still owed (desktop thumb-md). */ + upgradePending: boolean; + /** True once the original request has been issued. */ + upgrading: boolean; +} + +/** + * The URL for one variant. `original` is the CANONICAL, no-`?variant` download — + * that is what "the original" IS on the server (it serves the original when no + * derived row exists); the thumb↔original distinction IS the query param. + */ +function variantUrl(wsSlug: string, id: string, variant: 'thumb-md' | 'original'): string { + return variant === 'original' + ? attachmentDownloadUrl(wsSlug, id) + : attachmentDownloadUrl(wsSlug, id, 'thumb-md'); +} + +export function createViewerImageLoader(): ViewerImageLoader { + let displaySrc = $state(''); + let phase = $state('idle'); + // Bumped on each fresh request so the viewer can key its and force a + // re-request even when the URL is unchanged (a retry) — see `loadToken`. + let loadToken = $state(0); + // Non-reactive: never read in an $effect's tracked scope, so writing it can't + // self-invalidate a flush (CONVE-1688). + let active: ActiveLoad | null = null; + + function start(): void { + const a = active; + if (!a) return; + // DR-16 as a LOADING gate, at the request CHOKEPOINT: both load() and + // retry() funnel through here, so an unsafe or unresolved MIME never issues + // a request from either — even were a prior state to leave `active` set. The + // gate is restated where the request is actually made (not only at the + // renderer), because the renderer showing nothing is not the same as the + // loader asking for nothing. + if (!canOpenInViewer(a.img.mime_type)) { + active = null; + displaySrc = ''; + phase = 'idle'; + return; + } + // A NEW request (load / retry). The upgrade in `decoded` does NOT call + // `start`, so it never bumps this — the element (and its bitmap) is reused. + loadToken++; + const decision = decideFirstRequest(a.img.width, a.img.height, a.platform); + if (decision.variant === null) { + // Mobile large/unknown: nothing loads now (the tap affordance is + // TASK-2460). Not an error — an intentional idle. + phase = 'idle'; + displaySrc = ''; + return; + } + a.upgradePending = decision.variant === 'thumb-md' && decision.upgrade === true; + a.upgrading = false; + phase = 'loading'; + displaySrc = variantUrl(a.wsSlug, a.img.id, decision.variant); + } + + function load(img: LightboxImage | undefined, wsSlug: string, platform: Platform): void { + // Repoint: the `src` reassignment below (or the '' here) drops the old + // request. No reference to release — nothing is retained across images. + active = null; + displaySrc = ''; + if (!img) { + // No image to show. The DR-16 MIME gate lives in `start()` — the request + // chokepoint, so retry is gated too — leaving only "no image" here. + phase = 'idle'; + return; + } + active = { img, wsSlug, platform, upgradePending: false, upgrading: false }; + start(); + } + + function decoded(naturalWidth: number, naturalHeight: number, decodedSrc: string, gen: number): void { + const a = active; + // Staleness fence, two parts. The generation fence (`gen !== loadToken`) + // rejects a DETACHED element's late completion even when it carries the + // live URL — an A→B→A nav reuses A's exact URL, so the src check alone would + // let the old A element's late decode drive the new A load. The src check + // then handles the within-element thumb→original sequencing. + if (!a || gen !== loadToken || decodedSrc === '' || decodedSrc !== displaySrc) return; + phase = 'ready'; + if (!a.upgradePending || a.upgrading) return; + // The first (thumb-md) bitmap decoded. If its long edge exceeds the + // thumbnail bound the server fell back to the ORIGINAL — do NOT request it + // again (the double decode the whole policy exists to prevent). + if (servedOriginal(naturalWidth, naturalHeight)) { + a.upgradePending = false; + return; + } + // DR-16 restated at the upgrade request edge too. Defensive: `a.img` is the + // same entry `start()` already gated, but every place that issues a request + // re-checks, so the invariant is local rather than argued. + if (!canOpenInViewer(a.img.mime_type)) { + a.upgradePending = false; + return; + } + // Background-request the original. `phase` returns to 'loading' while it is + // in flight; the thumbnail stays displayed until the original decodes. + a.upgrading = true; + a.upgradePending = false; + phase = 'loading'; + displaySrc = variantUrl(a.wsSlug, a.img.id, 'original'); + } + + function errored(erroredSrc: string, gen: number): void { + // Generation + src fence (see `decoded`): a detached same-URL element's late + // error must not flip the live element's success into 'error'. + if (!active || gen !== loadToken || erroredSrc === '' || erroredSrc !== displaySrc) return; + phase = 'error'; + } + + function retry(): void { + if (!active) return; + // A fresh request, never a replay: the `src` is cleared and reissued, and + // the server may now have the variant (derivation completes async). + displaySrc = ''; + start(); + } + + function dispose(): void { + active = null; + displaySrc = ''; + phase = 'idle'; + } + + return { + get displaySrc() { + return displaySrc; + }, + get phase() { + return phase; + }, + get loadToken() { + return loadToken; + }, + load, + decoded, + errored, + retry, + dispose, + }; +} diff --git a/web/src/lib/attachments/viewerLoading.test.ts b/web/src/lib/attachments/viewerLoading.test.ts new file mode 100644 index 00000000..6e916e52 --- /dev/null +++ b/web/src/lib/attachments/viewerLoading.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect } from 'vitest'; +import { + THUMB_LONG_EDGE, + AUTO_LOAD_MAX_PIXELS, + classify, + decideFirstRequest, + servedOriginal, +} from './viewerLoading'; + +describe('viewerLoading constants', () => { + it('mirror the server bounds by value', () => { + expect(THUMB_LONG_EDGE).toBe(1024); + expect(AUTO_LOAD_MAX_PIXELS).toBe(8_000_000); + }); +}); + +describe('classify — pixels only, never bytes', () => { + it('is small at or under the pixel ceiling, large strictly above', () => { + expect(classify(2000, 2000)).toBe('small'); // 4 MP + expect(classify(4000, 2000)).toBe('small'); // exactly 8 MP — still small + expect(classify(4000, 2001)).toBe('large'); // just over + expect(classify(5000, 5000)).toBe('large'); // 25 MP + }); + + it('classifies a SMALL-byte, LARGE-pixel image as large (bytes are never consulted)', () => { + // A heavily-compressed 50 MP JPEG is a few MB on the wire but 200 MiB + // decoded — `large` is about the decoded cost, not the download. + expect(classify(10000, 5000)).toBe('large'); // 50 MP + }); + + it('yields `unknown` (a third value, NOT `large`) when a dimension is missing', () => { + expect(classify(null, 900)).toBe('unknown'); + expect(classify(900, null)).toBe('unknown'); + expect(classify(null, null)).toBe('unknown'); + expect(classify(undefined, 900)).toBe('unknown'); + // Explicitly distinct from `large`. + expect(classify(null, 900)).not.toBe('large'); + }); + + it('treats non-finite / non-positive dimensions as `unknown`', () => { + expect(classify(0, 900)).toBe('unknown'); + expect(classify(-1, 900)).toBe('unknown'); + expect(classify(NaN, 900)).toBe('unknown'); + expect(classify(Infinity, 900)).toBe('unknown'); + }); +}); + +describe('decideFirstRequest — the DR-5b decision table', () => { + it('small, long edge <= 1024: the original directly, either platform', () => { + expect(decideFirstRequest(800, 600, 'desktop')).toEqual({ variant: 'original' }); + expect(decideFirstRequest(800, 600, 'mobile')).toEqual({ variant: 'original' }); + expect(decideFirstRequest(1024, 700, 'desktop')).toEqual({ variant: 'original' }); // exactly 1024 + }); + + it('small, long edge > 1024: thumb-md — upgrade on desktop, not on mobile', () => { + // 2000x100 = 200k px (small) but long edge 2000 > 1024. + expect(decideFirstRequest(2000, 100, 'desktop')).toEqual({ variant: 'thumb-md', upgrade: true }); + expect(decideFirstRequest(2000, 100, 'mobile')).toEqual({ variant: 'thumb-md', upgrade: false }); + }); + + it('large (dims known): desktop thumb-md+upgrade, mobile nothing (tap)', () => { + expect(decideFirstRequest(5000, 5000, 'desktop')).toEqual({ variant: 'thumb-md', upgrade: true }); + expect(decideFirstRequest(5000, 5000, 'mobile')).toEqual({ variant: null }); + }); + + it('unknown dims: desktop the original DIRECTLY (one request), mobile nothing (tap)', () => { + // The unknown-desktop cell must not request a thumb — a bounded fallback + // original would defeat the detector and cause a second request. + expect(decideFirstRequest(null, 900, 'desktop')).toEqual({ variant: 'original' }); + expect(decideFirstRequest(null, null, 'desktop')).toEqual({ variant: 'original' }); + expect(decideFirstRequest(null, 900, 'mobile')).toEqual({ variant: null }); + }); +}); + +describe('servedOriginal — the fallback detector', () => { + it('is true when the decoded long edge exceeds the thumbnail bound', () => { + // The thumb request was served the original (variant absent). + expect(servedOriginal(2048, 1200)).toBe(true); + expect(servedOriginal(1200, 2048)).toBe(true); // long edge on either axis + expect(servedOriginal(1025, 10)).toBe(true); + }); + + it('is false when the decoded bitmap fits the thumbnail bound (a real thumb)', () => { + expect(servedOriginal(1024, 768)).toBe(false); // exactly at the bound + expect(servedOriginal(1000, 500)).toBe(false); + }); + + it('is false (never crashes) on missing / degenerate dimensions', () => { + expect(servedOriginal(null, null)).toBe(false); + expect(servedOriginal(0, 0)).toBe(false); + expect(servedOriginal(NaN, undefined)).toBe(false); + }); +}); diff --git a/web/src/lib/attachments/viewerLoading.ts b/web/src/lib/attachments/viewerLoading.ts new file mode 100644 index 00000000..de0c4790 --- /dev/null +++ b/web/src/lib/attachments/viewerLoading.ts @@ -0,0 +1,152 @@ +/** + * DR-5b image-loading policy for the attachment viewer (PLAN-2392 / TASK-2459). + * + * The memory-safety half of DR-5b: decide, from an image's PIXEL dimensions and + * the platform, what the viewer requests first and whether it upgrades to the + * original in the background — so a desktop promised a bounded first paint never + * silently decodes a 50 MP original twice, and a phone never auto-pulls one at + * all. + * + * PIXELS, NEVER BYTES. `size_bytes` is COMPRESSED size; a 3 MB JPEG can decode + * to 50 MP. The only honest ceiling for "how much RAM will this bitmap cost" is + * `width x height x 4` (RGBA), so classification reads dimensions and nothing + * else. When a dimension is missing the class is {@link SizeClass.unknown} — a + * genuine third value, NOT an alias for `large`: only the mobile + * no-auto-request decision treats the two alike; the desktop paths differ + * (unknown asks for the original directly, large asks for the thumbnail). + * + * This is a MEMORY POLICY, not a security boundary — the server authorizes the + * parent before it resolves any variant (`handlers_storage.go`). It is also + * FORMAT-BLIND on purpose: the load-bearing piece, {@link servedOriginal}, reads + * the decoded bitmap's own long edge rather than mirroring the server's decoder + * set, which would be a silently-rotting copy (the mirror DR-3a's shared fixture + * exists to avoid). + */ + +/** + * The `thumb-md` variant's long-edge bound, in CSS px — it mirrors the server's + * (`handlers_attachments.go`: a 1024 px long-edge derivation). A decoded bitmap + * whose long edge EXCEEDS this cannot be that thumbnail, which is exactly what + * {@link servedOriginal} keys off. + */ +export const THUMB_LONG_EDGE = 1024; + +/** + * The pixel ceiling for auto-fetching an ORIGINAL without asking: ~8 MP, about + * 32 MiB decoded RGBA. At or below it a dimensioned image is `small`; above it + * `large`. This is a CLIENT policy threshold, distinct from the server's 64 MP + * upload ceiling (`MaxPixelsDefault`). + */ +export const AUTO_LOAD_MAX_PIXELS = 8_000_000; + +/** + * The size class of an image, from its pixels alone. + * + * - `small` — dimensioned, at or under {@link AUTO_LOAD_MAX_PIXELS}. + * - `large` — dimensioned, over it. + * - `unknown` — a dimension is missing / non-finite / non-positive. A THIRD + * value, never folded into `large`. + */ +export type SizeClass = 'small' | 'large' | 'unknown'; + +export type Platform = 'desktop' | 'mobile'; + +function usableDimension(value: number | null | undefined): value is number { + return typeof value === 'number' && Number.isFinite(value) && value > 0; +} + +/** + * Classify on `width x height` and NOTHING else (never `size_bytes`). + * + * `classify(null, 900) === 'unknown'` — one missing dimension is enough, because + * a policy that can't see one axis can't bound the pixel count. A dimensioned + * image is `large` strictly ABOVE the ceiling, so an exactly-ceiling image is + * still `small`. + */ +export function classify( + width: number | null | undefined, + height: number | null | undefined +): SizeClass { + if (!usableDimension(width) || !usableDimension(height)) return 'unknown'; + return width * height > AUTO_LOAD_MAX_PIXELS ? 'large' : 'small'; +} + +/** + * True when the image's OWN pixels already fit inside the thumbnail bound, so + * the server would skip derivation and the download IS the original. Only + * meaningful for a dimensioned (`small`) image; `unknown` can't answer it. + */ +function withinThumbBound(width: number, height: number): boolean { + return Math.max(width, height) <= THUMB_LONG_EDGE; +} + +/** Which variant the viewer requests for FIRST paint, and whether it upgrades. */ +export type FirstRequest = + /** Request the original directly; there is no separate upgrade. */ + | { variant: 'original' } + /** + * Request the bounded thumbnail. `upgrade` is whether to background-fetch the + * original once it paints (desktop yes; mobile no — the original arrives on + * zoom past fit, TASK-2460). Suppressed anyway if {@link servedOriginal}. + */ + | { variant: 'thumb-md'; upgrade: boolean } + /** Request NOTHING now — the mobile tap affordance loads it (TASK-2460). */ + | { variant: null }; + +/** + * The DR-5b decision table, as one pure function. + * + * | dims | desktop | mobile | + * | -------------------------- | -------------------------- | ---------------------- | + * | small, long edge <= 1024 | original directly | original directly | + * | small, long edge > 1024 | thumb-md, upgrade | thumb-md, no upgrade | + * | large (dims known) | thumb-md, upgrade | nothing (tap) | + * | unknown dims | original directly | nothing (tap) | + * + * The `unknown`-desktop cell asks for the ORIGINAL, not a thumb: an unknown + * image whose original is <= 1024 px would defeat {@link servedOriginal} (the + * fallback serves that bounded original and the detector, seeing <= 1024, + * wrongly concludes it got a real thumbnail and fetches again). Requesting the + * original outright is exactly one request, always. + */ +export function decideFirstRequest( + width: number | null | undefined, + height: number | null | undefined, + platform: Platform +): FirstRequest { + const cls = classify(width, height); + if (cls === 'unknown') { + return platform === 'desktop' ? { variant: 'original' } : { variant: null }; + } + if (cls === 'small' && withinThumbBound(width as number, height as number)) { + // The image IS within the thumbnail bound — the download is the original. + return { variant: 'original' }; + } + // small-but-long (> 1024 px long edge) OR large, both with known dims. + if (platform === 'mobile') { + // large → tap affordance (nothing now); small-but-long → thumb, no auto original. + return cls === 'large' ? { variant: null } : { variant: 'thumb-md', upgrade: false }; + } + return { variant: 'thumb-md', upgrade: true }; +} + +/** + * THE FALLBACK DETECTOR — the load-bearing piece. + * + * A cell that requested `thumb-md` but got a bitmap whose long edge exceeds the + * thumbnail bound was served the ORIGINAL (the server falls back to it when the + * variant is absent — a fresh upload mid-derivation, or any format the pure-Go + * processor doesn't derive: WebP / AVIF). Two distinct URLs would then decode + * the original TWICE, so the background upgrade is skipped. + * + * Format-blind and dimension-free: it reads only the decoded bitmap, so it can't + * rot when the server's decoder set changes. + */ +export function servedOriginal( + naturalWidth: number | null | undefined, + naturalHeight: number | null | undefined +): boolean { + const w = usableDimension(naturalWidth) ? naturalWidth : 0; + const h = usableDimension(naturalHeight) ? naturalHeight : 0; + return Math.max(w, h) > THUMB_LONG_EDGE; +} diff --git a/web/src/lib/components/common/Lightbox.svelte b/web/src/lib/components/common/Lightbox.svelte index 722199c5..a1bc0741 100644 --- a/web/src/lib/components/common/Lightbox.svelte +++ b/web/src/lib/components/common/Lightbox.svelte @@ -21,8 +21,10 @@ * does not. */ import { untrack } from 'svelte'; - import { attachmentDownloadUrl } from '$lib/markdown/attachments'; import { paneFocusables, nextTrapTarget, handoffFocus } from '$lib/collections/paneFocus'; + import { createViewerImageLoader } from '$lib/attachments/viewerImageLoader.svelte'; + import type { Platform } from '$lib/attachments/viewerLoading'; + import { viewport } from '$lib/stores/breakpoint.svelte'; import { acquire, isBlockedByModal, @@ -175,12 +177,30 @@ Math.min(Math.max(current, 0), Math.max(viewable.length - 1, 0)) ); let img = $derived(viewable[shownIndex]); - 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'); + // ── Image loading (PLAN-2392 phase 3b / TASK-2459) ──────────────────────── + // + // The DR-5b thumb-then-original policy. The loader owns which URL the + // shows (`displaySrc`, the canonical attachment URL) and the load phase; this + // component drives it from the shown image and reports each decode / error. + const loader = createViewerImageLoader(); + // The id AND the pixel dimensions, as ONE stable primitive string — never the + // `img` object. A prop re-emit with the same VALUES (a re-derived `viewable` + // array) must not re-fire the load effect, but a genuine dimension change (an + // async metadata fill that flips `unknown` → a sized class) MUST: the DR-5b + // policy is a function of the pixels, so a stale dimension is a stale policy. + // A shrink to no image collapses to `'::'`, still a change → the effect fires + // and releases the load. + let loadKey = $derived(`${img?.id ?? ''}:${img?.width ?? ''}:${img?.height ?? ''}`); + // Captured NON-reactively at load time (see the effect): a breakpoint flip + // alone must not reload — desktop→mobile must not abort an in-flight original, + // mobile→desktop must not retroactively auto-fetch (TASK-2459). + let platform = $derived(viewport.isMobile ? 'mobile' : 'desktop'); + // 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). @@ -379,11 +399,13 @@ // leaves `maybeDrag` latched and the next move engages a phantom drag from a // dead baseline. (We already returned above if a drag is live.) maybeDrag = false; - // A press ON a control (close / nav) is that control's click, never a pan: - // arming here would let a drag OFF a button still fire its click, since the - // buttons' own handlers don't consult `suppressClick` (the house pattern in - // ItemGraph excludes its interactive overlays the same way). - if ((e.target as Element | null)?.closest?.('.lightbox-close, .lightbox-nav')) return; + // A press ON a control (close / nav / the DR-10 retry) is that control's + // click, never a pan: arming here would let a drag OFF a button still fire + // its click, since the buttons' own handlers don't consult `suppressClick` + // (the house pattern in ItemGraph excludes its interactive overlays the same + // way). Retry sits over the (broken) stage in the error state, so it needs + // the same exclusion as the always-present chrome. + if ((e.target as Element | null)?.closest?.('.lightbox-close, .lightbox-nav, .lightbox-retry')) return; const el = rootEl; if (!el || !pointerGatesOpen(el)) return; // START gate maybeDrag = true; @@ -499,9 +521,10 @@ // gesture is either a drag or a double-click, never both. Same swallow the // backdrop click consults. if (suppressClick) return; - // A double-click ON a control (close / nav) is that control's, not a zoom - // toggle — without this, double-clicking Next navigates twice AND toggles. - if ((e.target as Element | null)?.closest?.('.lightbox-close, .lightbox-nav')) return; + // A double-click ON a control (close / nav / retry) is that control's, not a + // zoom toggle — without this, double-clicking Next navigates twice AND + // toggles, or a double-click on Retry toggles zoom on the broken stage. + if ((e.target as Element | null)?.closest?.('.lightbox-close, .lightbox-nav, .lightbox-retry')) return; const el = rootEl; if (!el || !pointerGatesOpen(el)) return; const g = readGeometry(); @@ -652,6 +675,20 @@ // `zoom`-writing effect would self-invalidate it (CONVE-1688). }); + // (Re)load whenever the SHOWN image changes — nav, a dimension fill, or a set + // shrink. This is also the ABORT + release point: `loader.load` drops the URL + // the user left, and it re-runs on the set shrinking to empty (`loadKey` → + // `'::'`) so a closed / emptied viewer holds no in-flight request. Reads only + // `loadKey` (tracked, a stable string so a same-values prop re-emit doesn't + // re-fire); `img`, `openWsSlug` and `platform` are captured non-reactively so a + // breakpoint flip alone can't reload (TASK-2459). + $effect(() => { + void loadKey; + untrack(() => loader.load(img, openWsSlug, platform)); + }); + // Drop the load on unmount (close) — one teardown, no dependencies. + $effect(() => () => loader.dispose()); + // Re-clamp on stage resize. `maxScale` is geometry-dependent and geometry is // viewport-dependent, so ENLARGING the window lowers the ceiling and can // strand a previously-valid scale above it. `clampState` re-clamps SCALE @@ -702,9 +739,15 @@ // element state and mutates focus — a DOM side effect, not $state — so it // cannot self-invalidate its own flush (CONVE-1688). $effect(() => { - // Tracked so the effect re-runs when a focus-holding control mounts or - // unmounts; a true→false flip is the shrink that removes the focused button. + // Tracked so the effect re-runs whenever a focus-holding control mounts or + // unmounts: the nav buttons (`hasMultiple`), the DR-10 retry button + // (`phase === 'error'`), and the image itself (`img`) — a set shrinking to + // empty removes the image, and an errored image navigated away removes the + // retry button, either of which could otherwise strand focus on + // (TASK-2459). void hasMultiple; + void (loader.phase === 'error'); + void img; const el = rootEl; if (!el || !isViewerFrontmost(el) || isBlockedByModal(el)) return; handoffFocus(el); @@ -874,17 +917,79 @@ sit ABOVE the stage (their own `z-index`), never inside it. -->
- {#if img} - - {img.alt + {#if img && loader.displaySrc} + + {#key loader.loadToken} + + + {img.alt { + const el = e.currentTarget as HTMLImageElement; + loader.decoded(el.naturalWidth, el.naturalHeight, el.getAttribute('src') ?? '', Number(el.dataset.gen)); + }} + onerror={(e) => { + const el = e.currentTarget as HTMLImageElement; + loader.errored(el.getAttribute('src') ?? '', Number(el.dataset.gen)); + }} + style="transform: translate({zoom.x}px, {zoom.y}px) scale({zoom.scale});" + /> + {/key} + {/if} + + {#if img && loader.phase === 'loading'} + +
+ +
+ {/if} + + {#if img && loader.phase === 'error'} + {/if}
@@ -990,6 +1095,74 @@ } } + /* Loading spinner + error, centred over the stage (TASK-2459). */ + .lightbox-status { + position: absolute; + inset: 0; + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + gap: var(--space-3); + z-index: 1; + } + + .lightbox-loading { + /* Never intercept a pan / double-click over the stage. */ + pointer-events: none; + } + + .lightbox-spinner { + width: 42px; + height: 42px; + border: 3px solid rgba(255, 255, 255, 0.25); + border-top-color: rgba(255, 255, 255, 0.9); + border-radius: 50%; + animation: lightbox-spin 0.8s linear infinite; + } + + @media (prefers-reduced-motion: reduce) { + /* Slowed, not stopped — it must still read as "in progress". */ + .lightbox-spinner { + animation-duration: 2.4s; + } + } + + @keyframes lightbox-spin { + to { + transform: rotate(360deg); + } + } + + .lightbox-error { + /* The layer is inert; the button re-enables itself below. */ + pointer-events: none; + color: #fff; + text-align: center; + } + + .lightbox-error-text { + margin: 0; + font-size: 0.95rem; + } + + .lightbox-retry { + /* EXPLICIT: the stage sets `pointer-events: none`, so the one interactive + control in this layer has to turn them back on (TASK-2459). */ + pointer-events: auto; + padding: var(--space-2) var(--space-4); + background: rgba(0, 0, 0, 0.5); + border: 1px solid rgba(255, 255, 255, 0.3); + border-radius: var(--radius); + color: #fff; + font-size: 0.9rem; + cursor: pointer; + } + + .lightbox-retry:hover { + background: rgba(0, 0, 0, 0.75); + } + .lightbox-close { position: absolute; /* Above the stage's stacking context. Small on purpose: the viewer's own diff --git a/web/src/lib/components/common/Lightbox.svelte.test.ts b/web/src/lib/components/common/Lightbox.svelte.test.ts index fb1ea076..8ed9db77 100644 --- a/web/src/lib/components/common/Lightbox.svelte.test.ts +++ b/web/src/lib/components/common/Lightbox.svelte.test.ts @@ -2265,3 +2265,236 @@ describe('Lightbox — drag-to-pan and double-click (TASK-2458)', () => { expect(scaleOf(front)).toBeCloseTo(2000 / 900); // frontmost did }); }); + +// TASK-2459 — the DR-5b loader WIRED into the viewer. The request logic is +// proven in viewerImageLoader.svelte.test.ts; these pin the wiring: the +// shows the loader's URL, its decode feeds the fallback detector / upgrade, and +// the spinner / error / retry states render. + +/** A LightboxImage with explicit pixel dimensions. */ +function sized(id: string, alt: string, width: number, height: number, mime = 'image/png'): LightboxImage { + return { id, alt, filename: null, mime_type: mime, size_bytes: null, width, height }; +} +/** Simulate the decoding at `naturalWidth x naturalHeight`, then flush. */ +function fireLoad(naturalWidth: number, naturalHeight: number, scope: HTMLElement = root()): void { + const el = scope.querySelector('.lightbox-image'); + if (!el) throw new Error('no image to load'); + Object.defineProperty(el, 'naturalWidth', { configurable: true, get: () => naturalWidth }); + Object.defineProperty(el, 'naturalHeight', { configurable: true, get: () => naturalHeight }); + el.dispatchEvent(new Event('load')); + flushSync(); +} + +describe('Lightbox — DR-5b image loading (TASK-2459)', () => { + it('a large image loads the THUMBNAIL first, then upgrades to the original on decode', () => { + mountViewer({ images: [sized(IMG_A, 'big', 5000, 5000)] }); + expect(imageSrc()).toContain('variant=thumb-md'); + expect(imageSrc()).toContain(IMG_A); + + // The thumb decodes at a bounded size → the background upgrade swaps in. + fireLoad(1024, 768); + expect(imageSrc()).toContain(IMG_A); + expect(imageSrc()).not.toContain('variant='); // the original, no variant + }); + + it('the FALLBACK case does not upgrade (a thumb-md request served the original)', () => { + mountViewer({ images: [sized(IMG_A, 'big', 5000, 5000)] }); + expect(imageSrc()).toContain('variant=thumb-md'); + // Decoded ABOVE the thumbnail bound → it WAS the original: no second request. + fireLoad(5000, 5000); + expect(imageSrc()).toContain('variant=thumb-md'); + }); + + it('an unknown-dimensions image requests the ORIGINAL directly (no thumb)', () => { + mountViewer({ images: [image(IMG_A, 'a')] }); // the helper leaves dims null + expect(imageSrc()).toContain(IMG_A); + expect(imageSrc()).not.toContain('variant='); + }); + + it('shows a loading spinner until the image decodes, then hides it', () => { + mountViewer({ images: [sized(IMG_A, 'big', 5000, 5000)] }); + expect(root().querySelector('.lightbox-loading')).not.toBeNull(); + fireLoad(1024, 768); // thumb decoded → upgrade requested (still loading) + expect(root().querySelector('.lightbox-loading')).not.toBeNull(); + fireLoad(5000, 5000); // original decoded → ready + expect(root().querySelector('.lightbox-loading')).toBeNull(); + }); + + it('a load error shows a retryable error; retry RE-REQUESTS and hands off focus', () => { + mountViewer({ images: [sized(IMG_A, 'big', 5000, 5000)] }); + const el = root().querySelector('.lightbox-image')!; + el.dispatchEvent(new Event('error')); + flushSync(); + expect(root().querySelector('.lightbox-error')).not.toBeNull(); + + const retry = root().querySelector('.lightbox-retry')!; + // The retry control carries pointer-events:auto explicitly (the stage is off). + expect(getComputedStyle(retry).pointerEvents).not.toBe('none'); + retry.focus(); + retry.click(); + flushSync(); + // Re-requested → loading again → the error (and its button) are gone, and + // focus was handed off before the button disappeared (TASK-2456). + expect(root().querySelector('.lightbox-error')).toBeNull(); + expect(document.activeElement).toBe(closeButton()); + }); + + it('ABORTS on navigate: the src points at the new image, dropping the old URL', () => { + mountViewer({ + images: [sized(IMG_A, 'a', 5000, 5000), sized(IMG_B, 'b', 800, 600, 'image/jpeg')], + }); + expect(imageSrc()).toContain(IMG_A); + expect(imageSrc()).toContain('variant=thumb-md'); + + expect(press('ArrowRight')).toBe(true); + expect(imageSrc()).toContain(IMG_B); + expect(imageSrc()).not.toContain(IMG_A); // the old URL is gone at once + expect(imageSrc()).not.toContain('variant='); // B is small ≤1024 → original + }); + + it('retry RE-MOUNTS the so a same-URL failure is actually re-requested', () => { + mountViewer({ images: [image(IMG_A, 'a')] }); // unknown dims → original URL directly + const before = root().querySelector('.lightbox-image')!; + const beforeSrc = before.getAttribute('src'); + before.dispatchEvent(new Event('error')); + flushSync(); + root().querySelector('.lightbox-retry')!.click(); + flushSync(); + const after = root().querySelector('.lightbox-image'); + // A NEW element (the load token changed the {#key}) at the SAME URL — the + // browser re-requests even though a naive `src=''`→same-URL would no-op. + expect(after).not.toBe(before); + expect(after?.getAttribute('src')).toBe(beforeSrc); + }); + + it('a focused retry that unmounts on set-shrink does not strand focus on ', () => { + liveProps.images = [image(IMG_A, 'a')]; + const app = mount(Lightbox, { target: appRoot, props: liveProps }); + mounted.push(app); + flushSync(); + root().querySelector('.lightbox-image')!.dispatchEvent(new Event('error')); + flushSync(); + const retry = root().querySelector('.lightbox-retry')!; + retry.focus(); + expect(document.activeElement).toBe(retry); + + // The set shrinks to empty — the errored image and its retry button unmount. + liveProps.images = []; + flushSync(); + expect(root().querySelector('.lightbox-retry')).toBeNull(); + expect(document.activeElement).toBe(closeButton()); + expect(document.activeElement).not.toBe(document.body); + }); + + it('a LATE decode for a navigated-away image does not corrupt the current one', () => { + mountViewer({ + images: [sized(IMG_A, 'a', 5000, 5000), sized(IMG_B, 'b', 5000, 5000, 'image/jpeg')], + }); + const staleImg = root().querySelector('.lightbox-image')!; // A's element + expect(staleImg.getAttribute('src')).toContain('variant=thumb-md'); + + // Navigate to B — the id-keyed remounts, detaching A's element. + expect(press('ArrowRight')).toBe(true); + expect(imageSrc()).toContain(IMG_B); + expect(imageSrc()).toContain('variant=thumb-md'); + + // A's thumbnail finishes decoding LATE at a FALLBACK size (>1024) on the now + // detached element. It must not mark B ready or suppress B's upgrade. + Object.defineProperty(staleImg, 'naturalWidth', { configurable: true, get: () => 5000 }); + Object.defineProperty(staleImg, 'naturalHeight', { configurable: true, get: () => 5000 }); + staleImg.dispatchEvent(new Event('load')); + flushSync(); + expect(imageSrc()).toContain(IMG_B); + expect(imageSrc()).toContain('variant=thumb-md'); // B untouched + + // B's OWN decode still drives B's upgrade. + fireLoad(1024, 768); + expect(imageSrc()).toContain(IMG_B); + expect(imageSrc()).not.toContain('variant='); + }); + + it('zoom/pan still work on the image the loader RE-MOUNTED after navigation', () => { + // The {#key} remounts the on nav, rebinding `imgEl`. The zoom reads + // geometry from `imgEl`; a stale bind would read the detached element and + // pan nothing. + mountViewer({ + images: [sized(IMG_A, 'a', 5000, 5000), sized(IMG_B, 'b', 5000, 5000, 'image/jpeg')], + }); + expect(press('ArrowRight')).toBe(true); + expect(imageSrc()).toContain(IMG_B); + + mockGeometry(root(), OVERFLOW_G); // mocks the NEW (B's) image element + root().dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 500, clientY: 500 })); + flushSync(); + expect(scaleOf()).toBeCloseTo(2000 / 900); // zoom read the rebound element + + root().dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(panX()).toBeCloseTo(100); + }); + + it('DR-16: an all-unsafe set renders no image and issues no request', () => { + mountViewer({ images: [image(IMG_A, 'svg', 'image/svg+xml')] }); + expect(root().querySelector('.lightbox-image')).toBeNull(); + expect(root().querySelector('.lightbox-loading')).toBeNull(); + }); + + it('a same-id re-emit that FILLS dimensions re-runs the DR-5b policy', () => { + // The load key is id + dimensions, so a re-derived set with the SAME values + // does not reload, but an async metadata fill (unknown → sized) MUST — a + // stale dimension is a stale policy. + liveProps.images = [image(IMG_A, 'a')]; // unknown dims → original directly + const app = mount(Lightbox, { target: appRoot, props: liveProps }); + mounted.push(app); + flushSync(); + expect(imageSrc()).not.toContain('variant='); // unknown → the original + + // The same id re-emits with LARGE dimensions filled in. + liveProps.images = [sized(IMG_A, 'a', 5000, 5000)]; + flushSync(); + expect(imageSrc()).toContain('variant=thumb-md'); // policy re-ran → thumb first + }); + + it('the retry button is excluded from pan + zoom-toggle gestures over the stage', () => { + mountViewer({ images: [sized(IMG_A, 'a', 5000, 5000)] }); + root().querySelector('.lightbox-image')!.dispatchEvent(new Event('error')); + flushSync(); + mockGeometry(root(), OVERFLOW_G); + const retry = root().querySelector('.lightbox-retry')!; + const before = scaleOf(); + // A double-click ON retry must not toggle zoom (excluded like close / nav). + retry.dispatchEvent(new MouseEvent('dblclick', { bubbles: true, clientX: 500, clientY: 500 })); + flushSync(); + expect(scaleOf()).toBeCloseTo(before); + // A drag STARTING on retry must not pan the broken stage. + retry.dispatchEvent(pointerEvent('pointerdown', 500, 500)); + root().dispatchEvent(pointerEvent('pointermove', 600, 500)); + flushSync(); + expect(panX()).toBeCloseTo(0); + }); + + it('an A→B→A same-URL late error on the DETACHED element does not clobber the live image', () => { + // The third A load reuses A's exact URL, so the URL fence alone cannot tell + // the detached first A element's late error from the live one — the per-mount + // generation is what rejects it (the no-`{#key}` switch-safety class). + mountViewer({ + images: [sized(IMG_A, 'a', 800, 600), sized(IMG_B, 'b', 800, 600, 'image/jpeg')], + }); + const firstA = root().querySelector('.lightbox-image')!; // E1 + expect(press('ArrowRight')).toBe(true); // → B (E1 detaches) + expect(press('ArrowLeft')).toBe(true); // → A again (E3 mounts, same URL) + const liveA = root().querySelector('.lightbox-image')!; + expect(liveA).not.toBe(firstA); + expect(liveA.getAttribute('src')).toBe(firstA.getAttribute('src')); // same URL + + fireLoad(800, 600); // the LIVE A decodes fine → ready, no error + expect(root().querySelector('.lightbox-error')).toBeNull(); + + // The detached E1 errors LATE at the same URL — the generation fence must + // reject it so the live, ready image is not flipped into 'error'. + firstA.dispatchEvent(new Event('error')); + flushSync(); + expect(root().querySelector('.lightbox-error')).toBeNull(); + }); +}); diff --git a/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts b/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts index bc5e5551..84104757 100644 --- a/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts +++ b/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts @@ -151,9 +151,15 @@ describe('inline image → viewer host → Lightbox', () => { expect(viewer.getAttribute('role')).toBe('dialog'); expect(viewer.parentElement).toBe(document.body); const shown = viewer.querySelector('img.lightbox-image'); + // The URL is built from the CAPTURED workspace this NodeView emitted in + // (the pane can switch workspace under a mounted viewer) — this pins + // which workspace, which is why it survives the DR-5b loading rework. expect(shown?.getAttribute('src')).toContain(UUID); - // The full-resolution blob, as the deleted dialog loaded: the viewer - // asks for the canonical URL with no `variant`. + // NO `?variant`: an inline editor image carries no dimensions, so it is + // `unknown` to the DR-5b classifier, and the unknown-dims desktop cell + // requests the ORIGINAL directly — the canonical, no-variant URL — never + // a thumbnail (TASK-2459). A dimensioned large image WOULD ship + // `?variant=thumb-md` first; that path is covered in the loader tests. expect(shown?.getAttribute('src')).not.toContain('variant='); }); } diff --git a/web/src/lib/components/items/ItemAttachmentStrip.svelte.test.ts b/web/src/lib/components/items/ItemAttachmentStrip.svelte.test.ts index e10af635..be7bcc37 100644 --- a/web/src/lib/components/items/ItemAttachmentStrip.svelte.test.ts +++ b/web/src/lib/components/items/ItemAttachmentStrip.svelte.test.ts @@ -1499,6 +1499,8 @@ describe('ItemAttachmentStrip', () => { filename: `${id}.png`, mime_type: 'image/png', size_bytes: 4096, + width: null, + height: null, }); it('shows a dropped file immediately, without a refetch', async () => { From e79a9bbb955f4e9b021bb8c91e5191aff73af595 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 8 Aug 2026 04:40:39 +0000 Subject: [PATCH 7/9] =?UTF-8?q?feat(attachments):=20DR-5b=20mobile=20polic?= =?UTF-8?q?y=20=E2=80=94=20tap-to-load=20and=20fetch-on-zoom-past-fit=20(T?= =?UTF-8?q?ASK-2460)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Own the mobile cells of the DR-5b decision table (the classifier landed in TASK-2459). Add a `deferred` loader phase for the mobile large/unknown cell — no automatic request; the viewer shows a tap-to-load placeholder sized to the image's aspect ratio (a neutral box when dimensions are unknown), with explicit `pointer-events: auto` and focus-handoff since it is replaced by the image it loads. A single deduplicated `loadOriginal` serves BOTH triggers — the tap (deferred cell) and zoom-past-fit (`scale > 1`, the painted thumb cell) — so a tap racing a zoom, a retry, or a later pinch is one fetch, not two. The fallback detector now runs on every first decode, desktop and mobile: when `thumb-md` was served the original (fresh upload, WebP/AVIF), it clears the deferred original so zoom-past-fit cannot re-download bytes already held. The zoom-past-fit trigger gates on a `painted` flag (tracked, never written by loadOriginal, so no self-write) — a zoom made before the thumb paints upgrades the instant it does, rather than stranding the user on the thumbnail. Zoom is disabled, not just no-op, while nothing is decoded: the zoom keys short-circuit and drag no longer arms/captures over the deferred placeholder. Retry after an on-demand original failure re-requests the original directly rather than reverting to the affordance. Breakpoint flips do not retro-fetch (platform is captured at load). Every idle cleanup funnels through one `toIdle()` so the `painted === false in idle` invariant can't drift, and every request edge restates the DR-16 gate. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu --- .../viewerImageLoader.svelte.test.ts | 102 +++++++++- .../attachments/viewerImageLoader.svelte.ts | 168 +++++++++++++--- web/src/lib/components/common/Lightbox.svelte | 124 +++++++++++- .../components/common/Lightbox.svelte.test.ts | 179 ++++++++++++++++++ 4 files changed, 538 insertions(+), 35 deletions(-) diff --git a/web/src/lib/attachments/viewerImageLoader.svelte.test.ts b/web/src/lib/attachments/viewerImageLoader.svelte.test.ts index 7a8f53a9..3657f533 100644 --- a/web/src/lib/attachments/viewerImageLoader.svelte.test.ts +++ b/web/src/lib/attachments/viewerImageLoader.svelte.test.ts @@ -50,16 +50,110 @@ describe('viewerImageLoader — the decision table as requests (TASK-2459)', () expect(loader.displaySrc).toBe(ORIGINAL('A')); }); - it('large / unknown on mobile: NO request (idle — the tap affordance is TASK-2460)', () => { + it('large / unknown on mobile: NO request — `deferred`, the tap affordance (TASK-2460)', () => { const large = createViewerImageLoader(); large.load(image('A', { width: 5000, height: 5000 }), 'ws', 'mobile'); - expect(large.displaySrc).toBe(''); - expect(large.phase).toBe('idle'); + expect(large.displaySrc).toBe(''); // no request issued + expect(large.phase).toBe('deferred'); // NOT 'idle' — the original is on tap const unknown = createViewerImageLoader(); unknown.load(image('B', { width: null, height: 900 }), 'ws', 'mobile'); expect(unknown.displaySrc).toBe(''); - expect(unknown.phase).toBe('idle'); + expect(unknown.phase).toBe('deferred'); + }); +}); + +describe('viewerImageLoader — mobile on-demand original (TASK-2460)', () => { + it('deferred cell: loadOriginal (tap) requests the ORIGINAL directly, once', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'mobile'); + expect(loader.phase).toBe('deferred'); + expect(loader.displaySrc).toBe(''); + + loader.loadOriginal(); // the tap + expect(loader.displaySrc).toBe(ORIGINAL('A')); + expect(loader.phase).toBe('loading'); + + // A second trigger (a second tap, or a zoom-past-fit racing it) is a no-op. + const t = loader.loadToken; + loader.loadOriginal(); + expect(loader.displaySrc).toBe(ORIGINAL('A')); + expect(loader.loadToken).toBe(t); + + loader.decoded(5000, 5000, loader.displaySrc, loader.loadToken); + expect(loader.phase).toBe('ready'); + }); + + it('mobile thumb cell: thumb paints, then loadOriginal (zoom-past-fit) upgrades once', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 2000, height: 100 }), 'ws', 'mobile'); + expect(loader.displaySrc).toBe(THUMB('A')); // thumb painted, no auto-upgrade + const thumbToken = loader.loadToken; + loader.decoded(1024, 51, loader.displaySrc, loader.loadToken); + expect(loader.displaySrc).toBe(THUMB('A')); // mobile: still the thumb + + loader.loadOriginal(); // zoom past fit + expect(loader.displaySrc).toBe(ORIGINAL('A')); + // SAME element reused (token unchanged) so the thumb stays until the original + // decodes — no flash. + expect(loader.loadToken).toBe(thumbToken); + + // A second zoom step past fit does not re-request. + loader.loadOriginal(); + expect(loader.displaySrc).toBe(ORIGINAL('A')); + expect(loader.loadToken).toBe(thumbToken); + }); + + it('mobile thumb cell whose thumb-md SERVED THE ORIGINAL issues no zoom re-fetch', () => { + // The fallback detector must run on the mobile path too: a fresh upload / + // WebP / AVIF has no derived thumb, so `?variant=thumb-md` returns the + // ORIGINAL. Without clearing `originalDeferred`, a later zoom-past-fit would + // download and decode the original a SECOND time. + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 2000, height: 100 }), 'ws', 'mobile'); + expect(loader.displaySrc).toBe(THUMB('A')); + // The "thumb" decoded ABOVE the bound → it WAS the original. + loader.decoded(2000, 100, loader.displaySrc, loader.loadToken); + expect(loader.phase).toBe('ready'); + // Zoom past fit now finds nothing deferred — no second request. + loader.loadOriginal(); + expect(loader.displaySrc).toBe(THUMB('A')); // unchanged + }); + + it('desktop never defers: loadOriginal is a no-op (auto-upgrade owns the original)', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'desktop'); + expect(loader.displaySrc).toBe(THUMB('A')); + const before = loader.displaySrc; + loader.loadOriginal(); // no-op on desktop + expect(loader.displaySrc).toBe(before); + }); + + it('retry after an on-demand original FAILS re-requests the original, not the affordance', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 5000, height: 5000 }), 'ws', 'mobile'); + loader.loadOriginal(); // tap + expect(loader.displaySrc).toBe(ORIGINAL('A')); + loader.errored(loader.displaySrc, loader.loadToken); + expect(loader.phase).toBe('error'); + + const t = loader.loadToken; + loader.retry(); + // Back to the ORIGINAL (a fresh token remounts to refetch) — NOT reverted to + // the 'deferred' tap affordance. + expect(loader.phase).toBe('loading'); + expect(loader.displaySrc).toBe(ORIGINAL('A')); + expect(loader.loadToken).toBeGreaterThan(t); + }); + + it('retry after the INITIAL thumb fails re-requests the thumb (start path)', () => { + const loader = createViewerImageLoader(); + loader.load(image('A', { width: 2000, height: 100 }), 'ws', 'mobile'); + expect(loader.displaySrc).toBe(THUMB('A')); + loader.errored(loader.displaySrc, loader.loadToken); + expect(loader.phase).toBe('error'); + loader.retry(); + expect(loader.displaySrc).toBe(THUMB('A')); // the initial policy, re-run }); }); diff --git a/web/src/lib/attachments/viewerImageLoader.svelte.ts b/web/src/lib/attachments/viewerImageLoader.svelte.ts index cc132ae6..388eb98c 100644 --- a/web/src/lib/attachments/viewerImageLoader.svelte.ts +++ b/web/src/lib/attachments/viewerImageLoader.svelte.ts @@ -32,12 +32,20 @@ import { decideFirstRequest, servedOriginal, type Platform } from './viewerLoadi import type { LightboxImage } from './events'; export type LoadPhase = - /** Nothing to load — no image, an unsafe/unresolved entry (DR-16), or a - * mobile large/unknown cell whose original arrives on tap (TASK-2460). */ + /** Nothing to load and nothing to offer — no image, or an unsafe/unresolved + * entry (DR-16). No ``, no affordance. */ | 'idle' - /** A request is in flight (first paint, or the background upgrade). */ + /** A mobile large/unknown cell: nothing auto-loads, but the ORIGINAL is + * available on demand — the viewer shows the tap-to-load affordance, and a + * tap (or zoom-past-fit once a bitmap exists) calls {@link + * ViewerImageLoader.loadOriginal} (TASK-2460). Distinct from `idle`, which + * offers nothing. */ + | 'deferred' + /** A request is in flight (first paint, or the on-demand / background + * original). */ | 'loading' - /** A bitmap is displayed. The background upgrade may still be running. */ + /** A bitmap is displayed. A background/on-demand original may still be + * running. */ | 'ready' /** The image failed to load; retryable. */ | 'error'; @@ -54,6 +62,15 @@ export interface ViewerImageLoader { * while the upgrade reuses the element and does not flash. */ readonly loadToken: number; + /** + * Whether a bitmap has DECODED for the current image and is on screen (a thumb + * or the original). False from `load` until the first decode, and in `idle` / + * `deferred`; stays true across the thumb→original upgrade. The viewer's + * zoom-past-fit trigger reads it so a zoom made BEFORE the thumb paints still + * upgrades the moment it does — and, because `loadOriginal` never writes it, the + * trigger effect can track it without self-invalidating (CONVE-1688). + */ + readonly painted: boolean; /** * Point the loader at `img` in workspace `wsSlug` on `platform`, superseding * any prior load (its in-flight request is dropped by the `src` change). An @@ -61,6 +78,16 @@ export interface ViewerImageLoader { * loads nothing now. */ load(img: LightboxImage | undefined, wsSlug: string, platform: Platform): void; + /** + * Request the ORIGINAL on demand — the mobile fetch DR-5b defers. Called by + * BOTH the tap affordance (the `deferred` cell, where nothing painted) and by + * zoom-past-fit (the mobile thumb cell, where a thumbnail painted). The two are + * ONE deduplicated fetch: a call while the original is already requested or + * in flight is a no-op, so a tap racing a zoom, 3d's pinch, or a retry can + * never issue a second request (TASK-2460). A no-op on desktop and on any cell + * that already loaded the original directly, where nothing is deferred. + */ + loadOriginal(): void; /** * The `` decoded `decodedSrc` at `naturalWidth x naturalHeight`. `gen` is * the `loadToken` the reporting element was mounted under. Ignored unless BOTH @@ -93,6 +120,15 @@ interface ActiveLoad { upgradePending: boolean; /** True once the original request has been issued. */ upgrading: boolean; + /** + * Whether the ORIGINAL is available to fetch ON DEMAND and has not been + * requested yet — the mobile deferral (TASK-2460). True for a mobile + * large/unknown cell (fetched on tap) AND a mobile thumb cell (fetched on + * zoom-past-fit); false on desktop (auto-upgrades) and where the original was + * loaded directly. {@link loadOriginal}'s dedup guard reads it: it flips false + * on the first fetch, so a second trigger is a no-op. + */ + originalDeferred: boolean; } /** @@ -112,10 +148,25 @@ export function createViewerImageLoader(): ViewerImageLoader { // Bumped on each fresh request so the viewer can key its and force a // re-request even when the URL is unchanged (a retry) — see `loadToken`. let loadToken = $state(0); + // Whether a bitmap has decoded for the current image (see the interface). The + // zoom-past-fit trigger TRACKS this; `loadOriginal` deliberately never writes + // it, so that tracking cannot self-invalidate the trigger's flush (CONVE-1688). + let painted = $state(false); // Non-reactive: never read in an $effect's tracked scope, so writing it can't // self-invalidate a flush (CONVE-1688). let active: ActiveLoad | null = null; + // Retire to the neutral state — no active load, no URL, nothing decoded, no + // affordance. ONE cleanup so every "give up on this entry" site keeps the + // `painted === false in idle` contract; sprinkling the resets inline is how one + // gets left stale. + function toIdle(): void { + active = null; + displaySrc = ''; + painted = false; + phase = 'idle'; + } + function start(): void { const a = active; if (!a) return; @@ -126,24 +177,30 @@ export function createViewerImageLoader(): ViewerImageLoader { // renderer), because the renderer showing nothing is not the same as the // loader asking for nothing. if (!canOpenInViewer(a.img.mime_type)) { - active = null; - displaySrc = ''; - phase = 'idle'; + toIdle(); return; } - // A NEW request (load / retry). The upgrade in `decoded` does NOT call - // `start`, so it never bumps this — the element (and its bitmap) is reused. + // A NEW request (load / retry). The upgrade in `decoded` and the on-demand + // `loadOriginal` do NOT call `start`, so neither bumps this — the element + // (and its bitmap) is reused. loadToken++; const decision = decideFirstRequest(a.img.width, a.img.height, a.platform); + a.upgrading = false; + // The original is DEFERRED (fetched on tap / zoom-past-fit) precisely when + // the platform is mobile and the first request is not already the original: + // the mobile large/unknown cell (nothing painted) and the mobile thumb cell + // (a thumbnail painted, no auto-upgrade). Desktop auto-upgrades; a + // direct-original cell has nothing left to fetch (TASK-2460). + a.originalDeferred = a.platform === 'mobile' && decision.variant !== 'original'; + a.upgradePending = decision.variant === 'thumb-md' && decision.upgrade === true; if (decision.variant === null) { - // Mobile large/unknown: nothing loads now (the tap affordance is - // TASK-2460). Not an error — an intentional idle. - phase = 'idle'; + // Mobile large/unknown: nothing auto-loads. `deferred` (NOT `idle`) so the + // viewer shows the tap-to-load affordance; the original arrives via + // `loadOriginal` on tap. + phase = 'deferred'; displaySrc = ''; return; } - a.upgradePending = decision.variant === 'thumb-md' && decision.upgrade === true; - a.upgrading = false; phase = 'loading'; displaySrc = variantUrl(a.wsSlug, a.img.id, decision.variant); } @@ -153,16 +210,46 @@ export function createViewerImageLoader(): ViewerImageLoader { // request. No reference to release — nothing is retained across images. active = null; displaySrc = ''; + painted = false; // a new image has nothing decoded yet if (!img) { // No image to show. The DR-16 MIME gate lives in `start()` — the request // chokepoint, so retry is gated too — leaving only "no image" here. phase = 'idle'; return; } - active = { img, wsSlug, platform, upgradePending: false, upgrading: false }; + active = { img, wsSlug, platform, upgradePending: false, upgrading: false, originalDeferred: false }; start(); } + function loadOriginal(): void { + const a = active; + // Dedup: only fetch when an original is DEFERRED and none is already in + // flight. This single guard makes the two triggers (tap, zoom-past-fit) one + // fetch and closes every race — a second tap, 3d's pinch reaching + // zoom-past-fit again, or a retry that overlaps a zoom all find + // `originalDeferred` already false (or `upgrading` true) and no-op. + if (!a || !a.originalDeferred || a.upgrading) return; + // DR-16 restated at this request edge too (defensive: an entry only becomes + // `originalDeferred` in `start()` AFTER its gate, so `a.img` is already safe — + // but every place that issues a request re-checks, so the invariant is local + // rather than argued). RETIRE the entry to `idle` — the same cleanup `start` + // and `retry` do — rather than only clearing the flag, which would leave the + // tap affordance rendered (`phase` stuck `'deferred'`) over a dead, no-op + // button. + if (!canOpenInViewer(a.img.mime_type)) { + toIdle(); + return; + } + a.originalDeferred = false; + a.upgrading = true; + // No `loadToken` bump: the mobile THUMB cell reuses its painted element so + // the thumbnail stays visible until the original decodes (no flash, like the + // desktop upgrade); the DEFERRED cell has no element yet, so one mounts fresh + // at the current token. `phase` returns to 'loading' while it is in flight. + phase = 'loading'; + displaySrc = variantUrl(a.wsSlug, a.img.id, 'original'); + } + function decoded(naturalWidth: number, naturalHeight: number, decodedSrc: string, gen: number): void { const a = active; // Staleness fence, two parts. The generation fence (`gen !== loadToken`) @@ -172,14 +259,30 @@ export function createViewerImageLoader(): ViewerImageLoader { // then handles the within-element thumb→original sequencing. if (!a || gen !== loadToken || decodedSrc === '' || decodedSrc !== displaySrc) return; phase = 'ready'; - if (!a.upgradePending || a.upgrading) return; - // The first (thumb-md) bitmap decoded. If its long edge exceeds the - // thumbnail bound the server fell back to the ORIGINAL — do NOT request it - // again (the double decode the whole policy exists to prevent). + // A bitmap is now on screen (thumb or original) — this is the paint the + // zoom-past-fit trigger waits for, so a pre-paint zoom upgrades the instant + // it becomes true. + painted = true; + // Decoding the ORIGINAL (a desktop upgrade or a mobile on-demand fetch is in + // flight): nothing left to decide. + if (a.upgrading) return; + // Decoding the FIRST request (thumb-md, or an original-direct cell). The + // fallback detector runs on EVERY first decode, desktop or mobile: if the + // long edge exceeds the thumbnail bound the server served the ORIGINAL + // (thumb-md absent — fresh upload, or a WebP/AVIF it can't derive). There is + // then nothing more to fetch on EITHER path — clear the desktop upgrade AND + // the mobile deferred original, so neither a background upgrade nor a mobile + // zoom-past-fit re-requests bytes we already have (the double decode the + // whole policy exists to prevent). if (servedOriginal(naturalWidth, naturalHeight)) { a.upgradePending = false; + a.originalDeferred = false; return; } + // A real thumbnail (<= bound) decoded. The mobile thumb cell leaves + // `originalDeferred` true and waits for the zoom-past-fit trigger; only the + // DESKTOP cell background-upgrades now. + if (!a.upgradePending) return; // DR-16 restated at the upgrade request edge too. Defensive: `a.img` is the // same entry `start()` already gated, but every place that issues a request // re-checks, so the invariant is local rather than argued. @@ -203,17 +306,32 @@ export function createViewerImageLoader(): ViewerImageLoader { } function retry(): void { - if (!active) return; + const a = active; + if (!a) return; // A fresh request, never a replay: the `src` is cleared and reissued, and // the server may now have the variant (derivation completes async). displaySrc = ''; + if (a.upgrading) { + // The failed request was the on-demand / background ORIGINAL (a mobile tap + // or zoom-past-fit, or a desktop upgrade). Re-request the ORIGINAL directly + // — NOT the initial policy, which for a mobile deferred cell would revert + // to the tap affordance and drop the user's committed load intent. A fresh + // token remounts the element so the same URL actually refetches. + // DR-16 restated at this request edge too (like `start`). + if (!canOpenInViewer(a.img.mime_type)) { + toIdle(); + return; + } + loadToken++; + phase = 'loading'; + displaySrc = variantUrl(a.wsSlug, a.img.id, 'original'); + return; + } start(); } function dispose(): void { - active = null; - displaySrc = ''; - phase = 'idle'; + toIdle(); } return { @@ -226,7 +344,11 @@ export function createViewerImageLoader(): ViewerImageLoader { get loadToken() { return loadToken; }, + get painted() { + return painted; + }, load, + loadOriginal, decoded, errored, retry, diff --git a/web/src/lib/components/common/Lightbox.svelte b/web/src/lib/components/common/Lightbox.svelte index a1bc0741..0383dec3 100644 --- a/web/src/lib/components/common/Lightbox.svelte +++ b/web/src/lib/components/common/Lightbox.svelte @@ -41,6 +41,7 @@ zoomTo, toggleFitOrActual, stageCenter, + isAtFit, ZOOM_STEP, type Geometry, type ZoomState, @@ -200,6 +201,20 @@ // alone must not reload — desktop→mobile must not abort an in-flight original, // mobile→desktop must not retroactively auto-fetch (TASK-2459). let platform = $derived(viewport.isMobile ? 'mobile' : 'desktop'); + // Whether an exists to zoom. False in the mobile `deferred` cell (a + // placeholder, nothing decoded) and when there is no image — zoom is then + // DISABLED, not merely a no-op (TASK-2460). Non-`deferred` states (loading / + // ready / error) all set `displaySrc`, so the guard is exactly "the deferred + // placeholder is showing" without singling the phase out. + let bitmapPresent = $derived(!!img && !!loader.displaySrc); + // The tap-to-load placeholder's box. Where dimensions are known it takes the + // image's own aspect ratio (so the affordance previews the shape that will + // arrive); where they are not, a neutral box (TASK-2460). + let placeholderStyle = $derived( + img?.width && img?.height + ? `aspect-ratio: ${img.width} / ${img.height}; width: min(70vw, 520px); max-width: 90%; max-height: 80%;` + : `width: min(60vw, 360px); height: min(45vh, 270px); max-width: 90%; max-height: 80%;` + ); // 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 @@ -405,7 +420,12 @@ // (the house pattern in ItemGraph excludes its interactive overlays the same // way). Retry sits over the (broken) stage in the error state, so it needs // the same exclusion as the always-present chrome. - if ((e.target as Element | null)?.closest?.('.lightbox-close, .lightbox-nav, .lightbox-retry')) return; + if ((e.target as Element | null)?.closest?.('.lightbox-close, .lightbox-nav, .lightbox-retry, .lightbox-tap-load')) return; + // No decoded bitmap to pan (the mobile `deferred` placeholder shows no + // ``) — do NOT arm a drag, so the gesture stays fully inert instead of + // capturing the pointer and latching `dragging` for a pan that can never + // happen (TASK-2460). A plain press still reaches the backdrop to close. + if (!bitmapPresent) return; const el = rootEl; if (!el || !pointerGatesOpen(el)) return; // START gate maybeDrag = true; @@ -524,7 +544,7 @@ // A double-click ON a control (close / nav / retry) is that control's, not a // zoom toggle — without this, double-clicking Next navigates twice AND // toggles, or a double-click on Retry toggles zoom on the broken stage. - if ((e.target as Element | null)?.closest?.('.lightbox-close, .lightbox-nav, .lightbox-retry')) return; + if ((e.target as Element | null)?.closest?.('.lightbox-close, .lightbox-nav, .lightbox-retry, .lightbox-tap-load')) return; const el = rootEl; if (!el || !pointerGatesOpen(el)) return; const g = readGeometry(); @@ -689,6 +709,24 @@ // Drop the load on unmount (close) — one teardown, no dependencies. $effect(() => () => loader.dispose()); + // Zoom-past-fit is the mobile THUMB cell's trigger to fetch the original + // (TASK-2460): once a thumbnail has PAINTED, zooming past FIT (`scale > 1`, not + // past the thumb's own 1:1) upgrades to the original. Threshold is + // `!isAtFit(zoom)` — exactly "past fit", robust to the FIT_EPSILON float noise. + // Depends on `zoom` AND `loader.painted` (both tracked): gating on `painted` + // means a pre-paint zoom cannot fetch the original early, AND — because the flip + // to painted RE-RUNS this effect — a zoom made WHILE the thumb was still loading + // still upgrades the instant it paints (not stranded on the thumbnail). + // `loadOriginal` writes neither `zoom` nor `painted`, so tracking them cannot + // self-invalidate the flush (CONVE-1688). Fires on every zoom step past fit, but + // `loadOriginal`'s own dedup makes all but the first a no-op — the original is + // requested exactly once — and it no-ops entirely wherever nothing is deferred + // (desktop, or an already-loaded cell). + $effect(() => { + if (isAtFit(zoom) || !loader.painted) return; + untrack(() => loader.loadOriginal()); + }); + // Re-clamp on stage resize. `maxScale` is geometry-dependent and geometry is // viewport-dependent, so ENLARGING the window lowers the ceiling and can // strand a previously-valid scale above it. `clampState` re-clamps SCALE @@ -740,13 +778,15 @@ // cannot self-invalidate its own flush (CONVE-1688). $effect(() => { // Tracked so the effect re-runs whenever a focus-holding control mounts or - // unmounts: the nav buttons (`hasMultiple`), the DR-10 retry button - // (`phase === 'error'`), and the image itself (`img`) — a set shrinking to - // empty removes the image, and an errored image navigated away removes the - // retry button, either of which could otherwise strand focus on - // (TASK-2459). + // unmounts: the nav buttons (`hasMultiple`), the phase-gated controls (the + // DR-10 retry button in `error`, the tap-to-load button in `deferred` — both + // replaced by the image they load, TASK-2459 / TASK-2460), and the image + // itself (`img`). A set shrinking to empty, an errored image navigated away, + // or a tap that swaps the affordance for the bitmap could otherwise strand + // focus on . Tracking the whole `phase` covers every such transition; + // `handoffFocus` no-ops while focus is still inside the surface. void hasMultiple; - void (loader.phase === 'error'); + void loader.phase; void img; const el = rootEl; if (!el || !isViewerFrontmost(el) || isBlockedByModal(el)) return; @@ -812,6 +852,16 @@ // `shiftKey` is fine (on most layouts `+` IS Shift+`=`), so it is absent // from the guard. if (e.ctrlKey || e.metaKey || e.altKey) return; + // Zoom keys are DISABLED, not merely no-ops, while no bitmap exists — the + // mobile `deferred` cell shows a placeholder with nothing to zoom + // (TASK-2460). Consume the key so it can't leak past the modal, but do not + // act. (`+`/`-` are already inert via `readGeometry`; `0` would otherwise + // still reset.) + const isZoomKey = e.key === '+' || e.key === '=' || e.key === '-' || e.key === '0'; + if (isZoomKey && !bitmapPresent) { + e.preventDefault(); + return; + } if (e.key === '+' || e.key === '=') { // `+` — including the numpad, whose `.key` is also `'+'` — and bare `=`. e.preventDefault(); @@ -973,6 +1023,29 @@
{/if} + {#if img && loader.phase === 'deferred'} + +
+ +
+ {/if} + {#if img && loader.phase === 'error'}