Skip to content

fix(menu): Keep submenus beside scrolled parent menus - #4186

Open
minwookshin wants to merge 3 commits into
Workday:masterfrom
minwookshin:fix/submenu-scroll-placement
Open

minwookshin wants to merge 3 commits into
Workday:masterfrom
minwookshin:fix/submenu-scroll-placement

Conversation

@minwookshin

@minwookshin minwookshin commented Oct 1, 2026 •

Copy link
Copy Markdown

Summary

Fixes: #4180

When scrolling moves a submenu trigger out of view, the generic fallback modifier can place the submenu over its parent. Default submenus now flip horizontally to the opposite side and retain their start alignment. Explicit fallback placements and custom Popper options preserve their existing override behavior.

Release Category

Components

Release Note

Submenus stay beside their parent menu while scrolling, including at viewport edges. Custom placement and fallback options remain available.

Checklist

  • Added a Storybook example; no MDX changes.
  • Label ready for review has been added to PR (requires maintainer access).

Where Should the Reviewer Start?

modules/react/menu/lib/Submenu.tsx, followed by cypress/component/SubmenuPlacement.spec.tsx.

Areas for Feedback?

Default submenu positioning when its trigger is vertically clipped, and preservation of explicit placement overrides.

Validation

  • Reproduced parent-menu overlap before the fix in LTR and RTL at both viewport edges.
  • Cypress: 18 new placement/a11y cases and 44 existing Menu cases passed; one pre-existing case is pending.
  • Menu/popup unit suites: 100 passed.
  • Full lint, React source typecheck, Cypress typecheck, and Storybook typecheck passed.

Testing Manually

Open the ScrollableSubmenu Menu story, open More Items, then scroll the parent list until the trigger moves out of view. The submenu should remain beside the parent. The component suite also covers RTL, both viewport edges, custom offsets, explicit placement, and an empty fallback list.

Summary by CodeRabbit

  • New Features
    • Added a scrollable menu example with left-to-right and right-to-left layouts, alignment options, and configurable submenu placement.
    • Submenus now support custom placement, fallback placements, and positioning options.
  • Bug Fixes
    • Improved submenu positioning in scrolled menus, keeping submenus beside their parent and reducing overlap at menu edges.
    • Explicit placement and custom positioning settings are preserved when configuring submenu placement.

Use horizontal flipping for default submenu positioning while retaining explicit fallback placements and custom Popper options. Cover scrolled parents, viewport edges, RTL and overrides in component tests.

Fixes Workday#4180
@minwookshin
minwookshin requested a review from a team as a code owner October 1, 2026 06:32
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Repository: Workday/canvas-kit/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9f14f16e-f79f-4872-8fb1-6e8f97ef0f04
📥 Commits

Reviewing files that changed from the base of the PR and between 2bf41ab and 309c9f8.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: Workday/canvas-kit/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: ee341c37-5b4f-424f-b57f-2cfaebfc6805

📥 Commits

Reviewing files that changed from the base of the PR and between 098335a and 2bf41ab.

📒 Files selected for processing (2)
  • cypress/component/SubmenuPlacement.spec.tsx
  • modules/react/menu/lib/Submenu.tsx

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

SubmenuPopper now accepts fallback placements and Popper options. A scrollable submenu example exposes these options. Cypress tests cover LTR and RTL placement, scrolling, explicit placement, fallback behavior, custom offsets, and accessibility.

Changes

Submenu placement

Layer / File(s) Summary
Placement options and validation
modules/react/menu/lib/Submenu.tsx, modules/react/menu/stories/examples/ScrollableSubmenu.tsx, modules/react/menu/stories/Menu.stories.ts, cypress/component/SubmenuPlacement.spec.tsx
SubmenuPopper passes fallback placements and Popper options. When fallback placements are omitted, it disables the fallback modifier and limits flip behavior. The Storybook example exposes placement options. Cypress tests check placement at both menu edges, scrolling in LTR and RTL, custom offsets, and accessibility.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: mannycarrera4

Merge Risk: ⚪ Minimal · up to 2bf41

The change keeps default submenus beside their parent menu when it scrolls, while explicit placements and custom Popper options still apply. No merge-blocking risk was found.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 2bf41

The reviewed change adjusts submenu positioning without introducing a new security boundary or granting callers additional authority. Explicit positioning overrides were already supported. No introduced or materially worsened security risk was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected change affects submenu positioning and existing popup instance behavior within the browser document. The available consumer graph is capped, so this is a source-bounded assessment rather than a complete enumeration of downstream application exposure.

Security Findings and Attack Paths

  • inferred — No newly reachable privileged operation or weakened security control was identified in the reviewed flow. Custom modifier execution is not newly introduced: the base already forwarded caller Popper options to the same engine. Unexamined consumer trust relationships remain outside this conclusion.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: keeping submenus beside scrolled parent menus.
Linked Issues check ✅ Passed The PR addresses issue #4180. SubmenuPopper keeps the default right-start placement and disables alternate-axis and variation flips when no explicit fallback placements are provided. The offset ca…
Out of Scope Changes check ✅ Passed The changed files support issue #4180. ScrollableSubmenu and its Storybook story provide a reproducible case. The Cypress tests validate the required placement, scrolling, RTL, fallback, custom-opti…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@minwookshin minwookshin changed the title fix(menu): keep submenus beside scrolled parent menus fix(menu): Keep submenus beside scrolled parent menus Oct 1, 2026
@cypress

cypress Bot commented Oct 1, 2026

Copy link
Copy Markdown

Workday/canvas-kit    Run #11653

Run Properties:  status check failed Failed #11653  •  git commit f6575a3e18 ℹ️: Merge 098335aa2312079acdce689d37a2ca426ad29be9 into 8743b8d9f111efb6d11b7a8e2c5d...
Project Workday/canvas-kit
Branch Review fix/submenu-scroll-placement
Run status status check failed Failed #11653
Run duration 02m 30s
Commit git commit f6575a3e18 ℹ️: Merge 098335aa2312079acdce689d37a2ca426ad29be9 into 8743b8d9f111efb6d11b7a8e2c5d...
Committer Minwook Shin
View all properties for this run ↗︎

Test results
Tests that failed  Failures 2
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 4
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 362
View all changes introduced in this branch ↗︎

Warning

Partial Report: The results for the Application Quality reports may be incomplete.

UI Coverage  14.01%
  Untested elements 1547  
  Tested elements 252  
Accessibility  98.06%
  Failed rules  3 critical   3 serious   0 moderate   2 minor
  Failed elements 7  

Tests for review

Failed  SubmenuPlacement.spec.tsx • 2 failed tests

View Output

Test Artifacts
... > should remain beside the parent without covering it Test Replay Screenshots
... > should remain beside the parent without covering it Test Replay Screenshots
Failed  Popup.spec.tsx • 0 failed tests

View Output

Test Artifacts
Failed  Tooltip.spec.tsx • 0 failed tests

View Output

Test Artifacts
Failed  ColorPicker.spec.tsx • 0 failed tests

View Output

Test Artifacts
Failed  SegmentedControl.spec.tsx • 0 failed tests

View Output

Test Artifacts

The first 5 failed specs are shown, see all 29 specs in Cypress Cloud.

@minwookshin

Copy link
Copy Markdown
Author

Accounted for parent scrollbar gutters in submenu placement and made the regression reserve scrollbar space on every platform. The placement and existing menu browser tests, all 4,138 unit tests, typechecks, and lint pass locally.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Submenu covers parent menu when parent menu is scrolled

2 participants