fix(tabs): add correct aria-orientation and keyboard navigation for vertical tabs #5811
Conversation
🦋 Changeset detectedLatest commit: 15a3c05 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
@anusha-c18 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughFixes vertical Tabs accessibility by deriving orientation from props, setting Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Tabs as Tabs Component
participant Hook as useTabs
participant Aria as useTabList
participant DOM as TabList DOM
User->>Tabs: Render <Tabs isVertical|placement="start|end">
Tabs->>Hook: initialize(props)
Hook->>Hook: derive placement & orientation
Hook->>Aria: init({ orientation })
Aria-->>Hook: tabListProps (aria-orientation)
Hook->>DOM: render TabList with aria-orientation + data-*
Note right of DOM #lightblue: aria-orientation determines keyboard handlers
User->>DOM: Press ArrowUp / ArrowDown
DOM->>Aria: keyboard event -> request move
Aria-->>DOM: update focus & aria-selected to prev/next tab
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/five-kiwis-carry.md(1 hunks)packages/components/tabs/__tests__/tabs.test.tsx(1 hunks)packages/components/tabs/src/use-tabs.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/tabs/__tests__/tabs.test.tsx (1)
packages/utilities/test-utils/src/focus.ts (1)
focus(6-12)
🪛 LanguageTool
.changeset/five-kiwis-carry.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...Down arrow navigation for accessibility.
(QB_NEW_EN)
🔇 Additional comments (5)
.changeset/five-kiwis-carry.md (1)
1-5: LGTM!The changeset accurately describes the accessibility fix for vertical tabs.
packages/components/tabs/__tests__/tabs.test.tsx (2)
543-561: LGTM!The test correctly verifies that vertical tabs have
aria-orientation="vertical". The test structure follows existing patterns and appropriately uses theisVerticalprop.
563-604: LGTM!The test comprehensively verifies Up/Down arrow keyboard navigation for vertical tabs, checking that focus and
aria-selectedattributes update correctly. The sequential navigation pattern (Down → Down → Up) provides good coverage.packages/components/tabs/src/use-tabs.ts (2)
116-120: LGTM!Passing
orientationtouseTabListcorrectly enables React Aria's keyboard navigation handling, which responds to Up/Down arrows for vertical orientation and Left/Right for horizontal.
182-195: LGTM with a minor note.The
mergePropsapproach correctly combinestabListPropsfrom React Aria with custom attributes. The explicitaria-orientationon line 189 is technically redundant since React Aria'stabListPropsshould already include it (givenorientationis passed touseTabListon line 117), but the redundancy is harmless and makes the attribute assignment explicit for clarity.
anusha-c18
force-pushed
the
fix/tabs-vertical-orientation
branch
from
4ad633a to
15a3c05
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/tabs/__tests__/tabs.test.tsx (2)
563-604: Consider adding wrapping behavior tests for vertical navigation.The test correctly verifies Up/Down arrow navigation for vertical tabs. However, the existing horizontal navigation test (lines 153-194) includes wrapping behavior (ArrowRight on the last tab wraps to the first). Consider adding similar assertions here to verify ArrowDown on tab3 wraps to tab1 and ArrowUp on tab1 wraps to tab3.
Additionally, the PR description mentions "React Aria intentionally allows all arrow keys for vertical tabs to support movement in both axes." Consider adding assertions to verify that vertical tabs still respond to Left/Right arrows to document this behavior.
Example diff for wrapping:
await user.keyboard("[ArrowUp]"); expect(tab1).toHaveAttribute("aria-selected", "false"); expect(tab2).toHaveAttribute("aria-selected", "true"); expect(tab3).toHaveAttribute("aria-selected", "false"); + + await user.keyboard("[ArrowUp]"); + expect(tab1).toHaveAttribute("aria-selected", "true"); + expect(tab2).toHaveAttribute("aria-selected", "false"); + expect(tab3).toHaveAttribute("aria-selected", "false"); + + await user.keyboard("[ArrowUp]"); + expect(tab1).toHaveAttribute("aria-selected", "false"); + expect(tab2).toHaveAttribute("aria-selected", "false"); + expect(tab3).toHaveAttribute("aria-selected", "true");
563-604: Consider adding tests for placement="start" and placement="end".Per the PR objectives, tabs with
placement="start"orplacement="end"should also usearia-orientation="vertical"and support Up/Down navigation. The existing test at lines 278-303 verifiesdata-placementanddata-verticalattributes for these placements, but doesn't verifyaria-orientationor keyboard navigation.Consider adding dedicated tests to verify that:
placement="start"setsaria-orientation="vertical"on the tablistplacement="end"setsaria-orientation="vertical"on the tablist- Both placements support Up/Down arrow navigation
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/five-kiwis-carry.md(1 hunks)packages/components/tabs/__tests__/tabs.test.tsx(1 hunks)packages/components/tabs/src/use-tabs.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/tabs/src/use-tabs.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/tabs/__tests__/tabs.test.tsx (1)
packages/utilities/test-utils/src/focus.ts (1)
focus(6-12)
🪛 LanguageTool
.changeset/five-kiwis-carry.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...Down arrow navigation for accessibility.
(QB_NEW_EN)
🔇 Additional comments (2)
.changeset/five-kiwis-carry.md (1)
1-5: LGTM!The changeset format is correct and the description clearly documents the fix. The LanguageTool hint is a false positive.
packages/components/tabs/__tests__/tabs.test.tsx (1)
543-561: LGTM!The test correctly verifies that
isVerticalsetsaria-orientation="vertical"on the tablist, which aligns with the PR objectives.
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-aria-overlay
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-form-reset
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/use-viewport-size
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
| (props) => | ||
| mergeProps( | ||
| tabListProps, | ||
| { | ||
| ref: domRef, | ||
| "data-slot": "tabList", | ||
| "aria-orientation": orientation, | ||
| className: slots.tabList({class: clsx(classNames?.tabList, props?.className)}), | ||
| }, | ||
| props, | ||
| ), | ||
| [domRef, tabListProps, classNames, slots, orientation], |
There was a problem hiding this comment.
I don't think you need to update this part. aria-orientation is already included in tabListProps. Also don't see the reason why we need to wrap everything in mergeProps.
| "@heroui/tabs": patch | ||
| --- | ||
|
|
||
| Fix vertical tabs to use correct aria-orientation and support Up/Down arrow navigation for accessibility. |
There was a problem hiding this comment.
please include the issue number at the end. i.e.
Fix vertical tabs to use correct aria-orientation and support Up/Down arrow navigation for accessibility. (#5810)
2 participants
Closes #5810
📝 Description
This PR fixes the accessibility behavior for vertical tabs in the tabs component.
Previously, vertical tabs were not correctly using
aria-orientation="vertical"and did not respond to ArrowUp/ArrowDown keys for keyboard navigation.This update ensures that the correct ARIA orientation is applied with up/down keyboard navigation, and adds tests for the fixes.
⛳️ Current behavior (updates)
aria-orientation="horizontal"by default, even when the layout was vertical.🚀 New behavior
aria-orientation="vertical".tabs.test.tsxto verify both correct aria-orientation assignment and keyboard navigation behavior for vertical tabs.💣 Is this a breaking change (Yes/No):
No.
📝 Additional Information
The React Aria library intentionally allows all arrow keys (up, down, left, right) for vertical tabs to support users moving in both axes, so this behavior has been preserved.
Summary by CodeRabbit
Bug Fixes
Accessibility
Tests