Skip to content

Commit 4b0c841

Browse files
authored
Make popup menu hardcoded padding configurable (#150506)
## Description This PR exposed `PopupMenuButton.menuPadding` parameter to override the hardcoded value. Credits to @Moluram for the original PR flutter/flutter#81996. And to @arafaysaleem for the update in flutter/flutter#96657. flutter/flutter#96657 was reverted due to a Google testing failure. `PopupMenuButton` implementation has evolved since that time so maybe we will not hit this Google testing failure. And if we do, we will try to figure out what is going on. ## Related Issue Fixes flutter/flutter#143512. Fixes flutter/flutter#57110 ## Tests Adds 2 tests, updates several tests.
1 parent 80a65bd commit 4b0c841

5 files changed

Lines changed: 143 additions & 9 deletions

File tree

dev/tools/gen_defaults/lib/popup_menu_template.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,13 @@ class _${blockName}DefaultsM3 extends PopupMenuThemeData {
4444
@override
4545
ShapeBorder? get shape => ${shape("md.comp.menu.container")};
4646
47+
// TODO(bleroux): This is taken from https://m3.material.io/components/menus/specs
48+
// Update this when the token is available.
49+
@override
50+
EdgeInsets? get menuPadding => const EdgeInsets.symmetric(vertical: 8.0);
51+
4752
// TODO(tahatesser): This is taken from https://m3.material.io/components/menus/specs
4853
// Update this when the token is available.
49-
static EdgeInsets menuHorizontalPadding = const EdgeInsets.symmetric(horizontal: 12.0);
54+
static EdgeInsets menuItemPadding = const EdgeInsets.symmetric(horizontal: 12.0);
5055
}''';
5156
}

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

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ const double _kMenuCloseIntervalEnd = 2.0 / 3.0;
3939
const double _kMenuDividerHeight = 16.0;
4040
const double _kMenuMaxWidth = 5.0 * _kMenuWidthStep;
4141
const double _kMenuMinWidth = 2.0 * _kMenuWidthStep;
42-
const double _kMenuVerticalPadding = 8.0;
4342
const double _kMenuWidthStep = 56.0;
4443
const double _kMenuScreenPadding = 8.0;
4544

@@ -379,7 +378,7 @@ class PopupMenuItemState<T, W extends PopupMenuItem<T>> extends State<W> {
379378
child: Container(
380379
alignment: AlignmentDirectional.centerStart,
381380
constraints: BoxConstraints(minHeight: widget.height),
382-
padding: widget.padding ?? (theme.useMaterial3 ? _PopupMenuDefaultsM3.menuHorizontalPadding : _PopupMenuDefaultsM2.menuHorizontalPadding),
381+
padding: widget.padding ?? (theme.useMaterial3 ? _PopupMenuDefaultsM3.menuItemPadding : _PopupMenuDefaultsM2.menuItemPadding),
383382
child: buildChild(),
384383
),
385384
);
@@ -610,7 +609,6 @@ class _PopupMenuState<T> extends State<_PopupMenu<T>> {
610609
) {
611610
_setOpacities();
612611
}
613-
614612
}
615613

616614
void _setOpacities() {
@@ -687,9 +685,7 @@ class _PopupMenuState<T> extends State<_PopupMenu<T>> {
687685
explicitChildNodes: true,
688686
label: widget.semanticLabel,
689687
child: SingleChildScrollView(
690-
padding: const EdgeInsets.symmetric(
691-
vertical: _kMenuVerticalPadding,
692-
),
688+
padding: widget.route.menuPadding ?? popupMenuTheme.menuPadding ?? defaults.menuPadding,
693689
child: ListBody(children: children),
694690
),
695691
),
@@ -853,6 +849,7 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
853849
required this.barrierLabel,
854850
this.semanticLabel,
855851
this.shape,
852+
this.menuPadding,
856853
this.color,
857854
required this.capturedThemes,
858855
this.constraints,
@@ -874,6 +871,7 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
874871
final Color? shadowColor;
875872
final String? semanticLabel;
876873
final ShapeBorder? shape;
874+
final EdgeInsetsGeometry? menuPadding;
877875
final Color? color;
878876
final CapturedThemes capturedThemes;
879877
final BoxConstraints? constraints;
@@ -1040,6 +1038,7 @@ Future<T?> showMenu<T>({
10401038
Color? surfaceTintColor,
10411039
String? semanticLabel,
10421040
ShapeBorder? shape,
1041+
EdgeInsetsGeometry? menuPadding,
10431042
Color? color,
10441043
bool useRootNavigator = false,
10451044
BoxConstraints? constraints,
@@ -1074,6 +1073,7 @@ Future<T?> showMenu<T>({
10741073
semanticLabel: semanticLabel,
10751074
barrierLabel: MaterialLocalizations.of(context).menuDismissLabel,
10761075
shape: shape,
1076+
menuPadding: menuPadding,
10771077
color: color,
10781078
capturedThemes: InheritedTheme.capture(from: context, to: navigator.context),
10791079
constraints: constraints,
@@ -1194,6 +1194,7 @@ class PopupMenuButton<T> extends StatefulWidget {
11941194
this.shadowColor,
11951195
this.surfaceTintColor,
11961196
this.padding = const EdgeInsets.all(8.0),
1197+
this.menuPadding,
11971198
this.child,
11981199
this.splashRadius,
11991200
this.icon,
@@ -1274,6 +1275,14 @@ class PopupMenuButton<T> extends StatefulWidget {
12741275
/// to set the padding to zero.
12751276
final EdgeInsetsGeometry padding;
12761277

1278+
/// If provided, menu padding is used for empty space around the outside
1279+
/// of the popup menu.
1280+
///
1281+
/// If this property is null, then [PopupMenuThemeData.menuPadding] is used.
1282+
/// If [PopupMenuThemeData.menuPadding] is also null, then vertical padding
1283+
/// of 8 pixels is used.
1284+
final EdgeInsetsGeometry? menuPadding;
1285+
12771286
/// The splash radius.
12781287
///
12791288
/// If null, default splash radius of [InkWell] or [IconButton] is used.
@@ -1475,6 +1484,7 @@ class PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
14751484
initialValue: widget.initialValue,
14761485
position: position,
14771486
shape: widget.shape ?? popupMenuTheme.shape,
1487+
menuPadding: widget.menuPadding ?? popupMenuTheme.menuPadding,
14781488
color: widget.color ?? popupMenuTheme.color,
14791489
constraints: widget.constraints,
14801490
clipBehavior: widget.clipBehavior,
@@ -1571,7 +1581,10 @@ class _PopupMenuDefaultsM2 extends PopupMenuThemeData {
15711581
@override
15721582
TextStyle? get textStyle => _textTheme.titleMedium;
15731583

1574-
static EdgeInsets menuHorizontalPadding = const EdgeInsets.symmetric(horizontal: 16.0);
1584+
@override
1585+
EdgeInsets? get menuPadding => const EdgeInsets.symmetric(vertical: 8.0);
1586+
1587+
static EdgeInsets menuItemPadding = const EdgeInsets.symmetric(horizontal: 16.0);
15751588
}
15761589

15771590
// BEGIN GENERATED TOKEN PROPERTIES - PopupMenu
@@ -1613,8 +1626,13 @@ class _PopupMenuDefaultsM3 extends PopupMenuThemeData {
16131626
@override
16141627
ShapeBorder? get shape => const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(4.0)));
16151628

1629+
// TODO(bleroux): This is taken from https://m3.material.io/components/menus/specs
1630+
// Update this when the token is available.
1631+
@override
1632+
EdgeInsets? get menuPadding => const EdgeInsets.symmetric(vertical: 8.0);
1633+
16161634
// TODO(tahatesser): This is taken from https://m3.material.io/components/menus/specs
16171635
// Update this when the token is available.
1618-
static EdgeInsets menuHorizontalPadding = const EdgeInsets.symmetric(horizontal: 12.0);
1636+
static EdgeInsets menuItemPadding = const EdgeInsets.symmetric(horizontal: 12.0);
16191637
}
16201638
// END GENERATED TOKEN PROPERTIES - PopupMenu

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ class PopupMenuThemeData with Diagnosticable {
4747
const PopupMenuThemeData({
4848
this.color,
4949
this.shape,
50+
this.menuPadding,
5051
this.elevation,
5152
this.shadowColor,
5253
this.surfaceTintColor,
@@ -65,6 +66,11 @@ class PopupMenuThemeData with Diagnosticable {
6566
/// The shape of the popup menu.
6667
final ShapeBorder? shape;
6768

69+
/// If specified, the padding of the popup menu.
70+
///
71+
/// If [PopupMenuButton.menuPadding] is provided, [menuPadding] is ignored.
72+
final EdgeInsetsGeometry? menuPadding;
73+
6874
/// The elevation of the popup menu.
6975
final double? elevation;
7076

@@ -108,6 +114,7 @@ class PopupMenuThemeData with Diagnosticable {
108114
PopupMenuThemeData copyWith({
109115
Color? color,
110116
ShapeBorder? shape,
117+
EdgeInsetsGeometry? menuPadding,
111118
double? elevation,
112119
Color? shadowColor,
113120
Color? surfaceTintColor,
@@ -122,6 +129,7 @@ class PopupMenuThemeData with Diagnosticable {
122129
return PopupMenuThemeData(
123130
color: color ?? this.color,
124131
shape: shape ?? this.shape,
132+
menuPadding: menuPadding ?? this.menuPadding,
125133
elevation: elevation ?? this.elevation,
126134
shadowColor: shadowColor ?? this.shadowColor,
127135
surfaceTintColor: surfaceTintColor ?? this.surfaceTintColor,
@@ -147,6 +155,7 @@ class PopupMenuThemeData with Diagnosticable {
147155
return PopupMenuThemeData(
148156
color: Color.lerp(a?.color, b?.color, t),
149157
shape: ShapeBorder.lerp(a?.shape, b?.shape, t),
158+
menuPadding: EdgeInsetsGeometry.lerp(a?.menuPadding, b?.menuPadding, t),
150159
elevation: lerpDouble(a?.elevation, b?.elevation, t),
151160
shadowColor: Color.lerp(a?.shadowColor, b?.shadowColor, t),
152161
surfaceTintColor: Color.lerp(a?.surfaceTintColor, b?.surfaceTintColor, t),
@@ -164,6 +173,7 @@ class PopupMenuThemeData with Diagnosticable {
164173
int get hashCode => Object.hash(
165174
color,
166175
shape,
176+
menuPadding,
167177
elevation,
168178
shadowColor,
169179
surfaceTintColor,
@@ -187,6 +197,7 @@ class PopupMenuThemeData with Diagnosticable {
187197
return other is PopupMenuThemeData
188198
&& other.color == color
189199
&& other.shape == shape
200+
&& other.menuPadding == menuPadding
190201
&& other.elevation == elevation
191202
&& other.shadowColor == shadowColor
192203
&& other.surfaceTintColor == surfaceTintColor
@@ -204,6 +215,7 @@ class PopupMenuThemeData with Diagnosticable {
204215
super.debugFillProperties(properties);
205216
properties.add(ColorProperty('color', color, defaultValue: null));
206217
properties.add(DiagnosticsProperty<ShapeBorder>('shape', shape, defaultValue: null));
218+
properties.add(DiagnosticsProperty<EdgeInsetsGeometry>('menuPadding', menuPadding, defaultValue: null));
207219
properties.add(DoubleProperty('elevation', elevation, defaultValue: null));
208220
properties.add(ColorProperty('shadowColor', shadowColor, defaultValue: null));
209221
properties.add(ColorProperty('surfaceTintColor', surfaceTintColor, defaultValue: null));

packages/flutter/test/material/popup_menu_test.dart

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1671,6 +1671,44 @@ void main() {
16711671
expect(tester.widget<Container>(find.widgetWithText(Container, 'Item 1')).padding, const EdgeInsets.symmetric(horizontal: 12.0));
16721672
});
16731673

1674+
testWidgets('PopupMenu default padding', (WidgetTester tester) async {
1675+
final Key popupMenuButtonKey = UniqueKey();
1676+
await tester.pumpWidget(
1677+
MaterialApp(
1678+
home: Scaffold(
1679+
body: Center(
1680+
child: PopupMenuButton<String>(
1681+
key: popupMenuButtonKey,
1682+
child: const Text('button'),
1683+
onSelected: (String result) { },
1684+
itemBuilder: (BuildContext context) {
1685+
return <PopupMenuEntry<String>>[
1686+
const PopupMenuItem<String>(
1687+
value: '0',
1688+
enabled: false,
1689+
child: Text('Item 0'),
1690+
),
1691+
const PopupMenuItem<String>(
1692+
value: '1',
1693+
child: Text('Item 1'),
1694+
),
1695+
];
1696+
},
1697+
),
1698+
),
1699+
),
1700+
),
1701+
);
1702+
1703+
// Show the menu.
1704+
await tester.tap(find.byKey(popupMenuButtonKey));
1705+
await tester.pump(const Duration(milliseconds: 300));
1706+
1707+
// Check popup menu padding.
1708+
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
1709+
expect(popupMenu.padding, const EdgeInsets.symmetric(vertical: 8.0));
1710+
});
1711+
16741712
testWidgets('Material2 - PopupMenuItem default padding', (WidgetTester tester) async {
16751713
final Key popupMenuButtonKey = UniqueKey();
16761714
await tester.pumpWidget(
@@ -1709,6 +1747,45 @@ void main() {
17091747
expect(tester.widget<Container>(find.widgetWithText(Container, 'Item 1')).padding, const EdgeInsets.symmetric(horizontal: 16.0));
17101748
});
17111749

1750+
testWidgets('Material2 - PopupMenuItem default padding', (WidgetTester tester) async {
1751+
final Key popupMenuButtonKey = UniqueKey();
1752+
await tester.pumpWidget(
1753+
MaterialApp(
1754+
theme: ThemeData(useMaterial3: false),
1755+
home: Scaffold(
1756+
body: Center(
1757+
child: PopupMenuButton<String>(
1758+
key: popupMenuButtonKey,
1759+
child: const Text('button'),
1760+
onSelected: (String result) { },
1761+
itemBuilder: (BuildContext context) {
1762+
return <PopupMenuEntry<String>>[
1763+
const PopupMenuItem<String>(
1764+
value: '0',
1765+
enabled: false,
1766+
child: Text('Item 0'),
1767+
),
1768+
const PopupMenuItem<String>(
1769+
value: '1',
1770+
child: Text('Item 1'),
1771+
),
1772+
];
1773+
},
1774+
),
1775+
),
1776+
),
1777+
),
1778+
);
1779+
1780+
// Show the menu.
1781+
await tester.tap(find.byKey(popupMenuButtonKey));
1782+
await tester.pump(const Duration(milliseconds: 300));
1783+
1784+
// Check popup menu padding.
1785+
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
1786+
expect(popupMenu.padding, const EdgeInsets.symmetric(vertical: 8.0));
1787+
});
1788+
17121789
testWidgets('PopupMenuItem custom padding', (WidgetTester tester) async {
17131790
final Key popupMenuButtonKey = UniqueKey();
17141791
final Type menuItemType = const PopupMenuItem<String>(child: Text('item')).runtimeType;

packages/flutter/test/material/popup_menu_theme_test.dart

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ PopupMenuThemeData _popupMenuThemeM3() {
2626
return PopupMenuThemeData(
2727
color: Colors.orange,
2828
shape: const BeveledRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(12))),
29+
menuPadding: const EdgeInsets.symmetric(vertical: 9.0),
2930
elevation: 12.0,
3031
shadowColor: const Color(0xff00ff00),
3132
surfaceTintColor: const Color(0xff00ff00),
@@ -62,6 +63,7 @@ void main() {
6263
const PopupMenuThemeData popupMenuTheme = PopupMenuThemeData();
6364
expect(popupMenuTheme.color, null);
6465
expect(popupMenuTheme.shape, null);
66+
expect(popupMenuTheme.menuPadding, null);
6567
expect(popupMenuTheme.elevation, null);
6668
expect(popupMenuTheme.shadowColor, null);
6769
expect(popupMenuTheme.surfaceTintColor, null);
@@ -88,6 +90,7 @@ void main() {
8890
PopupMenuThemeData(
8991
color: const Color(0xfffffff1),
9092
shape: const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(2.0))),
93+
menuPadding: const EdgeInsets.symmetric(vertical: 12.0),
9194
elevation: 2.0,
9295
shadowColor: const Color(0xfffffff2),
9396
surfaceTintColor: const Color(0xfffffff3),
@@ -113,6 +116,7 @@ void main() {
113116
expect(description, <String>[
114117
'color: Color(0xfffffff1)',
115118
'shape: RoundedRectangleBorder(BorderSide(width: 0.0, style: none), BorderRadius.circular(2.0))',
119+
'menuPadding: EdgeInsets(0.0, 12.0, 0.0, 12.0)',
116120
'elevation: 2.0',
117121
'shadowColor: Color(0xfffffff2)',
118122
'surfaceTintColor: Color(0xfffffff3)',
@@ -244,6 +248,10 @@ void main() {
244248
// Test checked CheckedPopupMenuItem label.
245249
listTile = tester.widget<ListTile>(find.byType(ListTile).last);
246250
expect(listTile.titleTextStyle?.color, theme.colorScheme.onSurface);
251+
252+
// Check popup menu padding.
253+
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
254+
expect(popupMenu.padding, const EdgeInsets.symmetric(vertical: 8.0));
247255
});
248256

249257
testWidgets('Popup menu uses values from PopupMenuThemeData', (WidgetTester tester) async {
@@ -360,6 +368,10 @@ void main() {
360368
// Test checked CheckedPopupMenuItem label.
361369
listTile = tester.widget<ListTile>(find.byType(ListTile).last);
362370
expect(listTile.titleTextStyle, popupMenuTheme.labelTextStyle?.resolve(enabled));
371+
372+
// Check popup menu padding.
373+
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
374+
expect(popupMenu.padding, popupMenuTheme.menuPadding);
363375
});
364376

365377
testWidgets('Popup menu widget properties take priority over theme', (WidgetTester tester) async {
@@ -374,6 +386,7 @@ void main() {
374386
const ShapeBorder shape = RoundedRectangleBorder(
375387
borderRadius: BorderRadius.all(Radius.circular(9.0)),
376388
);
389+
const EdgeInsets menuPadding = EdgeInsets.zero;
377390
const double elevation = 7.0;
378391
const TextStyle textStyle = TextStyle(color: Color(0xfff14fff), fontSize: 19.0);
379392
const MouseCursor cursor = SystemMouseCursors.forbidden;
@@ -393,6 +406,7 @@ void main() {
393406
surfaceTintColor: surfaceTintColor,
394407
color: color,
395408
shape: shape,
409+
menuPadding: menuPadding,
396410
iconColor: iconColor,
397411
iconSize: iconSize,
398412
itemBuilder: (BuildContext context) {
@@ -460,6 +474,10 @@ void main() {
460474
// Test CheckedPopupMenuItem label.
461475
final ListTile listTile = tester.widget<ListTile>(find.byType(ListTile).first);
462476
expect(listTile.titleTextStyle, textStyle);
477+
478+
// Check popup menu padding.
479+
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
480+
expect(popupMenu.padding, EdgeInsets.zero);
463481
});
464482

465483
group('Material 2', () {
@@ -564,6 +582,10 @@ void main() {
564582
RendererBinding.instance.mouseTracker.debugDeviceActiveCursor(1),
565583
SystemMouseCursors.click,
566584
);
585+
586+
// Check popup menu padding.
587+
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
588+
expect(popupMenu.padding, const EdgeInsets.symmetric(vertical: 8.0));
567589
});
568590

569591
testWidgets('Popup menu uses values from PopupMenuThemeData', (WidgetTester tester) async {

0 commit comments

Comments
 (0)