Skip to content

feat(design-system): add shared contextual menus - #89

Merged
wesbillman merged 2 commits into
mainfrom
tho/sidebar-menus
Sep 22, 2026
Merged

wesbillman merged 2 commits into
mainfrom
tho/sidebar-menus

Conversation

@tellaho

@tellaho tellaho commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

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-popover on 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

  1. Start the design-system viewer (bin/just design) and open Menu.
  2. In light and dark modes, open More actions with the pointer: focused items must not acquire a keyboard ring. Use Tab/ArrowDown instead and navigate the action, checkbox and Sort submenu: the focused item must have a visible ring.
  3. Press Escape in the submenu and again in its parent; focus returns through the submenu trigger to More actions.
  4. Right-click Context actions and exercise the action/disabled item. Check constrained windows; automated coverage places real triggers at viewport collision boundaries at 390, 800 and 1280px.
  5. Open menu dialog, open its More actions menu and click the action. It must receive the click above the dialog, dismiss, and return focus without dismissing the dialog.
  6. The production sidebar is unchanged. Grouping/Star, sorting, mute/read and lifecycle behavior remain out of scope.

Validation and limits

Refreshed onto main 298a50a9861c19d33e45b2b52bdadb81b128f15d; pushed head bb8706730b4f6eef56c5e82d77d02706ce79fd6c (two DCO-audited commits).

  • Full bin/pnpm design:test at clean final head: 12 files / 72 tests passed, 1.40s wall / 1.19s summed test execution, including three new Menu tests.
  • Mandatory pre-commit formatting/lint/icon checks passed with no fixes. Mandatory pre-push passed application TypeScript, 5 related test files / 17 tests, design TypeScript and all design guards.
  • Application and design builds passed during implementation. Design build retains the existing large-chunk warning; no dependency or bundle restructuring.
  • Full affected browser file: bin/pnpm exec playwright test --config tests/fixtures/design-system/playwright.config.ts viewer.spec.ts42/42 passed (21 cases in Chromium and WebKit), macOS, one worker, 36.1s wall. This ran on 02d270b plus the final pending changes, committed unchanged as bb87067; pre-commit made no edits. New cases took 3.1/3.0s (focus) and 3.7/3.7s (geometry/stacking) in Chromium/WebKit.
  • Browser cases added: 2; removed: 0. Browser-only justification: computed :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.
  • Fail then pass: without the focus fix, both engines received outline-style: none at 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.
  • Full PR diff and git diff --check reviewed 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.

Dark-mode shared menu example with action, checkbox, nested radio choices, disabled action and visible keyboard focus

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 d9beaa683d38b04989effa57a60133fbe3bc4cce has 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.

Carl added 2 commits September 22, 2026 12:27
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>
@tellaho

tellaho commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

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 bb8706730b4f6eef56c5e82d77d02706ce79fd6c adds the ring through the existing modality owner and puts the shared popover layer on the Base UI positioner, so dialogs cannot intercept menu clicks. The branch is refreshed onto 298a50a9 with current icon/material/token contracts; sidebar feature behavior remains excluded.

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 bb87067; optional Windows validation was skipped.

@wesbillman — ready for re-review. No merge or readiness-state change.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 6312cd1b86338c647ef3c544817a49d6fd5dc6a5 of 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.

@wesbillman
wesbillman merged commit c5762f4 into main Sep 22, 2026
12 checks passed
@wesbillman
wesbillman deleted the tho/sidebar-menus branch September 22, 2026 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants