From 6d18c20a9575648a1de20358ecbb4ed5b8cfc6c2 Mon Sep 17 00:00:00 2001 From: Hans Muller Date: Tue, 17 Sep 2019 08:48:27 -0700 Subject: [PATCH] Update PopupMenu layout (#40179) --- .../flutter/lib/src/material/popup_menu.dart | 143 +++++++++++----- .../test/material/popup_menu_test.dart | 154 ++++++++++++++++++ .../flutter_localizations/test/text_test.dart | 32 ++-- 3 files changed, 271 insertions(+), 58 deletions(-) diff --git a/packages/flutter/lib/src/material/popup_menu.dart b/packages/flutter/lib/src/material/popup_menu.dart index 7d31c6dc7c8..94d399c4b66 100644 --- a/packages/flutter/lib/src/material/popup_menu.dart +++ b/packages/flutter/lib/src/material/popup_menu.dart @@ -5,6 +5,7 @@ import 'dart:async'; import 'package:flutter/foundation.dart'; +import 'package:flutter/rendering.dart'; import 'package:flutter/widgets.dart'; import 'constants.dart'; @@ -27,10 +28,8 @@ import 'theme.dart'; // void setState(VoidCallback fn) { } const Duration _kMenuDuration = Duration(milliseconds: 300); -const double _kBaselineOffsetFromBottom = 20.0; const double _kMenuCloseIntervalEnd = 2.0 / 3.0; const double _kMenuHorizontalPadding = 16.0; -const double _kMenuItemHeight = kMinInteractiveDimension; const double _kMenuDividerHeight = 16.0; const double _kMenuMaxWidth = 5.0 * _kMenuWidthStep; const double _kMenuMinWidth = 2.0 * _kMenuWidthStep; @@ -122,6 +121,49 @@ class _PopupMenuDividerState extends State { Widget build(BuildContext context) => Divider(height: widget.height); } +// This widget only exists to enable _PopupMenuRoute to save the sizes of +// each menu item. The sizes are used by _PopupMenuRouteLayout to compute the +// y coordinate of the menu's origin so that the center of selected menu +// item lines up with the center of its PopupMenuButton. +class _MenuItem extends SingleChildRenderObjectWidget { + const _MenuItem({ + Key key, + @required this.onLayout, + Widget child, + }) : assert(onLayout != null), super(key: key, child: child); + + final ValueChanged onLayout; + + @override + RenderObject createRenderObject(BuildContext context) { + return _RenderMenuItem(onLayout); + } + + @override + void updateRenderObject(BuildContext context, covariant _RenderMenuItem renderObject) { + renderObject.onLayout = onLayout; + } +} + +class _RenderMenuItem extends RenderShiftedBox { + _RenderMenuItem(this.onLayout, [RenderBox child]) : assert(onLayout != null), super(child); + + ValueChanged onLayout; + + @override + void performLayout() { + if (child == null) { + size = Size.zero; + } else { + child.layout(constraints, parentUsesSize: true); + size = constraints.constrain(child.size); + } + final BoxParentData childParentData = child.parentData; + childParentData.offset = Offset.zero; + onLayout(size); + } +} + /// An item in a material design popup menu. /// /// To show a popup menu, use the [showMenu] function. To create a button that @@ -166,12 +208,12 @@ class PopupMenuItem extends PopupMenuEntry { /// /// By default, the item is [enabled]. /// - /// The `height` and `enabled` arguments must not be null. + /// The `enabled` and `height` arguments must not be null. const PopupMenuItem({ Key key, this.value, this.enabled = true, - this.height = _kMenuItemHeight, + this.height = kMinInteractiveDimension, this.textStyle, @required this.child, }) : assert(enabled != null), @@ -181,19 +223,19 @@ class PopupMenuItem extends PopupMenuEntry { /// The value that will be returned by [showMenu] if this entry is selected. final T value; - /// Whether the user is permitted to select this entry. + /// Whether the user is permitted to select this item. /// /// Defaults to true. If this is false, then the item will not react to /// touches. final bool enabled; - /// The height of the entry. + /// The minimum height height of the menu item. /// - /// Defaults to kMinInteractiveDimension pixels. + /// Defaults to [kMinInteractiveDimension] pixels. @override final double height; - /// The text style of the popup menu entry. + /// The text style of the popup menu item. /// /// If this property is null, then [PopupMenuThemeData.textStyle] is used. /// If [PopupMenuThemeData.textStyle] is also null, then [ThemeData.textTheme.subhead] is used. @@ -262,12 +304,14 @@ class PopupMenuItemState> extends State { Widget item = AnimatedDefaultTextStyle( style: style, duration: kThemeChangeDuration, - child: Baseline( - baseline: widget.height - _kBaselineOffsetFromBottom, - baselineType: style.textBaseline ?? TextBaseline.alphabetic, + child: Container( + alignment: AlignmentDirectional.centerStart, + constraints: BoxConstraints(minHeight: widget.height), + padding: const EdgeInsets.symmetric(horizontal: _kMenuHorizontalPadding), child: buildChild(), ), ); + if (!widget.enabled) { final bool isDark = theme.brightness == Brightness.dark; item = IconTheme.merge( @@ -278,11 +322,7 @@ class PopupMenuItemState> extends State { return InkWell( onTap: widget.enabled ? handleTap : null, - child: Container( - height: widget.height, - padding: const EdgeInsets.symmetric(horizontal: _kMenuHorizontalPadding), - child: item, - ), + child: item, ); } } @@ -293,8 +333,8 @@ class PopupMenuItemState> extends State { /// shows a popup menu, consider using [PopupMenuButton]. /// /// A [CheckedPopupMenuItem] is kMinInteractiveDimension pixels high, which -/// matches the default height of a [PopupMenuItem]. The horizontal layout uses -/// a [ListTile]; the checkmark is an [Icons.done] icon, shown in the +/// matches the default minimum height of a [PopupMenuItem]. The horizontal +/// layout uses [ListTile]; the checkmark is an [Icons.done] icon, shown in the /// [ListTile.leading] position. /// /// {@tool sample} @@ -460,10 +500,17 @@ class _PopupMenu extends StatelessWidget { child: item, ); } - children.add(FadeTransition( - opacity: opacity, - child: item, - )); + children.add( + _MenuItem( + onLayout: (Size size) { + route.itemSizes[i] = size; + }, + child: FadeTransition( + opacity: opacity, + child: item, + ), + ), + ); } final CurveTween opacity = CurveTween(curve: const Interval(0.0, 1.0 / 3.0)); @@ -518,15 +565,18 @@ class _PopupMenu extends StatelessWidget { // Positioning of the menu on the screen. class _PopupMenuRouteLayout extends SingleChildLayoutDelegate { - _PopupMenuRouteLayout(this.position, this.selectedItemOffset, this.textDirection); + _PopupMenuRouteLayout(this.position, this.itemSizes, this.selectedItemIndex, this.textDirection); // Rectangle of underlying button, relative to the overlay's dimensions. final RelativeRect position; - // The distance from the top of the menu to the middle of selected item. - // - // This will be null if there's no item to position in this way. - final double selectedItemOffset; + // The sizes of each item are computed when the menu is laid out, and before + // the route is laid out. + List itemSizes; + + // The index of the selected item, or null if PopupMenuButton.initialValue + // was not specified. + final int selectedItemIndex; // Whether to prefer going to the left or to the right. final TextDirection textDirection; @@ -549,10 +599,12 @@ class _PopupMenuRouteLayout extends SingleChildLayoutDelegate { // getConstraintsForChild. // Find the ideal vertical position. - double y; - if (selectedItemOffset == null) { - y = position.top; - } else { + double y = position.top; + if (selectedItemIndex != null && itemSizes != null) { + double selectedItemOffset = _kMenuVerticalPadding; + for (int index = 0; index < selectedItemIndex; index += 1) + selectedItemOffset += itemSizes[index].height; + selectedItemOffset += itemSizes[selectedItemIndex].height / 2; y = position.top + (size.height - position.top - position.bottom) / 2.0 - selectedItemOffset; } @@ -592,7 +644,15 @@ class _PopupMenuRouteLayout extends SingleChildLayoutDelegate { @override bool shouldRelayout(_PopupMenuRouteLayout oldDelegate) { - return position != oldDelegate.position; + // If called when the old and new itemSizes have been initialized then + // we expect them to have the same length because there's no practical + // way to change length of the items list once the menu has been shown. + assert(itemSizes.length == oldDelegate.itemSizes.length); + + return position != oldDelegate.position + || selectedItemIndex != oldDelegate.selectedItemIndex + || textDirection != oldDelegate.textDirection + || !listEquals(itemSizes, oldDelegate.itemSizes); } } @@ -610,10 +670,11 @@ class _PopupMenuRoute extends PopupRoute { this.color, this.showMenuContext, this.captureInheritedThemes, - }); + }) : itemSizes = List(items.length); final RelativeRect position; final List> items; + final List itemSizes; final dynamic initialValue; final double elevation; final ThemeData theme; @@ -647,15 +708,12 @@ class _PopupMenuRoute extends PopupRoute { @override Widget buildPage(BuildContext context, Animation animation, Animation secondaryAnimation) { - double selectedItemOffset; + + int selectedItemIndex; if (initialValue != null) { - double y = _kMenuVerticalPadding; - for (PopupMenuEntry entry in items) { - if (entry.represents(initialValue)) { - selectedItemOffset = y + entry.height / 2.0; - break; - } - y += entry.height; + for (int index = 0; selectedItemIndex == null && index < items.length; index += 1) { + if (items[index].represents(initialValue)) + selectedItemIndex = index; } } @@ -681,7 +739,8 @@ class _PopupMenuRoute extends PopupRoute { return CustomSingleChildLayout( delegate: _PopupMenuRouteLayout( position, - selectedItemOffset, + itemSizes, + selectedItemIndex, Directionality.of(context), ), child: menu, diff --git a/packages/flutter/test/material/popup_menu_test.dart b/packages/flutter/test/material/popup_menu_test.dart index e8d53471c15..f654af6dd35 100644 --- a/packages/flutter/test/material/popup_menu_test.dart +++ b/packages/flutter/test/material/popup_menu_test.dart @@ -690,6 +690,160 @@ void main() { await tester.tap(find.text('Menu Button')); }); + testWidgets('PopupMenuItem child height is a minimum, child is vertically centered', (WidgetTester tester) async { + final Key popupMenuButtonKey = UniqueKey(); + final Type menuItemType = const PopupMenuItem(child: Text('item')).runtimeType; + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Center( + child: PopupMenuButton( + key: popupMenuButtonKey, + child: const Text('button'), + onSelected: (String result) { }, + itemBuilder: (BuildContext context) { + return >[ + // This menu item's height will be 48 because the the default minimum height + // is 48 and the height of the text is less than 48. + const PopupMenuItem( + value: '0', + child: Text('Item 0'), + ), + // This menu item's height parameter specifies its minium height. The + // overall height of the menu item will be 50 because the child's + // height 40, is less than 50. + const PopupMenuItem( + height: 50, + value: '1', + child: SizedBox( + height: 40, + child: Text('Item 1'), + ), + ), + // This menu item's height parameter specifies its minium height, so the + // overall height of the menu item will be 75. + const PopupMenuItem( + height: 75, + value: '2', + child: SizedBox( + child: Text('Item 2'), + ), + ), + // This menu item's height will be 100. + const PopupMenuItem( + value: '3', + child: SizedBox( + height: 100, + child: Text('Item 3'), + ), + ), + ]; + }, + ), + ), + ), + ), + ); + + // Show the menu + await tester.tap(find.byKey(popupMenuButtonKey)); + await tester.pumpAndSettle(); + + // The menu items and their InkWells should have the expected vertical size + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 0')).height, 48); + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 1')).height, 50); + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 2')).height, 75); + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 3')).height, 100); + expect(tester.getSize(find.widgetWithText(InkWell, 'Item 0')).height, 48); + expect(tester.getSize(find.widgetWithText(InkWell, 'Item 1')).height, 50); + expect(tester.getSize(find.widgetWithText(InkWell, 'Item 2')).height, 75); + expect(tester.getSize(find.widgetWithText(InkWell, 'Item 3')).height, 100); + + // Menu item children which whose height is less than the PopupMenuItem + // are vertically centered. + expect( + tester.getRect(find.widgetWithText(menuItemType, 'Item 0')).center.dy, + tester.getRect(find.text('Item 0')).center.dy, + ); + expect( + tester.getRect(find.widgetWithText(menuItemType, 'Item 2')).center.dy, + tester.getRect(find.text('Item 2')).center.dy, + ); + }); + + testWidgets('Update PopupMenuItem layout while the menu is visible', (WidgetTester tester) async { + final Key popupMenuButtonKey = UniqueKey(); + final Type menuItemType = const PopupMenuItem(child: Text('item')).runtimeType; + + Widget buildFrame({ + TextDirection textDirection = TextDirection.ltr, + double fontSize = 24, + }) { + return MaterialApp( + builder: (BuildContext context, Widget child) { + return Directionality( + textDirection: textDirection, + child: PopupMenuTheme( + data: PopupMenuTheme.of(context).copyWith( + textStyle: Theme.of(context).textTheme.subhead.copyWith(fontSize: fontSize), + ), + child: child, + ), + ); + }, + home: Scaffold( + body: PopupMenuButton( + key: popupMenuButtonKey, + child: const Text('button'), + onSelected: (String result) { }, + itemBuilder: (BuildContext context) { + return >[ + const PopupMenuItem( + value: '0', + child: Text('Item 0'), + ), + const PopupMenuItem( + value: '1', + child: Text('Item 1'), + ), + ]; + }, + ), + ), + ); + } + + // Show the menu + await tester.pumpWidget(buildFrame()); + await tester.tap(find.byKey(popupMenuButtonKey)); + await tester.pumpAndSettle(); + + // The menu items should have their default heights and horizontal alignment. + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 0')).height, 48); + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 1')).height, 48); + expect(tester.getTopLeft(find.text('Item 0')).dx, 24); + expect(tester.getTopLeft(find.text('Item 1')).dx, 24); + + // While the menu is up, change its font size to 64 (default is 16). + await tester.pumpWidget(buildFrame(fontSize: 64)); + await tester.pumpAndSettle(); // Theme changes are animated. + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 0')).height, 128); + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 1')).height, 128); + expect(tester.getSize(find.text('Item 0')).height, 128); + expect(tester.getSize(find.text('Item 1')).height, 128); + expect(tester.getTopLeft(find.text('Item 0')).dx, 24); + expect(tester.getTopLeft(find.text('Item 1')).dx, 24); + + // While the menu is up, change the textDirection to rtl. Now menu items + // will be aligned right. + await tester.pumpWidget(buildFrame(textDirection: TextDirection.rtl)); + await tester.pumpAndSettle(); // Theme changes are animated. + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 0')).height, 48); + expect(tester.getSize(find.widgetWithText(menuItemType, 'Item 1')).height, 48); + expect(tester.getTopLeft(find.text('Item 0')).dx, 72); + expect(tester.getTopLeft(find.text('Item 1')).dx, 72); + }); } class TestApp extends StatefulWidget { diff --git a/packages/flutter_localizations/test/text_test.dart b/packages/flutter_localizations/test/text_test.dart index 28b395f1c5e..d03264bc585 100644 --- a/packages/flutter_localizations/test/text_test.dart +++ b/packages/flutter_localizations/test/text_test.dart @@ -78,20 +78,20 @@ void main() { Offset bottomLeft = tester.getBottomLeft(find.text('hello, world')); Offset bottomRight = tester.getBottomRight(find.text('hello, world')); - expect(topLeft, const Offset(392.0, 298.3999996185303)); - expect(topRight, const Offset(596.0, 298.3999996185303)); - expect(bottomLeft, const Offset(392.0, 315.3999996185303)); - expect(bottomRight, const Offset(596.0, 315.3999996185303)); + expect(topLeft, const Offset(392.0, 299.5)); + expect(topRight, const Offset(596.0, 299.5)); + expect(bottomLeft, const Offset(392.0, 316.5)); + expect(bottomRight, const Offset(596.0, 316.5)); topLeft = tester.getTopLeft(find.text('你好，世界')); topRight = tester.getTopRight(find.text('你好，世界')); bottomLeft = tester.getBottomLeft(find.text('你好，世界')); bottomRight = tester.getBottomRight(find.text('你好，世界')); - expect(topLeft, const Offset(392.0, 346.3999996185303)); - expect(topRight, const Offset(477.0, 346.3999996185303)); - expect(bottomLeft, const Offset(392.0, 363.3999996185303)); - expect(bottomRight, const Offset(477.0, 363.3999996185303)); + expect(topLeft, const Offset(392.0, 347.5)); + expect(topRight, const Offset(477.0, 347.5)); + expect(bottomLeft, const Offset(392.0, 364.5)); + expect(bottomRight, const Offset(477.0, 364.5)); }, skip: !isLinux); testWidgets('Text baseline with EN locale', (WidgetTester tester) async { @@ -165,19 +165,19 @@ void main() { Offset bottomRight = tester.getBottomRight(find.text('hello, world')); - expect(topLeft, const Offset(392.0, 299.19999980926514)); - expect(topRight, const Offset(584.0, 299.19999980926514)); - expect(bottomLeft, const Offset(392.0, 315.19999980926514)); - expect(bottomRight, const Offset(584.0, 315.19999980926514)); + expect(topLeft, const Offset(392.0, 300.0)); + expect(topRight, const Offset(584.0, 300.0)); + expect(bottomLeft, const Offset(392.0, 316)); + expect(bottomRight, const Offset(584.0, 316)); topLeft = tester.getTopLeft(find.text('你好，世界')); topRight = tester.getTopRight(find.text('你好，世界')); bottomLeft = tester.getBottomLeft(find.text('你好，世界')); bottomRight = tester.getBottomRight(find.text('你好，世界')); - expect(topLeft, const Offset(392.0, 347.19999980926514)); - expect(topRight, const Offset(472.0, 347.19999980926514)); - expect(bottomLeft, const Offset(392.0, 363.19999980926514)); - expect(bottomRight, const Offset(472.0, 363.19999980926514)); + expect(topLeft, const Offset(392.0, 348.0)); + expect(topRight, const Offset(472.0, 348.0)); + expect(bottomLeft, const Offset(392.0, 364.0)); + expect(bottomRight, const Offset(472.0, 364.0)); }, skip: !isLinux); }