Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -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*
Expand Down
9 changes: 8 additions & 1 deletion docs/annotations.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 26 additions & 4 deletions frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1238,24 +1238,46 @@ 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
// promise a move that cannot happen — with the hot body kept, because
// a highlight aids the one gesture a viewer has, which is selecting.
viewerAffordanceAt(
{ document: visibleRendered, selection: snapshot.selection, tolerances },
hover,
pointing,
)
: affordanceAt(
interaction,
// Built from what is **rendered**, where the machine's context is the
// 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;

Expand Down Expand Up @@ -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 })}
Expand Down
5 changes: 4 additions & 1 deletion frontend/annotator/src/adapters/react/TransientLayer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,10 @@ export function TransientLayer({
// with it.
<g data-testid="transient-layer" pointerEvents="none">
{crosshair !== null && (
<g opacity={0.55}>
// 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.
<g data-testid="crosshair" opacity={0.55}>
<line x1={0} y1={crosshair[1]} x2={asset.width} y2={crosshair[1]} stroke="#ffffff" strokeWidth={screenPx(1, zoom)} strokeDasharray={DASH} />
<line x1={crosshair[0]} y1={0} x2={crosshair[0]} y2={asset.height} stroke="#ffffff" strokeWidth={screenPx(1, zoom)} strokeDasharray={DASH} />
</g>
Expand Down
52 changes: 51 additions & 1 deletion frontend/app/e2e/annotate.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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.
*
Expand Down
30 changes: 29 additions & 1 deletion frontend/ui-core/src/annotator/AnnotationPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,7 @@ function Workspace({
detail,
onDetail: setDetail,
activeClass,
onActivateClass: activateClass,
onActivateClass: armClass,
onNavigate,
onOpenGallery,
onConfigureInference,
Expand Down Expand Up @@ -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.
Expand Down
50 changes: 35 additions & 15 deletions frontend/ui-core/src/annotator/ToolPalette.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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. */}
<PaletteButton
testId="tool-hand"
label="Hand (H)"
active={hand.active}
onMouseDown={keepFocus}
onClick={hand.onToggle}
>
<Hand className="size-4" />
</PaletteButton>

{!readOnly &&
toolChoices(schema).map((choice) => (
<PaletteButton
key={choice.tool}
testId={`tool-${choice.tool}`}
label={choice.unavailable ?? `${choice.label} (${choice.hotkey})`}
active={tool === choice.tool}
active={!hand.active && tool === choice.tool}
disabled={choice.unavailable !== null}
onMouseDown={keepFocus}
// (1) above: the tool did not move, so nothing moves.
// (1) above: the tool did not move, so nothing moves — except that
// putting the hand down *is* a move. Without the second line, Select
// is unreachable from the hand on a page whose derived tool is
// already `select`: the press is a no-op, so nothing clears the mode
// and the only way back is to arm some other tool first.
onClick={() => {
if (choice.unavailable !== null) return;
if (tool !== choice.tool) onActivateClass(choice.labelClass);
else if (hand.active) hand.onToggle();
}}
>
<ToolIcon tool={choice.tool} />
Expand Down Expand Up @@ -389,6 +392,23 @@ export function ToolPalette({
</PaletteButton>
)}

{/* 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. */}
<PaletteButton
testId="tool-hand"
label="Hand (H)"
active={hand.active}
onMouseDown={keepFocus}
onClick={hand.onToggle}
>
<Hand className="size-4" />
</PaletteButton>

{!readOnly && history !== undefined && (
<>
<div className="my-1 h-px w-6 bg-border" />
Expand Down
61 changes: 61 additions & 0 deletions frontend/ui-core/src/annotator/toolPalette.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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)", () => {
Expand Down
Loading