Skip to content

Support materialTapTargetSize in PopupMenuButton #158357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hannah-hyj merged 4 commits into from
Nov 12, 2024
+48 −1
Merged

Support materialTapTargetSize in PopupMenuButton #158357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40460c2
PopupMenuButton respects materialTapTargetSize
hannah-hyj Nov 8, 2024
cf46100
Update popup_menu.dart
hannah-hyj Nov 8, 2024
fcb1325
Update popup_menu.dart
hannah-hyj Nov 11, 2024
379967d
Revert "Update popup_menu.dart"
hannah-hyj Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion packages/flutter/lib/src/material/popup_menu.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import 'material_state.dart';
import 'popup_menu_theme.dart';
import 'text_theme.dart';
import 'theme.dart';
import 'theme_data.dart';
import 'tooltip.dart';

// Examples can assume:
Expand Down Expand Up @@ -1554,7 +1555,7 @@ class PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
assert(debugCheckHasMaterialLocalizations(context));

if (widget.child != null) {
return Tooltip(
final Widget child = Tooltip(
message: widget.tooltip ?? MaterialLocalizations.of(context).showMenuTooltip,
child: InkWell(
borderRadius: widget.borderRadius,
Expand All @@ -1565,6 +1566,17 @@ class PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
child: widget.child,
),
);
final MaterialTapTargetSize tapTargetSize = widget.style?.tapTargetSize ?? MaterialTapTargetSize.shrinkWrap;
Copy link
Contributor

@QuncCccccc QuncCccccc Nov 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we can default to the ThemeData.materialTapTargetSize value. Then here would be final MaterialTapTargetSize tapTargetSize = widget.style?.tapTargetSize ?? popupMenuTheme.tapTargetSize ?? Theme.of(context).materialTapTargetSize; We can also add this property to PopupMenuTheme so developers can control the button size easier:)

Suggested change
final MaterialTapTargetSize tapTargetSize = widget.style?.tapTargetSize ?? MaterialTapTargetSize.shrinkWrap;
final MaterialTapTargetSize tapTargetSize = widget.style?.tapTargetSize ?? Theme.of(context).materialTapTargetSize;

Copy link
Member Author

@hannah-hyj hannah-hyj Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this but

if it defaults to Theme.of(context).materialTapTargetSize, this PR will be a breaking change because some buttons' size will be increased to at least 48X48, looks like some flutter_cacoon tests are failing because of that.(some g3 tests too.)

since I can't apply a flutter_cacoon fix together with this PR. I'm thinking the only way to land this PR with it default to Theme.of(context).materialTapTargetSize is by 4 steps.

  1. Merge this PR with default to MaterialTapTargetSize.shrinkWrap so it's not a breaking change.
  2. Update flutter_cacoon tests and g3 tests to use MaterialTapTargetSize.padded in its PopupMenuButton
  3. Update PopupMenuButton to default to Theme.of(context).materialTapTargetSize
  4. Remove now redundant MaterialTapTargetSize.padded in flutter_cacoon tests and g3 tests

wdyt?

Copy link
Contributor

@QuncCccccc QuncCccccc Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Sounds good to me! If we want to add this property to PopupMenuTheme, we can also add a separate PR later. This PR LGTM!

if (tapTargetSize == MaterialTapTargetSize.padded) {
return ConstrainedBox(
constraints: const BoxConstraints(
minWidth: kMinInteractiveDimension,
minHeight: kMinInteractiveDimension,
),
child: child,
);
}
return child;
}

return IconButton(
Expand Down
35 changes: 35 additions & 0 deletions packages/flutter/test/material/popup_menu_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4318,6 +4318,41 @@ void main() {
expect(inkWell.borderRadius, borderRadius);
});

testWidgets('PopupMenuButton respects materialTapTargetSize', (WidgetTester tester) async {
const double buttonSize = 10.0;


Widget buildPopupMenu({required MaterialTapTargetSize tapTargetSize}) {
return MaterialApp(
home: Scaffold(
body: Center(
child: PopupMenuButton<String>(
style: ButtonStyle(tapTargetSize: tapTargetSize),
itemBuilder: (_) => <PopupMenuEntry<String>>[
const PopupMenuItem<String>(
value: 'value',
child: Text('Item 0'),
),
],
child: const SizedBox(height: buttonSize, width: buttonSize),
),
),
),
);
}
// Popup menu with MaterialTapTargetSize.padded.
await tester.pumpWidget(buildPopupMenu(tapTargetSize: MaterialTapTargetSize.padded));
await tester.pumpAndSettle();

expect(tester.getSize(find.byType(InkWell)), const Size(48.0, 48.0));

// Popup menu with MaterialTapTargetSize.shrinkWrap.
await tester.pumpWidget(buildPopupMenu(tapTargetSize: MaterialTapTargetSize.shrinkWrap));
await tester.pumpAndSettle();

expect(tester.getSize(find.byType(InkWell)), const Size(buttonSize, buttonSize));
});

testWidgets('If requestFocus is false, the original focus should be preserved upon menu appearance.', (WidgetTester tester) async {
final FocusNode fieldFocusNode = FocusNode();
addTearDown(fieldFocusNode.dispose);
Expand Down