From 1166807d7be43344e39d8e4d9eb5dd91dc3a6dc5 Mon Sep 17 00:00:00 2001 From: Owais Jamil Date: Sun, 23 Aug 2026 06:10:41 +0000 Subject: [PATCH] feat: standardize context menus & popovers * update stories --- ROADMAP.md | 10 +++++----- TODO.md | 16 ++++++++-------- apps/web/e2e/editor-controls.spec.ts | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ apps/web/src/lib/tests/Toolbar.accessibility.test.ts | 9 ++++----- packages/ui/src/lib/components/BrushPopover.svelte | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------ packages/ui/src/lib/components/ContextMenu.svelte | 21 +++++++++++++++++++-- packages/ui/src/lib/components/stories/IconButton.stories.ts | 1 + packages/ui/src/lib/editor/canvas/NavigationControls.svelte | 116 +++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------- packages/ui/src/lib/editor/components/Toolbar.svelte | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------------------------------------- packages/ui/src/lib/editor/components/stories/Toolbar.stories.ts | 2 ++ 10 file(s) changed, 269 insertion(s)(+), 254 deletion(s)(-) diff --git a/ROADMAP.md b/ROADMAP.md --- a/ROADMAP.md +++ b/ROADMAP.md @@ -62,11 +62,11 @@ ### Editor polish -The editor already separates drawing tools, application actions, contextual -selection controls, and library insertion. After the workflows above settle, -finish the consistency pass across control states, menus, popovers, pointer -targets, viewport edges, and Storybook examples. Light and dark themes should -continue to use the shared Inkfinite tokens with theme-specific contrast. +The editor separates drawing tools, application actions, contextual selection +controls, and library insertion. Shared controls now cover interaction states, +pointer targets, menu and popover behavior, viewport-edge placement, and focus +restoration. Storybook and end-to-end tests cover the important combinations in +light and dark themes. ### Permissioned MCP diff --git a/TODO.md b/TODO.md --- a/TODO.md +++ b/TODO.md @@ -55,18 +55,18 @@ ## Editor polish -- [ ] Standardize hover, pressed, selected, disabled, busy, and focus-visible +- [x] Standardize hover, pressed, selected, disabled, busy, and focus-visible states -- [ ] Standardize menu and popover placement, dismissal, and focus restoration -- [ ] Standardize control heights, icon sizes, spacing, and minimum pointer +- [x] Standardize menu and popover placement, dismissal, and focus restoration +- [x] Standardize control heights, icon sizes, spacing, and minimum pointer targets -- [ ] Remove controls whose hover state is visually indistinguishable from +- [x] Remove controls whose hover state is visually indistinguishable from idle -- [ ] Verify menus and popovers remain inside the viewport at editor edges -- [ ] Verify tool changes, selection changes, and viewport actions do not cause +- [x] Verify menus and popovers remain inside the viewport at editor edges +- [x] Verify tool changes, selection changes, and viewport actions do not cause unintended layout jumps -- [ ] Add Storybook coverage for important component states and combinations -- [ ] Add end-to-end coverage for menus, popovers, focus restoration, and +- [x] Add Storybook coverage for important component states and combinations +- [x] Add end-to-end coverage for menus, popovers, focus restoration, and viewport-edge placement ## Permissioned MCP diff --git a/apps/web/e2e/editor-controls.spec.ts b/apps/web/e2e/editor-controls.spec.ts new file mode 100644 --- /dev/null +++ b/apps/web/e2e/editor-controls.spec.ts @@ -0,0 +1,49 @@ +import { expect, test } from './fixtures/editor'; + +async function expectInsideViewport(locator: import('@playwright/test').Locator) { + const box = await locator.boundingBox(); + const viewport = locator.page().viewportSize(); + expect(box).not.toBeNull(); + expect(viewport).not.toBeNull(); + expect(box!.x).toBeGreaterThanOrEqual(0); + expect(box!.y).toBeGreaterThanOrEqual(0); + expect(box!.x + box!.width).toBeLessThanOrEqual(viewport!.width); + expect(box!.y + box!.height).toBeLessThanOrEqual(viewport!.height); +} + +test('menus stay in the viewport and restore focus', async ({ editor, page }) => { + await editor.open(); + + const exportButton = page.getByRole('button', { name: 'Export drawing' }); + await exportButton.click(); + const exportMenu = page.getByRole('menu', { name: 'Export options' }); + await expect(exportMenu).toBeVisible(); + await expectInsideViewport(exportMenu); + await page.keyboard.press('Escape'); + await expect(exportButton).toBeFocused(); + + const shapesButton = page.getByRole('button', { name: 'Shapes', exact: true }); + await shapesButton.click(); + const shapesMenu = page.getByRole('menu', { name: 'Shape tools' }); + await expectInsideViewport(shapesMenu); + await page.keyboard.press('Escape'); + await expect(shapesButton).toBeFocused(); + + const zoomButton = page.getByRole('button', { name: 'Zoom level' }); + await zoomButton.click(); + await expectInsideViewport(page.getByRole('menu', { name: 'Zoom options' })); + await page.keyboard.press('Escape'); + await expect(zoomButton).toBeFocused(); +}); + +test('tool and menu changes do not move the canvas', async ({ editor, page }) => { + await editor.open(); + const canvas = page.locator('canvas').first(); + const initial = await canvas.boundingBox(); + + await page.getByRole('button', { name: 'Direct Select', exact: true }).click(); + await page.getByRole('button', { name: 'Shapes', exact: true }).click(); + await page.keyboard.press('Escape'); + + expect(await canvas.boundingBox()).toEqual(initial); +}); diff --git a/apps/web/src/lib/tests/Toolbar.accessibility.test.ts b/apps/web/src/lib/tests/Toolbar.accessibility.test.ts --- a/apps/web/src/lib/tests/Toolbar.accessibility.test.ts +++ b/apps/web/src/lib/tests/Toolbar.accessibility.test.ts @@ -40,7 +40,7 @@ const exportButton = container.querySelector('.toolbar__export-button'); expect(exportButton?.getAttribute('aria-label')).toBe('Export drawing'); - expect(exportButton?.getAttribute('aria-haspopup')).toBe('true'); + expect(exportButton?.getAttribute('aria-haspopup')).toBe('menu'); expect(exportButton?.getAttribute('aria-expanded')).toBe('false'); const importButton = container.querySelector('.toolbar__import-button'); expect(importButton?.getAttribute('aria-label')).toBe('Import'); @@ -59,11 +59,10 @@ await new Promise((resolve) => setTimeout(resolve, 0)); - const exportMenu = container.querySelector('.toolbar__export-menu'); - expect(exportMenu?.getAttribute('role')).toBe('menu'); - expect(exportMenu?.getAttribute('aria-label')).toBe('Export options'); + const exportMenu = container.querySelector('[role="menu"][aria-label="Export options"]'); + expect(exportMenu).toBeTruthy(); - const menuItems = container.querySelectorAll('.toolbar__export-menu .toolbar__menu-item'); + const menuItems = exportMenu?.querySelectorAll('[role="menuitem"]') ?? []; expect(menuItems.length).toBe(5); menuItems.forEach((item) => { expect(item.getAttribute('role')).toBe('menuitem'); diff --git a/packages/ui/src/lib/components/BrushPopover.svelte b/packages/ui/src/lib/components/BrushPopover.svelte --- a/packages/ui/src/lib/components/BrushPopover.svelte +++ b/packages/ui/src/lib/components/BrushPopover.svelte @@ -19,11 +19,15 @@