fix(web): pin image annotations to the picture, not the letterboxed box

CHANGELOG.md

@@ -7,7 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+- **Image annotations no longer drift when the viewer's aspect ratio changes** — drawings on images were authored and replayed against the full letterboxed container box rather than the picture inside it, so the same annotation pointed at a different part of the image depending on where it was viewed: sidebar collapsed vs expanded, a resized window, a compare pane, or a share link. A 3:1 image annotated in a 900x500 viewer and reopened in a 400x700 one put the mark 54% of the image height away from where it was drawn, far enough to land off the picture entirely. Images now get the same treatment video already had (`VideoFrameConstraint`), applied in the single viewer, both side-by-side compare panes, and the wipe view. Annotations saved before this change are migrated on read rather than left behind: the picture box inside their original container is reconstructed from the image's intrinsic size, so an old mark is re-expressed relative to the picture and lands where it was actually drawn, at any viewer size. **This means existing image annotations can appear in a different place after upgrading than they did before** — the new position is the correct one, and it no longer moves when the viewer is resized. New annotations record which coordinate space they use, so no future reader has to guess. (#185)
+
 ### Changed
+- **Image and video overlays now share one contain-fit implementation** — the geometry that positions an annotation over the visible picture was written twice, once for `<img>` and once for `<video>`, with the two comparing aspect ratios differently. Both now call a single tested helper that measures the element's own box, which is correct whether the element fills its container (`w-full h-full`, the main video player) or only ever shrinks to hug the media (`max-*`, every image and the compare-pane videos). That last case was previously fitted against the container, which upscaled the box and misplaced annotations on any compare-pane video smaller than its pane. The overlay also spans the element instead of collapsing to 0x0 when measured before layout, and re-measures when its box changes, so an annotation opened from a deep link before the image has decoded is no longer scaled against the wrong box.
 - **Pinned `starlette` and `botocore` so self-hosted Docker builds are reproducible** — both were floating transitives, so rebuilding the same commit on a different day could silently install different versions with no diff and no PR. FastAPI declares `starlette>=0.46.0` with no upper bound, meaning builds were free to cross a Starlette major (the ASGI layer under the SSE endpoint and the middleware stack); `botocore` is where S3 request signing lives, and unreviewed moves there have broken S3 compatibility before. Both are pinned to the versions already resolving, so no installed version changes.
 
 ### Fixed

apps/web/components/review/__tests__/annotation-overlay-frame.test.tsx

@@ -0,0 +1,160 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
+import { render } from '@testing-library/react'
+import { stubGeometry, restoreGeometry } from '@/test/geometry'
+
+/**
+ * The frame fix moved image annotations from container space to picture space.
+ * Annotations saved before that carry CONTAINER dimensions in
+ * `_canvasWidth`/`_canvasHeight`, with the picture occupying only a band inside
+ * them. The overlay reconstructs that band from the image's intrinsic size and
+ * re-expresses the mark relative to the picture, so old drawings land where
+ * they were actually drawn at any viewer size. These pin both spaces.
+ */
+
+const loaded: Array<Record<string, unknown>> = []
+let objects: Array<Record<string, unknown>> = []
+let dimensions: Array<{ width: number; height: number }> = []
+
+vi.mock('fabric', () => ({
+  Canvas: class {
+    setDimensions(d: { width: number; height: number }) {
+      dimensions.push(d)
+    }
+    async loadFromJSON(json: Record<string, unknown>) {
+      loaded.push(json)
+    }
+    getObjects() {
+      return objects
+    }
+    renderAll() {}
+    dispose() {}
+  },
+}))
+
+import { AnnotationOverlay } from '../annotation-overlay'
+import { MediaFrameContext } from '../media-frame-context'
+
+/** A Fabric-ish object that records what the overlay sets on it. */
+function makeObject(left: number, top: number) {
+  return {
+    left,
+    top,
+    scaleX: 1,
+    scaleY: 1,
+    set(patch: Record<string, number>) {
+      Object.assign(this, patch)
+    },
+    setCoords() {},
+  } as unknown as Record<string, unknown>
+}
+
+// A 3000x1000 image authored in a 900x500 pane: the picture box was 900x300
+// at top 100, so a mark at container y=175 is 25% down the picture.
+const NATURAL = { naturalWidth: 3000, naturalHeight: 1000 }
+const AUTHORED = { canvasWidth: 900, canvasHeight: 500 }
+const PICTURE = { width: 900, height: 300 }
+
+async function renderOverlay(
+  annotation: Record<string, unknown>,
+  legacy: React.ContextType<typeof MediaFrameContext>,
+) {
+  const view = render(
+    <MediaFrameContext.Provider value={legacy}>
+      <AnnotationOverlay annotation={annotation} />
+    </MediaFrameContext.Provider>,
+  )
+  await vi.waitFor(() => expect(loaded.length).toBeGreaterThan(0))
+  return view
+}
+
+describe('AnnotationOverlay coordinate spaces', () => {
+  beforeEach(() => {
+    loaded.length = 0
+    objects = []
+    dimensions = []
+    // The overlay is mounted inside the picture box, so that is what it measures.
+    stubGeometry({ offsetWidth: PICTURE.width, offsetHeight: PICTURE.height })
+    vi.stubGlobal('ResizeObserver', class {
+      observe() {}
+      unobserve() {}
+      disconnect() {}
+    })
+  })
+  afterEach(() => {
+    restoreGeometry()
+    vi.unstubAllGlobals()
+    vi.restoreAllMocks()
+  })
+
+  it('places a legacy container-space mark where it was drawn on the picture', async () => {
+    // Container y=175, i.e. 25% down a picture box that ran y=100..400.
+    const obj = makeObject(0, 175)
+    objects = [obj as never]
+
+    await renderOverlay(
+      { objects: [{}], _canvasWidth: AUTHORED.canvasWidth, _canvasHeight: AUTHORED.canvasHeight },
+      NATURAL,
+    )
+
+    // Replayed into a picture box the same size as the authored one.
+    expect(obj.scaleX).toBe(1)
+    expect(obj.scaleY).toBe(1)
+    expect(obj.top).toBe(75) // 25% of 300
+    expect(obj.left).toBe(0)
+  })
+
+  it('keeps a legacy mark at the same point on the picture in a bigger viewer', async () => {
+    const obj = makeObject(0, 175)
+    objects = [obj as never]
+
+    // Same stored data, replayed in a picture box twice as large.
+    stubGeometry({ offsetWidth: 1800, offsetHeight: 600 })
+    await renderOverlay(
+      { objects: [{}], _canvasWidth: AUTHORED.canvasWidth, _canvasHeight: AUTHORED.canvasHeight },
+      NATURAL,
+    )
+
+    // Still 25% down the picture, now 25% of 600.
+    expect(obj.top).toBe(150)
+    expect(obj.scaleY).toBe(2)
+  })
+
+  it('leaves a picture-space annotation alone when the box matches', async () => {
+    const obj = makeObject(0, 75)
+    objects = [obj as never]
+
+    await renderOverlay(
+      { objects: [{}], _canvasWidth: 900, _canvasHeight: 300, _frameSpace: 'media' },
+      NATURAL,
+    )
+
+    expect(obj.top).toBe(75)
+    expect(obj.scaleY).toBe(1)
+  })
+
+  it('rescales a picture-space annotation authored in a differently sized picture box', async () => {
+    const obj = makeObject(0, 150)
+    objects = [obj as never]
+
+    // Authored in a 450x150 picture box, replayed in 900x300: everything doubles.
+    await renderOverlay(
+      { objects: [{}], _canvasWidth: 450, _canvasHeight: 150, _frameSpace: 'media' },
+      NATURAL,
+    )
+
+    expect(obj.scaleY).toBe(2)
+    expect(obj.top).toBe(300)
+  })
+
+  it('treats unmarked data as picture-space when there is no legacy frame (video)', async () => {
+    // VideoFrameConstraint predates the fix, so video annotations were always
+    // authored in picture space and must not be shifted.
+    const obj = makeObject(0, 75)
+    objects = [obj as never]
+
+    await renderOverlay({ objects: [{}], _canvasWidth: 900, _canvasHeight: 300 }, null)
+
+    expect(obj.top).toBe(75)
+    expect(obj.scaleY).toBe(1)
+  })
+})

apps/web/components/review/__tests__/image-frame-constraint.test.tsx

@@ -0,0 +1,154 @@
+import { describe, expect, it, beforeEach, vi } from 'vitest'
+import * as React from 'react'
+import { render, screen, fireEvent, act } from '@testing-library/react'
+import { ImageFrameConstraint } from '../image-frame-constraint'
+
+/**
+ * jsdom performs no layout, so offsetWidth/naturalWidth are always 0. These
+ * tests stub those DOM properties to describe a laid-out image and then assert
+ * on the style the component actually computes — the component's real effect
+ * runs, nothing about it is mocked.
+ */
+function layOut(
+  img: HTMLImageElement,
+  g: { natural: [number, number]; box: [number, number]; offset: [number, number] },
+) {
+  const props: Record<string, number> = {
+    naturalWidth: g.natural[0],
+    naturalHeight: g.natural[1],
+    offsetWidth: g.box[0],
+    offsetHeight: g.box[1],
+    offsetLeft: g.offset[0],
+    offsetTop: g.offset[1],
+  }
+  for (const [k, value] of Object.entries(props)) {
+    Object.defineProperty(img, k, { value, configurable: true })
+  }
+}
+
+function Harness({
+  geometry,
+}: {
+  geometry: { natural: [number, number]; box: [number, number]; offset: [number, number] }
+}) {
+  const imgRef = React.useRef<HTMLImageElement>(null)
+  // Lay the image out before the constraint's effect reads it.
+  React.useLayoutEffect(() => {
+    if (imgRef.current) layOut(imgRef.current, geometry)
+  }, [geometry])
+  return (
+    <div style={{ position: 'relative' }}>
+      {/* eslint-disable-next-line @next/next/no-img-element */}
+      <img ref={imgRef} alt="subject" />
+      <ImageFrameConstraint imgRef={imgRef}>
+        <div data-testid="child" />
+      </ImageFrameConstraint>
+    </div>
+  )
+}
+
+const wrapper = () => screen.getByTestId('child').parentElement as HTMLElement
+
+beforeEach(() => {
+  vi.stubGlobal('ResizeObserver', class {
+    observe() {}
+    unobserve() {}
+    disconnect() {}
+  })
+})
+
+describe('ImageFrameConstraint', () => {
+  it('positions the annotation layer over the picture, not the letterboxed container', () => {
+    // 100x200 image centred in an 800x400 box: max-* leaves it at natural size.
+    render(<Harness geometry={{ natural: [100, 200], box: [100, 200], offset: [350, 100] }} />)
+
+    expect(wrapper()).toHaveStyle({
+      position: 'absolute',
+      left: '350px',
+      top: '100px',
+      width: '100px',
+      height: '200px',
+    })
+  })
+
+  it('excludes the letterbox bands when the element itself fills the container', () => {
+    // The <video>-style case: element stretched to 800x400, picture is 200x400.
+    render(<Harness geometry={{ natural: [100, 200], box: [800, 400], offset: [0, 0] }} />)
+
+    expect(wrapper()).toHaveStyle({ left: '300px', top: '0px', width: '200px', height: '400px' })
+  })
+
+  it('recalculates once the image decodes and reports its intrinsic size', () => {
+    const { container } = render(
+      // naturalWidth 0 — not decoded yet, so the element box is all we know.
+      <Harness geometry={{ natural: [0, 0], box: [800, 400], offset: [0, 0] }} />,
+    )
+    expect(wrapper()).toHaveStyle({ width: '800px', height: '400px' })
+
+    const img = container.querySelector('img') as HTMLImageElement
+    layOut(img, { natural: [100, 200], box: [800, 400], offset: [0, 0] })
+    act(() => { fireEvent.load(img) })
+
+    expect(wrapper()).toHaveStyle({ left: '300px', width: '200px', height: '400px' })
+  })
+
+  it('renders its children inside the constrained box', () => {
+    render(<Harness geometry={{ natural: [100, 200], box: [100, 200], offset: [350, 100] }} />)
+    // AnnotationOverlay and AnnotationCanvas both size themselves from their
+    // parent, so being INSIDE this wrapper is what puts them in image space.
+    expect(wrapper()).toContainElement(screen.getByTestId('child'))
+  })
+
+  it('recomputes when the container resizes', () => {
+    const observers: Array<() => void> = []
+    vi.stubGlobal('ResizeObserver', class {
+      constructor(cb: () => void) { observers.push(cb) }
+      observe() {}
+      unobserve() {}
+      disconnect() {}
+    })
+
+    const { container } = render(
+      <Harness geometry={{ natural: [100, 200], box: [800, 400], offset: [0, 0] }} />,
+    )
+    expect(wrapper()).toHaveStyle({ left: '300px', width: '200px' })
+
+    // Sidebar collapses: the pane gets wider, so the picture recentres.
+    const img = container.querySelector('img') as HTMLImageElement
+    layOut(img, { natural: [100, 200], box: [1000, 400], offset: [0, 0] })
+    act(() => { observers.forEach((cb) => cb()) })
+
+    expect(wrapper()).toHaveStyle({ left: '400px', width: '200px' })
+  })
+
+  it('recomputes when the container resizes but the picture only recentres', () => {
+    // Records what each observer was pointed at, so a resize can be delivered to
+    // one element and not another. That distinction is the whole point here: the
+    // test above fires every callback regardless of target, which passes whether
+    // the component watches the picture or its container.
+    const observed = new Map<Element, Array<() => void>>()
+    vi.stubGlobal('ResizeObserver', class {
+      cb: () => void
+      constructor(cb: () => void) { this.cb = cb }
+      observe(el: Element) { observed.set(el, [...(observed.get(el) ?? []), this.cb]) }
+      unobserve() {}
+      disconnect() {}
+    })
+
+    // A 100x200 picture at natural size in a wide pane: max-* only ever shrinks,
+    // so the element hugs the picture and the pane centres it.
+    const { container } = render(
+      <Harness geometry={{ natural: [100, 200], box: [100, 200], offset: [400, 100] }} />,
+    )
+    expect(wrapper()).toHaveStyle({ left: '400px', top: '100px' })
+
+    // Sidebar collapses: the pane widens, the picture stays 100x200 and only
+    // recentres. The <img>'s own box never changes, so an observer on the <img>
+    // alone never fires — only one on the container does.
+    const img = container.querySelector('img') as HTMLImageElement
+    layOut(img, { natural: [100, 200], box: [100, 200], offset: [600, 100] })
+    act(() => { observed.get(img.parentElement as Element)?.forEach((cb) => cb()) })
+
+    expect(wrapper()).toHaveStyle({ left: '600px', top: '100px' })
+  })
+})