-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix MenuItemButton overflow
#143932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix MenuItemButton overflow
#143932
Conversation
5b73649 to
a0c4069
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your continued help with the fix! I'm actually thinking about the property name. Based on the doc and the name, it feels like setting menuDirection: Axis.vertical can control the menu to make it show up vertically. Not sure if that can cause any confusion.
I'm now actually thinking maybe we don't need to add an extra property here. In _MenuItemButtonState, we can have a
_MenuAnchorState? get _anchor => _MenuAnchorState._maybeOf(context);then we can just use _anchor._orientation to get the menu direction information(vertical or horizontal). Do you think if this makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would only work for MenuItemButton created by MenuAnchor.
When using MenuAnchor, DropdownMenu, and SubmenuButton as these create MenuAnchor state. The _anchor._orientation is null when MenuItemButton is used directly. The users might use this without MenuAnchor, as seen in #129439
So the following example would still overflow as the button is directly created.
SizedBox(
width: 200,
child: MenuItemButton(
onPressed: () {},
leadingIcon: const Icon(Icons.menu),
trailingIcon: const Icon(Icons.arrow_forward_ios),
child: const Text(
'This is a very long text that will wrap to the multiple lines.',
maxLines: 1,
overflow: TextOverflow.ellipsis,
),
),
),Instead of just menuDirection, we could call it menuFlexDirection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When writing test for this property, I did write comments to make it clear how this property is adjusted for the layout to flex in certain direction.
// Test a long MenuItemButton in an unconstrained layout with vertical menuDirection.
await tester.pumpWidget(buildMenuButton(menuDirection: Axis.vertical));
expect(tester.getSize(find.byType(MenuItemButton)), const Size(800.0, 48.0));
// Test a long MenuItemButton in an unconstrained layout with horizontal menuDirection.
await tester.pumpWidget(buildMenuButton(menuDirection: Axis.horizontal));
expect(tester.getSize(find.byType(MenuItemButton)), const Size(800.0, 48.0));
// Test a long MenuItemButton in a constrained layout with vertical menuDirection.
await tester.pumpWidget(buildMenuButton(menuDirection: Axis.vertical, constrainedLayout: true));
expect(tester.getSize(find.byType(MenuItemButton)), const Size(200.0, 120.0));
// Test a long MenuItemButton in a constrained layout with horizontal menuDirection.
await tester.pumpWidget(buildMenuButton(menuDirection: Axis.horizontal, constrainedLayout: true));
expect(tester.getSize(find.byType(MenuItemButton)), const Size(200.0, 48.0));
// This should throw an error.
final AssertionError exception = tester.takeException() as AssertionError;
expect(exception, isAssertionError);You're absolutely right we need to make clear for the users to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _anchor._orientation is null when MenuItemButton is used directly.
I see! You are right. I'm thinking about 3 options. One is still getting the direction from anchor, but we need to explicitly mention in the doc that these buttons are just used for menus; Other use cases might cause overflow. Because I'm not sure whether menuItemButton should be used without any menu involved🤣.
The second option is just to give a better property name, menuFlexDirection is a good one, I'm also thinking about a boolean allowHorizontalExpansion or something else. If the button is placed on a vertical menu, it is false and we do clip to avoid overflow; if it is placed on a horizontal menu, we should allow the expansion. But for this option, it is still a breaking change for MenuBars if they already use MenuItemButton. The default property value will cause exception and users need to explicitly assign a value to menuFlexDirection.
The third one is to combine the first two. We still expose this property, but if anchor is not null, the anchor direction should override the property to avoid breaking change.🙂 Let me know which one is preferred!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also let me know if I missed any other PR review!🙂 Thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the name and condition where _anchor orientational can override the property. This implemented the third option you suggest.
01006f3
Please let me know what do you think.
a0c4069 to
6a7d5ac
Compare
a744054 to
3e9a930
Compare
|
This is failing HTML tests for some reason which didn't fail earlier. I was busy updating others today, i will finish updating this by tomorrow. |
I see! Please take your time! As for the web tests, it looks like #142804 also has a similar web failure and the failure is also about |
|
I tried running canvaskit tests locally, they pass. Something off with HTML renderer and computeDryLayout. |
|
The HTML render probably doesn't handle FP errors while doing line breaks. In that failing test, the input max width is |
3e9a930 to
03e1ab2
Compare
QuncCccccc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM:) Thanks for the fix!
|
auto label is removed for flutter/flutter/143932, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
01006f3 to
a5575cc
Compare
|
@QuncCccccc |
Checking! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about overflowDirection or overflowAxis as a name? This one isn't great: it has menu, which is redundant, and Flex which might indicate a Flex widget or just being "flexible", which this isn't.
This seems because when the button is used in a |
cc684a3 to
bcc1d29
Compare
This looks like an expected error given the layout. We want menu item button with vertical direction to take maximum available width (e.g. in The user have to wrap the widget |
Yes, you are right. For the default setting, I'm thinking whether we should set the default value to be |
8df795b to
21b17fd
Compare
gspencergoog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MenuItemButton is not really created by them, but it can appear as their child. Might want to reword this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Sounds good to me. |
b3a139b to
f961094
Compare
|
@QuncCccccc |
Roll Flutter from a418568 to e868e2b (34 revisions) flutter/flutter@a418568...e868e2b 2024-04-03 [email protected] Add SegmentedButton expand feature (flutter/flutter#142804) 2024-04-03 [email protected] Roll Flutter Engine from e36b9b10c36f to 56fa2c33a5f7 (1 revision) (flutter/flutter#146205) 2024-04-03 [email protected] Roll Packages from 83f3842 to 0e848fa (3 revisions) (flutter/flutter#146201) 2024-04-03 [email protected] Enhance ColorScheme.fromSeed with a new `variant` parameter (flutter/flutter#144805) 2024-04-03 [email protected] Roll Flutter Engine from 0280de5be276 to e36b9b10c36f (1 revision) (flutter/flutter#146200) 2024-04-03 [email protected] Fix typos in bottom_sheet.dart (flutter/flutter#146188) 2024-04-03 [email protected] Roll Flutter Engine from 979030d81f8d to 0280de5be276 (2 revisions) (flutter/flutter#146199) 2024-04-03 [email protected] Roll Flutter Engine from bef3bbe3f74e to 979030d81f8d (1 revision) (flutter/flutter#146186) 2024-04-03 [email protected] Roll Flutter Engine from 5fc83bc24b2e to bef3bbe3f74e (1 revision) (flutter/flutter#146183) 2024-04-03 [email protected] Roll Flutter Engine from 0da1b2eb370a to 5fc83bc24b2e (1 revision) (flutter/flutter#146180) 2024-04-03 [email protected] Avoid calling `TextPainter.plainText` for simple static text (flutter/flutter#146084) 2024-04-03 [email protected] Roll Flutter Engine from e603f89844a9 to 0da1b2eb370a (2 revisions) (flutter/flutter#146179) 2024-04-03 [email protected] Roll Flutter Engine from ef60a95d78c1 to e603f89844a9 (3 revisions) (flutter/flutter#146177) 2024-04-03 [email protected] Fix chip baseline implementation (flutter/flutter#146162) 2024-04-03 [email protected] Roll Flutter Engine from bb4ec2d7eb39 to ef60a95d78c1 (1 revision) (flutter/flutter#146176) 2024-04-03 [email protected] Add tests for material_state_mouse_cursor.0.dart API example. (flutter/flutter#145987) 2024-04-03 [email protected] Update material_color_utilities package version to latest 0.11.1 (flutter/flutter#145959) 2024-04-03 [email protected] Roll Flutter Engine from 5f6dec8bd877 to bb4ec2d7eb39 (4 revisions) (flutter/flutter#146169) 2024-04-03 [email protected] Roll Flutter Engine from 5dbcfdc2a456 to 5f6dec8bd877 (1 revision) (flutter/flutter#146163) 2024-04-02 [email protected] Dispose FocusNode in tests. (flutter/flutter#146161) 2024-04-02 [email protected] Add `none` language strings to code blocks. (flutter/flutter#146154) 2024-04-02 [email protected] Refactor docs (flutter/flutter#145998) 2024-04-02 [email protected] Roll Flutter Engine from c60b00a20fc3 to 5dbcfdc2a456 (3 revisions) (flutter/flutter#146159) 2024-04-02 [email protected] Marks Linux_pixel_7pro complex_layout_scroll_perf_impeller__timeline_summary to be unflaky (flutter/flutter#140038) 2024-04-02 [email protected] Roll Flutter Engine from 5bf8b94505a4 to c60b00a20fc3 (2 revisions) (flutter/flutter#146157) 2024-04-02 [email protected] Refactor analyze (flutter/flutter#146138) 2024-04-02 [email protected] Implement SelectionArea triple click gestures (flutter/flutter#144563) 2024-04-02 [email protected] Roll Flutter Engine from 6883f7313da0 to 5bf8b94505a4 (2 revisions) (flutter/flutter#146152) 2024-04-02 [email protected] Fix border color is wrong for a focused and hovered TextField (flutter/flutter#146127) 2024-04-02 [email protected] Sync lints and enable `annotate_redeclares` (flutter/flutter#146144) 2024-04-02 [email protected] Implements `RenderBox.computeDryBaseline` for material render boxes (flutter/flutter#146027) 2024-04-02 [email protected] Roll Flutter Engine from 523fc953ebc8 to 6883f7313da0 (2 revisions) (flutter/flutter#146140) 2024-04-02 [email protected] Fix `MenuItemButton` overflow (flutter/flutter#143932) 2024-04-02 [email protected] Roll Packages from d5aff19 to 83f3842 (4 revisions) (flutter/flutter#146134) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 ...
Roll Flutter from a418568 to e868e2b (34 revisions) flutter/flutter@a418568...e868e2b 2024-04-03 [email protected] Add SegmentedButton expand feature (flutter/flutter#142804) 2024-04-03 [email protected] Roll Flutter Engine from e36b9b10c36f to 56fa2c33a5f7 (1 revision) (flutter/flutter#146205) 2024-04-03 [email protected] Roll Packages from 83f3842 to 0e848fa (3 revisions) (flutter/flutter#146201) 2024-04-03 [email protected] Enhance ColorScheme.fromSeed with a new `variant` parameter (flutter/flutter#144805) 2024-04-03 [email protected] Roll Flutter Engine from 0280de5be276 to e36b9b10c36f (1 revision) (flutter/flutter#146200) 2024-04-03 [email protected] Fix typos in bottom_sheet.dart (flutter/flutter#146188) 2024-04-03 [email protected] Roll Flutter Engine from 979030d81f8d to 0280de5be276 (2 revisions) (flutter/flutter#146199) 2024-04-03 [email protected] Roll Flutter Engine from bef3bbe3f74e to 979030d81f8d (1 revision) (flutter/flutter#146186) 2024-04-03 [email protected] Roll Flutter Engine from 5fc83bc24b2e to bef3bbe3f74e (1 revision) (flutter/flutter#146183) 2024-04-03 [email protected] Roll Flutter Engine from 0da1b2eb370a to 5fc83bc24b2e (1 revision) (flutter/flutter#146180) 2024-04-03 [email protected] Avoid calling `TextPainter.plainText` for simple static text (flutter/flutter#146084) 2024-04-03 [email protected] Roll Flutter Engine from e603f89844a9 to 0da1b2eb370a (2 revisions) (flutter/flutter#146179) 2024-04-03 [email protected] Roll Flutter Engine from ef60a95d78c1 to e603f89844a9 (3 revisions) (flutter/flutter#146177) 2024-04-03 [email protected] Fix chip baseline implementation (flutter/flutter#146162) 2024-04-03 [email protected] Roll Flutter Engine from bb4ec2d7eb39 to ef60a95d78c1 (1 revision) (flutter/flutter#146176) 2024-04-03 [email protected] Add tests for material_state_mouse_cursor.0.dart API example. (flutter/flutter#145987) 2024-04-03 [email protected] Update material_color_utilities package version to latest 0.11.1 (flutter/flutter#145959) 2024-04-03 [email protected] Roll Flutter Engine from 5f6dec8bd877 to bb4ec2d7eb39 (4 revisions) (flutter/flutter#146169) 2024-04-03 [email protected] Roll Flutter Engine from 5dbcfdc2a456 to 5f6dec8bd877 (1 revision) (flutter/flutter#146163) 2024-04-02 [email protected] Dispose FocusNode in tests. (flutter/flutter#146161) 2024-04-02 [email protected] Add `none` language strings to code blocks. (flutter/flutter#146154) 2024-04-02 [email protected] Refactor docs (flutter/flutter#145998) 2024-04-02 [email protected] Roll Flutter Engine from c60b00a20fc3 to 5dbcfdc2a456 (3 revisions) (flutter/flutter#146159) 2024-04-02 [email protected] Marks Linux_pixel_7pro complex_layout_scroll_perf_impeller__timeline_summary to be unflaky (flutter/flutter#140038) 2024-04-02 [email protected] Roll Flutter Engine from 5bf8b94505a4 to c60b00a20fc3 (2 revisions) (flutter/flutter#146157) 2024-04-02 [email protected] Refactor analyze (flutter/flutter#146138) 2024-04-02 [email protected] Implement SelectionArea triple click gestures (flutter/flutter#144563) 2024-04-02 [email protected] Roll Flutter Engine from 6883f7313da0 to 5bf8b94505a4 (2 revisions) (flutter/flutter#146152) 2024-04-02 [email protected] Fix border color is wrong for a focused and hovered TextField (flutter/flutter#146127) 2024-04-02 [email protected] Sync lints and enable `annotate_redeclares` (flutter/flutter#146144) 2024-04-02 [email protected] Implements `RenderBox.computeDryBaseline` for material render boxes (flutter/flutter#146027) 2024-04-02 [email protected] Roll Flutter Engine from 523fc953ebc8 to 6883f7313da0 (2 revisions) (flutter/flutter#146140) 2024-04-02 [email protected] Fix `MenuItemButton` overflow (flutter/flutter#143932) 2024-04-02 [email protected] Roll Packages from d5aff19 to 83f3842 (4 revisions) (flutter/flutter#146134) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 ...




fixes MenuItemButton does not constrain its child
fixes DropdownMenuEntry Text Overflow when width of DropdownMenu is not specified
Description
MenuBarchildren to fix the unbounded width issue which blocked the PR earlier. (Widgets with non-zero flex value cannot be laid out in a horizontal scroll view which is created byMenuBarwidget)Code sample
expand to view the code sample
Before
After
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.