Skip to content

Commit efa7d52

Browse files
authored
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. <br> <br> <br> <br> <br> *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]. <!-- Links --> [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
1 parent f9f42be commit efa7d52

File tree

2 files changed

+110
-18
lines changed

2 files changed

+110
-18
lines changed

packages/flutter/lib/src/material/popup_menu.dart

Lines changed: 62 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -852,7 +852,8 @@ class _PopupMenuRouteLayout extends SingleChildLayoutDelegate {
852852

853853
class _PopupMenuRoute<T> extends PopupRoute<T> {
854854
_PopupMenuRoute({
855-
required this.position,
855+
this.position,
856+
this.positionBuilder,
856857
required this.items,
857858
required this.itemKeys,
858859
this.initialValue,
@@ -870,12 +871,15 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
870871
super.settings,
871872
super.requestFocus,
872873
this.popUpAnimationStyle,
873-
}) : itemSizes = List<Size?>.filled(items.length, null),
874+
}) : assert((position != null) != (positionBuilder != null),
875+
'Either position or positionBuilder must be provided.'),
876+
itemSizes = List<Size?>.filled(items.length, null),
874877
// Menus always cycle focus through their items irrespective of the
875878
// focus traversal edge behavior set in the Navigator.
876879
super(traversalEdgeBehavior: TraversalEdgeBehavior.closedLoop);
877880

878-
final RelativeRect position;
881+
final RelativeRect? position;
882+
final PopupMenuPositionBuilder? positionBuilder;
879883
final List<PopupMenuEntry<T>> items;
880884
final List<GlobalKey> itemKeys;
881885
final List<Size?> itemSizes;
@@ -955,11 +959,11 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
955959
removeBottom: true,
956960
removeLeft: true,
957961
removeRight: true,
958-
child: Builder(
959-
builder: (BuildContext context) {
962+
child: LayoutBuilder(
963+
builder: (BuildContext context, BoxConstraints constraints) {
960964
return CustomSingleChildLayout(
961965
delegate: _PopupMenuRouteLayout(
962-
position,
966+
positionBuilder?.call(context, constraints) ?? position!,
963967
itemSizes,
964968
selectedItemIndex,
965969
Directionality.of(context),
@@ -984,10 +988,39 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
984988
}
985989
}
986990

987-
/// Show a popup menu that contains the `items` at `position`.
991+
/// A builder that creates a [RelativeRect] to position a popup menu.
992+
/// Both [BuildContext] and [BoxConstraints] are from the [PopupRoute] that
993+
/// displays this menu.
994+
///
995+
/// The returned [RelativeRect] determines the position of the popup menu relative
996+
/// to the bounds of the [Navigator]'s overlay. The menu dimensions are not yet
997+
/// known when this callback is invoked, as they depend on the items and other
998+
/// properties of the menu.
999+
///
1000+
/// The coordinate system used by the [RelativeRect] has its origin at the top
1001+
/// left of the [Navigator]'s overlay. Positive y coordinates are down (below the
1002+
/// origin), and positive x coordinates are to the right of the origin.
1003+
///
1004+
/// See also:
1005+
///
1006+
/// * [RelativeRect.fromLTRB], which creates a [RelativeRect] from left, top,
1007+
/// right, and bottom coordinates.
1008+
/// * [RelativeRect.fromRect], which creates a [RelativeRect] from two [Rect]s,
1009+
/// one representing the size of the popup menu and one representing the size
1010+
/// of the overlay.
1011+
typedef PopupMenuPositionBuilder = RelativeRect Function(
1012+
BuildContext context, BoxConstraints constraints);
1013+
1014+
/// Shows a popup menu that contains the `items` at `position`.
9881015
///
9891016
/// The `items` parameter must not be empty.
9901017
///
1018+
/// Only one of [position] or [positionBuilder] should be provided. Providing both
1019+
/// throws an assertion error. The [positionBuilder] is called at the time the
1020+
/// menu is shown to compute its position and every time the layout is updated,
1021+
/// which is useful when the position needs
1022+
/// to be determined at runtime based on the current layout.
1023+
///
9911024
/// If `initialValue` is specified then the first item with a matching value
9921025
/// will be highlighted and the value of `position` gives the rectangle whose
9931026
/// vertical center will be aligned with the vertical center of the highlighted
@@ -1047,7 +1080,8 @@ class _PopupMenuRoute<T> extends PopupRoute<T> {
10471080
/// semantics.
10481081
Future<T?> showMenu<T>({
10491082
required BuildContext context,
1050-
required RelativeRect position,
1083+
RelativeRect? position,
1084+
PopupMenuPositionBuilder? positionBuilder,
10511085
required List<PopupMenuEntry<T>> items,
10521086
T? initialValue,
10531087
double? elevation,
@@ -1066,6 +1100,8 @@ Future<T?> showMenu<T>({
10661100
}) {
10671101
assert(items.isNotEmpty);
10681102
assert(debugCheckHasMaterialLocalizations(context));
1103+
assert((position != null) != (positionBuilder != null),
1104+
'Either position or positionBuilder must be provided.');
10691105

10701106
switch (Theme.of(context).platform) {
10711107
case TargetPlatform.iOS:
@@ -1082,6 +1118,7 @@ Future<T?> showMenu<T>({
10821118
final NavigatorState navigator = Navigator.of(context, rootNavigator: useRootNavigator);
10831119
return navigator.push(_PopupMenuRoute<T>(
10841120
position: position,
1121+
positionBuilder: positionBuilder,
10851122
items: items,
10861123
itemKeys: menuItemKeys,
10871124
initialValue: initialValue,
@@ -1468,15 +1505,8 @@ class PopupMenuButton<T> extends StatefulWidget {
14681505
/// See [showButtonMenu] for a way to programmatically open the popup menu
14691506
/// of your button state.
14701507
class PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
1471-
/// A method to show a popup menu with the items supplied to
1472-
/// [PopupMenuButton.itemBuilder] at the position of your [PopupMenuButton].
1473-
///
1474-
/// By default, it is called when the user taps the button and [PopupMenuButton.enabled]
1475-
/// is set to `true`. Moreover, you can open the button by calling the method manually.
1476-
///
1477-
/// You would access your [PopupMenuButtonState] using a [GlobalKey] and
1478-
/// show the menu of the button with `globalKey.currentState.showButtonMenu`.
1479-
void showButtonMenu() {
1508+
1509+
RelativeRect _positionBuilder(BuildContext _, BoxConstraints constraints) {
14801510
final PopupMenuThemeData popupMenuTheme = PopupMenuTheme.of(context);
14811511
final RenderBox button = context.findRenderObject()! as RenderBox;
14821512
final RenderBox overlay = Navigator.of(
@@ -1502,6 +1532,20 @@ class PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
15021532
),
15031533
Offset.zero & overlay.size,
15041534
);
1535+
1536+
return position;
1537+
}
1538+
1539+
/// A method to show a popup menu with the items supplied to
1540+
/// [PopupMenuButton.itemBuilder] at the position of your [PopupMenuButton].
1541+
///
1542+
/// By default, it is called when the user taps the button and [PopupMenuButton.enabled]
1543+
/// is set to `true`. Moreover, you can open the button by calling the method manually.
1544+
///
1545+
/// You would access your [PopupMenuButtonState] using a [GlobalKey] and
1546+
/// show the menu of the button with `globalKey.currentState.showButtonMenu`.
1547+
void showButtonMenu() {
1548+
final PopupMenuThemeData popupMenuTheme = PopupMenuTheme.of(context);
15051549
final List<PopupMenuEntry<T>> items = widget.itemBuilder(context);
15061550
// Only show the menu if there is something to show
15071551
if (items.isNotEmpty) {
@@ -1513,7 +1557,7 @@ class PopupMenuButtonState<T> extends State<PopupMenuButton<T>> {
15131557
surfaceTintColor: widget.surfaceTintColor ?? popupMenuTheme.surfaceTintColor,
15141558
items: items,
15151559
initialValue: widget.initialValue,
1516-
position: position,
1560+
positionBuilder: _positionBuilder,
15171561
shape: widget.shape ?? popupMenuTheme.shape,
15181562
menuPadding: widget.menuPadding ?? popupMenuTheme.menuPadding,
15191563
color: widget.color ?? popupMenuTheme.color,

packages/flutter/test/material/popup_menu_test.dart

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4387,6 +4387,54 @@ void main() {
43874387
await tester.pump();
43884388
expect(fieldFocusNode.hasFocus, isTrue);
43894389
});
4390+
4391+
// Regression test for https://github.com/flutter/flutter/issues/152475
4392+
testWidgets('PopupMenuButton updates position on orientation change', (WidgetTester tester) async {
4393+
const Size initialSize = Size(400, 800);
4394+
const Size newSize = Size(1024, 768);
4395+
4396+
await tester.binding.setSurfaceSize(initialSize);
4397+
4398+
final GlobalKey buttonKey = GlobalKey();
4399+
4400+
await tester.pumpWidget(
4401+
MaterialApp(
4402+
home: Scaffold(
4403+
body: Center(
4404+
child: PopupMenuButton<int>(
4405+
key: buttonKey,
4406+
itemBuilder: (BuildContext context) => <PopupMenuItem<int>>[
4407+
const PopupMenuItem<int>(
4408+
value: 1,
4409+
child: Text('Option 1'),
4410+
),
4411+
],
4412+
),
4413+
),
4414+
),
4415+
),
4416+
);
4417+
4418+
await tester.tap(find.byType(PopupMenuButton<int>));
4419+
await tester.pumpAndSettle();
4420+
4421+
final Rect initialButtonRect = tester.getRect(find.byKey(buttonKey));
4422+
final Rect initialMenuRect = tester.getRect(find.text('Option 1'));
4423+
4424+
await tester.binding.setSurfaceSize(newSize);
4425+
await tester.pumpAndSettle();
4426+
4427+
final Rect newButtonRect = tester.getRect(find.byKey(buttonKey));
4428+
final Rect newMenuRect = tester.getRect(find.text('Option 1'));
4429+
4430+
expect(newButtonRect, isNot(equals(initialButtonRect)));
4431+
4432+
expect(newMenuRect, isNot(equals(initialMenuRect)));
4433+
4434+
expect(newMenuRect.topLeft - newButtonRect.topLeft, initialMenuRect.topLeft - initialButtonRect.topLeft);
4435+
4436+
await tester.binding.setSurfaceSize(null);
4437+
});
43904438
}
43914439

43924440
Matcher overlaps(Rect other) => OverlapsMatcher(other);

0 commit comments

Comments
 (0)