fix(headless): stop a menu inside a popover acting as a submenu - #9315
Conversation
A menu rendered inside a popover has a floating parent node id, so keying nesting off that alone made it hover-open, place itself side-start, and swallow mouse clicks. Require a parent menu context too.
A popover is not a menu, so a menu trigger rendered inside one is a plain button. floating-ui derives submenu-ness from the shared floating tree, which every floating element joins for dismissal, and labels the trigger a menu item of a menu that does not exist. Take the role from whether there is a parent menu.
🦋 Changeset detectedLatest commit: 9beaf02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
📝 WalkthroughWalkthroughThe menu primitive now treats a menu as nested only when both a floating parent ID and a Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
🧹 Nitpick comments (1)
packages/headless/src/primitives/menu/menu.test.tsx (1)
563-589: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the menu interaction change in this regression test.
Lines 563-589 open only the
Popover. They do not clickActions. The change inpackages/headless/src/primitives/menu/menu-root.tsxalso controls hover, click, and placement throughisNested. This test could pass while the trigger remains unclickable as a submenu.After opening
Account, clickActionsand assert thatSign outis rendered.Suggested assertions
await user.click(screen.getByText('Account')); + await user.click(screen.getByRole('button', { name: 'Actions' })); + expect(screen.getByRole('menu')).toBeInTheDocument(); + expect(screen.getByText('Sign out')).toBeInTheDocument(); + // A popover is not a menu, so its children are not menu items.As per coding guidelines, unit tests are required for all new functionality and must verify behavior and edge cases.
🤖 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 `@packages/headless/src/primitives/menu/menu.test.tsx` around lines 563 - 589, Extend the regression test around the nested Menu.Trigger by clicking the visible “Actions” button after opening the Popover and asserting that the “Sign out” menu item is rendered. Keep the existing plain-button and role assertions, and verify the interaction path controlled by isNested rather than only checking initial rendering.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/headless/src/primitives/menu/menu.test.tsx`:
- Around line 563-589: Extend the regression test around the nested Menu.Trigger
by clicking the visible “Actions” button after opening the Popover and asserting
that the “Sign out” menu item is rendered. Keep the existing plain-button and
role assertions, and verify the interaction path controlled by isNested rather
than only checking initial rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b7489856-3077-45fa-be41-735b58a34503
📒 Files selected for processing (3)
.changeset/menu-role-in-popover.mdpackages/headless/src/primitives/menu/menu-root.tsxpackages/headless/src/primitives/menu/menu.test.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
2 participants
Description
A
Menurendered inside aPopoverwas treating itself as a submenu. Its trigger gotrole="menuitem", opened on hover, placed itself to the side, and ignored mouse clicks, even though the popover it sits in is arole="dialog"with no menu anywhere.Both symptoms come from the same assumption: that a floating parent is a menu parent.
FloatingTreeis generic plumbing that every floating element joins souseDismissknows a click inside a child isn't an outside press, so a popover ancestor setsparentIdjust like a parent menu does.Two places had to learn the difference:
MenuRootnow derivesisNestedfromparentId != null && parentContext != null, so a submenu is a menu with a parent menu.useRolere-derives nesting internally fromuseFloatingParentNodeId()alone and takes no override, so itsrole="menuitem"reference prop is dropped when there is no parent menu. Everything else it emits (aria-expanded,aria-haspopup,aria-controls,id, and the whole floating side) is correct in both cases and is left alone.The floating tree itself is untouched. Giving the menu its own tree would make
useRoleself-correct, but the popover would then read clicks on menu items as outside presses and close underneath them.role="menuitem"on a trigger with no menu ancestor is also wrong for assistive tech, which announces it as an item of a set that does not exist.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change