diff --git a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx index f92baad8..df940d7a 100644 --- a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx +++ b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx @@ -489,13 +489,47 @@ export function AnnotatorCanvas({ [store, activeClass, mint], ); - /** The whole asset, centred. What `mod+0` answers and what a mount starts at. */ + /** + * The whole asset, centred. What `mod+0` answers and what a mount starts at. + * + * **Its dependencies are the asset's three numbers, never the descriptor + * object**, and that distinction is the whole of a defect that cost people + * their place in the picture on every save. + * + * A fit is a function of the frame: an id and a size. The *object* carrying + * them is minted afresh by `documentFromWire` on every rebuild, and a host + * rebuilds its document for reasons that have nothing to do with the frame — + * `ui-core` refetches after a save so the kernel's own annotation ids replace + * its client-minted ones, which is a materially different payload and so a new + * document, a new descriptor, and — while this closed over `asset` — a new + * `fit` and a layout effect that ran again. Zoom into a detail, store your + * work, and the stage jumped back to the fitted view. + * + * Depending on the numbers makes the effect below fire when the *picture* + * changes and at no other time, which is what it always meant. It is not a + * throttle on an effect that was otherwise right: a document rebuild is not a + * reason to move a camera, and an equality check on the object could never + * have told the two apart. + * + * The primitives live in **this** list rather than in the effect's, because + * `react-hooks/exhaustive-deps` is an error in this package and reports an + * unnecessary dependency as loudly as a missing one — so the honest spelling + * is a callback whose identity already tracks the right thing. + */ + const { id: assetId, width: assetWidth, height: assetHeight } = asset; const fit = useCallback(() => { const pane = paneRef.current; if (pane === null) return; const rect = pane.getBoundingClientRect(); - applyViewport(fitToViewport(asset, rect.width, rect.height, FIT_PADDING_PX)); - }, [asset, applyViewport]); + applyViewport( + fitToViewport( + { id: assetId, width: assetWidth, height: assetHeight }, + rect.width, + rect.height, + FIT_PADDING_PX, + ), + ); + }, [assetId, assetWidth, assetHeight, applyViewport]); // Before the first paint, so the asset does not flash at native scale first. useLayoutEffect(fit, [fit]); diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 845ecfbc..ccf55fee 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -2831,3 +2831,63 @@ test("Save and stay teaches its chord in a tooltip now the keycap is gone", asyn // half that does not move. await expect(tip).toContainText(/Save and stay \((⌘|Ctrl)S\)/); }); + +/** + * Saving must not move the camera. + * + * The viewport is `AnnotatorCanvas`'s own state and only a real browser has one: + * jsdom's `getBoundingClientRect` answers all zeros, so there is no fit to + * disturb, no wheel notch to apply and no pan to measure. A component test for + * this would pass with the bug fully present. + * + * Both halves are read at once off the ``'s box, which is `_frame.ts`'s own + * idiom: the element is laid out at the asset's native size inside the + * `translate(pan) scale(zoom)` wrapper, so its on-screen rect folds zoom, pan and + * the pane's origin into one measurement. The readout is asserted beside it + * because a zoom that survived while the pan did not would otherwise read as a + * pass. + */ +test("saving leaves the viewport exactly where it was", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent); + + const canvas = page.getByTestId("annotator-canvas"); + const pane = (await page.getByTestId("annotator-pane").boundingBox())!; + + // Off the fitted view in both dimensions: a wheel notch over a point that is + // not the pane's centre changes the zoom *and* the pan, and the secondary drag + // after it moves the pan again on its own. + await page.mouse.move(pane.x + pane.width * 0.35, pane.y + pane.height * 0.35); + await page.mouse.wheel(0, -600); + await page.mouse.down({ button: "right" }); + await page.mouse.move(pane.x + pane.width * 0.55, pane.y + pane.height * 0.5, { steps: 8 }); + await page.mouse.up({ button: "right" }); + + const zoomBefore = await page.getByTestId("zoom-readout").textContent(); + expect(zoomBefore).not.toBe("100%"); + const frameBefore = (await canvas.boundingBox())!; + + // Something to actually store — a save with an empty plan sends no request and + // rebuilds nothing, so a clean frame could not reproduce this at all. + await drawOneUnsavedBox(page); + await page.getByTestId("save-and-stay").click(); + await expect(page.getByTestId("save-state")).toContainText("Saved"); + // The refetch the save triggers is what rebuilds the store; wait for the + // rebuilt document rather than for the button, or the assertion below can run + // in the window before the camera has been moved. + await expect(page.getByTestId("object-total")).toContainText("1 object"); + + expect(await page.getByTestId("zoom-readout").textContent()).toBe(zoomBefore); + const frameAfter = (await canvas.boundingBox())!; + expect({ + x: Math.round(frameAfter.x), + y: Math.round(frameAfter.y), + width: Math.round(frameAfter.width), + height: Math.round(frameAfter.height), + }).toEqual({ + x: Math.round(frameBefore.x), + y: Math.round(frameBefore.y), + width: Math.round(frameBefore.width), + height: Math.round(frameBefore.height), + }); +});