From 96e02c61dcc318e7062770f5db6d760b85a1e075 Mon Sep 17 00:00:00 2001 From: Taha Tesser Date: Thu, 10 Aug 2023 19:05:03 +0300 Subject: [PATCH] Fix `PopupMenuItem` & `CheckedPopupMenuItem` has redundant `ListTile` padding and update default horizontal padding for Material 3 (#131609) fixes [`PopupMenuItem` adds redundant padding when using `ListItem`](https://github.com/flutter/flutter/issues/128553) ### Description - Fixed redundant `ListTile` padding when using `CheckedPopupMenuItem` or `PopupMenuItem` with the `ListTile` child for complex popup menu items as suggested in the docs. - Updated default horizontal padding for popup menu items. ### Code sample
expand to view the code sample ```dart import 'package:flutter/material.dart'; /// Flutter code sample for [PopupMenuButton]. // This is the type used by the popup menu below. enum SampleItem { itemOne, itemTwo, itemThree } void main() => runApp(const PopupMenuApp()); class PopupMenuApp extends StatelessWidget { const PopupMenuApp({super.key}); @override Widget build(BuildContext context) { return MaterialApp( theme: ThemeData(useMaterial3: true), home: const PopupMenuExample(), ); } } class PopupMenuExample extends StatefulWidget { const PopupMenuExample({super.key}); @override State createState() => _PopupMenuExampleState(); } class _PopupMenuExampleState extends State { SampleItem? selectedMenu; @override Widget build(BuildContext context) { return Scaffold( appBar: AppBar(title: const Text('PopupMenuButton')), body: Center( child: SizedBox( width: 150, height: 100, child: Align( alignment: Alignment.topLeft, child: PopupMenuButton( initialValue: selectedMenu, // Callback that sets the selected popup menu item. onSelected: (SampleItem item) { setState(() { selectedMenu = item; }); }, itemBuilder: (BuildContext context) => >[ const PopupMenuItem( value: SampleItem.itemOne, child: Text('PopupMenuItem'), ), const CheckedPopupMenuItem( checked: true, value: SampleItem.itemTwo, child: Text('CheckedPopupMenuItem'), ), const PopupMenuItem( value: SampleItem.itemOne, child: ListTile( leading: Icon(Icons.cloud), title: Text('ListTile'), contentPadding: EdgeInsets.zero, trailing: Icon(Icons.arrow_right_rounded), ), ), ], ), ), ), ), ); } } ```
### Before ![image](https://github.com/flutter/flutter/assets/48603081/aad15ffb-ca11-4997-81d1-b46288161a4e) - Default horizontal padding is the same as M2 (16.0), while the specs use a smaller value (12.0) - `ListTile` nested by default in `CheckedPopupMenuItem` has redundant padding - `PopupMenuItem` using `ListTile` as a child for complex menu items contains redundant padding. ![Screenshot 2023-07-31 at 17 17 08](https://github.com/flutter/flutter/assets/48603081/75ad1fe5-e051-42ba-badf-e20c799dee96) ### After - Default horizontal padding is updated for Material 3. - `PopupMenuItem` & `CheckedPopupMenuItem` override `ListTile` padding (similar to how `ExpansionTile` overrides `ListTile` text and icon color. ![Screenshot 2023-07-31 at 17 17 25](https://github.com/flutter/flutter/assets/48603081/288cf892-5b51-4365-9855-5ef0ed2928e9) --- .../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, 197 insertions(+), 11 deletions(-) diff --git a/dev/tools/gen_defaults/lib/popup_menu_template.dart b/dev/tools/gen_defaults/lib/popup_menu_template.dart index f11b89c9500..bed11d0b8b0 100644 --- a/dev/tools/gen_defaults/lib/popup_menu_template.dart +++ b/dev/tools/gen_defaults/lib/popup_menu_template.dart @@ -42,5 +42,9 @@ 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 ef8f4367c6b..7a3f7a5249b 100644 --- a/packages/flutter/lib/src/material/popup_menu.dart +++ b/packages/flutter/lib/src/material/popup_menu.dart @@ -14,6 +14,7 @@ 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'; @@ -32,7 +33,6 @@ 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,7 +255,11 @@ 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. /// - /// When null, the horizontal padding defaults to 16.0 on both sides. + /// 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. final EdgeInsets? padding; /// The text style of the popup menu item. @@ -372,7 +376,7 @@ class PopupMenuItemState> extends State { child: Container( alignment: AlignmentDirectional.centerStart, constraints: BoxConstraints(minHeight: widget.height), - padding: widget.padding ?? const EdgeInsets.symmetric(horizontal: _kMenuHorizontalPadding), + padding: widget.padding ?? (theme.useMaterial3 ? _PopupMenuDefaultsM3.menuHorizontalPadding : _PopupMenuDefaultsM2.menuHorizontalPadding), child: buildChild(), ), ); @@ -393,7 +397,10 @@ class PopupMenuItemState> extends State { onTap: widget.enabled ? handleTap : null, canRequestFocus: widget.enabled, mouseCursor: _EffectiveMouseCursor(widget.mouseCursor, popupMenuTheme.mouseCursor), - child: item, + child: ListTileTheme.merge( + contentPadding: EdgeInsets.zero, + child: item, + ), ), ), ); @@ -540,14 +547,17 @@ class _CheckedPopupMenuItemState extends PopupMenuItemState _textTheme.subtitle1; + + static EdgeInsets menuHorizontalPadding = const EdgeInsets.symmetric(horizontal: 16.0); } // BEGIN GENERATED TOKEN PROPERTIES - PopupMenu @@ -1465,5 +1477,9 @@ 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 f26ed5a03ad..60c576797dc 100644 --- a/packages/flutter/test/material/popup_menu_test.dart +++ b/packages/flutter/test/material/popup_menu_test.dart @@ -1553,6 +1553,82 @@ 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; @@ -3415,6 +3491,96 @@ 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 {