[MenuAnchor] fix: MenuAnchor submenu topStart alignment overlapping parent menu#187719
[MenuAnchor] fix: MenuAnchor submenu topStart alignment overlapping parent menu#187719lucas-goldner wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates the submenu positioning logic in MenuAnchor to correctly handle alignment for same-axis submenus by resolving the alignment against the text direction, and adds corresponding regression tests for both LTR and RTL layouts. The feedback suggests simplifying a switch statement on textDirection to an if statement to improve code readability.
| switch (textDirection) { | ||
| case TextDirection.rtl: | ||
| x -= childSize.width; | ||
| case TextDirection.ltr: | ||
| break; | ||
| } |
There was a problem hiding this comment.
Since textDirection is an enum with only two possible values (TextDirection.rtl and TextDirection.ltr), we can simplify this switch statement into a simple if statement. This improves readability and simplifies the code, adhering to the repository's general philosophy of optimizing for readability.
if (textDirection == TextDirection.rtl) {
x -= childSize.width;
}References
- Optimize for readability: Code is read more often than it is written. Suggest simplification and refactoring to enhance readability and maintainability. (link)
64c0e47 to
a463779
Compare
|
@ reviewer looking at this PR: Hi! I noticed that the Material library in flutter/flutter has been frozen since April 7th as part of the decoupling effort. Since this PR modifies button_style_button.dart in the Material library, should I port this to the material_ui package in flutter/packages instead? Or should I keep this PR open here as-is until migration instructions are provided? Thanks! |
|
@lucas-goldner We can continue the review here and push as far as we can. When the freeze is lifted we will publish a guide on how to port the changes. :) |
|
@davidhicks980 Would you mind reviewing this PR? |
|
Prior to this PR, the alignment specified the point on the anchor's surface that the top-left corner of the submenu was attached to: #181895 (comment) This PR is flipping the horizontal attachment point for same-axis submenus. This is a clever fix, but I think it may lead to unpredictable code. For example (using the code from the PR): New behavior:With Alignment(0.1, -1): 
With Alignment (-0.1, -1): 
Previous behavior:With Alignment(0.1, -1): 
With Alignment (-0.1, -1): 
If I was using MenuAnchor, I wouldn't expect a 0.2 change in horizontal alignment to flip my menu, especially when the menu isn't constrained by the screen. I think a better fix would be to add an additional alignment that lets users specify the attachment point on the menu itself. This is how most other menus and windowing systems behave: |
Description
When a
SubmenuButtoninside a vertical menu usesAlignmentDirectional.topStartin itsmenuStyle, the submenu should open to the left of the parent menu (in LTR). Previously, it overlapped the parent menu because the x-position adjustment in_MenuLayout._positionChild()only considered RTL text direction, not the resolved alignment direction for same-axis submenus.Before (bug)
The submenu's top-left corner was placed at the parent menu item's top-left corner, causing the submenu to overlap the parent menu
before.mov
After (fix)
The submenu's top-right edge aligns with the parent menu item's left edge, opening the submenu to the left as expected.
after.mov
What changed
In
_positionChild(), the x-position adjustment now distinguishes between:resolvedAlignment.x < 0, the submenu grows leftward.Fixes #181895
Pre-launch Checklist
///).