Make organization switch breadcrumb trigger keyboard accessible#10501
Make organization switch breadcrumb trigger keyboard accessible#10501jscervantes wants to merge 1 commit into
Conversation
|
|
📝 WalkthroughWalkthroughThis PR adds keyboard accessibility to the organization-switch breadcrumb trigger by including role="button", aria-haspopup attribute, and an onKeyDown handler that activates the trigger on Enter or Space key press, along with a corresponding changeset. ChangesBreadcrumb Accessibility Fix
Sequence Diagram(s)sequenceDiagram
participant User
participant BreadcrumbTrigger
User->>BreadcrumbTrigger: Press Enter/Space
BreadcrumbTrigger->>BreadcrumbTrigger: preventDefault()
BreadcrumbTrigger->>BreadcrumbTrigger: trigger onClick
Related Issues: Not specified in the provided context. Related PRs: Not specified in the provided context. Suggested labels: accessibility, a11y, patch Suggested reviewers: Not specified in the provided context. Poem: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/admin.organizations.v1/components/organization-switch/organization-switch-breadcrumb.tsx (1)
490-516: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSplit the breadcrumb trigger from the breadcrumb actions
generateBreadcrumb()renders clickableBreadcrumb.Sections and aDropdown/Dropdown.Iteminside a container withrole="button"andtabIndex={ 0 }. That makes the wrapper present as a single button while still containing separate org-switch controls. Move the toggle behavior to a dedicated control or remove the outer button semantics around the interactive breadcrumb content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.organizations.v1/components/organization-switch/organization-switch-breadcrumb.tsx` around lines 490 - 516, The breadcrumb trigger is currently wrapped in a single button-like container while generateBreadcrumb() still renders its own clickable Breadcrumb.Section and Dropdown/Dropdown.Item controls, creating nested interactive behavior. Refactor organization-switch-breadcrumb.tsx so the outer organization-breadcrumb wrapper no longer uses button semantics for the full breadcrumb area; instead, move the dropdown toggle to a dedicated control or keep the wrapper purely presentational and let generateBreadcrumb handle its own interactions.
🧹 Nitpick comments (1)
features/admin.organizations.v1/components/organization-switch/organization-switch-breadcrumb.tsx (1)
498-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: activation via
currentTarget.click()duplicates intent instead of calling the handler directly.Simulating a click via
.click()works but couples the keyboard path to DOM click dispatch rather than invoking the shared toggle logic directly. Extracting a named handler and calling it from bothonClickandonKeyDownwould be more explicit and avoid relying on synthetic click semantics.♻️ Optional refactor
+ const toggleDropdown = (): void => setIsDropDownOpen(!isDropDownOpen); + <div role="button" aria-haspopup="true" tabIndex={ 0 } onBlur={ () => setIsDropDownOpen(false) } className="organization-breadcrumb" - onClick={ () => setIsDropDownOpen(!isDropDownOpen) } + onClick={ toggleDropdown } onKeyDown={ (e: KeyboardEvent<HTMLDivElement>): void => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); - e.currentTarget.click(); + toggleDropdown(); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/admin.organizations.v1/components/organization-switch/organization-switch-breadcrumb.tsx` around lines 498 - 503, The keyboard activation in organization-switch-breadcrumb currently calls e.currentTarget.click(), which duplicates intent through DOM click dispatch instead of reusing the shared toggle logic. Extract a named handler for the toggle action in organization-switch-breadcrumb and invoke it from both onClick and onKeyDown, using the existing keyboard check for Enter/Space in the onKeyDown path. Keep the handler close to the current interactive element so the behavior stays centralized and explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@features/admin.organizations.v1/components/organization-switch/organization-switch-breadcrumb.tsx`:
- Around line 490-516: The breadcrumb trigger is currently wrapped in a single
button-like container while generateBreadcrumb() still renders its own clickable
Breadcrumb.Section and Dropdown/Dropdown.Item controls, creating nested
interactive behavior. Refactor organization-switch-breadcrumb.tsx so the outer
organization-breadcrumb wrapper no longer uses button semantics for the full
breadcrumb area; instead, move the dropdown toggle to a dedicated control or
keep the wrapper purely presentational and let generateBreadcrumb handle its own
interactions.
---
Nitpick comments:
In
`@features/admin.organizations.v1/components/organization-switch/organization-switch-breadcrumb.tsx`:
- Around line 498-503: The keyboard activation in organization-switch-breadcrumb
currently calls e.currentTarget.click(), which duplicates intent through DOM
click dispatch instead of reusing the shared toggle logic. Extract a named
handler for the toggle action in organization-switch-breadcrumb and invoke it
from both onClick and onKeyDown, using the existing keyboard check for
Enter/Space in the onKeyDown path. Keep the handler close to the current
interactive element so the behavior stays centralized and explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 03306dea-f4ec-4160-8c9a-e79a6cab147a
📒 Files selected for processing (2)
.changeset/a11y-org-breadcrumb-tabindex.mdfeatures/admin.organizations.v1/components/organization-switch/organization-switch-breadcrumb.tsx
2 participants
Purpose
Fix the
jsx-a11y/no-noninteractive-tabindexaccessibility warning (reported by React Doctor) onthe organization-switch breadcrumb trigger in
features/admin.organizations.v1/components/organization-switch/organization-switch-breadcrumb.tsx.The trigger was a non-interactive
<div>carryingtabIndex={ 0 }and anonClickhandler, but nointeractive
roleand no keyboard handler. As a result it was reachable by Tab yet exposed no roleor keyboard behavior to assistive technology, and was effectively mouse-only (Enter/Space did
nothing once focused).
Rather than simply removing
tabIndex, which would silence the linter but make the controlunreachable by keyboard (a regression), this PR turns the element into a legitimate dropdown
trigger:
role="button"so thetabIndexis justified and screen readers announce it as a control.aria-haspopup="true"so assistive tech identifies it as a dropdown trigger.onKeyDownhandler that activates on Enter/Space viae.currentTarget.click(), sokeyboard activation follows the same event path as a mouse click and reaches the
TenantDropdownthat owns the real menu open/close state.
aria-expandedis intentionally omitted: the breadcrumb's localisDropDownOpenstate only drivesthe chevron icon and can drift from the actual
TenantDropdownmenu state, so bindingaria-expandedto it could announce a stale "expanded" state. This follows the existing keyboardbutton pattern in
features/common.workflow-approvals.v1/pages/approvals.tsx.Note: the repo's ESLint config loads jsx-a11y but doesn't enable its rules, so pnpm lint won't flag this. Reproduced/verified by running the single rule in isolation against the file (0 warnings after the fix).
Related Issues
tabIndexto non-interactive elements — keyboard users would have ... (1 occurrence) product-is#27930Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.