feat: more accessible keyboard nav focus outlines for menu items

[dustinhealy]

Why?

Menu items in Dropdown, Select, and ContextMenu have no visible keyboard focus indicator, violating WCAG SC 2.4.7 (Focus Visible) and SC 1.4.11 (Non-text Contrast). outline: none is hardcoded on GenericMenuItem and the only visual change on keyboard focus is a background shift from #ffffff to #f6f7fa (~1.02:1 contrast), which is effectively invisible and far below the required 3:1.

How?

• Added scoped stroke.focus tokens to genericMenu.item.color in both light (#437eef) and dark (#faff69) themes for default and danger variants
• Created a useInputModality hook that tracks keyboard vs pointer interaction via React event props on the menu container, setting data-input-modality on the element without causing re-renders
• Updated GenericMenuItem CSS to show a 2px inset outline (HCM-compatible) only when the ancestor container has data-input-modality="keyboard", keeping hover unaffected since WCAG does not mandate hover contrast
• Wired useInputModality into DropdownContent, ContextMenuContent, and SelectPopoverContent

Tickets?

• N/A

Contribution checklist?

• You've done enough research before writing
• You have reviewed the PR
• The commit messages are detailed
• The build command runs locally
• Assets or static content are linked and stored in the project
• For documentation, guides or references, you've tested the commands

Security checklist?

• All user inputs are validated and sanitized
• No usage of dangerouslySetInnerHTML
• Sensitive data has been identified and is being protected properly
• Build output contains no secrets or API keys

Preview?

Screen.Recording.2026-03-09.at.6.41.33.AM.mov

Screen.Recording.2026-03-09.at.6.40.38.AM.mov

[changeset-bot]

🦋 Changeset detected

Latest commit: 3cc7ace

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@clickhouse/click-ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

Claude finished @dustinhealy's task in 2m 20s —— View job

---

PR Review: feat: more accessible keyboard nav focus outlines for menu items

Solid a11y improvement — the approach of using a data-input-modality attribute on the container to distinguish keyboard vs pointer is idiomatic and avoids re-renders. Theme tokens are properly structured, and the CSS ancestor selector in the styled-component template is correct.

---

🚨 Must Fix

• Missing changeset: useInputModality is exported from src/hooks/index.ts (public API), and new theme tokens are added to the design system. Per the repo conventions (CONVENTIONS.md), a changeset is required for any public API change or feature. Run yarn changeset:add and add a patch entry describing the new hook and focus tokens. Fix this →

---

🧪 Testing

• useInputModality has no tests: useUpdateEffect has a co-located useUpdateEffect.test.ts — useInputModality should too. At minimum: verify onKeyDownCapture sets data-input-modality="keyboard" for nav keys and ignores non-nav keys, and onPointerMove sets "pointer". Fix this →

---

💡 Suggestions (non-blocking)

• Typeahead not covered by NAV_KEYS: Radix menus support character-key typeahead (e.g., pressing d jumps to "Delete"). Since those keys aren't in NAV_KEYS, the focus ring won't appear when a user navigates via typeahead. Consider broadening the condition to any keydown without metaKey/ctrlKey/altKey, or alternatively listen for Radix's data-highlighted mutations. Low-traffic path but worth noting.

• Space key missing from NAV_KEYS: In some Radix menu variants, Space can open sub-menus or confirm a selection without closing. Adding ' ' (Space) to NAV_KEYS would make the ring visible for those interactions.

• stroke.default added to danger variant in light theme: The diff adds stroke: { default: 'rgba(0, 0, 0, 0)', focus: '#437eef' } to danger in variables.light.ts. The transparent default is fine, but worth double-checking this didn't previously exist (could cause a TypeScript-level type shape divergence if the dark theme had it and the light theme didn't before — which is now fixed, so this is fine).

---

Overall a clean, well-reasoned implementation. The primary blocker is the missing changeset; please add one before merging.

[punkbit]

⚠️ No Changeset found

Latest commit: 88d948c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dustinhealy Thanks for your contribution! Mind add a changeset for these changes, please? https://github.com/clickHouse/click-ui?tab=readme-ov-file#add-a-new-changeset

[Copilot]

Pull request overview

