From 7ce2d83b848fb2081855ec47deddedb554348cd4 Mon Sep 17 00:00:00 2001 From: Casey Hillers Date: Sun, 13 Aug 2023 14:27:01 -0700 Subject: [PATCH] Revert "Fix `PopupMenuItem` & `CheckedPopupMenuItem` has redundant `ListTile` padding and update default horizontal padding for Material 3" (#132457) Reverts flutter/flutter#131609 b/295497265 - This broke Google Testing. We'll need the internal patch before landing as there's a large number of customer codebases impacted. --- .../gen_defaults/lib/popup_menu_template.dart | 4 - .../flutter/lib/src/material/popup_menu.dart | 38 ++-- .../test/material/popup_menu_test.dart | 166 ------------------ 3 files changed, 11 insertions(+), 197 deletions(-) diff --git a/dev/tools/gen_defaults/lib/popup_menu_template.dart b/dev/tools/gen_defaults/lib/popup_menu_template.dart index bed11d0b8b..f11b89c950 100644 --- a/dev/tools/gen_defaults/lib/popup_menu_template.dart +++ b/dev/tools/gen_defaults/lib/popup_menu_template.dart @@ -42,9 +42,5 @@ class _${blockName}DefaultsM3 extends PopupMenuThemeData { @override ShapeBorder? get shape => ${shape("md.comp.menu.container")}; - - // 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); }'''; } diff --git a/packages/flutter/lib/src/material/popup_menu.dart b/packages/flutter/lib/src/material/popup_menu.dart index 7a3f7a5249..ef8f4367c6 100644 --- a/packages/flutter/lib/src/material/popup_menu.dart +++ b/packages/flutter/lib/src/material/popup_menu.dart @@ -14,7 +14,6 @@ import 'icon_button.dart'; import 'icons.dart'; import 'ink_well.dart'; import 'list_tile.dart'; -import 'list_tile_theme.dart'; import 'material.dart'; import 'material_localizations.dart'; import 'material_state.dart'; @@ -33,6 +32,7 @@ import 'tooltip.dart'; const Duration _kMenuDuration = Duration(milliseconds: 300); const double _kMenuCloseIntervalEnd = 2.0 / 3.0; +const double _kMenuHorizontalPadding = 16.0; const double _kMenuDividerHeight = 16.0; const double _kMenuMaxWidth = 5.0 * _kMenuWidthStep; const double _kMenuMinWidth = 2.0 * _kMenuWidthStep; @@ -255,11 +255,7 @@ class PopupMenuItem extends PopupMenuEntry { /// If a [height] greater than the height of the sum of the padding and [child] /// is provided, then the padding's effect will not be visible. /// - /// If this is null and [ThemeData.useMaterial3] is true, the horizontal padding - /// defaults to 12.0 on both sides. - /// - /// If this is null and [ThemeData.useMaterial3] is false, the horizontal padding - /// defaults to 16.0 on both sides. + /// When null, the horizontal padding defaults to 16.0 on both sides. final EdgeInsets? padding; /// The text style of the popup menu item. @@ -376,7 +372,7 @@ class PopupMenuItemState> extends State { child: Container( alignment: AlignmentDirectional.centerStart, constraints: BoxConstraints(minHeight: widget.height), - padding: widget.padding ?? (theme.useMaterial3 ? _PopupMenuDefaultsM3.menuHorizontalPadding : _PopupMenuDefaultsM2.menuHorizontalPadding), + padding: widget.padding ?? const EdgeInsets.symmetric(horizontal: _kMenuHorizontalPadding), child: buildChild(), ), ); @@ -397,10 +393,7 @@ class PopupMenuItemState> extends State { onTap: widget.enabled ? handleTap : null, canRequestFocus: widget.enabled, mouseCursor: _EffectiveMouseCursor(widget.mouseCursor, popupMenuTheme.mouseCursor), - child: ListTileTheme.merge( - contentPadding: EdgeInsets.zero, - child: item, - ), + child: item, ), ), ); @@ -547,17 +540,14 @@ class _CheckedPopupMenuItemState extends PopupMenuItemState _textTheme.subtitle1; - - static EdgeInsets menuHorizontalPadding = const EdgeInsets.symmetric(horizontal: 16.0); } // BEGIN GENERATED TOKEN PROPERTIES - PopupMenu @@ -1477,9 +1465,5 @@ class _PopupMenuDefaultsM3 extends PopupMenuThemeData { @override ShapeBorder? get shape => const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(4.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); } // END GENERATED TOKEN PROPERTIES - PopupMenu diff --git a/packages/flutter/test/material/popup_menu_test.dart b/packages/flutter/test/material/popup_menu_test.dart index 60c576797d..f26ed5a03a 100644 --- a/packages/flutter/test/material/popup_menu_test.dart +++ b/packages/flutter/test/material/popup_menu_test.dart @@ -1553,82 +1553,6 @@ void main() { ); }); - testWidgets('Material3 - PopupMenuItem default padding', (WidgetTester tester) async { - final Key popupMenuButtonKey = UniqueKey(); - await tester.pumpWidget( - MaterialApp( - theme: ThemeData(useMaterial3: true), - home: Scaffold( - body: Center( - child: PopupMenuButton( - key: popupMenuButtonKey, - child: const Text('button'), - onSelected: (String result) { }, - itemBuilder: (BuildContext context) { - return >[ - const PopupMenuItem( - value: '0', - enabled: false, - child: Text('Item 0'), - ), - const PopupMenuItem( - value: '1', - child: Text('Item 1'), - ), - ]; - }, - ), - ), - ), - ), - ); - - // Show the menu. - await tester.tap(find.byKey(popupMenuButtonKey)); - await tester.pumpAndSettle(); - - expect(tester.widget(find.widgetWithText(Container, 'Item 0')).padding, const EdgeInsets.symmetric(horizontal: 12.0)); - expect(tester.widget(find.widgetWithText(Container, 'Item 1')).padding, const EdgeInsets.symmetric(horizontal: 12.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( - key: popupMenuButtonKey, - child: const Text('button'), - onSelected: (String result) { }, - itemBuilder: (BuildContext context) { - return >[ - const PopupMenuItem( - value: '0', - enabled: false, - child: Text('Item 0'), - ), - const PopupMenuItem( - value: '1', - child: Text('Item 1'), - ), - ]; - }, - ), - ), - ), - ), - ); - - // Show the menu. - await tester.tap(find.byKey(popupMenuButtonKey)); - await tester.pumpAndSettle(); - - expect(tester.widget(find.widgetWithText(Container, 'Item 0')).padding, const EdgeInsets.symmetric(horizontal: 16.0)); - expect(tester.widget(find.widgetWithText(Container, 'Item 1')).padding, const EdgeInsets.symmetric(horizontal: 16.0)); - }); - testWidgets('PopupMenuItem custom padding', (WidgetTester tester) async { final Key popupMenuButtonKey = UniqueKey(); final Type menuItemType = const PopupMenuItem(child: Text('item')).runtimeType; @@ -3491,96 +3415,6 @@ void main() { labelTextStyle.resolve({MaterialState.selected}) ); }); - - testWidgets('CheckedPopupMenuItem overrides redundant ListTile content padding', (WidgetTester tester) async { - final Key popupMenuButtonKey = UniqueKey(); - await tester.pumpWidget( - MaterialApp( - theme: ThemeData(useMaterial3: false), - home: Scaffold( - body: Center( - child: PopupMenuButton( - key: popupMenuButtonKey, - child: const Text('button'), - onSelected: (String result) { }, - itemBuilder: (BuildContext context) { - return >[ - const CheckedPopupMenuItem( - value: '0', - child: Text('Item 0'), - ), - const CheckedPopupMenuItem( - value: '1', - checked: true, - child: Text('Item 1'), - ), - ]; - }, - ), - ), - ), - ), - ); - - // Show the menu. - await tester.tap(find.byKey(popupMenuButtonKey)); - await tester.pumpAndSettle(); - - SafeArea getItemSafeArea(String label) { - return tester.widget(find.ancestor( - of: find.text(label), - matching: find.byType(SafeArea), - )); - } - - expect(getItemSafeArea('Item 0').minimum, EdgeInsets.zero); - expect(getItemSafeArea('Item 1').minimum, EdgeInsets.zero); - }); - - testWidgets('PopupMenuItem overrides redundant ListTile content padding', (WidgetTester tester) async { - final Key popupMenuButtonKey = UniqueKey(); - await tester.pumpWidget( - MaterialApp( - theme: ThemeData(useMaterial3: false), - home: Scaffold( - body: Center( - child: PopupMenuButton( - key: popupMenuButtonKey, - child: const Text('button'), - onSelected: (String result) { }, - itemBuilder: (BuildContext context) { - return >[ - const PopupMenuItem( - value: '0', - enabled: false, - child: ListTile(title: Text('Item 0')), - ), - const PopupMenuItem( - value: '1', - child: ListTile(title: Text('Item 1')), - ), - ]; - }, - ), - ), - ), - ), - ); - - // Show the menu. - await tester.tap(find.byKey(popupMenuButtonKey)); - await tester.pumpAndSettle(); - - SafeArea getItemSafeArea(String label) { - return tester.widget(find.ancestor( - of: find.text(label), - matching: find.byType(SafeArea), - )); - } - - expect(getItemSafeArea('Item 0').minimum, EdgeInsets.zero); - expect(getItemSafeArea('Item 1').minimum, EdgeInsets.zero); - }); } class TestApp extends StatelessWidget {