fix(a11y): use a roving tabindex for connect menu#1849
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds roving tabindex and keyboard navigation to the AccountMenu dropdown: introduces refs for the menu button and menu container, initialises and manages focus on the first menu item when opened, implements ArrowDown/ArrowUp/Home/End navigation with circular traversal and a safe index wrap helper, handles Escape to focus the menu button before closing, closes the menu when focus leaves the menu region, converts menu item buttons to Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/components/Header/AccountMenu.client.vue (1)
61-70:⚠️ Potential issue | 🟡 MinorAvoid hard throws in focus/index edge paths; fail gracefully instead
Line 66, Line 100, and Line 123 throw for recoverable UI states. In keyboard/focus handlers this can surface runtime errors instead of safely closing or no-oping the menu. Also, ensure the “active item not found” path is handled explicitly before dereferencing the current item.
Suggested patch
watch(menuRef, () => { if (!menuRef.value) return // Set up focus for the first menu item - const firstMenuItem = menuRef.value.querySelector('[role="menuitem"]') as HTMLButtonElement + const firstMenuItem = menuRef.value.querySelector<HTMLElement>('[role="menuitem"]') if (!firstMenuItem) { - throw new Error('Cannot find a menuitem to focus') + isOpen.value = false + return } firstMenuItem.tabIndex = 0 firstMenuItem.focus() }) @@ function onMenuKeyDown(event: KeyboardEvent) { const menu = event.currentTarget as HTMLElement if (!menu) return // Collect the menu items (i.e. focusable candidates) const menuItems: HTMLElement[] = Array.from(menu.querySelectorAll('[role="menuitem"]')) + if (menuItems.length === 0) { + isOpen.value = false + return + } // Find the current item - let currentIndex = menuItems.findIndex(menuItem => menuItem.tabIndex !== -1) + let currentIndex = menuItems.findIndex(menuItem => menuItem.tabIndex === 0) + if (currentIndex < 0) currentIndex = 0 const currentMenuItem = menuItems.at(currentIndex) if (!currentMenuItem) { - throw new Error(`Missing menuitem at index ${currentIndex}`) + return } @@ const menuItemToFocus = menuItems.at(currentIndex) if (!menuItemToFocus) { - throw new RangeError(`currentIndex (${currentIndex}) outside of range of menu items`) + return }As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index."
#!/bin/bash # Verify the current focus/index edge handling in AccountMenu handlers rg -n -C3 'watch\(menuRef|onMenuKeyDown|findIndex\(|throw new Error|throw new RangeError' app/components/Header/AccountMenu.client.vueAlso applies to: 95-124
|
Looks like using mouse click at connect doesn't add "focus" styles to first item on chrome, works pressing enter. |
|
@userquin I wouldn’t expect the focus styles to appear on mouse click since focus styles only appear for focus visible. I did notice an issue that if you click the menu button while it’s open, since the menu is checking for loss of focus to close itself, it will cause the menu to immediately open again. |
|
I mean, the first item in the list without focus styles (tabindex=0) when using mouse click, I'm not talking about focusing the "selector/menu". Mouse click and pressing enter should have the same behavior (? ), I'm just asking. pressing enter |
|
@userquin |
serhalp
left a comment
serhalp
left a comment
There was a problem hiding this comment.
This LGTM and seems to work well.
This is a wild amount of boilerplate for basic a11y though. We should be using a headless component library or something, right??? https://reka-ui.com/?
|
I might eventually make a roving tabindex composable since that's a very common pattern. Please no one merge until I fix the light dismiss double trigger issue. |
|
I’m making this one a draft. I won’t have time to finish it before I turn in for the night. There are a few problems here:
My code is also a bit messy and trying to add the focus trap on top only makes it messier. The roving tabindex bit is pretty self-contained (i.e. the |
4 participants
A
menu(composite) widget is navigated with arrow keys as opposed to the Tab key. This means we need to implement a roving tabindex for our connect menu.Here is the expected behaviour:
Since
role=menuhas an implicitaria-orientation=verticalproperty, I’m just implementing the up/down arrow keys. It could be worth just doing up/left and down/right in all cases as the direction isn’t often exposed to users.I’ve done my best with the Vue and TypeScript code, however, I imagine I’m doing a few things a bit strange, so feel free to edit or make suggestions.