Skip to content

Commit cc4abe9

Browse files
authored
Correct menu position when menu is constrained (#143121)
Fixes #142896 The original code below is to always place the selected item above(overlap) the popup button so that the selected item can always be visible: https://github.com/flutter/flutter/blob/f8a77225f360556322934a9a831ffc9c9f3125da/packages/flutter/lib/src/material/popup_menu.dart#L723-L732 But when menu height is constrained and the menu itself is super long, the selected item still assumes there is enough space to push up all the items whose index is smaller than the selected index. As a result, every time when the menu is open, the calculation provides a different result to be the offset for the selected index, and then with a constrained height, the menu looks jumping all over the place based on the different selected index. https://github.com/flutter/flutter/assets/36861262/ad761f95-0ff5-4311-a81d-dac56df879c5 Even though the original calculation is to make the selected item visible when open the menu, the menu doesn't auto scroll and only expands itself as much as possible to show the selected one. In this case, if the screen it too small to show the selected item, we still cannot see it. This can be fixed by using `Scrollable.ensureVisible()`(#143118). So we remove the calculation in this PR and the menu will always show up based on the top left of the anchor(button). https://github.com/flutter/flutter/assets/36861262/03272f26-9440-4ac4-a701-9a0b41776ff9
1 parent e64a372 commit cc4abe9

File tree

2 files changed

+77
-14
lines changed

2 files changed

+77
-14
lines changed

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

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -715,23 +715,12 @@ class _PopupMenuRouteLayout extends SingleChildLayoutDelegate {
715715

716716
@override
717717
Offset getPositionForChild(Size size, Size childSize) {
718+
final double y = position.top;
719+
720+
// Find the ideal horizontal position.
718721
// size: The size of the overlay.
719722
// childSize: The size of the menu, when fully open, as determined by
720723
// getConstraintsForChild.
721-
722-
final double buttonHeight = size.height - position.top - position.bottom;
723-
// Find the ideal vertical position.
724-
double y = position.top;
725-
if (selectedItemIndex != null) {
726-
double selectedItemOffset = _kMenuVerticalPadding;
727-
for (int index = 0; index < selectedItemIndex!; index += 1) {
728-
selectedItemOffset += itemSizes[index]!.height;
729-
}
730-
selectedItemOffset += itemSizes[selectedItemIndex!]!.height / 2;
731-
y = y + buttonHeight / 2.0 - selectedItemOffset;
732-
}
733-
734-
// Find the ideal horizontal position.
735724
double x;
736725
if (position.left > position.right) {
737726
// Menu button is closer to the right edge, so grow to the left, aligned to the right edge.

packages/flutter/test/material/popup_menu_test.dart

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3955,6 +3955,80 @@ void main() {
39553955

39563956
expect(tester.getSize(find.byType(Material).last), within(distance: 0.1, from: const Size(112.0, 160.0)));
39573957
});
3958+
3959+
testWidgets('PopupMenuButton properly positions a constrained-size popup', (WidgetTester tester) async {
3960+
final Size windowSize = tester.view.physicalSize / tester.view.devicePixelRatio;
3961+
const int length = 50;
3962+
await tester.pumpWidget(
3963+
MaterialApp(
3964+
home: Scaffold(
3965+
body: Padding(
3966+
padding: const EdgeInsets.all(50),
3967+
child: Align(
3968+
alignment: Alignment.bottomCenter,
3969+
child: PopupMenuButton<int>(
3970+
itemBuilder: (BuildContext context) {
3971+
return List<PopupMenuEntry<int>>.generate(length, (int index) {
3972+
return PopupMenuItem<int>(value: index, child: Text('item #$index'));
3973+
});
3974+
},
3975+
constraints: BoxConstraints(maxHeight: windowSize.height / 3),
3976+
popUpAnimationStyle: AnimationStyle.noAnimation,
3977+
initialValue: length - 1,
3978+
child: const Text('click here'),
3979+
),
3980+
),
3981+
),
3982+
),
3983+
),
3984+
);
3985+
await tester.tap(find.text('click here'));
3986+
await tester.pump();
3987+
3988+
// Set up finders and verify basic widget structure
3989+
final Finder findButton = find.byType(PopupMenuButton<int>);
3990+
final Finder findLastItem = find.text('item #49');
3991+
final Finder findListBody = find.byType(ListBody);
3992+
final Finder findListViewport = find.ancestor(
3993+
of: findListBody,
3994+
matching: find.byType(SingleChildScrollView),
3995+
);
3996+
expect(findButton, findsOne);
3997+
expect(findLastItem, findsOne);
3998+
expect(findListBody, findsOne);
3999+
expect(findListViewport, findsOne);
4000+
4001+
// The button and the list viewport should overlap
4002+
final RenderBox button = tester.renderObject<RenderBox>(findButton);
4003+
final Rect buttonBounds = button.localToGlobal(Offset.zero) & button.size;
4004+
final RenderBox listViewport = tester.renderObject<RenderBox>(findListViewport);
4005+
final Rect listViewportBounds = listViewport.localToGlobal(Offset.zero) & listViewport.size;
4006+
expect(listViewportBounds.topLeft.dy, lessThanOrEqualTo(windowSize.height));
4007+
expect(listViewportBounds.bottomRight.dy, lessThanOrEqualTo(windowSize.height));
4008+
expect(listViewportBounds, overlaps(buttonBounds));
4009+
});
4010+
}
4011+
4012+
Matcher overlaps(Rect other) => OverlapsMatcher(other);
4013+
4014+
class OverlapsMatcher extends Matcher {
4015+
OverlapsMatcher(this.other);
4016+
4017+
final Rect other;
4018+
4019+
@override
4020+
Description describe(Description description) {
4021+
return description.add('<Rect that overlaps with $other>');
4022+
}
4023+
4024+
@override
4025+
bool matches(Object? item, Map<dynamic, dynamic> matchState) => item is Rect && item.overlaps(other);
4026+
4027+
@override
4028+
Description describeMismatch(dynamic item, Description mismatchDescription,
4029+
Map<dynamic, dynamic> matchState, bool verbose) {
4030+
return mismatchDescription.add('does not overlap');
4031+
}
39584032
}
39594033

39604034
class TestApp extends StatelessWidget {

0 commit comments

Comments
 (0)