From 2799175858f20e377a091989820a269dfb30c74b Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Wed, 12 Aug 2026 15:05:35 -0700 Subject: [PATCH 1/2] fix(annotator): the detail slider drags again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A range input drags on its default action, so the `preventDefault` guarding the canvas' focus left a slider that looked alive and could only be moved with the brackets. Focus now goes back to the canvas on release instead, so the drag works and the chords are live again the moment it ends. The test that missed it asserted the guard *fired* — the one assertion a dead control passes. Replaced with a real pointer drag in chromium, which is the only place a range input's default action exists at all. cf. #557 --- frontend/app/e2e/annotate.spec.ts | 53 +++++++++++++++++++ .../ui-core/src/annotator/SuggestPanel.tsx | 38 +++++++++---- .../src/annotator/suggestPanel.test.tsx | 31 +++++++++-- 3 files changed, 107 insertions(+), 15 deletions(-) diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 1ebd9c95..6da47e5c 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -3235,6 +3235,59 @@ test("the preview draws its vertices, and a committed shape does not", async ({ await expect(committed.locator("polygon")).not.toHaveAttribute("stroke-dasharray", "10 6"); }); +test("the detail slider moves under the pointer, and hands the keyboard back", async ({ + page, +}) => { + const sent: Request[] = []; + await openJob(page, sent, undefined, undefined, undefined, undefined, true); + await servePolygonSuggestion(page); + + await page.getByTestId("tool-suggest").click(); + const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; + await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); + await expect(page.getByTestId("suggestion-shape")).toBeVisible(); + await page.getByTestId("suggest-adjust-open").click(); + + const slider = page.getByTestId("suggest-detail"); + await expect(slider).toHaveValue("1"); + const before = asks(sent); + + // A real drag: press the thumb, travel, release. `fill()` and `click()` both + // set the value without ever exercising the default action, which is exactly + // the gap that let a slider ship unmovable — `preventDefault` on the press + // cancelled the drag and every jsdom assertion still passed (#563). + const track = (await slider.boundingBox())!; + await page.mouse.move(track.x + track.width / 2, track.y + track.height / 2); + await page.mouse.down(); + await page.mouse.move(track.x + track.width - 1, track.y + track.height / 2, { steps: 8 }); + await page.mouse.up(); + + await expect(slider).toHaveValue("2"); + await expect(page.getByTestId("suggest-detail-label")).toContainText("Fine"); + const fine = await drawnVertices(page); + // Still no round trip: the drag is arithmetic, like the brackets. + expect(asks(sent)).toBe(before); + + // Dragging the other way, to the coarsest stop. Two *client* simplifications + // compared against each other — the answer's own geometry arrives already + // reduced by the server and is not one of the three steps. + await page.mouse.move(track.x + track.width / 2, track.y + track.height / 2); + await page.mouse.down(); + await page.mouse.move(track.x + 1, track.y + track.height / 2, { steps: 8 }); + await page.mouse.up(); + await expect(slider).toHaveValue("0"); + await expect(page.getByTestId("suggest-detail-label")).toContainText("Coarse"); + expect(fine).toBeGreaterThan(await drawnVertices(page)); + + // And the canvas has its keyboard back the moment the drag ended — without + // this the brackets, Esc and Enter are all dead and nothing says why. + await page.keyboard.press("]"); + await expect(slider).toHaveValue("1"); + await page.keyboard.press("Escape"); + await expect(page.getByTestId("suggest-adjustments")).toHaveCount(0); + await expect(page.getByTestId("suggestion-shape")).toBeVisible(); +}); + test("a press on the suggest panel never reaches the picture underneath", async ({ page }) => { const sent: Request[] = []; await openJob(page, sent, undefined, undefined, undefined, undefined, true); diff --git a/frontend/ui-core/src/annotator/SuggestPanel.tsx b/frontend/ui-core/src/annotator/SuggestPanel.tsx index 986ae1c7..a934c28b 100644 --- a/frontend/ui-core/src/annotator/SuggestPanel.tsx +++ b/frontend/ui-core/src/annotator/SuggestPanel.tsx @@ -508,16 +508,33 @@ function Chip({ children }: { readonly children: ReactNode }): JSX.Element { * would stop stepping, and `Esc` would stop being the preview's undo, both with * nothing on screen to say why. Found in a browser: jsdom has no focus to move. * - * On every control here, the slider included: all of them act on the canvas, so - * none of them has any business holding focus. `mousedown` rather than the whole - * control, so Tab still reaches the slider for somebody who wants to drive it - * from the keyboard — that is a deliberate arrival, not a side effect of pointing - * at it. + * On the buttons only. **A range input drags on the default action**, so + * cancelling its `mousedown` leaves a slider that looks alive and cannot be moved + * by hand at all — which is what shipped, and what nothing caught, because the + * test asserted the guard fired rather than that the thumb followed the pointer. + * The slider uses {@link returnFocusToCanvas} instead: it takes focus for the + * duration of the drag, like any form control, and hands it back on release. */ function keepFocusOnCanvas(event: { preventDefault: () => void }): void { event.preventDefault(); } +/** + * Give the canvas its keyboard back once a pointer gesture on a control is over. + * + * The other half of the same rule, for a control whose default action is the + * whole point of it. `FrameGallery` already returns focus this way after its + * overlay closes, and by the same route — `ui-core` holds no ref to the + * annotator's root, and threading one down for this would be a prop on every + * layer between here and there. + * + * On release rather than on change: a drag emits a change per step, and pulling + * focus mid-drag would end the gesture under the pointer. + */ +function returnFocusToCanvas(): void { + document.querySelector('[data-testid="annotator-root"]')?.focus(); +} + /** * The settings, inside the card that is already on screen. * @@ -593,11 +610,12 @@ function Adjustments({ aria-label="Detail" aria-valuetext={`${labelFor(detail)}, ${vertexCount(session)} points`} data-testid="suggest-detail" - // Dragging this must not take focus off the canvas: every chord in - // the editor is a keydown on the annotator's own root, so a control - // that took focus would switch `[`, `]`, Esc and Enter off with - // nothing on screen to say why. Tab still reaches it deliberately. - onMouseDown={keepFocusOnCanvas} + // No `preventDefault` here: a range input *drags* on its default + // action, so cancelling the press is what made this unmovable by + // hand (#563). Focus goes back to the canvas on release instead, so + // `[`, `]`, Esc and Enter are live again the moment the drag ends. + onMouseUp={returnFocusToCanvas} + onTouchEnd={returnFocusToCanvas} onChange={(event) => onDetail(DETAIL_STEPS[Number(event.target.value)] ?? detail)} /> {/* diff --git a/frontend/ui-core/src/annotator/suggestPanel.test.tsx b/frontend/ui-core/src/annotator/suggestPanel.test.tsx index 1c75687a..84e3646a 100644 --- a/frontend/ui-core/src/annotator/suggestPanel.test.tsx +++ b/frontend/ui-core/src/annotator/suggestPanel.test.tsx @@ -524,14 +524,35 @@ describe("the adjustments, which are a section and never a popup", () => { expect(onDetail).toHaveBeenCalledWith("fine"); }); - it("does not let a press on the slider take focus off the canvas", () => { - // Every chord in the editor is a keydown on the annotator's own root, so a - // control that took focus would switch `[`, `]`, Esc and Enter off with - // nothing on screen to say why (#557). + it("lets a press on the slider through, because that press is the drag", () => { + // The defect this replaces: `preventDefault` on the press cancelled a range + // input's own drag, leaving a control that looked alive and could only be + // moved with the brackets. The old test asserted the guard *fired*, which is + // exactly the assertion a dead control passes (#563). render(mount({ session: showingPolygon(), adjusting: true, onAdjusting: vi.fn(), onDetail: vi.fn() })); const press = fireEvent.mouseDown(screen.getByTestId("suggest-detail")); - expect(press).toBe(false); + expect(press).toBe(true); + }); + + it("hands the keyboard back to the canvas when the drag ends", () => { + // The other half: the slider may hold focus while it is being dragged, but + // not after, or `[`, `]`, Esc and Enter stay dead with nothing to say why. + const root = document.createElement("div"); + root.setAttribute("data-testid", "annotator-root"); + root.tabIndex = 0; + document.body.appendChild(root); + + render(mount({ session: showingPolygon(), adjusting: true, onAdjusting: vi.fn(), + onDetail: vi.fn() })); + const slider = screen.getByTestId("suggest-detail"); + slider.focus(); + expect(document.activeElement).toBe(slider); + + fireEvent.mouseUp(slider); + expect(document.activeElement).toBe(root); + + root.remove(); }); it("keeps the controls operable on an answer with nothing in it", () => { From 84a46f289dfd748b71b5b9cdf9003381f08e88a6 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Wed, 12 Aug 2026 16:31:38 -0700 Subject: [PATCH 2/2] fix(annotator): the plain arrow over a shape in Select mode Hovering a box, its edge band or a vertex showed the four-arrow move cursor. A press there selects, and only becomes a move if the pointer travels, so it advertised the rarer outcome. The hover highlight is unchanged and still reports which shape a press would take. A drag in flight keeps move, and a selected box's grips keep their resize keywords. This reverses a decision argued in the code; the new argument is at the change site and in DESIGN.md. cf. #567 --- DESIGN.md | 14 +++++-- .../src/adapters/react/TransientLayer.tsx | 5 +-- .../src/core/interaction/affordance.test.ts | 41 +++++++++++-------- .../src/core/interaction/affordance.ts | 27 ++++++------ frontend/app/e2e/annotate.spec.ts | 24 +++++------ 5 files changed, 59 insertions(+), 52 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index f440c4f7..9497924d 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1079,9 +1079,10 @@ What a viewer is (decisions of 2026-08-07, #426, and 2026-08-08, #439): viewer can ask. - **Selection highlights; it does not advertise.** A selected shape renders the selected treatment — stroke 3, the label — with **no grips and no vertex - dots**, and the cursor is the **default arrow everywhere**: no `move`, no - resize keywords, because no such gesture exists. The tool strip is not - rendered at all, for the same reason it never was. + dots**, and the cursor is the **default arrow everywhere**: no resize keywords, + because no such gesture exists. The tool strip is not rendered at all, for the + same reason it never was. Since #567 the editor also shows the plain arrow over + a shape, so what separates the modes is the grips rather than the cursor. - **Selection is one state, reflected everywhere.** A press on a shape selects it — the one pointer gesture a viewer keeps, resolved by the same hit rule the right-click menu uses — and the objects panel's row highlights and scrolls @@ -1163,6 +1164,13 @@ gallery badges (#55) — and it **already exists, shipped and unit-tested**: on the picture made it a control somebody moved blind. Its outline is dashed and an accepted annotation's is solid, which is what tells proposed from committed at a glance. +**The cursor promises the common outcome, not the rare one** (#567). In Select mode, +hovering a shape — its body, its edge band, or a vertex — is the **plain arrow**: a press +there *selects*, and only becomes a move if the pointer then travels. What reports which +shape a press would take is the hover **highlight**, not the cursor. The four-arrow `move` +appears only while a drag is actually in flight, and the directional resize keywords only on +a selected box's grips, where they name an axis the arrow cannot. + **The canvas label is part of what selection looks like.** A frame carrying forty boxes drew forty class names over the picture at all times, which hides the asset behind the annotations of it. The panel is the full inventory; the canvas answers *what is this one* for the shape diff --git a/frontend/annotator/src/adapters/react/TransientLayer.tsx b/frontend/annotator/src/adapters/react/TransientLayer.tsx index 2726d5fd..cc459ead 100644 --- a/frontend/annotator/src/adapters/react/TransientLayer.tsx +++ b/frontend/annotator/src/adapters/react/TransientLayer.tsx @@ -377,9 +377,8 @@ function PendingPolygonShape({ points, cursor, color, zoom, closeRing }: { * * `body` and `empty` draw nothing: a hot body is the committed layer's to fill, * and empty canvas has nothing to promise. An `edge` shows where a double-click - * would insert a vertex, which is the one affordance with no shape of its own — - * `affordance.ts` notes that it answers a `move` cursor and leaves the hint to - * whoever renders `hot`. + * would insert a vertex — the one affordance with no shape of its own, and since + * #567 no cursor of its own either, so this is the whole of the hint. */ function HotTarget({ hot, zoom }: { readonly hot: Target; readonly zoom: number }): JSX.Element | null { if (hot.kind === "handle") { diff --git a/frontend/annotator/src/core/interaction/affordance.test.ts b/frontend/annotator/src/core/interaction/affordance.test.ts index 402d4768..3bd001a6 100644 --- a/frontend/annotator/src/core/interaction/affordance.test.ts +++ b/frontend/annotator/src/core/interaction/affordance.test.ts @@ -129,30 +129,29 @@ describe("hovering, with nothing in flight", () => { } }); - it("offers a move over a body, and names the body it would move", () => { + it("answers default over a body, and names the body a press would take", () => { expect(at(IDLE, BOX_BODY)).toEqual({ - cursor: "move", + cursor: "default", hot: { kind: "body", id: BOX_ID }, }); }); - it("offers a move over a vertex of the picked polygon", () => { + it("answers default over a vertex of the picked polygon", () => { const answer = at(IDLE, POLY_VERTEX, "select", scene(selectOnly(POLY_ID))); - expect(answer.cursor).toBe("move"); + expect(answer.cursor).toBe("default"); expect(answer.hot).toEqual({ kind: "vertex", id: POLY_ID, index: 0, point: POLY_VERTEX }); }); - it("offers a move over an edge, because a press on one starts a move", () => { - // `IDLE_ROW` groups edge with body — `if (kind === "body" || kind === "edge") - // return pressOnShape(...)`. Showing `default` here would be the drawing-tool - // lie inverted: under-promising, and still a disagreement with the table. The - // press is asserted in the same test so the two cannot drift apart. + it("answers default over an edge, where the press still starts a move", () => { + // The cursor stopped advertising the move (#567), so the press is the half + // that matters here: asserted below, in the same test, so the two cannot + // drift apart. // Seven pixels outside the polygon's top edge: past the 4-px shape tolerance // that would make it a body hit, inside the 15-px edge band. const point: Point = [350, 293]; const where = scene(selectOnly(POLY_ID)); const answer = affordanceAt(IDLE, where, "select", point); - expect(answer.cursor).toBe("move"); + expect(answer.cursor).toBe("default"); expect(answer.hot.kind).toBe("edge"); const pressed = transition(IDLE, down(point), { @@ -174,7 +173,7 @@ describe("hovering, with nothing in flight", () => { // `resolveTarget` ranks grips for selected boxes only, and the body underneath // is what a press would actually take. const answer = at(IDLE, BOX_NW, "select", scene()); - expect(answer.cursor).toBe("move"); + expect(answer.cursor).toBe("default"); expect(answer.hot).toEqual({ kind: "body", id: BOX_ID }); }); @@ -405,12 +404,15 @@ describe("the cursor table", () => { it("can actually produce every cursor the union declares", () => { // The union is vocabulary, and vocabulary nobody speaks is dead weight that // reads as capability. Every member has to come out of a real call: the four - // resize keywords from the grips, `move` from a body, `crosshair` from a - // drawing tool, `default` from empty canvas, `pointer` from the first vertex of - // a polygon long enough to close. + // resize keywords from the grips, `move` from a drag in flight — its only + // source since a hover stopped offering it (#567) — `crosshair` from a drawing + // tool, `default` from empty canvas, `pointer` from the first vertex of a + // polygon long enough to close. const closeable = drawing(...PENDING); + const dragging = worldIn("moving"); const produced = new Set([ ...BBOX_HANDLES.map((handle) => at(IDLE, GRIP_POSITIONS[handle]).cursor), + affordanceAt(dragging.state, sceneOfWorld(dragging), "select", EMPTY_POINT).cursor, at(IDLE, BOX_BODY).cursor, at(IDLE, EMPTY_POINT, "bbox").cursor, at(IDLE, EMPTY_POINT).cursor, @@ -459,10 +461,13 @@ describe("the viewer's affordance (#426)", () => { expect(affordance.hot).toEqual({ kind: "body", id: BOX_ID }); }); - it("answers default over a body, where the editor's select tool says move", () => { - expect(affordanceAt(IDLE, viewerScene(), "select", BOX_BODY).cursor).toBe("move"); - expect(viewerAffordanceAt(viewerScene(), BOX_BODY).cursor).toBe("default"); - expect(viewerAffordanceAt(viewerScene(), BOX_BODY).hot).toEqual({ kind: "body", id: BOX_ID }); + it("resolves no grip at all, which is what the editor still does differently", () => { + // Both modes answer `default` over a body since #567, so the cursor no longer + // tells them apart. What does: a viewer never resolves a grip or a vertex, so + // no resize keyword can appear anywhere in it. + expect(viewerAffordanceAt(viewerScene(), BOX_NW).cursor).toBe("default"); + expect(viewerAffordanceAt(viewerScene(), BOX_NW).hot).toEqual({ kind: "body", id: BOX_ID }); + expect(at(IDLE, BOX_NW, "select", scene(selectOnly(BOX_ID))).hot.kind).toBe("handle"); }); it("answers default and no target over empty canvas", () => { diff --git a/frontend/annotator/src/core/interaction/affordance.ts b/frontend/annotator/src/core/interaction/affordance.ts index 0fe84cc3..ca0b1a41 100644 --- a/frontend/annotator/src/core/interaction/affordance.ts +++ b/frontend/annotator/src/core/interaction/affordance.ts @@ -194,15 +194,13 @@ function hovering(scene: Scene, tool: Tool, point: Point): Affordance { case "vertex": case "body": case "edge": - // `edge` is grouped with `body` because `IDLE_ROW` groups them: - // `if (target.kind === "body" || target.kind === "edge") return pressOnShape(...)`. - // A press in the 15-px band around a selected polygon picks it and starts a - // move, so `move` is what the cursor owes. Showing `default` there would be - // the drawing-tool lie inverted — under-promising rather than over — and - // still a disagreement with the table. The double-click that inserts a - // vertex is a second meaning for the same band, not the only one; a renderer - // that wants to hint at it has the `edge` target in `hot`. - return { cursor: "move", hot: target }; + // `edge` is grouped with `body` because `IDLE_ROW` groups them. + // + // `default`, reversing an earlier `move` (#567): a press here *selects*, + // and only becomes a move if the pointer travels — so `move` advertised the + // rarer outcome. `hot` is unchanged, so the shape still highlights, and a + // drag in flight still answers `move` below. + return { cursor: "default", hot: target }; case "empty": return { cursor: "default", hot: NO_TARGET }; } @@ -257,11 +255,12 @@ export function affordanceAt( /** * The affordance a **viewer** answers, where selection is the only gesture. * - * The cursor is `default` everywhere: a read-only page never - * shows `move`, because no move exists to promise. What survives is the hot - * body, so hovering still says *this is the shape a press would pick*: a - * highlight aids selection, which is a read, where a cursor change advertises - * an edit. + * The cursor is `default` everywhere: a read-only page never shows a resize + * keyword, because no such gesture exists. What survives is the hot body, so + * hovering still says *this is the shape a press would pick*. + * + * The editor now answers `default` over a shape too (#567); what still separates + * the modes is below — a viewer resolves no grip and no vertex at all. * * It deliberately does not call `resolveTarget`: that resolver offers grips and * vertices on the selected shape, and a viewer draws none (the same mirror as diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 6da47e5c..7524cc08 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -1421,12 +1421,12 @@ function storedBox(assetId: string): Record { } /** - * Read-only selection highlights — stroke and - * label — and advertises nothing. No move cursor anywhere, no grips or vertex - * dots on the selected shape. The editor is asserted beside it, so the claim is - * about the mode and not about the fixture. + * Read-only selection highlights and advertises nothing: no grips, no vertex + * dots. The editor is asserted beside it, so the claim is about the mode and not + * about the fixture. The cursor no longer separates them (#567) and is not + * compared here. */ -test("read-only selection shows no move cursor and no handles; the editor shows both", async ({ +test("read-only selection grows no handles, where the editor's does", async ({ page, }) => { const sent: Request[] = []; @@ -1447,16 +1447,9 @@ test("read-only selection shows no move cursor and no handles; the editor shows await expect(page.locator("[data-handle]")).toHaveCount(0); await expect(page.locator("[data-vertex]")).toHaveCount(0); - // (b) …and hovering the body promises nothing: the pane's cursor is the - // default arrow, not `move`. - await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); - const viewing = await page - .getByTestId("annotator-pane") - .evaluate((node) => getComputedStyle(node).cursor); - expect(viewing).toBe("default"); }); -test("the editor still offers what the viewer withholds — move cursor and grips", async ({ +test("the editor grows grips on selection, and hovering a shape stays a plain arrow", async ({ page, }) => { const sent: Request[] = []; @@ -1469,11 +1462,14 @@ test("the editor still offers what the viewer withholds — move cursor and grip await expect(page.getByTestId("object-row-0")).toHaveAttribute("data-selected", "true"); await expect(page.locator("[data-handle]").first()).toBeVisible(); + + // Hovering the body is a plain arrow, not the four-arrow `move` (#567). Only a + // browser has a computed cursor at all. await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); const editing = await page .getByTestId("annotator-pane") .evaluate((node) => getComputedStyle(node).cursor); - expect(editing).toBe("move"); + expect(editing).toBe("default"); }); /**