This PR improves WCAG-compliant keyboard focus visibility for menu items across Dropdown, Select, and ContextMenu by introducing theme focus-stroke tokens and an input-modality mechanism that allows CSS to distinguish keyboard focus from pointer hover.

Changes:

• Added stroke.focus tokens under genericMenu.item.color for light/dark themes (default + danger variants).
• Introduced useInputModality hook to set data-input-modality on menu containers without React re-renders.
• Updated GenericMenuItem styling to render an inset 2px outline only for keyboard modality when Radix sets data-highlighted, and wired the modality props into Dropdown/ContextMenu/Select popover content.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/theme/tokens/variables.light.ts	Adds stroke.focus tokens for generic menu items in the light theme.
src/theme/tokens/variables.dark.ts	Adds stroke.focus tokens for generic menu items in the dark theme.
src/hooks/useInputModality.ts	New hook that tracks keyboard vs pointer interaction via event handlers and sets data-input-modality.
src/hooks/index.ts	Exports useInputModality from the hooks barrel.
src/components/Select/common/InternalSelect.tsx	Applies input-modality props to SelectPopoverContent.
src/components/GenericMenu/GenericMenu.tsx	Adds keyboard-only outline styling for highlighted menu items using new focus-stroke tokens.
src/components/Dropdown/Dropdown.tsx	Applies input-modality props to dropdown content container.
src/components/ContextMenu/ContextMenu.tsx	Applies input-modality props to context menu content container.

Comments suppressed due to low confidence (2)

src/components/Dropdown/Dropdown.tsx:127

• ...props is spread after ...inputModalityProps, so any consumer-provided onKeyDownCapture/onPointerMove will override the modality tracking and can silently remove the keyboard-only focus outlines. Consider composing handlers or moving ...props before ...inputModalityProps so the accessibility behavior is preserved.

      <DropdownMenuContent
        {...inputModalityProps}
        $type="dropdown-menu"
        $showArrow={showArrow}
        as={ContentElement}
        sideOffset={4}
        loop
        avoidCollisions={responsivePositioning}
        collisionPadding={responsivePositioning ? 100 : undefined}
        {...props}
      >

src/components/ContextMenu/ContextMenu.tsx:134

• ...props is spread after ...inputModalityProps, so consumer-provided onKeyDownCapture/onPointerMove can override the modality tracking and effectively disable the keyboard-only focus outline behavior. Consider composing the event handlers or changing spread order to ensure modality tracking can’t be accidentally removed.

      <RightMenuContent
        {...inputModalityProps}
        $type="context-menu"
        $showArrow={showArrow}
        as={ContentElement}
        {...props}
      >

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Claude finished @dustinhealy's task in 1m 43s —— View job

---

PR Review: feat: more accessible keyboard nav focus outlines for menu items

Great work — the changeset and tests are now in place, and the overall approach is clean and idiomatic. Here's what I found in the updated code:

---

🚨 Must Fix

• ...props spread overrides modality handlers in Dropdown and ContextMenu: In both DropdownContent (Dropdown.tsx:126) and ContextMenuContent (ContextMenu.tsx:133), {...props} is spread after {...inputModalityProps}. This means any consumer-provided onKeyDownCapture, onPointerMove, onPointerDown, or onFocusCapture will silently disable keyboard focus ring tracking. For a component library this is a real footgun — the a11y behavior needs to be unconditional. Fix: move {...props} before {...inputModalityProps}, or compose handlers explicitly. Note: InternalSelect.tsx doesn't have this issue (only onFocus is set explicitly after the spread, which doesn't conflict with onFocusCapture). Fix this →

---

💡 Suggestions (non-blocking)

• Module-level document.addEventListener is permanent: The three global listeners (keydown, pointerdown, pointermove) registered on document at module load time are never cleaned up. In production this is fine (one set of listeners for the app lifetime), but in test environments they persist across test files and can cause subtle cross-test bleed. If this becomes a problem, consider exporting a __resetModalityForTesting or wrapping the setup in an app-level useEffect that cleans up on unmount.

