From 04d418beaca19bc37dc83f0304f6c3a11d1a3d2f Mon Sep 17 00:00:00 2001 From: Hans Muller Date: Tue, 23 May 2017 16:55:57 -0700 Subject: [PATCH] Correct the initial rendering of non-scrollable dropdown menus (#10255) --- .../flutter/lib/src/material/dropdown.dart | 112 ++++++++++------ .../lib/src/widgets/scroll_position.dart | 3 - .../flutter/test/material/dropdown_test.dart | 123 +++++++++++++++++- 3 files changed, 189 insertions(+), 49 deletions(-) diff --git a/packages/flutter/lib/src/material/dropdown.dart b/packages/flutter/lib/src/material/dropdown.dart index 31e05a23d9..c4473969a7 100644 --- a/packages/flutter/lib/src/material/dropdown.dart +++ b/packages/flutter/lib/src/material/dropdown.dart @@ -5,7 +5,6 @@ import 'dart:math' as math; import 'package:flutter/foundation.dart'; -import 'package:flutter/scheduler.dart'; import 'package:flutter/widgets.dart'; import 'colors.dart'; @@ -191,13 +190,11 @@ class _DropdownMenuState extends State<_DropdownMenu> { } class _DropdownMenuRouteLayout extends SingleChildLayoutDelegate { - _DropdownMenuRouteLayout({ this.route }); + _DropdownMenuRouteLayout({ this.buttonRect, this.menuTop, this.menuHeight }); - final _DropdownRoute route; - - Rect get buttonRect => route.buttonRect; - int get selectedIndex => route.selectedIndex; - ScrollController get scrollController => route.scrollController; + final Rect buttonRect; + final double menuTop; + final double menuHeight; @override BoxConstraints getConstraintsForChild(BoxConstraints constraints) { @@ -217,45 +214,28 @@ class _DropdownMenuRouteLayout extends SingleChildLayoutDelegate { @override Offset getPositionForChild(Size size, Size childSize) { - final double buttonTop = buttonRect.top; - final double selectedItemOffset = selectedIndex * _kMenuItemHeight + kMaterialListPadding.top; - double top = (buttonTop - selectedItemOffset) - (_kMenuItemHeight - buttonRect.height) / 2.0; - final double topPreferredLimit = _kMenuItemHeight; - if (top < topPreferredLimit) - top = math.min(buttonTop, topPreferredLimit); - double bottom = top + childSize.height; - final double bottomPreferredLimit = size.height - _kMenuItemHeight; - if (bottom > bottomPreferredLimit) { - bottom = math.max(buttonTop + _kMenuItemHeight, bottomPreferredLimit); - top = bottom - childSize.height; - } assert(() { final Rect container = Offset.zero & size; if (container.intersect(buttonRect) == buttonRect) { // If the button was entirely on-screen, then verify // that the menu is also on-screen. // If the button was a bit off-screen, then, oh well. - assert(top >= 0.0); - assert(top + childSize.height <= size.height); + assert(menuTop >= 0.0); + assert(menuTop + menuHeight <= size.height); } return true; }); - if (route.initialLayout) { - route.initialLayout = false; - final double scrollOffset = selectedItemOffset - (buttonTop - top); - SchedulerBinding.instance.addPostFrameCallback((Duration timeStamp) { - // TODO(ianh): Compute and set this during layout instead of being - // lagged by one frame. https://github.com/flutter/flutter/issues/5751 - scrollController.jumpTo(scrollOffset); - }); - } - - return new Offset(buttonRect.left, top); + final double width = buttonRect.width + 8.0; + return new Offset(buttonRect.left.clamp(0.0, size.width - width), menuTop); } @override - bool shouldRelayout(_DropdownMenuRouteLayout oldDelegate) => oldDelegate.route != route; + bool shouldRelayout(_DropdownMenuRouteLayout oldDelegate) { + return buttonRect != oldDelegate.buttonRect + || menuTop != oldDelegate.menuTop + || menuHeight != oldDelegate.menuHeight; + } } // We box the return value so that the return value can be null. Otherwise, @@ -290,7 +270,6 @@ class _DropdownRoute extends PopupRoute<_DropdownRouteResult> { assert(style != null); } - final ScrollController scrollController = new ScrollController(); final List> items; final Rect buttonRect; final int selectedIndex; @@ -298,9 +277,7 @@ class _DropdownRoute extends PopupRoute<_DropdownRouteResult> { final ThemeData theme; final TextStyle style; - // The layout gets this route's scrollController so that it can scroll the - // selected item into position, but only on the initial layout. - bool initialLayout = true; + ScrollController scrollController; @override Duration get transitionDuration => _kDropdownMenuDuration; @@ -313,12 +290,41 @@ class _DropdownRoute extends PopupRoute<_DropdownRouteResult> { @override Widget buildPage(BuildContext context, Animation animation, Animation secondaryAnimation) { + final double screenHeight = MediaQuery.of(context).size.height; + final double maxMenuHeight = screenHeight - 2.0 * _kMenuItemHeight; + final double preferredMenuHeight = (items.length * _kMenuItemHeight) + kMaterialListPadding.vertical; + final double menuHeight = math.min(maxMenuHeight, preferredMenuHeight); + + final double buttonTop = buttonRect.top; + final double selectedItemOffset = selectedIndex * _kMenuItemHeight + kMaterialListPadding.top; + double menuTop = (buttonTop - selectedItemOffset) - (_kMenuItemHeight - buttonRect.height) / 2.0; + final double topPreferredLimit = _kMenuItemHeight; + if (menuTop < topPreferredLimit) + menuTop = math.min(buttonTop, topPreferredLimit); + double bottom = menuTop + menuHeight; + final double bottomPreferredLimit = screenHeight - _kMenuItemHeight; + if (bottom > bottomPreferredLimit) { + bottom = math.max(buttonTop + _kMenuItemHeight, bottomPreferredLimit); + menuTop = bottom - menuHeight; + } + + if (scrollController == null) { + double scrollOffset = 0.0; + if (preferredMenuHeight > maxMenuHeight) + scrollOffset = selectedItemOffset - (buttonTop - menuTop); + scrollController = new ScrollController(initialScrollOffset: scrollOffset); + } + Widget menu = new _DropdownMenu(route: this); if (theme != null) menu = new Theme(data: theme, child: menu); return new CustomSingleChildLayout( - delegate: new _DropdownMenuRouteLayout(route: this), + delegate: new _DropdownMenuRouteLayout( + buttonRect: buttonRect, + menuTop: menuTop, + menuHeight: menuHeight, + ), child: menu, ); } @@ -466,16 +472,33 @@ class DropdownButton extends StatefulWidget { _DropdownButtonState createState() => new _DropdownButtonState(); } -class _DropdownButtonState extends State> { +class _DropdownButtonState extends State> with WidgetsBindingObserver { int _selectedIndex; + _DropdownRoute _dropdownRoute; @override void initState() { super.initState(); _updateSelectedIndex(); + WidgetsBinding.instance.addObserver(this); } - @override + @override + void dispose() { + WidgetsBinding.instance.removeObserver(this); + //TODO(hansmuller) if _dropDownRoute != null Navigator.remove(context, _dropdownRoute) + super.dispose(); + } + + // Typically called because the device's orientation has changed. + // Defined by WidgetsBindingObserver + @override + void didChangeMetrics() { + //TODO(hansmuller) if _dropDownRoute != null Navigator.remove(context, _dropdownRoute) + _dropdownRoute = null; + } + + @override void didUpdateWidget(DropdownButton oldWidget) { super.didUpdateWidget(oldWidget); _updateSelectedIndex(); @@ -498,14 +521,19 @@ class _DropdownButtonState extends State> { void _handleTap() { final RenderBox itemBox = context.findRenderObject(); final Rect itemRect = itemBox.localToGlobal(Offset.zero) & itemBox.size; - Navigator.push(context, new _DropdownRoute( + + assert(_dropdownRoute == null); + _dropdownRoute = new _DropdownRoute( items: widget.items, buttonRect: _kMenuHorizontalPadding.inflateRect(itemRect), selectedIndex: _selectedIndex ?? 0, elevation: widget.elevation, theme: Theme.of(context, shadowThemeOnly: true), style: _textStyle, - )).then((_DropdownRouteResult newValue) { + ); + + Navigator.push(context, _dropdownRoute).then((_DropdownRouteResult newValue) { + _dropdownRoute = null; if (!mounted || newValue == null) return null; if (widget.onChanged != null) diff --git a/packages/flutter/lib/src/widgets/scroll_position.dart b/packages/flutter/lib/src/widgets/scroll_position.dart index 360b7a814e..4b4e580183 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart @@ -466,9 +466,6 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { /// If this method changes the scroll position, a sequence of start/update/end /// scroll notifications will be dispatched. No overscroll notifications can /// be generated by this method. - /// - /// If settle is true then, immediately after the jump, a ballistic activity - /// is started, in case the value was out of range. void jumpTo(double value); /// Deprecated. Use [jumpTo] or a custom [ScrollPosition] instead. diff --git a/packages/flutter/test/material/dropdown_test.dart b/packages/flutter/test/material/dropdown_test.dart index 9b679d37bd..0f3b38219d 100644 --- a/packages/flutter/test/material/dropdown_test.dart +++ b/packages/flutter/test/material/dropdown_test.dart @@ -7,7 +7,12 @@ import 'dart:math' as math; import 'package:flutter_test/flutter_test.dart'; import 'package:flutter/material.dart'; -final List menuItems = ['one', 'two', 'three', 'four']; +const List menuItems = const ['one', 'two', 'three', 'four']; + +final Type dropdownButtonType = new DropdownButton( + onChanged: (_){ }, + items: const >[] +).runtimeType; Widget buildFrame({ Key buttonKey, @@ -15,17 +20,20 @@ Widget buildFrame({ ValueChanged onChanged, bool isDense: false, Widget hint, + List items: menuItems, + FractionalOffset alignment: FractionalOffset.center, }) { return new MaterialApp( home: new Material( - child: new Center( + child: new Align( + alignment: alignment, child: new DropdownButton( key: buttonKey, value: value, hint: hint, onChanged: onChanged, isDense: isDense, - items: menuItems.map((String item) { + items: items.map((String item) { return new DropdownMenuItem( key: new ValueKey(item), value: item, @@ -57,7 +65,6 @@ bool sameGeometry(RenderBox box1, RenderBox box2) { return true; } - void main() { testWidgets('Dropdown button control test', (WidgetTester tester) async { String value = 'one'; @@ -348,4 +355,112 @@ void main() { expect(buttonBox.size, equals(buttonBoxHintValue.size)); }); + testWidgets('Dropdown menus must fit within the screen', (WidgetTester tester) async { + + // The dropdown menu isn't readaily accessible. To find it we're assuming that it + // contains a ListView and that it's an instance of _DropdownMenu. + Rect getMenuRect() { + Rect menuRect; + tester.element(find.byType(ListView)).visitAncestorElements((Element element) { + if (element.toString().startsWith("_DropdownMenu")) { + final RenderBox box = element.findRenderObject(); + assert(box != null); + menuRect = box.localToGlobal(Offset.zero) & box.size; + return false; + } + return true; + }); + assert(menuRect != null); + return menuRect; + } + + // In all of the tests that follow we're assuming that the dropdown menu + // is horizontally aligned with the center of the dropdown button and padded + // on the top, left, and right. + const EdgeInsets buttonPadding = const EdgeInsets.only(top: 8.0, left: 16.0, right: 24.0); + + Rect getExpandedButtonRect() { + final RenderBox box = tester.renderObject(find.byType(dropdownButtonType)); + final Rect buttonRect = box.localToGlobal(Offset.zero) & box.size; + return buttonPadding.inflateRect(buttonRect); + } + + Rect buttonRect; + Rect menuRect; + + Future popUpAndDown(Widget frame) async { + await tester.pumpWidget(frame); + await tester.tap(find.byType(dropdownButtonType)); + await tester.pumpAndSettle(); + menuRect = getMenuRect(); + buttonRect = getExpandedButtonRect(); + await tester.tap(find.byType(dropdownButtonType)); + } + + // Dropdown button is along the top of the app. The top of the menu is + // aligned with the top of the expanded button and shifted horizontally + // so that it fits within the frame. + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.topLeft, value: menuItems.last) + ); + expect(menuRect.topLeft, Offset.zero); + expect(menuRect.topRight, new Offset(menuRect.width, 0.0)); + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.topCenter, value: menuItems.last) + ); + expect(menuRect.topLeft, new Offset(buttonRect.left, 0.0)); + expect(menuRect.topRight, new Offset(buttonRect.right, 0.0)); + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.topRight, value: menuItems.last) + ); + expect(menuRect.topLeft, new Offset(800.0 - menuRect.width, 0.0)); + expect(menuRect.topRight, const Offset(800.0, 0.0)); + + // Dropdown button is along the middle of the app. The top of the menu is + // aligned with the top of the expanded button (because the 1st item + // is selected) and shifted horizontally so that it fits within the frame. + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.centerLeft, value: menuItems.first) + ); + expect(menuRect.topLeft, new Offset(0.0, buttonRect.top)); + expect(menuRect.topRight, new Offset(menuRect.width, buttonRect.top)); + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.center, value: menuItems.first) + ); + expect(menuRect.topLeft, buttonRect.topLeft); + expect(menuRect.topRight, buttonRect.topRight); + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.centerRight, value: menuItems.first) + ); + expect(menuRect.topLeft, new Offset(800.0 - menuRect.width, buttonRect.top)); + expect(menuRect.topRight, new Offset(800.0, buttonRect.top)); + + // Dropdown button is along the bottom of the app. The bottom of the menu is + // aligned with the bottom of the expanded button and shifted horizontally + // so that it fits within the frame. + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.bottomLeft, value: menuItems.first) + ); + expect(menuRect.bottomLeft, const Offset(0.0, 600.0)); + expect(menuRect.bottomRight, new Offset(menuRect.width, 600.0)); + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.bottomCenter, value: menuItems.first) + ); + expect(menuRect.bottomLeft, new Offset(buttonRect.left, 600.0)); + expect(menuRect.bottomRight, new Offset(buttonRect.right, 600.0)); + + await popUpAndDown( + buildFrame(alignment: FractionalOffset.bottomRight, value: menuItems.first) + ); + expect(menuRect.bottomLeft, new Offset(800.0 - menuRect.width, 600.0)); + expect(menuRect.bottomRight, const Offset(800.0, 600.0)); + }); }