fix: Close nested Menus when hovering over other items - #4090
fix: Close nested Menus when hovering over other items#4090mannycarrera4 wants to merge 23 commits into
Conversation
mannycarrera4
added
the
ai author
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe menu now closes stale sibling submenus, ignores disabled targets and items, restores focus after reopening, applies default list sizing and nested container styling, and adds nested-menu stories and tests. ChangesMenu interaction behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ 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 |
4bb117a to
34a699d
Compare
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/react/menu/lib/Submenu.tsx`:
- Around line 94-98: Update the target ID resolution in Submenu’s
sibling-closing logic to fall back to the ID captured from the target event when
model.state.targetRef.current is unavailable. Keep the existing parentCursorId
comparison and model.events.hide() behavior, and add coverage using a target
component that does not forward its ref.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d15f24a6-71bf-4f9b-b890-ad9aca92fc1a
📒 Files selected for processing (4)
cypress/component/Menu.spec.tsxmodules/react/menu/lib/Submenu.tsxmodules/react/menu/stories/Menu.stories.tsmodules/react/menu/stories/examples/NestedSiblings.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@modules/react/menu/spec/MenuDisabled.spec.tsx`:
- Around line 102-126: Extend the sibling submenu test around NestedSiblings to
cover opening the second submenu via hover, using the project’s established
hover event helper or event sequence instead of only fireEvent.click(thirdItem).
Assert that the previously opened Second submenu closes, Second Item becomes
aria-expanded="false", Third Item’s submenu appears, and the menu count remains
unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f89c24cc-b4db-42a6-921a-6b19f42c5dec
⛔ Files ignored due to path filters (1)
.cursor/debug-dea6ae.logis excluded by!**/*.log
📒 Files selected for processing (5)
modules/react/collection/lib/useListItemSelect.tsxmodules/react/menu/lib/MenuItem.tsxmodules/react/menu/lib/MenuList.tsxmodules/react/menu/lib/Submenu.tsxmodules/react/menu/spec/MenuDisabled.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/react/menu/lib/Submenu.tsx
| it('should close sibling submenu when another is opened by click', async () => { | ||
| render(<NestedSiblings />); | ||
|
|
||
| fireEvent.click(screen.getByRole('button', {name: 'Open Menu'})); | ||
| await screen.findByRole('menu'); | ||
|
|
||
| const secondItem = screen.getByRole('menuitem', {name: 'Second Item'}); | ||
| const thirdItem = screen.getByRole('menuitem', {name: 'Third Item'}); | ||
|
|
||
| fireEvent.click(secondItem); | ||
| await waitFor(() => { | ||
| expect(screen.getByRole('menuitem', {name: 'Second: First Sub Item'})).toBeInTheDocument(); | ||
| }); | ||
| expect(secondItem).toHaveAttribute('aria-expanded', 'true'); | ||
|
|
||
| fireEvent.click(thirdItem); | ||
| await waitFor(() => { | ||
| expect( | ||
| screen.queryByRole('menuitem', {name: 'Second: First Sub Item'}) | ||
| ).not.toBeInTheDocument(); | ||
| }); | ||
| expect(secondItem).toHaveAttribute('aria-expanded', 'false'); | ||
| expect(screen.getByRole('menuitem', {name: 'Third: First Sub Item'})).toBeInTheDocument(); | ||
| expect(screen.getAllByRole('menu')).toHaveLength(2); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the hover interaction.
This test opens the sibling submenu with fireEvent.click(thirdItem). The stated regression occurs when the user hovers over another submenu target. A click-only test can pass while the hover handler leaves the first submenu open.
Proposed test update
- it('should close sibling submenu when another is opened by click', async () => {
+ it('should close sibling submenu when another is opened by hover', async () => {
...
- fireEvent.click(thirdItem);
+ fireEvent.mouseEnter(thirdItem);📝 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('should close sibling submenu when another is opened by click', async () => { | |
| render(<NestedSiblings />); | |
| fireEvent.click(screen.getByRole('button', {name: 'Open Menu'})); | |
| await screen.findByRole('menu'); | |
| const secondItem = screen.getByRole('menuitem', {name: 'Second Item'}); | |
| const thirdItem = screen.getByRole('menuitem', {name: 'Third Item'}); | |
| fireEvent.click(secondItem); | |
| await waitFor(() => { | |
| expect(screen.getByRole('menuitem', {name: 'Second: First Sub Item'})).toBeInTheDocument(); | |
| }); | |
| expect(secondItem).toHaveAttribute('aria-expanded', 'true'); | |
| fireEvent.click(thirdItem); | |
| await waitFor(() => { | |
| expect( | |
| screen.queryByRole('menuitem', {name: 'Second: First Sub Item'}) | |
| ).not.toBeInTheDocument(); | |
| }); | |
| expect(secondItem).toHaveAttribute('aria-expanded', 'false'); | |
| expect(screen.getByRole('menuitem', {name: 'Third: First Sub Item'})).toBeInTheDocument(); | |
| expect(screen.getAllByRole('menu')).toHaveLength(2); | |
| }); | |
| it('should close sibling submenu when another is opened by hover', async () => { | |
| render(<NestedSiblings />); | |
| fireEvent.click(screen.getByRole('button', {name: 'Open Menu'})); | |
| await screen.findByRole('menu'); | |
| const secondItem = screen.getByRole('menuitem', {name: 'Second Item'}); | |
| const thirdItem = screen.getByRole('menuitem', {name: 'Third Item'}); | |
| fireEvent.click(secondItem); | |
| await waitFor(() => { | |
| expect(screen.getByRole('menuitem', {name: 'Second: First Sub Item'})).toBeInTheDocument(); | |
| }); | |
| expect(secondItem).toHaveAttribute('aria-expanded', 'true'); | |
| fireEvent.mouseEnter(thirdItem); | |
| await waitFor(() => { | |
| expect( | |
| screen.queryByRole('menuitem', {name: 'Second: First Sub Item'}) | |
| ).not.toBeInTheDocument(); | |
| }); | |
| expect(secondItem).toHaveAttribute('aria-expanded', 'false'); | |
| expect(screen.getByRole('menuitem', {name: 'Third: First Sub Item'})).toBeInTheDocument(); | |
| expect(screen.getAllByRole('menu')).toHaveLength(2); | |
| }); |
🤖 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 `@modules/react/menu/spec/MenuDisabled.spec.tsx` around lines 102 - 126, Extend
the sibling submenu test around NestedSiblings to cover opening the second
submenu via hover, using the project’s established hover event helper or event
sequence instead of only fireEvent.click(thirdItem). Assert that the previously
opened Second submenu closes, Second Item becomes aria-expanded="false", Third
Item’s submenu appears, and the menu count remains unchanged.
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
mc-fix-nested-menu
|
| Run status |
|
| Run duration | 02m 41s |
| Commit |
|
| Committer | Manuel Carrera |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
824
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.34%
|
|
|---|---|
|
|
1570
|
|
|
374
|
Accessibility
99.44%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
68
|
…y/canvas-kit into mc-fix-nested-menu
A prior commit accidentally left in fetch() calls to a local debug ingest server on every menu click/render, plus a stray debug log file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…/canvas-kit into mc-fix-nested-menu
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/component/Menu.spec.tsx (1)
235-240: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd accessibility checks for each mounted story.
Both new story contexts mount an example without
cy.checkA11y(). Add an accessibility test in each context.
cypress/component/Menu.spec.tsx#L235-L240: Addcy.checkA11y()forNestedSiblings.cypress/component/Menu.spec.tsx#L328-L333: Addcy.checkA11y()forNestedDynamic.As per coding guidelines, Cypress component tests must “include
cy.checkA11y()for every mounted example.”🤖 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 `@cypress/component/Menu.spec.tsx` around lines 235 - 240, Add cy.checkA11y() to both mounted story contexts: cypress/component/Menu.spec.tsx lines 235-240 for NestedSiblings and lines 328-333 for NestedDynamic, ensuring each context’s beforeEach accessibility-checks its mounted example.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.
Outside diff comments:
In `@cypress/component/Menu.spec.tsx`:
- Around line 235-240: Add cy.checkA11y() to both mounted story contexts:
cypress/component/Menu.spec.tsx lines 235-240 for NestedSiblings and lines
328-333 for NestedDynamic, ensuring each context’s beforeEach
accessibility-checks its mounted example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7951fd41-250f-43f8-a3cf-df70151812bb
📒 Files selected for processing (6)
cypress/component/Menu.spec.tsxmodules/react/collection/lib/useListItemSelect.tsxmodules/react/menu/lib/MenuItem.tsxmodules/react/menu/lib/MenuList.tsxmodules/react/menu/lib/Submenu.tsxmodules/react/menu/spec/MenuDisabled.spec.tsx
💤 Files with no reviewable changes (3)
- modules/react/collection/lib/useListItemSelect.tsx
- modules/react/menu/lib/MenuList.tsx
- modules/react/menu/lib/Submenu.tsx
| boxShadow: system.depth[3], | ||
| minWidth, | ||
| maxHeight, | ||
| maxHeight: cssVar(maxHeight, '60vh'), |
There was a problem hiding this comment.
add a default value here
| if (!element) { | ||
| return false; | ||
| } | ||
| return ( | ||
| element.getAttribute('aria-disabled') === 'true' || | ||
| (element as HTMLButtonElement | HTMLInputElement).disabled === true | ||
| ); |
There was a problem hiding this comment.
| if (!element) { | |
| return false; | |
| } | |
| return ( | |
| element.getAttribute('aria-disabled') === 'true' || | |
| (element as HTMLButtonElement | HTMLInputElement).disabled === true | |
| ); | |
| return ( | |
| Boolean(element?.getAttribute('aria-disabled')) || | |
| (element as HTMLButtonElement | HTMLInputElement)?.disabled | |
| ); |
There was a problem hiding this comment.
But honestly, I would change that function to be isElementActive with the next return:
return (
element &&
!Boolean(element?.getAttribute('aria-disabled')) &&
!(element as HTMLButtonElement | HTMLInputElement)?.disabled
);
it would be more precise checking and you will not need to have many returns in other functions
| if (isElementDisabled(event.currentTarget) || state.nonInteractiveIds.includes(name)) { | ||
| return null; | ||
| } | ||
| events.select({id: name}); | ||
| return undefined; |
There was a problem hiding this comment.
Do you need anything to be returned here?
| if (isElementDisabled(event.currentTarget)) { | ||
| return; | ||
| } | ||
| currentTargetIdRef.current = event.currentTarget.getAttribute('data-id')!; | ||
| mouseEnterTimer.start(); |
There was a problem hiding this comment.
why do not check "If element is not disabled" and do not have return at all?
| const isItemDisabled = isElementDisabled(localRef.current); | ||
|
|
||
| if (model.state.mode === 'single' && isCursor(model.state, id) && isItemDisabled) { | ||
| model.events.goToFirst(); |
There was a problem hiding this comment.
this part of the code highlights that if the cursor was at a disabled item and the menu is closed, the first item is focus upon reopening
| return null; | ||
| } | ||
| events.select({id: name}); | ||
| return undefined; |
There was a problem hiding this comment.
returning undefined here allows the event to continue or if someone passes an onclick, that it would still be called, the return null explicitly stops the function from being called.
| model.events.hide(event); | ||
| hideParent(model); | ||
| if (isElementDisabled(event.currentTarget as Element)) { | ||
| return null; |
There was a problem hiding this comment.
If element is disabled, we don't fire the on click event, this fixes the bug that previously allowed on click events for disabled items.
| '.wd-no-animation &': { | ||
| animation: 'none', | ||
| }, | ||
| '&:where(:has([data-part="list-box-container"]))': { |
There was a problem hiding this comment.
In cases where we're hardcoding data-part values here that are reliant on a stencil (in this case, listBoxContainerStencil), do we normally add a comment noting that it's reliant on a certain stencil? More of an edge case, but if that stencil ever had a rename for the list box container, these styles would break.
There was a problem hiding this comment.
this was annoying, i added a .selector
| * disabled through either mechanism are consistently blocked from activating (click, Enter/Space, | ||
| * hover-intent, etc.). | ||
| */ | ||
| export const isElementDisabled = (element: Element | null | undefined): boolean => { |
There was a problem hiding this comment.
Just curious. How do we decide which lib functions get tests? I see some in modules/react/collection/spec but definitely not for all.
| boxShadow: system.depth[3], | ||
| minWidth, | ||
| maxHeight, | ||
| maxHeight: cssVar(maxHeight, '60vh'), |
There was a problem hiding this comment.
Worth noting this maxHeight default in our docs?
| <Menu.Popper> | ||
| <Menu.Card> | ||
| <Menu.List> | ||
| <Menu.Item data-id="first-item">First Item</Menu.Item> |
There was a problem hiding this comment.
Do we also want to add a disabled menu item here with children, for visual testing that hovering it doesn't open a submenu?
| }, | ||
| '& :where([data-part="list-box-container"])': { | ||
| borderRadius: system.legacy.shape.xxl, | ||
| overflow: 'hidden', |
There was a problem hiding this comment.
When I add ton of menu items, I'm not able to scroll down to the ones that exceed the viewport height, and it looks like this overflow: hidden is the culprit. Do we want to allow this scrolling and remove this property, or are we expecting people to know to override maxHeight for long menu lists (ex. set it to fit-content)?
I see that listBoxContainerStencil has a default of overflowY: auto for vertical orientation.
sheelah
left a comment
sheelah
left a comment
There was a problem hiding this comment.
Looking good. What a tricky one! Left a few minor comments.
|
Also one note for later: we'll want to double check the styling on the nested menu story when we merge this into |
…/canvas-kit into mc-fix-nested-menu
…/canvas-kit into mc-fix-nested-menu
| }; | ||
| ``` | ||
|
|
||
| To target a part from another stencil, use `.selector`. Spreading the part onto an element still |
5 participants
Summary
Fixes: #3333, #4119, #4118, #4117
Release Category
Components
Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests