From efa7d52303e3e565027b2927e6b2597d9502766f Mon Sep 17 00:00:00 2001 From: YeungKC <11473691+YeungKC@users.noreply.github.com> Date: Thu, 5 Dec 2024 03:43:08 +0900 Subject: [PATCH] Fix: Update PopupMenu position when layout changes (#157983) This PR fixes an issue where PopupMenu doesn't update its position when the screen layout changes (e.g. during rotation or window resizing). ## Changes - Introduces `positionBuilder` instead of directly using position - Calls positionBuilder after layout changes to get the updated position - Adds tests to verify position updates correctly ## Related Issues Fixes #152475 - PopupMenu retains wrong position on layout change ## Implementation Notes This implementation uses a builder pattern to dynamically calculate positions. This approach may be applicable to other popup widgets (like DropdownButton mentioned in #156980) that have similar positioning requirements.




*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../flutter/lib/src/material/popup_menu.dart | 80 ++++++++++++++----- .../test/material/popup_menu_test.dart | 48 +++++++++++ 2 files changed, 110 insertions(+), 18 deletions(-) diff --git a/packages/flutter/lib/src/material/popup_menu.dart b/packages/flutter/lib/src/material/popup_menu.dart index 1aef889e31..0a8b65612d 100644 --- a/packages/flutter/lib/src/material/popup_menu.dart +++ b/packages/flutter/lib/src/material/popup_menu.dart @@ -852,7 +852,8 @@ class _PopupMenuRouteLayout extends SingleChildLayoutDelegate { class _PopupMenuRoute extends PopupRoute { _PopupMenuRoute({ - required this.position, + this.position, + this.positionBuilder, required this.items, required this.itemKeys, this.initialValue, @@ -870,12 +871,15 @@ class _PopupMenuRoute extends PopupRoute { super.settings, super.requestFocus, this.popUpAnimationStyle, - }) : itemSizes = List.filled(items.length, null), + }) : assert((position != null) != (positionBuilder != null), + 'Either position or positionBuilder must be provided.'), + itemSizes = List.filled(items.length, null), // Menus always cycle focus through their items irrespective of the // focus traversal edge behavior set in the Navigator. super(traversalEdgeBehavior: TraversalEdgeBehavior.closedLoop); - final RelativeRect position; + final RelativeRect? position; + final PopupMenuPositionBuilder? positionBuilder; final List> items; final List itemKeys; final List itemSizes; @@ -955,11 +959,11 @@ class _PopupMenuRoute extends PopupRoute { removeBottom: true, removeLeft: true, removeRight: true, - child: Builder( - builder: (BuildContext context) { + child: LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { return CustomSingleChildLayout( delegate: _PopupMenuRouteLayout( - position, + positionBuilder?.call(context, constraints) ?? position!, itemSizes, selectedItemIndex, Directionality.of(context), @@ -984,10 +988,39 @@ class _PopupMenuRoute extends PopupRoute { } } -/// Show a popup menu that contains the `items` at `position`. +/// A builder that creates a [RelativeRect] to position a popup menu. +/// Both [BuildContext] and [BoxConstraints] are from the [PopupRoute] that +/// displays this menu. +/// +/// The returned [RelativeRect] determines the position of the popup menu relative +/// to the bounds of the [Navigator]'s overlay. The menu dimensions are not yet +/// known when this callback is invoked, as they depend on the items and other +/// properties of the menu. +/// +/// The coordinate system used by the [RelativeRect] has its origin at the top +/// left of the [Navigator]'s overlay. Positive y coordinates are down (below the +/// origin), and positive x coordinates are to the right of the origin. +/// +/// See also: +/// +/// * [RelativeRect.fromLTRB], which creates a [RelativeRect] from left, top, +/// right, and bottom coordinates. +/// * [RelativeRect.fromRect], which creates a [RelativeRect] from two [Rect]s, +/// one representing the size of the popup menu and one representing the size +/// of the overlay. +typedef PopupMenuPositionBuilder = RelativeRect Function( + BuildContext context, BoxConstraints constraints); + +/// Shows a popup menu that contains the `items` at `position`. /// /// The `items` parameter must not be empty. /// +/// Only one of [position] or [positionBuilder] should be provided. Providing both +/// throws an assertion error. The [positionBuilder] is called at the time the +/// menu is shown to compute its position and every time the layout is updated, +/// which is useful when the position needs +/// to be determined at runtime based on the current layout. +/// /// If `initialValue` is specified then the first item with a matching value /// will be highlighted and the value of `position` gives the rectangle whose /// vertical center will be aligned with the vertical center of the highlighted @@ -1047,7 +1080,8 @@ class _PopupMenuRoute extends PopupRoute { /// semantics. Future showMenu({ required BuildContext context, - required RelativeRect position, + RelativeRect? position, + PopupMenuPositionBuilder? positionBuilder, required List> items, T? initialValue, double? elevation, @@ -1066,6 +1100,8 @@ Future showMenu({ }) { assert(items.isNotEmpty); assert(debugCheckHasMaterialLocalizations(context)); + assert((position != null) != (positionBuilder != null), + 'Either position or positionBuilder must be provided.'); switch (Theme.of(context).platform) { case TargetPlatform.iOS: @@ -1082,6 +1118,7 @@ Future showMenu({ final NavigatorState navigator = Navigator.of(context, rootNavigator: useRootNavigator); return navigator.push(_PopupMenuRoute( position: position, + positionBuilder: positionBuilder, items: items, itemKeys: menuItemKeys, initialValue: initialValue, @@ -1468,15 +1505,8 @@ class PopupMenuButton extends StatefulWidget { /// See [showButtonMenu] for a way to programmatically open the popup menu /// of your button state. class PopupMenuButtonState extends State> { - /// A method to show a popup menu with the items supplied to - /// [PopupMenuButton.itemBuilder] at the position of your [PopupMenuButton]. - /// - /// By default, it is called when the user taps the button and [PopupMenuButton.enabled] - /// is set to `true`. Moreover, you can open the button by calling the method manually. - /// - /// You would access your [PopupMenuButtonState] using a [GlobalKey] and - /// show the menu of the button with `globalKey.currentState.showButtonMenu`. - void showButtonMenu() { + + RelativeRect _positionBuilder(BuildContext _, BoxConstraints constraints) { final PopupMenuThemeData popupMenuTheme = PopupMenuTheme.of(context); final RenderBox button = context.findRenderObject()! as RenderBox; final RenderBox overlay = Navigator.of( @@ -1502,6 +1532,20 @@ class PopupMenuButtonState extends State> { ), Offset.zero & overlay.size, ); + + return position; + } + + /// A method to show a popup menu with the items supplied to + /// [PopupMenuButton.itemBuilder] at the position of your [PopupMenuButton]. + /// + /// By default, it is called when the user taps the button and [PopupMenuButton.enabled] + /// is set to `true`. Moreover, you can open the button by calling the method manually. + /// + /// You would access your [PopupMenuButtonState] using a [GlobalKey] and + /// show the menu of the button with `globalKey.currentState.showButtonMenu`. + void showButtonMenu() { + final PopupMenuThemeData popupMenuTheme = PopupMenuTheme.of(context); final List> items = widget.itemBuilder(context); // Only show the menu if there is something to show if (items.isNotEmpty) { @@ -1513,7 +1557,7 @@ class PopupMenuButtonState extends State> { surfaceTintColor: widget.surfaceTintColor ?? popupMenuTheme.surfaceTintColor, items: items, initialValue: widget.initialValue, - position: position, + positionBuilder: _positionBuilder, shape: widget.shape ?? popupMenuTheme.shape, menuPadding: widget.menuPadding ?? popupMenuTheme.menuPadding, color: widget.color ?? popupMenuTheme.color, diff --git a/packages/flutter/test/material/popup_menu_test.dart b/packages/flutter/test/material/popup_menu_test.dart index a6a56c5577..bf56f5567f 100644 --- a/packages/flutter/test/material/popup_menu_test.dart +++ b/packages/flutter/test/material/popup_menu_test.dart @@ -4387,6 +4387,54 @@ void main() { await tester.pump(); expect(fieldFocusNode.hasFocus, isTrue); }); + + // Regression test for https://github.com/flutter/flutter/issues/152475 + testWidgets('PopupMenuButton updates position on orientation change', (WidgetTester tester) async { + const Size initialSize = Size(400, 800); + const Size newSize = Size(1024, 768); + + await tester.binding.setSurfaceSize(initialSize); + + final GlobalKey buttonKey = GlobalKey(); + + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: Center( + child: PopupMenuButton( + key: buttonKey, + itemBuilder: (BuildContext context) => >[ + const PopupMenuItem( + value: 1, + child: Text('Option 1'), + ), + ], + ), + ), + ), + ), + ); + + await tester.tap(find.byType(PopupMenuButton)); + await tester.pumpAndSettle(); + + final Rect initialButtonRect = tester.getRect(find.byKey(buttonKey)); + final Rect initialMenuRect = tester.getRect(find.text('Option 1')); + + await tester.binding.setSurfaceSize(newSize); + await tester.pumpAndSettle(); + + final Rect newButtonRect = tester.getRect(find.byKey(buttonKey)); + final Rect newMenuRect = tester.getRect(find.text('Option 1')); + + expect(newButtonRect, isNot(equals(initialButtonRect))); + + expect(newMenuRect, isNot(equals(initialMenuRect))); + + expect(newMenuRect.topLeft - newButtonRect.topLeft, initialMenuRect.topLeft - initialButtonRect.topLeft); + + await tester.binding.setSurfaceSize(null); + }); } Matcher overlaps(Rect other) => OverlapsMatcher(other);