From 34b8cef5b25f81ffcab41298c433b87831701acb Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Tue, 18 Jan 2022 12:10:19 -0800 Subject: [PATCH] Revert "feat: added custom padding in PopupMenuButton (#96657)" (#96781) --- .../flutter/lib/src/material/popup_menu.dart | 32 ++------- .../lib/src/material/popup_menu_theme.dart | 16 +---- .../test/material/popup_menu_test.dart | 66 ------------------- .../test/material/popup_menu_theme_test.dart | 18 ----- 4 files changed, 6 insertions(+), 126 deletions(-) diff --git a/packages/flutter/lib/src/material/popup_menu.dart b/packages/flutter/lib/src/material/popup_menu.dart index 171c77ddc2..a019c96e60 100644 --- a/packages/flutter/lib/src/material/popup_menu.dart +++ b/packages/flutter/lib/src/material/popup_menu.dart @@ -528,12 +528,10 @@ class _PopupMenu extends StatelessWidget { Key? key, required this.route, required this.semanticLabel, - this.padding, }) : super(key: key); final _PopupMenuRoute route; final String? semanticLabel; - final EdgeInsetsGeometry? padding; @override Widget build(BuildContext context) { @@ -585,7 +583,9 @@ class _PopupMenu extends StatelessWidget { explicitChildNodes: true, label: semanticLabel, child: SingleChildScrollView( - padding: padding, + padding: const EdgeInsets.symmetric( + vertical: _kMenuVerticalPadding, + ), child: ListBody(children: children), ), ), @@ -727,7 +727,6 @@ class _PopupMenuRoute extends PopupRoute { _PopupMenuRoute({ required this.position, required this.items, - this.padding, this.initialValue, this.elevation, required this.barrierLabel, @@ -741,7 +740,6 @@ class _PopupMenuRoute extends PopupRoute { final List> items; final List itemSizes; final T? initialValue; - final EdgeInsetsGeometry? padding; final double? elevation; final String? semanticLabel; final ShapeBorder? shape; @@ -780,7 +778,7 @@ class _PopupMenuRoute extends PopupRoute { } } - final Widget menu = _PopupMenu(padding: padding, route: this, semanticLabel: semanticLabel); + final Widget menu = _PopupMenu(route: this, semanticLabel: semanticLabel); final MediaQueryData mediaQuery = MediaQuery.of(context); return MediaQuery.removePadding( context: context, @@ -852,11 +850,6 @@ class _PopupMenuRoute extends PopupRoute { /// label is not provided, it will default to /// [MaterialLocalizations.popupMenuLabel]. /// -/// The `padding` argument is used to add empty space around the outside -/// of the popup menu. If this property is not provided, then [PopupMenuThemeData.padding] -/// is used. If [PopupMenuThemeData.padding] is also null, then -/// EdgeInsets.symmetric(vertical: 8.0) is used. -/// /// See also: /// /// * [PopupMenuItem], a popup menu entry for a single value. @@ -871,7 +864,6 @@ Future showMenu({ required RelativeRect position, required List> items, T? initialValue, - EdgeInsetsGeometry? padding, double? elevation, String? semanticLabel, ShapeBorder? shape, @@ -900,7 +892,6 @@ Future showMenu({ position: position, items: items, initialValue: initialValue, - padding: padding, elevation: elevation, semanticLabel: semanticLabel, barrierLabel: MaterialLocalizations.of(context).modalBarrierDismissLabel, @@ -993,7 +984,6 @@ class PopupMenuButton extends StatefulWidget { this.tooltip, this.elevation, this.padding = const EdgeInsets.all(8.0), - this.menuPadding, this.child, this.icon, this.iconSize, @@ -1088,14 +1078,6 @@ class PopupMenuButton extends StatefulWidget { /// Theme.of(context).cardColor is used. final Color? color; - /// If provided, menu padding is used for empty space around the outside - /// of the popup menu. - /// - /// If this property is null, then [PopupMenuThemeData.padding] is used. - /// If [PopupMenuThemeData.padding] is also null, then - /// EdgeInsets.symmetric(vertical: 8.0) is used. - final EdgeInsetsGeometry? menuPadding; - /// Whether detected gestures should provide acoustic and/or haptic feedback. /// /// For example, on Android a tap will produce a clicking sound and a @@ -1139,11 +1121,6 @@ class PopupMenuButtonState extends State> { ), Offset.zero & overlay.size, ); - - final EdgeInsetsGeometry menuPadding = widget.menuPadding ?? - popupMenuTheme.padding ?? - const EdgeInsets.symmetric(vertical: _kMenuVerticalPadding); - final List> items = widget.itemBuilder(context); // Only show the menu if there is something to show if (items.isNotEmpty) { @@ -1152,7 +1129,6 @@ class PopupMenuButtonState extends State> { elevation: widget.elevation ?? popupMenuTheme.elevation, items: items, initialValue: widget.initialValue, - padding: menuPadding, position: position, shape: widget.shape ?? popupMenuTheme.shape, color: widget.color ?? popupMenuTheme.color, diff --git a/packages/flutter/lib/src/material/popup_menu_theme.dart b/packages/flutter/lib/src/material/popup_menu_theme.dart index 93b85cfced..2a899f8e03 100644 --- a/packages/flutter/lib/src/material/popup_menu_theme.dart +++ b/packages/flutter/lib/src/material/popup_menu_theme.dart @@ -40,7 +40,6 @@ class PopupMenuThemeData with Diagnosticable { this.textStyle, this.enableFeedback, this.mouseCursor, - this.padding, }); /// The background color of the popup menu. @@ -65,11 +64,6 @@ class PopupMenuThemeData with Diagnosticable { /// If specified, overrides the default value of [PopupMenuItem.mouseCursor]. final MaterialStateProperty? mouseCursor; - /// If specified, defines the padding for the popup menu of [PopupMenuButton]. - /// - /// If [PopupMenuButton.menuPadding] is provided, [padding] is ignored. - final EdgeInsetsGeometry? padding; - /// Creates a copy of this object with the given fields replaced with the /// new values. PopupMenuThemeData copyWith({ @@ -79,7 +73,6 @@ class PopupMenuThemeData with Diagnosticable { TextStyle? textStyle, bool? enableFeedback, MaterialStateProperty? mouseCursor, - EdgeInsets? padding, }) { return PopupMenuThemeData( color: color ?? this.color, @@ -88,7 +81,6 @@ class PopupMenuThemeData with Diagnosticable { textStyle: textStyle ?? this.textStyle, enableFeedback: enableFeedback ?? this.enableFeedback, mouseCursor: mouseCursor ?? this.mouseCursor, - padding: padding ?? this.padding, ); } @@ -108,7 +100,6 @@ class PopupMenuThemeData with Diagnosticable { textStyle: TextStyle.lerp(a?.textStyle, b?.textStyle, t), enableFeedback: t < 0.5 ? a?.enableFeedback : b?.enableFeedback, mouseCursor: t < 0.5 ? a?.mouseCursor : b?.mouseCursor, - padding: EdgeInsetsGeometry.lerp(a?.padding, b?.padding, t), ); } @@ -120,8 +111,7 @@ class PopupMenuThemeData with Diagnosticable { elevation, textStyle, enableFeedback, - mouseCursor, - padding, + mouseCursor ); } @@ -137,8 +127,7 @@ class PopupMenuThemeData with Diagnosticable { && other.shape == shape && other.textStyle == textStyle && other.enableFeedback == enableFeedback - && other.mouseCursor == mouseCursor - && other.padding == padding; + && other.mouseCursor == mouseCursor; } @override @@ -150,7 +139,6 @@ class PopupMenuThemeData with Diagnosticable { properties.add(DiagnosticsProperty('text style', textStyle, defaultValue: null)); properties.add(DiagnosticsProperty('enableFeedback', enableFeedback, defaultValue: null)); properties.add(DiagnosticsProperty>('mouseCursor', mouseCursor, defaultValue: null)); - properties.add(DiagnosticsProperty('padding', padding, defaultValue: null)); } } diff --git a/packages/flutter/test/material/popup_menu_test.dart b/packages/flutter/test/material/popup_menu_test.dart index 60d2cf30e2..1be98e7831 100644 --- a/packages/flutter/test/material/popup_menu_test.dart +++ b/packages/flutter/test/material/popup_menu_test.dart @@ -1471,72 +1471,6 @@ void main() { expect(tester.widget(find.widgetWithText(Container, 'Item 3')).padding, const EdgeInsets.all(20)); }); -testWidgets('PopupMenu custom padding', (WidgetTester tester) async { - final Key popupMenuButtonWithDefaultPaddingKey = UniqueKey(); - final Key popupMenuButtonWithOverriddenPaddingKey = UniqueKey(); - - const EdgeInsets padding = EdgeInsets.zero; - - await tester.pumpWidget( - MaterialApp( - home: Scaffold( - body: Column( - children: >[ - PopupMenuButton( - key: popupMenuButtonWithDefaultPaddingKey, - child: const Text('button'), - onSelected: (String result) {}, - itemBuilder: (BuildContext context) { - return >[ - const PopupMenuItem( - value: '0', - child: Text('Item 0'), - ), - ]; - }, - ), - PopupMenuButton( - menuPadding: padding, - key: popupMenuButtonWithOverriddenPaddingKey, - child: const Text('button'), - onSelected: (String result) {}, - itemBuilder: (BuildContext context) { - return >[ - const PopupMenuItem( - value: '0', - child: Text('Item 0'), - ), - ]; - }, - ) - ], - ), - ), - ), - ); - - final Finder popupFinder = find.byType(SingleChildScrollView); - // Show the menu - await tester.tap(find.byKey(popupMenuButtonWithDefaultPaddingKey)); - await tester.pumpAndSettle(); - - expect(tester.widget(popupFinder).padding, - const EdgeInsets.symmetric(vertical: 8), - reason: 'The popup without explicit paddings should utilise the default ones.',); - - // Close previous menu - await tester.tap(find.byKey(popupMenuButtonWithDefaultPaddingKey), warnIfMissed: false); - await tester.pumpAndSettle(); - - // Show the menu - await tester.tap(find.byKey(popupMenuButtonWithOverriddenPaddingKey)); - await tester.pumpAndSettle(); - - expect(tester.widget(popupFinder).padding, - padding, - reason: 'The popup should utilise explicitly set paddings.',); - }); - testWidgets('CheckedPopupMenuItem child height is a minimum, child is vertically centered', (WidgetTester tester) async { final Key popupMenuButtonKey = UniqueKey(); final Type menuItemType = const CheckedPopupMenuItem(child: Text('item')).runtimeType; diff --git a/packages/flutter/test/material/popup_menu_theme_test.dart b/packages/flutter/test/material/popup_menu_theme_test.dart index 6cedb7997e..35a1f4560b 100644 --- a/packages/flutter/test/material/popup_menu_theme_test.dart +++ b/packages/flutter/test/material/popup_menu_theme_test.dart @@ -13,7 +13,6 @@ PopupMenuThemeData _popupMenuTheme() { shape: BeveledRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(12))), elevation: 12.0, textStyle: TextStyle(color: Color(0xffffffff), textBaseline: TextBaseline.alphabetic), - padding: EdgeInsets.symmetric(vertical: 6, horizontal: 6), ); } @@ -196,7 +195,6 @@ void main() { ); const double elevation = 7.0; const TextStyle textStyle = TextStyle(color: Color(0x00000000), textBaseline: TextBaseline.alphabetic); - const EdgeInsets menuPadding = EdgeInsets.zero; await tester.pumpWidget(MaterialApp( theme: ThemeData(popupMenuTheme: popupMenuTheme), @@ -209,7 +207,6 @@ void main() { elevation: elevation, color: color, shape: shape, - menuPadding: menuPadding, itemBuilder: (BuildContext context) { return >[ PopupMenuItem( @@ -253,12 +250,6 @@ void main() { ).last, ); expect(text.style, textStyle); - - /// PopupMenu widget is private so in order to test padding the widget - /// with the popup padding is extracted. - final SingleChildScrollView popupMenu = tester.widget - (find.byType(SingleChildScrollView)); - expect(popupMenu.padding, menuPadding); }); testWidgets('ThemeData.popupMenuTheme properties are utilized', (WidgetTester tester) async { @@ -267,8 +258,6 @@ void main() { final Key enabledPopupItemKey = UniqueKey(); final Key disabledPopupItemKey = UniqueKey(); - const EdgeInsets themePadding = EdgeInsets.zero; - await tester.pumpWidget(MaterialApp( key: popupButtonApp, home: Material( @@ -286,7 +275,6 @@ void main() { } return SystemMouseCursors.alias; }), - padding: themePadding, ), child: PopupMenuButton( key: popupButtonKey, @@ -345,11 +333,5 @@ void main() { await gesture.down(tester.getCenter(find.byKey(enabledPopupItemKey))); await tester.pumpAndSettle(); expect(RendererBinding.instance!.mouseTracker.debugDeviceActiveCursor(1), SystemMouseCursors.alias); - - /// PopupMenu widget is private so in order to test padding we extract - /// the widget which holds the padding. - final SingleChildScrollView popupMenu = tester.widget - (find.byType(SingleChildScrollView)); - expect(popupMenu.padding, themePadding); }); }