Skip to content

Commit 00d9b6f

Browse files
huycozydkwingsmt
andauthored
Fix Menu anchor reduce padding on web and desktop (flutter#172691)
- Fix flutter#171608 - Demo before the fix: | desktop | web | | --------------- | --------------- | <img width="400" src="https://github.com/user-attachments/assets/1abd3962-14bb-497c-9bea-f0ff847e2dd6"> | <img width="400" src="https://github.com/user-attachments/assets/783d96b4-b93f-4b26-8cc7-abdbcf362992"> - Demo after the fix: | desktop | web | | --------------- | --------------- | <img width="400" src="https://github.com/user-attachments/assets/8a7442b9-77e6-477e-afd9-82a304fadcba"> | <img width="400" src="https://github.com/user-attachments/assets/029ccecf-3cf6-40b3-9233-7eb3d7a88eda"> - Description: On desktop/web platforms, VisualDensity.compact is applied by default, which caused the issue as reported. Previously, this compressed menus vertically. In this PR: - Vertical padding affected by visual density is removed to follow [Material Design specs](https://www.figma.com/community/file/1035203688168086460) so that visual density should not affect vertical menu padding anymore. (*) - Some of test values (y coordinate/height) are also updated to align with this change and also add a regression test - Enhance `MenuAnchor` documentation to prevent user confusion. <details open> <summary>(*) Material Design specs inspection from Figma</summary> https://github.com/user-attachments/assets/3e348a51-3917-4217-ad66-ef8cdff256ba </details> ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Signed-off-by: huycozy <[email protected]> Co-authored-by: Tong Mu <[email protected]>
1 parent 52a4d54 commit 00d9b6f

File tree

3 files changed

+117
-22
lines changed

3 files changed

+117
-22
lines changed

packages/flutter/lib/src/material/menu_anchor.dart

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,9 @@ class _MenuAnchorScope extends InheritedWidget {
148148
///
149149
/// ** See code in examples/api/lib/material/menu_anchor/menu_anchor.3.dart **
150150
/// {@end-tool}
151+
///
152+
/// The [MenuStyle.visualDensity] setting only affects horizontal padding,
153+
/// and it will never make it negative. Vertical padding is not affected at all.
151154
class MenuAnchor extends StatefulWidget {
152155
/// Creates a const [MenuAnchor].
153156
///
@@ -3173,11 +3176,11 @@ class _MenuPanelState extends State<_MenuPanel> {
31733176
// Per the Material Design team: don't allow the VisualDensity
31743177
// adjustment to reduce the width of the left/right padding. If we
31753178
// did, VisualDensity.compact, the default for desktop/web, would
3176-
// reduce the horizontal padding to zero.
3177-
final double dy = densityAdjustment.dy;
3179+
// reduce the horizontal padding to zero. Vertical padding
3180+
// is not affected at all.
31783181
final double dx = math.max(0, densityAdjustment.dx);
31793182
final EdgeInsetsGeometry resolvedPadding = padding
3180-
.add(EdgeInsets.symmetric(horizontal: dx, vertical: dy))
3183+
.add(EdgeInsets.symmetric(horizontal: dx))
31813184
.clamp(EdgeInsets.zero, EdgeInsetsGeometry.infinity);
31823185

31833186
BoxConstraints effectiveConstraints = visualDensity.effectiveConstraints(
@@ -3333,16 +3336,15 @@ class _Submenu extends StatelessWidget {
33333336
// adjustment to reduce the width of the left/right padding. If we
33343337
// did, VisualDensity.compact, the default for desktop/web, would
33353338
// reduce the horizontal padding to zero.
3336-
final double dy = densityAdjustment.dy;
33373339
final double dx = math.max(0, densityAdjustment.dx);
33383340
final EdgeInsetsGeometry resolvedPadding = padding
3339-
.add(EdgeInsets.fromLTRB(dx, dy, dx, dy))
3341+
.add(EdgeInsets.fromLTRB(dx, 0, dx, 0))
33403342
.clamp(EdgeInsets.zero, EdgeInsetsGeometry.infinity);
33413343

33423344
final Rect anchorRect = layerLink == null
33433345
? Rect.fromLTRB(
33443346
menuPosition.anchorRect.left + dx,
3345-
menuPosition.anchorRect.top - dy,
3347+
menuPosition.anchorRect.top,
33463348
menuPosition.anchorRect.right,
33473349
menuPosition.anchorRect.bottom,
33483350
)

packages/flutter/test/material/menu_anchor_test.dart

Lines changed: 107 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -271,15 +271,15 @@ void main() {
271271
);
272272
expect(
273273
tester.getRect(find.widgetWithText(MenuItemButton, TestMenu.subMenu10.label)),
274-
equals(const Rect.fromLTRB(265.0, 40.0, 467.0, 80.0)),
274+
equals(const Rect.fromLTRB(265.0, 48.0, 467.0, 88.0)),
275275
);
276276
expect(
277277
tester.getRect(
278278
find
279279
.ancestor(of: find.text(TestMenu.subMenu10.label), matching: find.byType(Material))
280280
.at(1),
281281
),
282-
equals(const Rect.fromLTRB(265.0, 40.0, 467.0, 160.0)),
282+
equals(const Rect.fromLTRB(265.0, 40.0, 467.0, 176.0)),
283283
);
284284

285285
await tester.pumpWidget(Container());
@@ -295,7 +295,7 @@ void main() {
295295
// 590 = 695 - 105
296296
expect(
297297
tester.getRect(find.byType(MenuBar)),
298-
equals(const Rect.fromLTRB(105.0, 0.0, 695.0, 72.0)),
298+
equals(const Rect.fromLTRB(105.0, 0.0, 695.0, 56.0)),
299299
);
300300

301301
// Open and make sure things are the right size.
@@ -304,23 +304,19 @@ void main() {
304304

305305
expect(
306306
tester.getRect(find.byType(MenuBar)),
307-
equals(const Rect.fromLTRB(105.0, 0.0, 695.0, 72.0)),
308-
);
309-
expect(
310-
tester.getRect(find.byType(MenuBar)),
311-
equals(const Rect.fromLTRB(105.0, 0.0, 695.0, 72.0)),
307+
equals(const Rect.fromLTRB(105.0, 0.0, 695.0, 56.0)),
312308
);
313309
expect(
314310
tester.getRect(find.widgetWithText(MenuItemButton, TestMenu.subMenu10.label)),
315-
equals(const Rect.fromLTRB(257.0, 80.0, 491.0, 136.0)),
311+
equals(const Rect.fromLTRB(257.0, 64.0, 491.0, 120.0)),
316312
);
317313
expect(
318314
tester.getRect(
319315
find
320316
.ancestor(of: find.text(TestMenu.subMenu10.label), matching: find.byType(Material))
321317
.at(1),
322318
),
323-
equals(const Rect.fromLTRB(249.0, 64.0, 499.0, 264.0)),
319+
equals(const Rect.fromLTRB(249.0, 56.0, 499.0, 240.0)),
324320
);
325321
});
326322

@@ -3803,8 +3799,8 @@ void main() {
38033799
collectSubmenuRects(),
38043800
equals(const <Rect>[
38053801
Rect.fromLTRB(161.0, 0.0, 639.0, 40.0),
3806-
Rect.fromLTRB(265.0, 40.0, 467.0, 160.0),
3807-
Rect.fromLTRB(467.0, 80.0, 707.0, 240.0),
3802+
Rect.fromLTRB(265.0, 40.0, 467.0, 176.0),
3803+
Rect.fromLTRB(467.0, 80.0, 707.0, 256.0),
38083804
]),
38093805
);
38103806
});
@@ -3819,8 +3815,8 @@ void main() {
38193815
collectSubmenuRects(),
38203816
equals(const <Rect>[
38213817
Rect.fromLTRB(161.0, 0.0, 639.0, 40.0),
3822-
Rect.fromLTRB(333.0, 40.0, 535.0, 160.0),
3823-
Rect.fromLTRB(93.0, 80.0, 333.0, 240.0),
3818+
Rect.fromLTRB(333.0, 40.0, 535.0, 176.0),
3819+
Rect.fromLTRB(93.0, 80.0, 333.0, 256.0),
38243820
]),
38253821
);
38263822
});
@@ -3943,6 +3939,103 @@ void main() {
39433939
},
39443940
);
39453941

3942+
// Regression test for https://github.com/flutter/flutter/issues/171608
3943+
testWidgets('Menu vertical padding should not be reduced with compact visual density', (
3944+
WidgetTester tester,
3945+
) async {
3946+
// Helper function to get menu padding by measuring first/last items.
3947+
(double, double) getMenuPadding() {
3948+
// Find any menu items that are available.
3949+
final Finder menuItems = find.byType(SubmenuButton);
3950+
if (menuItems.evaluate().length < 2) {
3951+
return (0.0, 0.0);
3952+
}
3953+
3954+
final Rect firstItem = tester.getRect(menuItems.first);
3955+
final Rect lastItem = tester.getRect(menuItems.last);
3956+
final Rect menuPanel = tester.getRect(find.byType(Material).last);
3957+
3958+
final double topPadding = firstItem.top - menuPanel.top;
3959+
final double bottomPadding = menuPanel.bottom - lastItem.bottom;
3960+
return (topPadding, bottomPadding);
3961+
}
3962+
3963+
Future<void> buildSimpleMenuAnchor(
3964+
TextDirection textDirection, {
3965+
VisualDensity visualDensity = VisualDensity.standard,
3966+
}) async {
3967+
await tester.pumpWidget(
3968+
MaterialApp(
3969+
theme: ThemeData(visualDensity: visualDensity),
3970+
home: Directionality(
3971+
textDirection: textDirection,
3972+
child: Scaffold(
3973+
body: MenuAnchor(
3974+
style: const MenuStyle(
3975+
padding: WidgetStatePropertyAll<EdgeInsets>(
3976+
EdgeInsets.symmetric(vertical: 12, horizontal: 4),
3977+
),
3978+
),
3979+
menuChildren: const <Widget>[
3980+
DecoratedBox(
3981+
decoration: BoxDecoration(color: Colors.blue),
3982+
child: Text('Text 1'),
3983+
),
3984+
DecoratedBox(
3985+
decoration: BoxDecoration(color: Colors.blue),
3986+
child: Text('Text 2'),
3987+
),
3988+
],
3989+
builder: (BuildContext context, MenuController controller, Widget? child) {
3990+
return TextButton(
3991+
onPressed: () {
3992+
if (controller.isOpen) {
3993+
controller.close();
3994+
} else {
3995+
controller.open();
3996+
}
3997+
},
3998+
child: const Text('OPEN MENU'),
3999+
);
4000+
},
4001+
),
4002+
),
4003+
),
4004+
),
4005+
);
4006+
4007+
await tester.tap(find.text('OPEN MENU'));
4008+
await tester.pump();
4009+
}
4010+
4011+
// Pump widget with standard visual density.
4012+
await buildSimpleMenuAnchor(TextDirection.ltr);
4013+
4014+
final (double topStandard, double bottomStandard) = getMenuPadding();
4015+
4016+
// Pump widget with compact visual density.
4017+
await buildSimpleMenuAnchor(TextDirection.ltr, visualDensity: VisualDensity.compact);
4018+
4019+
final (double topCompact, double bottomCompact) = getMenuPadding();
4020+
4021+
// Compare standard vs compact padding.
4022+
expect(
4023+
topCompact,
4024+
equals(topStandard),
4025+
reason:
4026+
'Compact visual density should not change top padding. '
4027+
'Standard: $topStandard, Compact: $topCompact',
4028+
);
4029+
4030+
expect(
4031+
bottomCompact,
4032+
equals(bottomStandard),
4033+
reason:
4034+
'Compact visual density should not change bottom padding. '
4035+
'Standard: $bottomStandard, Compact: $bottomCompact',
4036+
);
4037+
});
4038+
39464039
group('LocalizedShortcutLabeler', () {
39474040
testWidgets('getShortcutLabel returns the right labels', (WidgetTester tester) async {
39484041
String expectedMeta;

packages/flutter/test/material/menu_style_test.dart

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,15 +287,15 @@ void main() {
287287
);
288288
expect(
289289
tester.getRect(find.text(TestMenu.subMenu10.label)),
290-
equals(const Rect.fromLTRB(372.0, 68.0, 565.0, 82.0)),
290+
equals(const Rect.fromLTRB(372.0, 70.0, 565.0, 84.0)),
291291
);
292292
expect(
293293
tester.getRect(
294294
find
295295
.ancestor(of: find.text(TestMenu.subMenu10.label), matching: find.byType(Material))
296296
.at(1),
297297
),
298-
equals(const Rect.fromLTRB(352.0, 48.0, 585.0, 186.0)),
298+
equals(const Rect.fromLTRB(352.0, 48.0, 585.0, 190.0)),
299299
);
300300
});
301301
});

0 commit comments

Comments
 (0)