feat(ui-core): the class picker gets a second anchor, on the shape itself (#380) - #397
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Annotations panel has offered class reassignment per row since #378 — every class the
pinned schema declares, the incompatible ones disabled and carrying the reason, applied
through
replaceAnnotationCommandso it lands in the undo history. #368's decision 9deferred the second anchor: the same picker on the canvas, over the shape.
This is that anchor, and the whole of it is that it is the same picker.
Closes #380
Same component, second mount point
ReassignMenu.tsxis new and holds everything the menu decides — the class list, thedeclared.geometry === annotation.geometry.typepredicate, the disabled-with-reasonrendering, the hotkey and the call that applies.
AnnotatorPanel's privateReassignMenuwas moved into it, not copied: what is left in the panel is
RowReassign, a triggerand a row number, which are the only two things a list legitimately has that a shape does
not.
So there is no second class-list derivation, no second legality rendering and no second
apply path — and that claim is mutation-verified rather than asserted (below).
align="end"align="start"ReassignMenuReassignMenuWhat is new for both anchors
The brief asks for class hotkeys shown, and a hotkey while the picker is open
reassigns. That lives in the shared component, so the panel's row menu gained it too —
a deliberate consequence of "same component", not scope creep, and
panel.test.tsxnowpins it at that anchor as well.
Two decisions inside it:
on the reason instead. A key chip on a row that refuses the key is a lie, and the reason
is the actionable half — the same argument the disabled rows exist for, one column over.
Radix's typeahead and move the highlight to whichever class name happens to start with
that character.
digitFromCode(event.code) ?? event.key, the spellingAnnotatorCanvasuses, because the digit row is a row of positions and on AZERTY
1arrives as&.Both mounts now hold
openthemselves rather than leaving it to Radix: a hotkey is not anitem selection, so nothing would dismiss the menu after a digit reassigns.
The canvas mount
CanvasReassign.tsx, a sibling of the canvas inside the stage —ToolPalette's placement,for its reason: the stage is
relativeand the annotator ships no chrome.imageToScreenover thegeometry's bbox, so the position is derived from the same transform the stage's own
translate/scaleis built from and cannot disagree with what was drawn — no DOM read,no
ResizeObserver. Above rather than on the corner because a selected bbox draws aresize grip exactly there, and a button centred on it would be the one thing standing
between somebody and resizing what they had just selected.
sets
openForand Radix positions the content against the button, with the flipping,the collision handling and the portal (which is what keeps the content out of the
stage's
overflow: hidden) all the primitive's own.openForis an id, not a boolean. Deleting the selected shape while the menu isopen, or selecting a different one from the panel, would otherwise leave
opentrueover a subject that had changed underneath it. An id is false by construction the moment
the subject is not the one that was opened — no effect, no cleanup.
selected, and a classification tag. The last two are the same rule from two sides — the
menu changes one annotation's class and anchors to one position, and a tag is the
only annotation the canvas never renders, so there is no position on the stage that
means it. Tags stay reachable from their panel row, which is a list and needs no anchor.
Read-only is enforced in
CanvasReassignand nowhere else. The page still handsAnnotatorCanvasitsonAnnotationMenuwhen the frame cannot be written, deliberately: asecond guard up there would keep the behaviour correct with this one deleted, which is a
test that cannot fail (#378's finding).
The one annotator change, and why it is in the adapter
AnnotatorCanvasgains one optional prop,onAnnotationMenu?: (annotationId: string) => void. It is a report, not an action — what a right-click should open is entirely thehost's business, but which annotation is under a client position is not something a host
can answer without re-deriving the transform and the hit test that already live here. The
brief's own constraint ("no new global listeners; pointer handling stays in the adapter
layer") is what puts it there.
It rides the browser's
contextmenurather than a secondarypointer-down, and that isdeliberate on both sides:
interaction-table rows that still have no browser spelling —
cf. #129, which staysexactly as it was.
contextmenuarrives after the press has already started its pan, so aclick-with-no-travel pans by zero and nothing about the existing grammar moves. The
onContextMenuhandler that was already there forpreventDefault()is the same one.It resolves through
topmostAnnotationAtoverwithoutHidden(...)attolerances.shape— the same three inputsresolveTarget's body branch uses, so what aright-click hits and what a left-click would have selected are one rule. A hidden shape
does not answer, for
visibility.ts's reason: a shape you cannot see must not swallow apress, and a menu is a press.
readOnlydoes not gate it — the rule that componentenforces is that input may not change the document, and reporting a hit changes nothing.
Tests
frontend/ui-core/src/annotator/canvasReassign.test.tsx— 13, against a realAnnotatorStore, no mock and no provider (panel.test.tsx's shape). Four new infrontend/app/e2e/annotate.spec.ts, chromium.canvasReassign› rides the one selected shape, and names what it will reassignannotate.spec.ts› right-clicking a shape opens its class picker, and the class lands through the commandcanvasReassign› applies through the command, so undo takes the class back · the e2e above (mod+zback tovehicle)canvasReassign› lists every class, disabling the ones this shape cannot become and saying why · the e2e above (needs a polygon)canvasReassign› reassigns on a class hotkey and closes itself… ·panel› reassigns on a class hotkey, at this anchor as at the other onecanvasReassign› changes nothing on a digit for a class this shape cannot become, and stays opencanvasReassign› is absent entirely when the document cannot be writtenannotate.spec.ts› the picker's button rides the shape, above the corner its resize grip ownsannotate.spec.ts› a right-click on empty canvas opens nothing, because the hit test is realcanvasReassign› closes on Escape with the document untouched ·annotate.spec.ts› Escape closes the canvas picker and leaves the object alonepanel.test.tsxcases ande2e/panel.spec.ts, untouched and greenThe split follows #47's precedent: jsdom's
getBoundingClientRectanswers all zeros, sothe transform — the riskiest part — is exactly what a unit test could never verify. The
two claims that need a real browser are in chromium; everything the menu decides is a
component claim.
Mutation-verified
Seven, each reverted by its exact recorded diff (
git apply -R), anchor asserted presentexactly once before and the replacement after.
CanvasReassign, diverging by filtering incompatible classes out instead of disabling themNumber.POSITIVE_INFINITYannotate.spec.ts› a right-click on empty canvas opens nothing, because the hit test is real (chromium)annotate.spec.ts› the picker's button rides the shape… (chromium)readOnlydropped from the guardselected.length === 1→selected[0]anchorForreturns an origin instead ofnullstore.executeThe first is the brief's own required mutation, and it is the one that makes
"single spelling" a claim rather than a hope.
The e2e fixture gained a class
annotate.spec.ts's pinned schema declared one bbox class (vehicle) and one polygonclass, so a bbox had nowhere to be reassigned to and the apply could not be proved in a
browser at all. It now also declares
pedestrian(bbox). It adds no tool — the paletteis per geometry — and one hotkey row, which the shortcut-sheet scenario counts and now
counts as three.
Gate
Full run, staged around the ~10-minute harness ceiling, exit codes verbatim:
scripts/check.sh frontend generatedscripts/check.sh pythonscripts/check.sh browser206 app/annotator e2e scenarios and 687 ui-core unit tests, all green. No kernel change,
no wire change, no migration:
openapi.json, the generated client andFORMAT_VERSIONare all byte-still.
Found, not fixed
align="end"and the canvas'salign="start"are the only two propsthat differ between the mounts, and they are passed rather than derived. If a third
anchor ever appears it should carry its own placement the same way — there is no rule
here to generalise yet.
CanvasReassignis not exported from@visionset/ui-core's public entry, so theshowcase (
frontend/app/src/demo/) still shows the panel anchor only. Mounting it therewould put a floating button over the demo canvas that ~40 existing showcase scenarios
drag through, and that is a decision worth taking on its own rather than as a side
effect of this one.