diff --git a/DESIGN.md b/DESIGN.md index 78171ce..ddb76f2 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -975,11 +975,21 @@ The page the reference design shows (#56), with measurements verified in v1's so variant** (the near-black), inactive = ghost; a `h-px w-6` divider; help at the bottom. Tooltips open right with the shortcut ("Select (V)", "Box (B)", "Polygon (P)"). Icons: MousePointer2 / Square / Spline; only tools the schema's geometries allow. - **Above them all, the hand** (#576, `Hand`, `H`) — and it is the one button here that + **Last of them, below the `+`, the hand** (#576, `Hand`, `H`) — the one button here that the schema does not gate, because it answers a question about the *device* rather than about the project: a pan had exactly one spelling, a middle- or secondary-button drag, and a trackpad, a tablet and a pen have no second button to offer. Cursor `grab`, and - `grabbing` while a drag is under way. + `grabbing` while a drag is under way. It sat *above* the strip for a release, as the one + control that does not draw, which read as a heading over the tools rather than as one of + them. **The hand and the derived tool are one lit button, not two**: while the hand is on + no tool row reads as active, because the canvas answers a primary press with a pan before + the machine ever hears it, and pressing any tool — including the one already derived — + puts the hand down. Reaching for a class puts it down too, through the one funnel every + route to a class already goes through. **Suggest is not in that group**: it is a mode over + the class it borrows, so it is legitimately lit beside a tool and keeps its own state. + The canvas says the same thing the strip does: while the hand is on there is **no crosshair + and no highlighted grip**, because the next press is answered by a pan and both would be + offers it cannot keep. Below a second divider, **undo and redo** (#368): the chords have worked since #46 and had no representation on screen at all, so the annotator's headline capability over v1 was invisible to anybody who did not already know it. Disabled *with the reason* diff --git a/docs/annotations.md b/docs/annotations.md index 1014a13..c62e021 100644 --- a/docs/annotations.md +++ b/docs/annotations.md @@ -487,7 +487,14 @@ The rest of the model is more spellings of the same two verbs. blur as well as on keyup, because its release lands in whatever took the focus and never here. - **The hand tool**, `h`, is the persistent one. Not a fifth `Tool`: `tool.ts` derives the tool from the active class and stores nothing, so the mode is the host's and arrives as `panTool`, - which is the arrangement the suggest tool already established. + which is the arrangement the suggest tool already established. **While it is on, the canvas + offers nothing else**: `pointing` is `hover` with the hand spent on it, and both readers of a + hover - the affordance and the drawing guides - go through it, so no grip lights up and no + crosshair is drawn. That is not tidiness. `handlePointerDown` answers the next press with a pan + before the machine or the suggest branch hears it, so a lit grip and a crosshair are offers + that press cannot keep. Applying the mode to the *cursor* alone, which is where it started, + made it a cursor rather than a mode - and left two render sites to remember it at, which is two + more than a mode should have. - **Two touch pointers** are a gesture whatever tool is armed. `pinchBetween` answers a scale about a travelling centroid - one gesture and not two, because a pinch that also drifts is one thing and answering it as a zoom followed by a pan makes the picture jump between them. The diff --git a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx index 287caab..5c352fd 100644 --- a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx +++ b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx @@ -1238,8 +1238,30 @@ export function AnnotatorCanvas({ [snapshot.rendered, hiddenIds], ); + /** + * Where the pointer is **for the tools**, which is nowhere while the hand is on. + * + * `hover` is one piece of state with two readers — the affordance below and the + * crosshair further down — and the hand used to reach neither. It was applied to + * the *cursor* alone, which made it a cursor rather than a mode: a raised hand + * still lit the grip under the pointer and still drew the drawing guides across + * the picture, both of them offers the very next press cannot keep, because + * `handlePointerDown` answers that press with a pan before the machine or the + * suggest branch hears it. + * + * So the mode is spent once, here, on the state both readers derive from. A hand + * that had to be remembered at each render site is a hand that would be + * forgotten at the next one — this is the third such site to be found and there + * is no reason to think it is the last. + * + * `hover` itself keeps tracking, deliberately: putting the hand down restores + * the crosshair and the highlight where the pointer already is, with no move + * needed to wake them. + */ + const pointing = hand ? null : hover; + const affordance = - hover === null + pointing === null ? { cursor: "default" as const, hot: NO_TARGET } : readOnly ? // The viewer's answer: `default` everywhere — no cursor may @@ -1247,7 +1269,7 @@ export function AnnotatorCanvas({ // a highlight aids the one gesture a viewer has, which is selecting. viewerAffordanceAt( { document: visibleRendered, selection: snapshot.selection, tolerances }, - hover, + pointing, ) : affordanceAt( interaction, @@ -1255,7 +1277,7 @@ export function AnnotatorCanvas({ // committed document — `affordance.ts` states that asymmetry. { document: visibleRendered, selection: snapshot.selection, tolerances }, tool, - hover, + pointing, ); const hotBodyId = affordance.hot.kind === "body" ? affordance.hot.id : null; @@ -1408,7 +1430,7 @@ export function AnnotatorCanvas({ drawColor={drawColor} zoom={view.zoom} closeRing={tolerances.closePolygon} - crosshair={tool === "select" ? null : hover} + crosshair={tool === "select" ? null : pointing} asset={asset} suggestions={painted} {...(suggestion === null ? {} : { promptPoints: suggestion.points })} diff --git a/frontend/annotator/src/adapters/react/TransientLayer.tsx b/frontend/annotator/src/adapters/react/TransientLayer.tsx index cc459ea..94e5c2f 100644 --- a/frontend/annotator/src/adapters/react/TransientLayer.tsx +++ b/frontend/annotator/src/adapters/react/TransientLayer.tsx @@ -110,7 +110,10 @@ export function TransientLayer({ // with it. {crosshair !== null && ( - + // Named so a browser can assert its absence: the guides are the most + // visible thing an armed tool puts on the picture, and "the hand put the + // tools away" is a claim about exactly that. + diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 2aafe2e..fd86cad 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -2115,10 +2115,23 @@ test("the hand turns a plain drag into a pan, from the key and from the button", // A pan is not an edit: the drag drew nothing and there is nothing to save. await expectNothingToSave(page); - // The button turns it back off, and the same drag draws again. + // The button turns it back off. await button.click(); await expect(button).toHaveAttribute("data-active", "false"); + + // And so does reaching for a class, which is the half that makes the strip + // readable: the hand is a mode, the canvas answers a primary press with a pan + // before the machine hears it, and a class armed under a raised hand would be + // a tool that draws nothing while the strip lit it and the hand at once. Every + // route to a class goes through one funnel on the page, so the digit proves + // the panel's list and the strip's own buttons too. + await page.keyboard.press("h"); + await expect(button).toHaveAttribute("data-active", "true"); await page.keyboard.press("1"); + await expect(button).toHaveAttribute("data-active", "false"); + await expect(page.getByTestId("tool-bbox")).toHaveAttribute("data-active", "true"); + + // And the same drag draws again. const draw = { x: pane.x + pane.width * 0.4, y: pane.y + pane.height * 0.4 }; await page.mouse.move(draw.x, draw.y); await page.mouse.down(); @@ -2127,6 +2140,43 @@ test("the hand turns a plain drag into a pan, from the key and from the button", await expect(page.getByTestId("object-total")).toContainText("1 object"); }); +/** + * The hand puts the *tools* away, not only the cursor. + * + * `hover` has two readers — the affordance and the drawing guides — and the hand + * used to reach neither, only the cursor. So a raised hand over an armed tool + * still drew the crosshair across the picture and still lit the grip under the + * pointer: offers the very next press cannot keep, because it is answered by a + * pan before the machine hears it. This is that half, in the one place it is + * visible from outside. + * + * The tool is armed with a digit rather than by pressing the strip, and the hand + * with `h` rather than the button, so neither half of the scenario depends on + * the palette wiring the scenario above already covers. Arming the suggest tool + * takes the same path — it activates a class, so `tool` is that class's geometry + * — and needs a model behind it, which this suite does not have. + */ +test("the hand takes the crosshair off the picture, and gives it back", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent); + + const pane = (await page.getByTestId("annotator-pane").boundingBox())!; + const crosshair = page.getByTestId("crosshair"); + + await page.getByTestId("annotator-root").focus(); + await page.keyboard.press("1"); + await page.mouse.move(pane.x + pane.width * 0.5, pane.y + pane.height * 0.5); + await expect(crosshair).toHaveCount(1); + + await page.keyboard.press("h"); + await expect(crosshair).toHaveCount(0); + + // And back, with no pointer move to wake it: `hover` never stopped tracking, + // so the guides return where the pointer already is. + await page.keyboard.press("h"); + await expect(crosshair).toHaveCount(1); +}); + /** * The surround must not be the rail's near-black navy. * diff --git a/frontend/ui-core/src/annotator/AnnotationPage.tsx b/frontend/ui-core/src/annotator/AnnotationPage.tsx index 9964623..64b6cb9 100644 --- a/frontend/ui-core/src/annotator/AnnotationPage.tsx +++ b/frontend/ui-core/src/annotator/AnnotationPage.tsx @@ -762,7 +762,7 @@ function Workspace({ detail, onDetail: setDetail, activeClass, - onActivateClass: activateClass, + onActivateClass: armClass, onNavigate, onOpenGallery, onConfigureInference, @@ -793,6 +793,34 @@ function Workspace({ * with the hand on is navigating the batch, not this asset. */ const [handTool, setHandTool] = useState(false); + /** + * Reaching for a drawing class puts the hand away. + * + * The two are modes over the same canvas and only one of them can be true of a + * primary press: `AnnotatorCanvas` answers one with a pan *before* the suggest + * branch and before the machine, so a class armed under a raised hand is a tool + * that cannot draw — and the strip would light both, which is what somebody + * looking at it reported. The hand is the mode, so picking anything else is + * what ends it. + * + * Wrapped **here**, around the one funnel `onActivateClass`'s own docstring + * already names — the panel's list, the tool strip, a digit hotkey and the + * canvas's `activate-class` all arrive through it — rather than at the four + * call sites, which is the same reason it is a funnel at all. `toggleSuggest` + * arms through it too, so the sparkle puts the hand away without knowing it + * has to. + * + * Only this direction is automatic. Raising the hand leaves the class where it + * was: it is a way of *looking* at the picture, and a person who pans and puts + * the hand down wants the class they were drawing with, not `select`. + */ + const activateClass = useCallback( + (labelClass: string | null): void => { + setHandTool(false); + armClass(labelClass); + }, + [armClass], + ); const [galleryOpen, setGalleryOpen] = useState(false); /** * Which shape's class picker is open, if any. diff --git a/frontend/ui-core/src/annotator/ToolPalette.tsx b/frontend/ui-core/src/annotator/ToolPalette.tsx index b9bbe03..3e817cd 100644 --- a/frontend/ui-core/src/annotator/ToolPalette.tsx +++ b/frontend/ui-core/src/annotator/ToolPalette.tsx @@ -274,6 +274,17 @@ export interface ToolPaletteProps { * Required rather than optional, unlike `suggest` and `history`: those are * capabilities a host may not have behind it, and this is one every host * already has — the canvas implements it, not the page. + * + * **It and the derived tool are one lit button, not two.** While the hand is + * on no tool row reads as active, and pressing any of them puts the hand down: + * the canvas answers a primary press with a pan before the machine hears it, + * so a tool lit beside a raised hand is one that cannot draw. The class half of + * that is the host's — every route to a drawing class puts the hand away, one + * funnel there rather than a rule repeated at each button here. + * + * The suggest button is deliberately **not** in this: it is a mode over the + * class it borrows, it is legitimately on together with a tool, and dimming it + * would make a press that turns it *off* look like one that turns it on. */ readonly hand: { readonly active: boolean; @@ -322,32 +333,24 @@ export function ToolPalette({ data-testid="tool-palette" className="absolute left-3 top-3 flex w-12 flex-col items-center gap-1 rounded-xl border border-border bg-muted p-2 shadow-lg" > - {/* First, and above the tools rather than among them: it is the one - control here that does not draw, and the one a person reaches for when - the picture is in the wrong place rather than when it is wrong. */} - - - - {!readOnly && toolChoices(schema).map((choice) => ( { if (choice.unavailable !== null) return; if (tool !== choice.tool) onActivateClass(choice.labelClass); + else if (hand.active) hand.onToggle(); }} > @@ -389,6 +392,23 @@ export function ToolPalette({ )} + {/* Last in the block, below the `+`, and in the block rather than above it. + It sat on top for a release, as the one control that does not draw — + which read as a heading over the tools instead of as one of them, and + the strip lit it *and* whichever tool was derived, so two buttons + claimed to be on at once. It is a mode like the rest, so it takes its + place among them and takes the lit state with it: while it is on, + nothing else here is. */} + + + + {!readOnly && history !== undefined && ( <>
diff --git a/frontend/ui-core/src/annotator/toolPalette.test.tsx b/frontend/ui-core/src/annotator/toolPalette.test.tsx index c804f28..1e4d4d0 100644 --- a/frontend/ui-core/src/annotator/toolPalette.test.tsx +++ b/frontend/ui-core/src/annotator/toolPalette.test.tsx @@ -383,6 +383,67 @@ describe("the hand is the one button here that is not about the schema (#576)", expect(onToggle).toHaveBeenCalledTimes(1); }); + + it("takes the lit state off the derived tool while it is on", () => { + // The bug this is here for: the hand is a mode beside the derived tool + // rather than one of its values, so the strip lit both and two buttons + // claimed to be on at once. The canvas answers a primary press with a pan + // before the machine ever hears it, so the derived tool cannot act while the + // hand is up — and a lit button for a tool that does nothing is the lie. + const { rerender } = render(mount({ tool: "bbox" })); + expect(screen.getByTestId("tool-bbox").getAttribute("data-active")).toBe("true"); + + rerender(mount({ tool: "bbox", hand: { active: true, onToggle: vi.fn() } })); + expect(screen.getByTestId("tool-bbox").getAttribute("data-active")).toBe("false"); + expect(screen.getByTestId("tool-hand").getAttribute("data-active")).toBe("true"); + }); + + it("is put down by a press on the tool that is already derived", () => { + // The bug the first cut of this shipped: a press whose tool has not moved is + // a no-op, so on a page sitting in `select` — which is where every frame + // opens — the Select button could not put the hand down, and the only way + // back was to arm some other tool first. + const onToggle = vi.fn(); + const onActivateClass = vi.fn(); + render(mount({ tool: "select", hand: { active: true, onToggle }, onActivateClass })); + + fireEvent.click(screen.getByTestId("tool-select")); + + expect(onToggle).toHaveBeenCalledTimes(1); + // And the class does not move, which is what the no-op rule was protecting: + // a schema with two bbox classes must not silently re-point at the other one. + expect(onActivateClass).not.toHaveBeenCalled(); + }); + + it("leaves the suggest tool lit, because that one is legitimately on beside a tool", () => { + // Not in the exclusive group: suggest is a mode over the class it borrows, and + // dimming it under the hand would make a press that turns it *off* look like + // one that turns it on. + const armed = { active: true, onToggle: vi.fn(), unavailable: null }; + render(mount({ suggest: armed, hand: { active: true, onToggle: vi.fn() } })); + + expect(screen.getByTestId("tool-suggest").getAttribute("data-active")).toBe("true"); + }); + + it("is the last of the tools, below the button that adds a class", () => { + // Order, asserted because it is the half a `getByTestId` cannot see. It was + // above the strip for a release, which read as a heading over the tools + // rather than as one of them. + render(mount({ onAddClass: vi.fn(), suggest: { active: false, onToggle: vi.fn() } })); + + const strip = screen.getByTestId("tool-palette"); + const order = [...strip.querySelectorAll("[data-testid^='tool-']")].map((node) => + node.getAttribute("data-testid"), + ); + + expect(order.slice(0, 3)).toEqual(["tool-select", "tool-bbox", "tool-polygon"]); + expect(order.slice(-4)).toEqual([ + "tool-suggest", + "tool-add-class", + "tool-hand", + "tool-help", + ]); + }); }); describe("a viewer gets the strip, carrying only what does not draw (#576)", () => {