development/flutter
development/flutter
1
0
Fork 0
mirror of https://github.com/flutter/flutter synced 2024-08-24 10:27:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

Make popup menu hardcoded padding configurable (#150506)

Browse source 
## Description

This PR exposed `PopupMenuButton.menuPadding` parameter to override the hardcoded value.

Credits to @Moluram for the original PR https://github.com/flutter/flutter/pull/81996.
And to @arafaysaleem for the update in https://github.com/flutter/flutter/pull/96657.

https://github.com/flutter/flutter/pull/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 https://github.com/flutter/flutter/issues/143512.
Fixes https://github.com/flutter/flutter/issues/57110

## Tests

Adds 2 tests, updates several tests.
This commit is contained in:
Bruno Leroux 2024-06-20 21:18:21 +02:00 committed by GitHub
parent 80a65bd781
commit 4b0c8414fc
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 143 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
dev/tools/gen_defaults/lib/popup_menu_template.dart
View file

@ -44,8 +44,13 @@ class _${blockName}DefaultsM3 extends PopupMenuThemeData {
@override
ShapeBorder? get shape => ${shape("md.comp.menu.container")};
// TODO(bleroux): This is taken from https://m3.material.io/components/menus/specs
// Update this when the token is available.
@override
EdgeInsets? get menuPadding => const EdgeInsets.symmetric(vertical: 8.0);
// TODO(tahatesser): This is taken from https://m3.material.io/components/menus/specs
// Update this when the token is available.
static EdgeInsets menuHorizontalPadding = const EdgeInsets.symmetric(horizontal: 12.0);
static EdgeInsets menuItemPadding = const EdgeInsets.symmetric(horizontal: 12.0);
}''';
}

34
packages/flutter/lib/src/material/popup_menu.dart
View file

@ -39,7 +39,6 @@ const double _kMenuCloseIntervalEnd = 2.0 / 3.0;
const double _kMenuDividerHeight = 16.0;
const double _kMenuMaxWidth = 5.0 * _kMenuWidthStep;
const double _kMenuMinWidth = 2.0 * _kMenuWidthStep;
const double _kMenuVerticalPadding = 8.0;
const double _kMenuWidthStep = 56.0;
const double _kMenuScreenPadding = 8.0;
@ -379,7 +378,7 @@ class PopupMenuItemState<T, W extends PopupMenuItem<T>> extends State<W> {
child: Container(
alignment: AlignmentDirectional.centerStart,
constraints: BoxConstraints(minHeight: widget.height),
padding: widget.padding ?? (theme.useMaterial3 ? _PopupMenuDefaultsM3.menuHorizontalPadding : _PopupMenuDefaultsM2.menuHorizontalPadding),
padding: widget.padding ?? (theme.useMaterial3 ? _PopupMenuDefaultsM3.menuItemPadding : _PopupMenuDefaultsM2.menuItemPadding),
child: buildChild(),
),
);
@ -610,7 +609,6 @@ class _PopupMenuState<T> extends State<_PopupMenu<T>> {
) {
_setOpacities();
}
}
void _setOpacities() {
@ -687,9 +685,7 @@ class _PopupMenuState<T> extends State<_PopupMenu<T>> {
explicitChildNodes: true,
label: widget.semanticLabel,
child: SingleChildScrollView(
padding: const EdgeInsets.symmetric(
vertical: _kMenuVerticalPadding,
),
padding: widget.route.menuPadding ?? popupMenuTheme.menuPadding ?? defaults.menuPadding,
child: ListBody(children: children),
),
),
@ -853,6 +849,7 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
required this.barrierLabel,
this.semanticLabel,
this.shape,
this.menuPadding,
this.color,
required this.capturedThemes,
this.constraints,
@ -874,6 +871,7 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
final Color? shadowColor;
final String? semanticLabel;
final ShapeBorder? shape;
final EdgeInsetsGeometry? menuPadding;
final Color? color;
final CapturedThemes capturedThemes;
final BoxConstraints? constraints;
@ -1040,6 +1038,7 @@ Future<T?> showMenu<T>({
Color? surfaceTintColor,
String? semanticLabel,
ShapeBorder? shape,
EdgeInsetsGeometry? menuPadding,
Color? color,
bool useRootNavigator = false,
BoxConstraints? constraints,
@ -1074,6 +1073,7 @@ Future<T?> showMenu<T>({
semanticLabel: semanticLabel,
barrierLabel: MaterialLocalizations.of(context).menuDismissLabel,
shape: shape,
menuPadding: menuPadding,
color: color,
capturedThemes: InheritedTheme.capture(from: context, to: navigator.context),
constraints: constraints,
@ -1194,6 +1194,7 @@ class PopupMenuButton<T> extends StatefulWidget {
this.shadowColor,
this.surfaceTintColor,
this.padding = const EdgeInsets.all(8.0),
this.menuPadding,
this.child,
this.splashRadius,
this.icon,
@ -1274,6 +1275,14 @@ class PopupMenuButton<T> extends StatefulWidget {
/// to set the padding to zero.
final EdgeInsetsGeometry padding;
/// If provided, menu padding is used for empty space around the outside
/// of the popup menu.
///
/// If this property is null, then [PopupMenuThemeData.menuPadding] is used.
/// If [PopupMenuThemeData.menuPadding] is also null, then vertical padding
/// of 8 pixels is used.
final EdgeInsetsGeometry? menuPadding;
/// The splash radius.
///
/// If null, default splash radius of [InkWell] or [IconButton] is used.
@ -1475,6 +1484,7 @@ class PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
initialValue: widget.initialValue,
position: position,
shape: widget.shape ?? popupMenuTheme.shape,
menuPadding: widget.menuPadding ?? popupMenuTheme.menuPadding,
color: widget.color ?? popupMenuTheme.color,
constraints: widget.constraints,
clipBehavior: widget.clipBehavior,
@ -1571,7 +1581,10 @@ class _PopupMenuDefaultsM2 extends PopupMenuThemeData {
@override
TextStyle? get textStyle => _textTheme.titleMedium;
static EdgeInsets menuHorizontalPadding = const EdgeInsets.symmetric(horizontal: 16.0);
@override
EdgeInsets? get menuPadding => const EdgeInsets.symmetric(vertical: 8.0);
static EdgeInsets menuItemPadding = const EdgeInsets.symmetric(horizontal: 16.0);
}
// BEGIN GENERATED TOKEN PROPERTIES - PopupMenu
@ -1613,8 +1626,13 @@ class _PopupMenuDefaultsM3 extends PopupMenuThemeData {
@override
ShapeBorder? get shape => const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(4.0)));
// TODO(bleroux): This is taken from https://m3.material.io/components/menus/specs
// Update this when the token is available.
@override
EdgeInsets? get menuPadding => const EdgeInsets.symmetric(vertical: 8.0);
// TODO(tahatesser): This is taken from https://m3.material.io/components/menus/specs
// Update this when the token is available.
static EdgeInsets menuHorizontalPadding = const EdgeInsets.symmetric(horizontal: 12.0);
static EdgeInsets menuItemPadding = const EdgeInsets.symmetric(horizontal: 12.0);
}
// END GENERATED TOKEN PROPERTIES - PopupMenu

12
packages/flutter/lib/src/material/popup_menu_theme.dart
View file

@ -47,6 +47,7 @@ class PopupMenuThemeData with Diagnosticable {
const PopupMenuThemeData({
this.color,
this.shape,
this.menuPadding,
this.elevation,
this.shadowColor,
this.surfaceTintColor,
@ -65,6 +66,11 @@ class PopupMenuThemeData with Diagnosticable {
/// The shape of the popup menu.
final ShapeBorder? shape;
/// If specified, the padding of the popup menu.
///
/// If [PopupMenuButton.menuPadding] is provided, [menuPadding] is ignored.
final EdgeInsetsGeometry? menuPadding;
/// The elevation of the popup menu.
final double? elevation;
@ -108,6 +114,7 @@ class PopupMenuThemeData with Diagnosticable {
PopupMenuThemeData copyWith({
Color? color,
ShapeBorder? shape,
EdgeInsetsGeometry? menuPadding,
double? elevation,
Color? shadowColor,
Color? surfaceTintColor,
@ -122,6 +129,7 @@ class PopupMenuThemeData with Diagnosticable {
return PopupMenuThemeData(
color: color ?? this.color,
shape: shape ?? this.shape,
menuPadding: menuPadding ?? this.menuPadding,
elevation: elevation ?? this.elevation,
shadowColor: shadowColor ?? this.shadowColor,
surfaceTintColor: surfaceTintColor ?? this.surfaceTintColor,
@ -147,6 +155,7 @@ class PopupMenuThemeData with Diagnosticable {
return PopupMenuThemeData(
color: Color.lerp(a?.color, b?.color, t),
shape: ShapeBorder.lerp(a?.shape, b?.shape, t),
menuPadding: EdgeInsetsGeometry.lerp(a?.menuPadding, b?.menuPadding, t),
elevation: lerpDouble(a?.elevation, b?.elevation, t),
shadowColor: Color.lerp(a?.shadowColor, b?.shadowColor, t),
surfaceTintColor: Color.lerp(a?.surfaceTintColor, b?.surfaceTintColor, t),
@ -164,6 +173,7 @@ class PopupMenuThemeData with Diagnosticable {
int get hashCode => Object.hash(
color,
shape,
menuPadding,
elevation,
shadowColor,
surfaceTintColor,
@ -187,6 +197,7 @@ class PopupMenuThemeData with Diagnosticable {
return other is PopupMenuThemeData
&& other.color == color
&& other.shape == shape
&& other.menuPadding == menuPadding
&& other.elevation == elevation
&& other.shadowColor == shadowColor
&& other.surfaceTintColor == surfaceTintColor
@ -204,6 +215,7 @@ class PopupMenuThemeData with Diagnosticable {
super.debugFillProperties(properties);
properties.add(ColorProperty('color', color, defaultValue: null));
properties.add(DiagnosticsProperty<ShapeBorder>('shape', shape, defaultValue: null));
properties.add(DiagnosticsProperty<EdgeInsetsGeometry>('menuPadding', menuPadding, defaultValue: null));
properties.add(DoubleProperty('elevation', elevation, defaultValue: null));
properties.add(ColorProperty('shadowColor', shadowColor, defaultValue: null));
properties.add(ColorProperty('surfaceTintColor', surfaceTintColor, defaultValue: null));

77
packages/flutter/test/material/popup_menu_test.dart
View file

@ -1671,6 +1671,44 @@ void main() {
expect(tester.widget<Container>(find.widgetWithText(Container, 'Item 1')).padding, const EdgeInsets.symmetric(horizontal: 12.0));
});
testWidgets('PopupMenu default padding', (WidgetTester tester) async {
final Key popupMenuButtonKey = UniqueKey();
await tester.pumpWidget(
MaterialApp(
home: Scaffold(
body: Center(
child: PopupMenuButton<String>(
key: popupMenuButtonKey,
child: const Text('button'),
onSelected: (String result) { },
itemBuilder: (BuildContext context) {
return <PopupMenuEntry<String>>[
const PopupMenuItem<String>(
value: '0',
enabled: false,
child: Text('Item 0'),
),
const PopupMenuItem<String>(
value: '1',
child: Text('Item 1'),
),
];
},
),
),
),
),
);
// Show the menu.
await tester.tap(find.byKey(popupMenuButtonKey));
await tester.pump(const Duration(milliseconds: 300));
// Check popup menu padding.
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
expect(popupMenu.padding, const EdgeInsets.symmetric(vertical: 8.0));
});
testWidgets('Material2 - PopupMenuItem default padding', (WidgetTester tester) async {
final Key popupMenuButtonKey = UniqueKey();
await tester.pumpWidget(
@ -1709,6 +1747,45 @@ void main() {
expect(tester.widget<Container>(find.widgetWithText(Container, 'Item 1')).padding, const EdgeInsets.symmetric(horizontal: 16.0));
});
testWidgets('Material2 - PopupMenuItem default padding', (WidgetTester tester) async {
final Key popupMenuButtonKey = UniqueKey();
await tester.pumpWidget(
MaterialApp(
theme: ThemeData(useMaterial3: false),
home: Scaffold(
body: Center(
child: PopupMenuButton<String>(
key: popupMenuButtonKey,
child: const Text('button'),
onSelected: (String result) { },
itemBuilder: (BuildContext context) {
return <PopupMenuEntry<String>>[
const PopupMenuItem<String>(
value: '0',
enabled: false,
child: Text('Item 0'),
),
const PopupMenuItem<String>(
value: '1',
child: Text('Item 1'),
),
];
},
),
),
),
),
);
// Show the menu.
await tester.tap(find.byKey(popupMenuButtonKey));
await tester.pump(const Duration(milliseconds: 300));
// Check popup menu padding.
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
expect(popupMenu.padding, const EdgeInsets.symmetric(vertical: 8.0));
});
testWidgets('PopupMenuItem custom padding', (WidgetTester tester) async {
final Key popupMenuButtonKey = UniqueKey();
final Type menuItemType = const PopupMenuItem<String>(child: Text('item')).runtimeType;

22
packages/flutter/test/material/popup_menu_theme_test.dart
View file

@ -26,6 +26,7 @@ PopupMenuThemeData _popupMenuThemeM3() {
return PopupMenuThemeData(
color: Colors.orange,
shape: const BeveledRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(12))),
menuPadding: const EdgeInsets.symmetric(vertical: 9.0),
elevation: 12.0,
shadowColor: const Color(0xff00ff00),
surfaceTintColor: const Color(0xff00ff00),
@ -62,6 +63,7 @@ void main() {
const PopupMenuThemeData popupMenuTheme = PopupMenuThemeData();
expect(popupMenuTheme.color, null);
expect(popupMenuTheme.shape, null);
expect(popupMenuTheme.menuPadding, null);
expect(popupMenuTheme.elevation, null);
expect(popupMenuTheme.shadowColor, null);
expect(popupMenuTheme.surfaceTintColor, null);
@ -88,6 +90,7 @@ void main() {
PopupMenuThemeData(
color: const Color(0xfffffff1),
shape: const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(2.0))),
menuPadding: const EdgeInsets.symmetric(vertical: 12.0),
elevation: 2.0,
shadowColor: const Color(0xfffffff2),
surfaceTintColor: const Color(0xfffffff3),
@ -113,6 +116,7 @@ void main() {
expect(description, <String>[
'color: Color(0xfffffff1)',
'shape: RoundedRectangleBorder(BorderSide(width: 0.0, style: none), BorderRadius.circular(2.0))',
'menuPadding: EdgeInsets(0.0, 12.0, 0.0, 12.0)',
'elevation: 2.0',
'shadowColor: Color(0xfffffff2)',
'surfaceTintColor: Color(0xfffffff3)',
@ -244,6 +248,10 @@ void main() {
// Test checked CheckedPopupMenuItem label.
listTile = tester.widget<ListTile>(find.byType(ListTile).last);
expect(listTile.titleTextStyle?.color, theme.colorScheme.onSurface);
// Check popup menu padding.
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
expect(popupMenu.padding, const EdgeInsets.symmetric(vertical: 8.0));
});
testWidgets('Popup menu uses values from PopupMenuThemeData', (WidgetTester tester) async {
@ -360,6 +368,10 @@ void main() {
// Test checked CheckedPopupMenuItem label.
listTile = tester.widget<ListTile>(find.byType(ListTile).last);
expect(listTile.titleTextStyle, popupMenuTheme.labelTextStyle?.resolve(enabled));
// Check popup menu padding.
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
expect(popupMenu.padding, popupMenuTheme.menuPadding);
});
testWidgets('Popup menu widget properties take priority over theme', (WidgetTester tester) async {
@ -374,6 +386,7 @@ void main() {
const ShapeBorder shape = RoundedRectangleBorder(
borderRadius: BorderRadius.all(Radius.circular(9.0)),
);
const EdgeInsets menuPadding = EdgeInsets.zero;
const double elevation = 7.0;
const TextStyle textStyle = TextStyle(color: Color(0xfff14fff), fontSize: 19.0);
const MouseCursor cursor = SystemMouseCursors.forbidden;
@ -393,6 +406,7 @@ void main() {
surfaceTintColor: surfaceTintColor,
color: color,
shape: shape,
menuPadding: menuPadding,
iconColor: iconColor,
iconSize: iconSize,
itemBuilder: (BuildContext context) {
@ -460,6 +474,10 @@ void main() {
// Test CheckedPopupMenuItem label.
final ListTile listTile = tester.widget<ListTile>(find.byType(ListTile).first);
expect(listTile.titleTextStyle, textStyle);
// Check popup menu padding.
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
expect(popupMenu.padding, EdgeInsets.zero);
});
group('Material 2', () {
@ -564,6 +582,10 @@ void main() {
RendererBinding.instance.mouseTracker.debugDeviceActiveCursor(1),
SystemMouseCursors.click,
);
// Check popup menu padding.
final SingleChildScrollView popupMenu = tester.widget<SingleChildScrollView>(find.byType(SingleChildScrollView));
expect(popupMenu.padding, const EdgeInsets.symmetric(vertical: 8.0));
});
testWidgets('Popup menu uses values from PopupMenuThemeData', (WidgetTester tester) async {