Skip to content

fix: adopt the shared menu keyboard contract in ChannelPage (#6269) - #6547

Merged
bolichen97 merged 1 commit into
mainfrom
fix/channelpage-menu-keyboard-6269
Aug 28, 2026
Merged

fix: adopt the shared menu keyboard contract in ChannelPage (#6269)#6547
bolichen97 merged 1 commit into
mainfrom
fix/channelpage-menu-keyboard-6269

Conversation

@NicholasRBowers

Copy link
Copy Markdown
Contributor

Problem / Motivation

The agents-panel listen-mode dropdown in ChannelPage.tsx declares role="menu" — which promises the WAI-ARIA menu keyboard contract (arrow-key row navigation, Home/End, Tab containment) — but wired no keyboard handling at all. The rows were reachable only by mouse, and a screen-reader user who was just told "menu" got nothing from the arrow keys they reach for first. This is the defect class #6231 fixed on its five inventoried surfaces via the shared useMenuKeyboard hook (PR #6267); ChannelPage was outside that inventory. Filed by the First Principles review lane on PR #6267 as the ChannelPage analogue of #6266.

Why it matters

A keyboard-only or assistive-technology user cannot change an agent's listen mode at all: the trigger opens the menu, but focus never enters it, arrows do nothing, and Tab walks out behind the still-open menu. The current mode was also conveyed only by colour, which assistive tech cannot perceive.

What changed (motivation → approach → change)

Symptom: role="menu" with zero keyboard wiring. Root cause: the surface predates the shared hook and was outside #6231's inventory. Change — a mechanical adoption of the merged hook, mirroring the closest #6267 surface (MicSourceMenu):

  • Attach a dedicated ref to the role="menu" element (narrower than the wrapper ref, so item discovery never picks up the trigger) and wire useMenuKeyboard({ enabled: menu, containerRef }): arrow walk with wrap, Home/End, Tab containment, document IME latch.
  • Focus entry is host-owned (focusFirstOnOpen: false + focus({ preventScroll: true })): this menu is not portalled and sits inside the agents rail's scroll container, so the hook's default entry would scroll the rail on every open, shifting the row the user just clicked out from under the pointer.
  • Rows become role="menuitemradio" with aria-checked, and the menu gets an accessible name (aria-label, new catalog key pages.channelPage.listen_mode, translated in all 11 locales + regenerated pseudolocale) so the announced radio state has a referent.
  • Focus restores to the trigger on explicit dismissal (Escape / row activation), matching the MicSourceMenu posture from fix: honour the role="menu" arrow-key contract on all five sibling menu surfaces #6267; outside-click dismissal is deliberately left alone (the browser routes focus per the click target).

Deliberately NOT touched, per the issue's own scope guard: the deferred adoptions the issue inventories (WindowsTitlebarMenu, SessionFlyout, SlotTagPopover, BusySendButton, MarkdownPanel) each need their own judgment.

Tests

New website/src/test/ChannelPage.menuKeyboard.test.tsx (9 tests, 8 verified red before the wiring):

  • focus enters the menu on open, landing on the first row
  • ArrowDown walks the rows and wraps past the last; ArrowUp wraps past the first
  • Home/End jump to the boundary rows
  • Tab and Shift-Tab are contained inside the open menu
  • Escape closes and hands focus back to the trigger
  • picking a row reports upstream (channelUpdateAgent) and hands focus back to the trigger
  • rows are menuitemradio with the current mode aria-checked
  • the menu carries its accessible name ("Listen mode")
  • a mousedown in the composer dismisses the menu without stealing focus to the trigger (pins the host's outside-mousedown dismissal shape, which is what keeps the hook's one-open-menu precondition true)

Manual verification

N/A — unit coverage sufficient: the keyboard contract is real-DOM-focus asserted per keystroke, and the hook itself carries its own suite (useMenuKeyboard.test.tsx). Focus-trace evidence below in place of screenshots.

Screenshots / video

Why no screenshot: keyboard focus behaviour and non-rendering ARIA attributes only — no pixel renders differently, so a still frame cannot show the delta. Evidence is the focus-trace assertion dump (real DOM focus moves asserted per keystroke), same posture as PR #6267:

✓ ChannelPage — listen-mode menu keyboard contract > moves focus into the menu when it opens
✓ ChannelPage — listen-mode menu keyboard contract > walks the rows with ArrowDown and wraps past the last one
✓ ChannelPage — listen-mode menu keyboard contract > walks the rows with ArrowUp and wraps past the first one
✓ ChannelPage — listen-mode menu keyboard contract > jumps to the boundary rows with Home and End
✓ ChannelPage — listen-mode menu keyboard contract > contains Tab and Shift-Tab inside the open menu
✓ ChannelPage — listen-mode menu keyboard contract > closes on Escape and hands focus back to the trigger
✓ ChannelPage — listen-mode menu keyboard contract > hands focus back to the trigger after picking a row
✓ ChannelPage — listen-mode menu keyboard contract > marks the rows as menuitemradio with the current mode checked
✓ ChannelPage — listen-mode menu keyboard contract > names the menu so the radio group has a referent
✓ ChannelPage — listen-mode menu keyboard contract > dismisses on a mousedown in the composer without stealing focus

Related Issues

Fixes #6269

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, no doc surface
  • No secrets, credentials, or internal references in the diff

The agents-panel listen-mode dropdown declares role="menu" — which
promises WAI-ARIA arrow-key item navigation — but wired no keyboard
handling at all: the defect class #6231 fixed on its five inventoried
surfaces via the shared useMenuKeyboard hook; ChannelPage was outside
that inventory (#6269, the ChannelPage analogue of #6266).

Wire the menu onto the merged hook (arrow walk with wrap, Home/End,
Tab containment, document IME latch), mark the rows role=menuitemradio
with aria-checked so the current mode is perceivable programmatically
rather than by colour alone, name the group (aria-label, new catalog
key) so the announced radio state has a referent, and restore focus to
the trigger on explicit dismissal (Escape / row activation), matching
the MicSourceMenu posture from #6267. Outside-click dismissal is left
alone, per the same posture. Focus entry is host-owned with
preventScroll: this menu is not portalled and lives inside the agents
rail's scroll container, so the hook's default entry would scroll the
rail on every open.

Co-authored-by: Kiro Crew <kirocrew@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of bd9f81896fb2a802c54b57563021a2cc82643eeb — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The diff is a scoped keyboard-contract adoption: shared useMenuKeyboard hook wired onto the listen-mode menu, menuitemradio/aria-checked on the rows, an accessible name ("Listen mode", translated in all locales), and focus restore to the trigger on Escape and row activation. The only new user-visible string is the aria-label "Listen mode", which reads clearly cold. Focus-entry uses preventScroll to avoid yanking the agents rail under the pointer, and outside-click dismissal deliberately leaves focus routing to the browser, matching the sibling MicSourceMenu posture. No visual pixels change; the current mode remains visible as text in the trigger badge, so no color-only state gap is introduced. Tests assert each keystroke against real DOM focus. Nothing here degrades comprehension, flow, feedback, or control — it closes a real keyboard/AT gap consistently with the established pattern.

UX-Verdict: PASS

Keyboard and screen-reader users can now actually operate the listen-mode menu, wired identically to the established sibling surfaces — no new copy, flow, or focus risks.

[UX-REVIEWED] bd9f818

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of bd9f81896fb2a802c54b57563021a2cc82643eeb — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Mechanical adoption of the established useMenuKeyboard contract on the one surface #6231's inventory missed — right hook, right layer, scope deliberately narrow, tests pin the contract.

[DESIGN-REVIEWED] bd9f818

@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of bd9f81896fb2a802c54b57563021a2cc82643eeb and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] bd9f818

False positive or not applicable? A repository writer can comment:
/ai-review override gpt bd9f81896fb2a802c54b57563021a2cc82643eeb: <one-sentence reason>

@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed bd9f81896fb2a802c54b57563021a2cc82643eeb — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] bd9f818

Verdict parsed from the review's SHA-scoped output markers for commit bd9f81896fb2a802c54b57563021a2cc82643eeb.

False positive or not applicable? A repository writer can comment:
/ai-review override fable bd9f81896fb2a802c54b57563021a2cc82643eeb: <one-sentence reason>

@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of bd9f81896fb2a802c54b57563021a2cc82643eeb — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks complete. The change is a faithful adoption of the shared hook; the one undeclared bit is two ARIA attributes on the trigger, and the five unadopted sibling menus are exactly the set the description declares deferred. Final review:

First-Principles-Verdict: PASS

A declared symptom-class fix landed at cause level: the shared menu contract adopted, not re-spelled, with every divergence from the reference surface named and derived.

What this change ships

Intent: let a keyboard or screen-reader user actually operate the agents-panel listen-mode menu — a FIX.

  1. Arrow keys walk the listen-mode rows and wrap at both ends — justified (the reported defect)
  2. Home/End jump to the boundary rows — justified
  3. Tab and Shift-Tab stay inside the open menu — justified
  4. Focus lands on the first row on open, without scrolling the rail — justified; host-owned entry is a hook-sanctioned posture, preventScroll derived from the non-portalled scroll container
  5. Escape and row activation hand focus back to the trigger — justified; hook doc names this posture (MicSourceMenu/SlotPopover)
  6. Rows announced as radio options with the current mode checked — justified (state was colour-only)
  7. Menu announced as "Listen mode"; new catalog key in 11 locales + pseudolocale — justified; i18n catalog is a documented invariant
  8. Trigger badge now announces it opens a menu and its open state (aria-haspopup/aria-expanded) — undeclared, rides along; same defect class, zero added surface
  9. Five sibling role="menu" surfaces stay unadopted — declared, accepted-and-deferred

Watch

  • Item 8 is the only diff content the description never mentions. Counted role="menu" in website/src excluding tests: the 5 remaining non-hook surfaces (WindowsTitlebarMenu, MarkdownPanel, BusySendButton, SlotTagPopover, SessionFlyout) match the deferred list verbatim, so no hidden sibling.

[FIRST-PRINCIPLES-REVIEWED] bd9f818

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 28, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) August 28, 2026 22:11
@bolichen97
bolichen97 merged commit 09c32bc into main Aug 28, 2026
65 checks passed
@bolichen97
bolichen97 deleted the fix/channelpage-menu-keyboard-6269 branch August 28, 2026 22:28
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 28, 2026
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.

ChannelPage role="menu" has no keyboard handling — same defect class as #6231, not in its five-surface inventory

2 participants