• Test global state isolation: useInputModality.test.tsx tests that touch lastGlobalModality via fireEvent.keyDown(document, ...) / fireEvent.pointerDown(document, ...) work because the module-level listeners pick them up. This is fine, but note that if test execution order changes, a keyboard test could leave lastGlobalModality='keyboard' and cause an unrelated test to see unexpected state. A beforeEach(() => fireEvent.pointerDown(document)) at the top of the describe block would make the suite fully order-independent.

• CSS selector specificity: [data-input-modality="keyboard"] &[data-highlighted] in the styled-component produces an ancestor selector — this correctly targets items inside a container with the attribute. Just confirming this is intentional and correct. ✅

---

✅ What's good

• Changeset present and accurate
• Tests are comprehensive: nav keys, typeahead characters, Space, bare modifier keys, pointer fallback, and global seed on focus — all covered
• onPointerDown added alongside onPointerMove — correctly handles tap/click without move
• onFocusCapture seeding from lastGlobalModality elegantly solves the "menu opened via keyboard, first item lacks ring" problem
• Theme tokens structured correctly for both light (#437eef) and dark (#faff69) with both default and danger variants
• outline-offset: -2px inset is HCM-compatible

The only blocker is the props spread order in Dropdown and ContextMenu. Once that's fixed, this is ready to merge.

[github-actions]

Claude finished @dustinhealy's task in 1m 17s —— View job

---

PR Review: feat: more accessible keyboard nav focus outlines for menu items

Great a11y work — the architecture is clean, the global modality seeding elegantly handles the "menu opened via keyboard" case, and the tests are thorough. One blocker remains from the previous review round.

---

🚨 Must Fix

• ...props spread order still overrides modality handlers in both Dropdown.tsx and ContextMenu.tsx. This was flagged in the prior review cycle and is still present in the latest push:

  Dropdown.tsx:117-127 — {...inputModalityProps} is spread first, then {...props} last, meaning any consumer-provided onKeyDownCapture, onPointerMove, onPointerDown, or onFocusCapture will silently disable keyboard focus ring tracking.

  // Current (broken — {...props} wins):
  <DropdownMenuContent
    {...inputModalityProps}
    ...
    {...props}   // ← overrides modality handlers
  >
  
  // Fix — swap order:
  <DropdownMenuContent
    {...props}   // ← consumer props first
    ...
    {...inputModalityProps}  // ← a11y handlers always win
  >

  Same pattern in ContextMenu.tsx:128-134. InternalSelect.tsx is fine (no conflicting capture handlers). Fix this →

---

✅ What's good

• onPointerDown added alongside onPointerMove — tap/click without movement now correctly resets to pointer modality
• onFocusCapture seeding from lastGlobalModality — first highlighted item gets the ring immediately when menu opens via keyboard
• All keys (nav keys, typeahead characters, Space) correctly trigger keyboard modality; bare modifier keys are excluded
• Tests are comprehensive and cover all edge cases including global state seeding
• Theme tokens properly structured for both light/dark with default and danger variants
• outline-offset: -2px inset is HCM-compatible
• Changeset present and accurate

---

💡 Suggestions (non-blocking)

• Test isolation: useInputModality.test.tsx uses fireEvent.keyDown(document, ...) / fireEvent.pointerDown(document, ...) which mutate the module-level lastGlobalModality. Tests are currently ordered safely, but adding beforeEach(() => fireEvent.pointerDown(document)) at the top of the describe block would make the suite order-independent.

• Global listeners persist in test environments: The three document.addEventListener calls at module load are never cleaned up, which can cause cross-test-file bleed. Not urgent for production, but worth noting for future test reliability.

Once the spread order is fixed in Dropdown and ContextMenu, this is ready to merge.

[github-actions]

Claude finished @dustinhealy's task in 1m 11s —— View job

---

PR Review: feat: more accessible keyboard nav focus outlines for menu items

Great a11y work — the spread order fix is in place, the implementation is clean, and the tests are comprehensive. Here's my final assessment:

---

✅ Previously flagged blocker: resolved

The {...props} spread order is now correct in both components:

• Dropdown.tsx:118 — {...props} spreads first, {...inputModalityProps} spreads last (line 126) ✅
• ContextMenu.tsx:129 — same pattern ✅

Consumer-provided handlers can no longer silently override the modality tracking behavior.

---

✅ What's good

• onFocusCapture seeding from lastGlobalModality — first highlighted item gets the ring immediately when menu opens via keyboard
• onPointerDown alongside onPointerMove — tap/click without movement correctly resets to pointer modality
• [data-input-modality="keyboard"] &[data-highlighted] ancestor selector in GenericMenu.tsx:103 — correctly scoped, doesn't affect hover
• outline-offset: -2px inset — HCM-compatible
• Theme tokens for both light (#437eef) and dark (#faff69) with default and danger variants
• Changeset present and accurate
• Tests cover nav keys, typeahead characters, Space, bare modifier keys, pointer fallback, and global state seeding

---

💡 Suggestions (non-blocking)

• Test isolation: useInputModality.test.tsx fires keyDown/pointerDown on document which mutates module-level lastGlobalModality. Tests are currently ordered safely, but a beforeEach(() => fireEvent.pointerDown(document)) at the top of the describe block would make the suite fully order-independent.

• Global listeners in test environments: The three document.addEventListener calls at module load (line 26–48 of useInputModality.ts) are never cleaned up. Fine for production, but they persist across test files. Not urgent, but worth noting for future test reliability.

---

This is ready to merge. 🙌

[dustinhealy]

@punkbit Comments all addressed + Changeset added. Ready for your review

[github-actions]

Claude finished @dustinhealy's task in 1m 10s —— View job

---

PR Review: feat: more accessible keyboard nav focus outlines for menu items

Solid a11y improvement — the implementation is clean, well-tested, and all previously flagged blockers have been resolved. Here's the current state:

---

✅ All blockers resolved

• Spread order (Dropdown.tsx:122,130 / ContextMenu.tsx:136,140): {...props} is now correctly spread before {...inputModalityProps}, so consumer-provided handlers can't silently override modality tracking ✅
• Changeset (.changeset/kind-menus-focus.md): Present and accurate ✅
• Tests (useInputModality.test.tsx): Comprehensive — covers nav keys, typeahead characters, Space, bare modifier keys, pointer fallback, and global state seeding ✅

---

✅ What's good

• onFocusCapture seeding from lastGlobalModality elegantly handles the "menu opened via keyboard, first item lacks ring" case
• onPointerDown alongside onPointerMove — tap/click without pointer movement correctly resets to pointer modality
• [data-input-modality="keyboard"] &[data-highlighted] ancestor CSS selector is correctly scoped — doesn't affect hover, works through Radix portals
• outline-offset: -2px inset outline is HCM-compatible
• Theme tokens properly structured for both light (#437eef) and dark (#faff69) with default and danger variants
• InternalSelect.tsx correctly spreads {...inputModalityProps} before onFocus — no conflict since the hook uses onFocusCapture (capture phase)

---

💡 Suggestions (non-blocking)

• Test suite order-dependence: Tests that fire fireEvent.keyDown(document, ...) / fireEvent.pointerDown(document, ...) mutate the module-level lastGlobalModality. Currently ordered safely, but a beforeEach(() => fireEvent.pointerDown(document)) at the top of the describe block would make the suite fully order-independent.

• Global listeners in test environments: The three document.addEventListener calls at module load (useInputModality.ts:26–48) are never cleaned up and persist across test files. Fine for production, but worth noting for future test reliability if flakiness is observed.

---

This is ready to merge. Nice work on a well-reasoned, complete a11y fix! 🙌

[workflow-authentication-public]

📚 Storybook Preview Deployed

✅ Preview URL: https://click-d6beokm1z-clickhouse.vercel.app

Built from commit: dc9d7558432ac9c9c5d57fcff0826b7860ea3c5f

[punkbit]

@dustinhealy I missed this one. For tokens, we have to update them in the Figma file. The process will be revised soon to avoid confusion, but just for your interest, as it seems these values were hard typed, so lost on the latest.

[dustinhealy]

Good to know - thanks Helder. In the meantime, feel free to let me know if I need to make any changes / open a new PR to account for this or anything else.

[punkbit]

@dustinhealy Not really, did a quick tweak for now #810 (started migrating out from styled components). Once I get the semantic versions for the tokens to reduce the file sizes and make it simpler, I'll tag you. Thanks!