feat: add keyboard navigation support to the EntityActions component#3656
feat: add keyboard navigation support to the EntityActions component#3656kasya merged 3 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds keyboard navigation and focus management to the EntityActions dropdown (Escape, ArrowUp/Down, Enter/Space), including trigger and menu-item refs, focusIndex state, outside-click handling, and an extensive Jest test suite validating keyboard interactions, focus behavior, activation, tabIndex management, and edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/EntityActions.test.tsx`:
- Around line 705-718: The test title is misleading: the spec inside the it
block for EntityActions uses editButton and asserts it has focus, so rename the
test description string (the it(...) first arg) to reflect that it validates
Enter when the first item is focused (e.g., "handles Enter key when first item
is focused" or similar) instead of "when no item is focused"; locate the it
block that references EntityActions, button/menu/editButton and mockPush and
update only the title text to match the actual assertions.
🧹 Nitpick comments (3)
frontend/src/components/EntityActions.tsx (2)
146-156: Consider addingaria-controlsfor complete WCAG compliance.The trigger button has
aria-expandedandaria-haspopup, butaria-controlsis recommended for associating the button with the menu element it controls. This helps assistive technologies understand the relationship.♿ Proposed enhancement
<button ref={triggerButtonRef} type="button" onClick={handleToggle} className="cursor-pointer rounded px-4 py-2 hover:bg-gray-200 dark:hover:bg-gray-700" aria-label={`${type === 'program' ? 'Program' : 'Module'} actions menu`} aria-expanded={dropdownOpen} aria-haspopup="true" + aria-controls={dropdownOpen ? 'entity-actions-menu' : undefined} >Then add the
idto the menu container:<div className="absolute right-0 z-20 mt-2 w-40 rounded-md border border-gray-200 bg-white shadow-lg dark:border-gray-700 dark:bg-gray-800" onKeyDown={handleKeyDown} role="menu" tabIndex={dropdownOpen ? 0 : -1} + id="entity-actions-menu" >Based on learnings: "When implementing dropdown menus, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls)."
164-177: Consider clearing stale refs when options change.The
menuItemsRefarray accumulates refs but is never trimmed. Ifoptions.lengthdecreases (e.g., status change removes menu items), stale refs may remain at higher indices. While unlikely to cause issues given the current logic, clearing the array when options change would be more robust.🧹 Proposed enhancement
Add a cleanup when
optionschanges:+ useEffect(() => { + menuItemsRef.current = [] + }, [options.length])Or inline in the map:
{options.map((option, index) => { + if (index === 0) menuItemsRef.current.length = options.length // ... })}frontend/__tests__/unit/components/EntityActions.test.tsx (1)
488-515: Keyboard navigation tests are partially orphaned from their describe block.The
describe('handles keyboard navigation', ...)block only contains two tests (Escape key behavior). The remaining keyboard navigation tests (lines 517-718) are defined outside this describe block at the top level of the maindescribe('EntityActions', ...). This makes the test organization inconsistent and harder to navigate.🧪 Suggested fix: Move all keyboard tests inside the describe block
describe('handles keyboard navigation', () => { it('closes menu when escape key is pressed', async () => { // ... }) it('returns focus to trigger button when escape is pressed', async () => { // ... }) - }) - - it('focuses first menu item when menu opens', () => { + + it('focuses first menu item when menu opens', () => { // ... + }) + + it('navigates down through menu items with ArrowDown', async () => { // ... + }) + + // ... move all other keyboard tests here ... + }) })
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3656 +/- ##
==========================================
- Coverage 85.56% 85.55% -0.02%
==========================================
Files 463 463
Lines 14306 14342 +36
Branches 1903 1912 +9
==========================================
+ Hits 12241 12270 +29
- Misses 1686 1688 +2
- Partials 379 384 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Works great! Thanks @Utkarsh-0304 !
2 participants
Proposed change
Resolves #3590
This PR adds keyboard navigation support to the EntityActions component in order to comply with WCAG 2.1 Level AA standards. Additionally, unit tests are added to verify keyboard navigation.
Checklist
make check-testlocally: all warnings addressed, tests passed