feat(design-system): add shared contextual menus - #89
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head 57dd36eab159367c27063c8632930d88fd6463c9 against base a9194b24a3cc208abf6f9a9a05eed77583e6c0fa.
Changes requested
P2: Preserve the design system’s keyboard-only focus ring on menu items.
components.css:718–732 unconditionally sets outline: none on every item wrapper and only adds the same data-highlighted fill used for pointer navigation. The popup’s data-buzz-ui boundary excludes its descendants from the host’s legacy focus outline, and neither host nor viewer supplies a replacement menu-item ring. This loses the focused-control ring explicitly required by the existing design contract and component rules.
Reproduction path for verification: in the Menu specimen, Tab to More actions, open with ArrowDown, navigate the action/checkbox items, then enter Sort and navigate the radio items. Base UI moves focus, but the new CSS suppresses its outline; only the shared hover/highlight background remains. This is a source-established styling-contract defect, not a claim that the existing highlight is invisible or that navigation itself is broken. I did not execute this browser path.
Smallest fix / exit criteria: add a visible item focus ring gated by html[data-keyboard-navigation] and :focus-visible, retaining quiet pointer focus and Base UI’s interaction ownership. Add a focused regression proving the keyboard ring and pointer suppression on the composed menu, including a submenu item, in light/dark mode. No broader menu framework or sidebar integration is required to address this finding.
Scope and evidence
- This is a shared-menu prerequisite. Production sidebar behavior, persistence and consumer integration are not delivered by these four files and are not requested as fixes here. API/prop composition, context-menu primitive reuse, controlled choice callbacks, portal styling boundaries and shared state attributes were source-reviewed against pinned Base UI 1.7.0.
- All 12 existing head-bound hosted checks passed. The JavaScript job passed 147 Vitest files / 1,552 tests. Its synthetic merge
d9beaa683d38b04989effa57a60133fbe3bc4ccehas the pinned base/head as parents and the same source tree as this reviewed head. - Those checks do not prove this new menu’s interactions: the specimen/registry checks do not open it. Review was source-only plus existing hosted evidence, with no checkout, build, test execution or CI rerun. Rendered collision/layering and live consumer acceptance remain unverified; no speculative overlay defect is asserted.
Add Base UI menu and context-menu primitives, shared floating styles, and the component registry/specimen entry. Signed-off-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Show keyboard-only item focus with the existing modality owner and keep the portal positioner above dialogs. Cover actions, disabled choices, context menus, submenu focus, collision boundaries and dialog hit testing without adding interaction owners. Signed-off-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
57dd36e to
bb87067
Compare
|
Implemented by Carl (AI agent), posted through the operator’s account at their direction. The missing keyboard-only item ring and menu portal stacking were blockers. Commit Both defects failed in Chromium and WebKit before their fixes and pass afterward. Validation: full affected viewer file 42/42 executions, full design unit suite 12 files / 72 tests, and mandatory hooks passed. Hosted CI and DCO now pass at @wesbillman — ready for re-review. No merge or readiness-state change. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head bb8706730b4f6eef56c5e82d77d02706ce79fd6c against base 298a50a9861c19d33e45b2b52bdadb81b128f15d.
Review clear
The prior keyboard-focus blocker is resolved; no remaining actionable defect was found in this bounded shared-menu re-review. This is a comment, not approval or merge authorization.
- The item ring requires the existing host keyboard-modality attribute and
:focus-visible, with an inset outline to avoid popup clipping. The browser regression covers pointer suppression and keyboard focus through action, checkbox, submenu and radio items in both themes, plus Escape focus return. - Thin wrappers retain Base UI ownership of activation, controlled choices, disabled semantics, positioning and dismissal. The popover layer now belongs to the actual portal positioner. Mounted tests cover callbacks/state; the additional browser case covers edge collision and unforced pointer hit testing above the existing Dialog. No browser cases were removed. Production sidebar behavior, persistence and native consumer integration remain outside this prerequisite.
- Hosted CI run 35775430224 succeeded for this head: 11 successful checks, optional Windows validation skipped. The JavaScript job used synthetic merge
6312cd1b86338c647ef3c544817a49d6fd5dc6a5of the pinned head/base and passed 200 files / 2,122 tests, including all three new Menu tests. The standalone viewer suite is not part of those hosted app-browser jobs. Its reported 42/42 Chromium/WebKit pass and fail-then-pass focus/layering evidence come from the PR author’s documented local final-tree run, not an independent execution by this review.
Source-only review on the pinned bare objects, with independent focus/producer and composition lanes. No checkout, dependency installation, code execution or CI rerun. Live/native acceptance remains unverified. Shared-host modality-policy expansion and speculative extra test matrices are not exit criteria for this fix.
Overview
Category: infrastructure
User Impact: Shared contextual menus now have accessible keyboard focus and remain usable near viewport edges and over dialogs; the Messages sidebar is unchanged.
Problem: Contextual actions need one accessible, viewport-aware menu primitive rather than feature-owned interaction machinery. The original extraction lacked a keyboard-only item ring and its popup layer could sit behind a dialog.
Solution: Keep Base UI responsible for interaction, focus, positioning and dismissal while Buzz supplies the shared appearance. Refresh the extraction to current-main icon/material/token contracts and apply the popover layer to the positioner that owns the stacking context.
Changes
File changes
src/shared/design-system/ui/Menu.tsx
Thin menu/context-menu wrappers for actions, links, choices and nested submenus. Uses the Phosphor gateway and a shared positioner class; no new interaction owner.
src/shared/design-system/styles/components.css
Shared floating surface, semantic colors, choice/disabled states, viewport bounds and reduced motion. Adds an inset item ring gated by the existing keyboard-modality attribute and
:focus-visible; puts--layer-popoveron the portal positioner.src/shared/design-system/ui/registry.ts
Registers the proposed menu component and its Base UI backing parts.
src/shared/design-system/ui/Menu.test.tsx
Three mounted-component tests cover action callbacks/dismissal, link semantics, disabled pointer/keyboard activation, controlled checkbox/radio selection and context-menu composition using real Base UI.
tests/fixtures/design-system/ui/componentSpecimens.tsx
Shows action/choice/submenu composition, a context menu and a menu inside the existing Dialog. Labels and selection are specimen-only; they do not implement sidebar sorting, notifications or read behavior.
tests/fixtures/design-system/viewer.spec.ts
Adds two real-browser cases for keyboard-versus-pointer ring styling, nested focus/Escape return, edge collision geometry and pointer hit testing above Dialog.
Reproduction Steps
bin/just design) and open Menu.Validation and limits
Refreshed onto main
298a50a9861c19d33e45b2b52bdadb81b128f15d; pushed headbb8706730b4f6eef56c5e82d77d02706ce79fd6c(two DCO-audited commits).bin/pnpm design:testat clean final head: 12 files / 72 tests passed, 1.40s wall / 1.19s summed test execution, including three new Menu tests.bin/pnpm exec playwright test --config tests/fixtures/design-system/playwright.config.ts viewer.spec.ts— 42/42 passed (21 cases in Chromium and WebKit), macOS, one worker, 36.1s wall. This ran on02d270bplus the final pending changes, committed unchanged asbb87067; pre-commit made no edits. New cases took 3.1/3.0s (focus) and 3.7/3.7s (geometry/stacking) in Chromium/WebKit.:focus-visible/input-modality styling, actual portal focus transfer, collision geometry and pointer hit testing. Callback/disabled/selection matrices stay in mounted-component tests; no coverage was moved or deleted.outline-style: noneat the keyboard action assertion; with the fix both pass. Without the positioner-layer fix, both engines rejected action clicks because Dialog/backdrop intercepted the pointer; with the fix both pass. Missing browser binaries and corrected test assumptions are setup/test issues, not product regression evidence.git diff --checkreviewed against fetched main: six menu-owned source/test/registry/specimen files only; no foundation, dependency, host modality or feature-behavior edits.Hosted snapshot at the new head: DCO Check passed; CI run 35775430224 was still running (JavaScript, Rust/tool integration, measurements and all four browser shards). Optional Windows validation was skipped. The September 17 results do not validate this head. The standalone design viewer browser suite is separate from the app journey CI configuration. No broad local native scan was run. Production consumer wiring, native GUI acceptance and actual-product screenshots remain deferred to consumer slices; no specimen screenshot is presented as production acceptance.
The PR remains open and non-draft; this update does not change readiness state or authorize merge. Re-review of the outstanding requested change is required.
Implemented by Carl (AI agent), at the operator's request.
Screenshot example
Actual rendered shared Menu from the built design-system viewer at
bb87067, dark mode, with a nested radio-choice menu and its keyboard focus ring visible. This is a design-system example, not production sidebar wiring; the example labels do not add sorting, read or notification behavior to the app. Captured by Carl (AI agent); only cropped, not composited or restyled.