Skip to content

F
Front-End
Unverified Commit 96e02c61 authored by Taha Tesser's avatar Taha Tesser Committed by GitHub
Browse files
Options

Fix `PopupMenuItem` & `CheckedPopupMenuItem` has redundant `ListTile` padding... 

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

<details> 
<summary>expand to view the code sample</summary> 

```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<PopupMenuExample> createState() => _PopupMenuExampleState();
}

class _PopupMenuExampleState extends State<PopupMenuExample> {
  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<SampleItem>(
              initialValue: selectedMenu,
              // Callback that sets the selected popup menu item.
              onSelected: (SampleItem item) {
                setState(() {
                  selectedMenu = item;
                });
              },
              itemBuilder: (BuildContext context) =>
                  <PopupMenuEntry<SampleItem>>[
                const PopupMenuItem<SampleItem>(
                  value: SampleItem.itemOne,
                  child: Text('PopupMenuItem'),
                ),
                const CheckedPopupMenuItem<SampleItem>(
                  checked: true,
                  value: SampleItem.itemTwo,
                  child: Text('CheckedPopupMenuItem'),
                ),
                const PopupMenuItem<SampleItem>(
                  value: SampleItem.itemOne,
                  child: ListTile(
                    leading: Icon(Icons.cloud),
                    title: Text('ListTile'),
                    contentPadding: EdgeInsets.zero,
                    trailing: Icon(Icons.arrow_right_rounded),
                  ),
                ),
              ],
            ),
          ),
        ),
      ),
    );
  }
}
``` 
	
</details>

### 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)
parent c428d915
Hide whitespace changes
Inline Side-by-side
Showing with 197 additions and 11 deletions
dev/tools/gen_defaults/lib/popup_menu_template.dart
View file @ 96e02c61
......@@ -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);
}''';
}
packages/flutter/lib/src/material/popup_menu.dart
View file @ 96e02c61
......@@ -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<T> extends PopupMenuEntry<T> {
/// 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<T, W extends PopupMenuItem<T>> extends State<W> {
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<T, W extends PopupMenuItem<T>> extends State<W> {
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<T> extends PopupMenuItemState<T, CheckedPopupMe
?? popupMenuTheme.labelTextStyle
?? defaults.labelTextStyle;
return IgnorePointer(
child: ListTile(
enabled: widget.enabled,
titleTextStyle: effectiveLabelTextStyle?.resolve(states),
leading: FadeTransition(
opacity: _opacity,
child: Icon(_controller.isDismissed ? null : Icons.done),
child: ListTileTheme.merge(
contentPadding: EdgeInsets.zero,
child: ListTile(
enabled: widget.enabled,
titleTextStyle: effectiveLabelTextStyle?.resolve(states),
leading: FadeTransition(
opacity: _opacity,
child: Icon(_controller.isDismissed ? null : Icons.done),
),
title: widget.child,
),
title: widget.child,
),
);
}
......@@ -1426,6 +1436,8 @@ class _PopupMenuDefaultsM2 extends PopupMenuThemeData {
@override
TextStyle? get textStyle => _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
packages/flutter/test/material/popup_menu_test.dart
View file @ 96e02c61
......@@ -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<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.pumpAndSettle();
expect(tester.widget<Container>(find.widgetWithText(Container, 'Item 0')).padding, const EdgeInsets.symmetric(horizontal: 12.0));
expect(tester.widget<Container>(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<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.pumpAndSettle();
expect(tester.widget<Container>(find.widgetWithText(Container, 'Item 0')).padding, const EdgeInsets.symmetric(horizontal: 16.0));
expect(tester.widget<Container>(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<String>(child: Text('item')).runtimeType;
......@@ -3415,6 +3491,96 @@ void main() {
labelTextStyle.resolve(<MaterialState>{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<String>(
key: popupMenuButtonKey,
child: const Text('button'),
onSelected: (String result) { },
itemBuilder: (BuildContext context) {
return <PopupMenuEntry<String>>[
const CheckedPopupMenuItem<String>(
value: '0',
child: Text('Item 0'),
),
const CheckedPopupMenuItem<String>(
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<SafeArea>(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<String>(
key: popupMenuButtonKey,
child: const Text('button'),
onSelected: (String result) { },
itemBuilder: (BuildContext context) {
return <PopupMenuEntry<String>>[
const PopupMenuItem<String>(
value: '0',
enabled: false,
child: ListTile(title: Text('Item 0')),
),
const PopupMenuItem<String>(
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<SafeArea>(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 {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment

HIAST - GitLab