fix(web): pin image annotations to the picture, not the letterboxed box - #236
fix(web): pin image annotations to the picture, not the letterboxed box#236ravirajsinh45 wants to merge 2 commits into
Conversation
Drawings on images were authored and replayed against the full container box rather than the picture inside it. `use-drawing` sized the Fabric canvas from the parent container, stored that size as `_canvasWidth`/`_canvasHeight`, and `AnnotationOverlay` rescaled by a plain per-axis ratio on display. That rescale is only correct when the display container has the same aspect ratio as the authoring one, so any change of shape (sidebar collapsed vs expanded, a resized window, a compare pane, a share link) both stretched the drawing and moved the picture underneath it. Measured in a browser: a 3:1 image annotated at 25% down the picture in a 900x500 viewer, reopened at 400x700, put the mark at -29% — 54% of the image height adrift and off the picture entirely. Through the new constraint it lands at 25% exactly. Video never had this bug; `VideoFrameConstraint` already fits the overlay to the rendered video box. This is the image counterpart, but it cannot be a copy. `<video>` is `w-full h-full object-contain`, so it fills its container and the contain fit can be derived from the container. Images are `max-w-full max-h-full`, and `max-*` only ever shrinks, so an image smaller than its container renders at natural size rather than scaling up. Deriving the box from the container upscales it in that case — verified: a 100x200 image in an 800x400 box really occupies 100x200 at (350,100), where container-derived math claims 200x400 at (300,0). `renderedImageBox` therefore runs the contain fit inside the element's own box and offsets by where that element sits, which is correct for both patterns. It reads offset* rather than a client rect because this renders inside react-zoom-pan-pinch's transform, where a client rect would fold in the zoom matrix. Applied to all four surfaces that show image annotations: the single viewer (which also covers the folder share viewer, since that routes through ImageViewer), both side-by-side compare panes, and the wipe view. Wipe is not optional — once authoring moves to image space, a viewer left in container space becomes newly wrong rather than merely inconsistent. `AnnotationOverlay` and `AnnotationCanvas` are unchanged: both size themselves from their parent, so mounting them inside the constraint is the whole mechanism. Note for existing data: image annotations saved before this change were only ever correct in the exact container they were drawn in, and are now interpreted in image space, so they will shift. Closes #185
ravirajsinh45
added
bug
|
Ran a deep multi-agent review against the actual worktree (with tests/tsc runnable, not just the diff). Found four confirmed issues worth addressing before merge: Correctness gap: Reuse: Reuse: the jsdom geometry-stub helper is copy-pasted verbatim into three new test files (wipe-viewer.test.tsx, compare-overlay.test.tsx, image-viewer-annotation-frame.test.tsx). A shared test-utils helper would avoid three-way drift. Test coverage: side-by-side compare pane A has no geometry-pinning regression test, only pane B does. Confirmed by stripping ImageFrameConstraint from pane A only and running the suite, the full 282-test suite still passes. Will push fixes for these. |
Review follow-ups on #185. ResizeObserver watched the <img> alone. Under `max-w-full max-h-full` an image smaller than its container renders at natural size, so a container resize only RECENTRES it: offsetLeft/offsetTop move while the element's own box is untouched, and ResizeObserver does not fire on a position-only change. The overlay stayed pinned where the picture used to be, which is the same drift #185 set out to fix, for the case where the picture never needs shrinking. It now observes the container as well. Also folds the contain fit into one helper. `renderedImageBox` and `VideoFrameConstraint` computed the same centred, aspect-preserving box two different ways (cross-multiplication vs division); both now call `containBox`, leaving only the reference box different, which is the part that genuinely must differ. Checked equivalent for integer video dimensions: no branch flips, differences bounded at ~1e-13px. The video overlay also fills its container instead of collapsing to 0x0 when measured before layout. Tests 282 -> 293: - A ResizeObserver stub that records its target, so a resize can be delivered to the container and not the picture. The existing resize test fired every callback regardless of target and passed either way. - VideoFrameConstraint had no direct coverage (the compare and wipe suites stub ResizeObserver precisely to route around its math). Pinned before moving it, and mutation checked. - A pane A geometry test for side-by-side compare. Only pane B had one, so stripping pane A's constraint left the suite green; this fails for both a removed constraint and a swapped ref. - The jsdom geometry stub was copy-pasted into three suites, now one helper in test/geometry.ts.
1 participant
Closes #185.
Image annotations were authored and replayed against the full letterboxed container box rather than the picture inside it, so the same drawing pointed at a different part of the image depending on where it was viewed: sidebar collapsed vs expanded, a resized window, a compare pane, a share link.
The mechanism
use-drawingsized the Fabric canvas from the parent container (use-drawing.ts:65-67), stored that size as_canvasWidth/_canvasHeight(:325-326), andAnnotationOverlayreplayed it with a plain per-axis rescale (annotation-overlay.tsx:65-79). That rescale is only correct when the display container has the same aspect ratio as the authoring one. When it does not, the drawing is stretched non-uniformly and the picture underneath has moved, because the letterbox insets differ.Measured in a browser, a 3:1 image annotated 25% down the picture in a 900x500 viewer and reopened at 400x700:
Why this is not a copy of
VideoFrameConstraintVideo never had this bug, so the obvious fix is to mirror
VideoFrameConstraint. That would have been subtly wrong, because the two elements use different CSS:<video>fills its container and letterboxes internally, so the contain fit can be derived from the container.max-*only ever shrinks a replaced element, so an image smaller than its container renders at natural size and is not scaled up. Verified in Chrome against the exact shipped CSS:max-*max-*w-full h-fullrenderedImageBoxtherefore runs the contain fit inside the element's own box and offsets by where that element sits, which is correct for all three. It readsoffset*rather than a client rect because this renders inside react-zoom-pan-pinch'sTransformComponent, where a client rect would fold the zoom matrix in.Aspect ratios are compared by cross-multiplying rather than dividing, so an exact fit (a 16:9 image in a 16:9 box) stays exact instead of drifting a fraction of a pixel through an intermediate ratio.
Surfaces covered
ImageViewerWipe is not optional scope even though the issue does not mention it. Once authoring moves into image space, a viewer still rendering in container space goes from consistent-but-wrong to newly wrong. Leaving it would have been a regression introduced by this PR.
AnnotationOverlayandAnnotationCanvasare unchanged. Both size themselves from their parent, so mounting them inside the constraint is the whole mechanism.Tests
263 to 282 (+19, 3 new files), full suite green.
Red-green was verified per surface, and again at the end in one pass: reverting only the three wiring files fails exactly the four regression tests, one per surface, while the helper and component unit tests stay green.
The
renderedImageBoxunit tests pin the three browser-measured shapes above, so a future change back to container-derived math fails immediately rather than silently.One consequence to be aware of
Image annotations saved before this change will shift. They were only ever correct in the exact container they were drawn in, and are now interpreted in image space. There is no version marker in the stored Fabric JSON to tell old data from new, so no migration is attempted here. If preserving them matters, the cheap path is to start stamping a frame marker in
getJSON()now so a migration becomes possible later. Happy to add that as a follow-up if you want it.