refactor(ui): Improve UserButton accessibility#8325
refactor(ui): Improve UserButton accessibility#8325alexcarpenter wants to merge 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 73355ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
📝 WalkthroughWalkthroughThis pull request updates the UserButton component's accessibility architecture by replacing the menu-based pattern (with Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/UserButton/__tests__/UserButton.test.tsx`:
- Around line 243-266: The current test "traps focus within the popover when
open" only asserts the trigger loses focus on Tab, which can pass even if focus
moves outside the dialog; update the loop to assert that the dialog element
(retrieved as dialog = screen.getByRole('dialog', { name: 'Account panel' }))
contains the active element after each await userEvent.tab() call instead of
checking expect(trigger).not.toHaveFocus(); i.e., replace the per-iteration
assertion with a containment check like dialog.contains(document.activeElement)
(using the existing dialog variable and userEvent.tab()) so each Tab verifies
focus remains inside the popover.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6d4f3cc8-0b08-4506-8719-c6ce29033b7f
📒 Files selected for processing (10)
.changeset/user-button-a11y.mdpackages/localizations/src/en-US.tspackages/shared/src/types/localization.tspackages/ui/src/components/UserButton/SessionActions.tsxpackages/ui/src/components/UserButton/UserButtonPopover.tsxpackages/ui/src/components/UserButton/UserButtonTrigger.tsxpackages/ui/src/components/UserButton/__tests__/UserButton.test.tsxpackages/ui/src/components/UserButton/index.tsxpackages/ui/src/elements/Popover.tsxpackages/ui/src/hooks/usePopover.ts
| it('traps focus within the popover when open', async () => { | ||
| const { wrapper } = await createFixtures(f => { | ||
| f.withUser({ | ||
| first_name: 'First', | ||
| last_name: 'Last', | ||
| username: 'username1', | ||
| email_addresses: ['test@clerk.com'], | ||
| }); | ||
| }); | ||
|
|
||
| const { getByRole, userEvent } = render(<UserButton />, { wrapper }); | ||
| const trigger = getByRole('button', { name: accountPanelButtonName('First Last') }); | ||
| await userEvent.click(trigger); | ||
|
|
||
| const dialog = screen.getByRole('dialog', { name: 'Account panel' }); | ||
| expect(dialog).toBeInTheDocument(); | ||
|
|
||
| for (let i = 0; i < 10; i++) { | ||
| await userEvent.tab(); | ||
| expect(trigger).not.toHaveFocus(); | ||
| } | ||
|
|
||
| expect(screen.getByRole('dialog', { name: 'Account panel' })).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Assert focus remains inside the dialog.
This test can still pass if Tab moves focus to document.body or any other outside element, because it only checks that the trigger is not focused. Since focus trapping is core to this change, assert the active element is contained by the dialog on each tab.
Proposed fix
for (let i = 0; i < 10; i++) {
await userEvent.tab();
+ expect(dialog).toContainElement(document.activeElement as HTMLElement);
expect(trigger).not.toHaveFocus();
}As per coding guidelines, “Test component behavior, not implementation details” and “Implement proper focus traps in React components.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('traps focus within the popover when open', async () => { | |
| const { wrapper } = await createFixtures(f => { | |
| f.withUser({ | |
| first_name: 'First', | |
| last_name: 'Last', | |
| username: 'username1', | |
| email_addresses: ['test@clerk.com'], | |
| }); | |
| }); | |
| const { getByRole, userEvent } = render(<UserButton />, { wrapper }); | |
| const trigger = getByRole('button', { name: accountPanelButtonName('First Last') }); | |
| await userEvent.click(trigger); | |
| const dialog = screen.getByRole('dialog', { name: 'Account panel' }); | |
| expect(dialog).toBeInTheDocument(); | |
| for (let i = 0; i < 10; i++) { | |
| await userEvent.tab(); | |
| expect(trigger).not.toHaveFocus(); | |
| } | |
| expect(screen.getByRole('dialog', { name: 'Account panel' })).toBeInTheDocument(); | |
| }); | |
| it('traps focus within the popover when open', async () => { | |
| const { wrapper } = await createFixtures(f => { | |
| f.withUser({ | |
| first_name: 'First', | |
| last_name: 'Last', | |
| username: 'username1', | |
| email_addresses: ['test@clerk.com'], | |
| }); | |
| }); | |
| const { getByRole, userEvent } = render(<UserButton />, { wrapper }); | |
| const trigger = getByRole('button', { name: accountPanelButtonName('First Last') }); | |
| await userEvent.click(trigger); | |
| const dialog = screen.getByRole('dialog', { name: 'Account panel' }); | |
| expect(dialog).toBeInTheDocument(); | |
| for (let i = 0; i < 10; i++) { | |
| await userEvent.tab(); | |
| expect(dialog).toContainElement(document.activeElement as HTMLElement); | |
| expect(trigger).not.toHaveFocus(); | |
| } | |
| expect(screen.getByRole('dialog', { name: 'Account panel' })).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/UserButton/__tests__/UserButton.test.tsx` around
lines 243 - 266, The current test "traps focus within the popover when open"
only asserts the trigger loses focus on Tab, which can pass even if focus moves
outside the dialog; update the loop to assert that the dialog element (retrieved
as dialog = screen.getByRole('dialog', { name: 'Account panel' })) contains the
active element after each await userEvent.tab() call instead of checking
expect(trigger).not.toHaveFocus(); i.e., replace the per-iteration assertion
with a containment check like dialog.contains(document.activeElement) (using the
existing dialog variable and userEvent.tab()) so each Tab verifies focus remains
inside the popover.
1 participant
Description
The UserButton popover was updated from role="menu" with role="menuitem" children to role="dialog" with role="group" sections. Focus management was fixed using floating-ui's
interaction system (useInteractions, useClick, useRole, useDismiss) instead of manual onClick/aria-* wiring.
Why dialog, not menu
The WAI-ARIA menu role is for a list of differing actions — like a right-click context menu or a menubar dropdown. Every child must be a menuitem, menuitemcheckbox, or
menuitemradio. The UserButton popover contains:
This is mixed content, not a flat action list. Forcing menuitem on non-action elements (previews, footers) creates a misleading experience for screen reader users — they hear
"menu with 8 items" when only 3 are actionable. The dialog role correctly communicates that this is a container with varied, grouped content.
Key implementation details
action.
Fixes USER-4842
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change