diff --git a/DESIGN.md b/DESIGN.md index 46dce52c..936e5322 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -176,7 +176,7 @@ primitive targets): | Button — hover | default `hover:bg-primary/80`; secondary `color-mix(in oklch, var(--secondary), var(--foreground) 5%)`; destructive soft | | Input | `h-8 rounded-lg border-input px-2.5 text-base md:text-sm`; dark theme `bg-input/30` | | Badge | `h-5 px-2 text-xs rounded-4xl`; icons `size-3`. The `quiet` variant alone is `rounded-md` — a square, colourless label for a fact read beside other facts, never a state | -| Menu — surface | `dark` subtree + `bg-popover p-1 rounded-lg ring-1 ring-foreground/10 shadow-md`, `min-w-32`, `duration-100` enter/exit | +| Menu — surface | `dark` subtree + `bg-popover p-1 rounded-lg ring-1 ring-foreground/10 shadow-md`, `min-w-32`, `duration-100` enter and **no exit** — see *Motion* | | Menu — item | `px-1.5 py-1 text-sm rounded-md focus:bg-accent focus:text-accent-foreground`; destructive item soft | | Card | `rounded-xl ring-1 ring-foreground/10 text-sm`; `--card-spacing` = `--spacing(4)` (16px; 12px at the `sm` size); footer `bg-muted/50` | | Dialog — overlay | `bg-black/10 supports-backdrop-filter:backdrop-blur-xs duration-100` | @@ -185,7 +185,7 @@ primitive targets): | Elevation | `ring-1 ring-foreground/10` + a resting shadow — never a coloured border | `frontend/ui-core/src/primitives/` carries the table itself, not a family resemblance to -it: the heights, the `ring-3` focus treatment, the `duration-100` enter/exit, the +it: the heights, the `ring-3` focus treatment, the `duration-100` enter, the `--card-spacing` variable and Nova's interaction idioms (uniform disabled opacity, soft destructive, `/80`-opacity hover, the inverted menu subtree) are the primitives' own declarations. The table is the contract and the primitives are where it is spelled, so a @@ -248,6 +248,33 @@ which is the only place the composition can be checked: Tailwind renders `ring-3 `box-shadow` layer, so "the ring is there" is a question about a computed shadow list rather than about a class string. +## Motion + +Motion orients or confirms, and never stands between somebody and their next action — +*Fast and quiet*, applied. An **enter** animation is free to play: nothing is waiting on +it, because the surface it introduces did not exist a frame ago. An **exit** animation is +not, and the difference is not a matter of taste. + +**A floating surface leaves on the frame it is dismissed.** While an exit animation runs, +Radix keeps the content mounted, and the dismissable layer stays mounted with it. A press +on the trigger inside that window is read twice — the trigger toggles the surface open, +and the layer still listening reads the same pointer-down as an interaction outside itself +and dismisses — so the two cancel and the surface never appears. At `duration-100` that +window covers the gap between an `Escape` and the click after it, which makes it a defect +a fast hand meets routinely rather than an edge case. The annotation workspace is where +that tempo is normal, and it is where the behaviour was measured; the fix belongs to the +primitive because every menu in the product shared the flaw. + +So `DropdownMenuContent` animates in and not out, and +`frontend/app/e2e/annotate.spec.ts` holds the two presses that would catch a fade being +restored. A surface that genuinely needs an exit — one whose trigger cannot be pressed +again straight away, as a modal's cannot — may keep one; the tooltip keeps its own, +because a hover has no toggle to swallow. + +`prefers-reduced-motion` sits above all of this: the base layer in `styles.css` collapses +every animation and transition to a single frame under that query, so none of the above is +something a component opts into. + ## Sidebar / Menu - **`menuColor: inverted`.** Every menu and popover subtree — dropdown content, select diff --git a/frontend/app/e2e/_frame.ts b/frontend/app/e2e/_frame.ts index 7ed90e7b..1fc7c4fc 100644 --- a/frontend/app/e2e/_frame.ts +++ b/frontend/app/e2e/_frame.ts @@ -294,10 +294,9 @@ export async function expectProgress(page: Page, progress: string): Promise { await page.getByTestId("more-actions").click(); @@ -305,19 +304,18 @@ export async function openOverflow(page: Page): Promise { } /** - * Escape out of the overflow menu, and wait until it has actually left. + * Escape out of the overflow menu, and assert it has actually left. * - * Nova's menu surface animates out (`data-closed:animate-out` over `duration-100`), - * which means Radix keeps the content — and its dismissable layer — mounted for a - * beat after `open` goes false. A press on the trigger inside that beat hits both - * handlers at once: the trigger toggles the menu open, and the still-listening - * layer reads the same pointer-down as an interaction *outside* itself and - * dismisses. The two cancel, and the menu never appears — which is exactly what - * reopening the overflow straight after an `Escape` used to run into. + * The wait is the assertion that `Escape` closed the menu at all, and it is on the + * menu leaving the DOM rather than on a clock — per + * `tests/scripts/e2e_discipline.test.mjs`. * - * So the close is a state to wait on, not a keystroke to fire and forget. The wait - * is on the menu leaving the DOM, which is also the assertion that `Escape` closed - * it at all — no clock anywhere, per `tests/scripts/e2e_discipline.test.mjs`. + * It is not a workaround for a surface that leaves slowly. The menu has no exit + * animation, deliberately, so there is no window in which it is still mounted and + * swallowing the next press — `DESIGN.md`'s *Motion* argues why, and + * `annotate.spec.ts` presses the trigger straight after an `Escape` to keep it + * true. A scenario that wants to reopen the menu immediately should do exactly + * that rather than reach for this. */ export async function closeOverflow(page: Page): Promise { await page.keyboard.press("Escape"); diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index a89317b8..59ff78dd 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -3517,3 +3517,38 @@ test("a press on the suggest panel never reaches the picture underneath", async await expect(page.getByTestId("suggest-adjustments")).toBeVisible(); expect(asks(sent)).toBe(before); }); + +/** + * ## Reopening a menu that was just dismissed + * + * `DESIGN.md`'s *Motion*: a floating surface leaves on the frame it is dismissed, + * because an animated exit keeps Radix's dismissable layer mounted and the next + * press on the trigger is then read as both an open and an outside-dismiss, which + * cancel. These two scenarios are the presses that catch a fade being restored — + * both failed against `duration-100` on the way out, the first in four runs of + * five and the second in five. + * + * Neither uses `closeOverflow`. Its wait is what a suite does to stay honest about + * a surface that is still leaving; the product's contract is that there is nothing + * to wait for. + */ +test("the overflow reopens on a press straight after Escape", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent); + + await openOverflow(page); + await page.keyboard.press("Escape"); + await page.getByTestId("more-actions").click(); + await expect(page.getByRole("menu")).toBeVisible(); +}); + +test("the overflow reopens on a press straight after a second Escape", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent); + + await openOverflow(page); + await page.keyboard.press("Escape"); + await page.keyboard.press("Escape"); + await page.getByTestId("more-actions").click(); + await expect(page.getByRole("menu")).toBeVisible(); +}); diff --git a/frontend/ui-core/src/annotator/AddClassDialog.tsx b/frontend/ui-core/src/annotator/AddClassDialog.tsx index 934be458..e67e356e 100644 --- a/frontend/ui-core/src/annotator/AddClassDialog.tsx +++ b/frontend/ui-core/src/annotator/AddClassDialog.tsx @@ -85,7 +85,7 @@ * where every other refusal here renders. */ -import { Plus, X } from "lucide-react"; +import { IconPlus, IconX } from "@tabler/icons-react"; import { useEffect, useRef, useState, type JSX } from "react"; import { asApiError } from "../data/errors"; @@ -634,7 +634,7 @@ export function AddClassDialog({ onBank?.(banked); }} > -