-
Notifications
You must be signed in to change notification settings - Fork 29.5k
CupertinoContextMenu child respects available screen width #175300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes an issue where the CupertinoContextMenu child widget would not respect the available screen width, potentially causing layout overflows. The fix involves calculating the available width for the child, accounting for screen padding and the context menu's own width in landscape mode, and applying this as a maxWidth constraint during layout. New widget tests are added for both portrait and landscape orientations to verify the fix.
My review focuses on improving the new tests to be more specific and robust. I've suggested adding explicit assertions to check that the child's width is correctly constrained, which will better validate the fix and prevent future regressions.
| testWidgets('CupertinoContextMenu respects available screen width - Portrait', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| const Size portraitScreenSize = Size(300.0, 350.0); | ||
| await binding.setSurfaceSize(portraitScreenSize); | ||
| addTearDown(() => binding.setSurfaceSize(null)); | ||
|
|
||
| final Widget child = getChild(); | ||
| await tester.pumpWidget( | ||
| MediaQuery( | ||
| data: const MediaQueryData(size: portraitScreenSize), | ||
| child: CupertinoApp( | ||
| home: Center( | ||
| child: CupertinoContextMenu( | ||
| actions: <Widget>[ | ||
| CupertinoContextMenuAction(child: const Text('Test'), onPressed: () {}), | ||
| ], | ||
| child: child, | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| expect(find.byWidget(child), findsOneWidget); | ||
| final Rect childRect = tester.getRect(find.byWidget(child)); | ||
|
|
||
| // Start a press on the child. | ||
| final TestGesture gesture = await tester.startGesture(childRect.center); | ||
| await tester.pump(); | ||
| await tester.pump(const Duration(milliseconds: 500)); | ||
| await gesture.up(); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(tester.takeException(), null); | ||
| }); | ||
|
|
||
| testWidgets('CupertinoContextMenu respects available screen width - Landscape', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| const Size landscapeScreenSize = Size(350.0, 300.0); | ||
| await binding.setSurfaceSize(landscapeScreenSize); | ||
| addTearDown(() => binding.setSurfaceSize(null)); | ||
|
|
||
| final Widget child = getChild(width: 500); | ||
| await tester.pumpWidget( | ||
| MediaQuery( | ||
| data: const MediaQueryData(size: landscapeScreenSize), | ||
| child: CupertinoApp( | ||
| home: Center( | ||
| child: CupertinoContextMenu( | ||
| actions: <Widget>[ | ||
| CupertinoContextMenuAction(child: const Text('Test'), onPressed: () {}), | ||
| ], | ||
| child: child, | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| expect(find.byWidget(child), findsOneWidget); | ||
| final Rect childRect = tester.getRect(find.byWidget(child)); | ||
|
|
||
| // Start a press on the child. | ||
| final TestGesture gesture = await tester.startGesture(childRect.center); | ||
| await tester.pump(); | ||
| await tester.pump(const Duration(milliseconds: 500)); | ||
| await gesture.up(); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(tester.takeException(), null); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new tests are great for ensuring CupertinoContextMenu respects screen width. However, they currently only check that no exceptions are thrown. To make them more robust and to specifically verify the fix, I suggest adding assertions to check that the child widget's size is correctly constrained in both portrait and landscape modes.
testWidgets('CupertinoContextMenu respects available screen width - Portrait', (
WidgetTester tester,
) async {
const Size portraitScreenSize = Size(300.0, 350.0);
await binding.setSurfaceSize(portraitScreenSize);
addTearDown(() => binding.setSurfaceSize(null));
final Widget child = getChild();
await tester.pumpWidget(
MediaQuery(
data: const MediaQueryData(size: portraitScreenSize),
child: CupertinoApp(
home: Center(
child: CupertinoContextMenu(
actions: <Widget>[
CupertinoContextMenuAction(child: const Text('Test'), onPressed: () {}),
],
child: child,
),
),
),
),
);
expect(find.byWidget(child), findsOneWidget);
final Rect childRect = tester.getRect(find.byWidget(child));
// Start a press on the child.
final TestGesture gesture = await tester.startGesture(childRect.center);
await tester.pump();
await tester.pump(const Duration(milliseconds: 500));
await gesture.up();
await tester.pumpAndSettle();
expect(tester.takeException(), null);
// Verify the child width is constrained correctly.
expect(findStatic(), findsOneWidget);
final Size fittedBoxSize = tester.getSize(findFittedBox());
// availableWidth = 300.0 (screen width) - 2 * 20.0 (padding) = 260.0
expect(fittedBoxSize.width, 260.0);
});
testWidgets('CupertinoContextMenu respects available screen width - Landscape', (
WidgetTester tester,
) async {
const Size landscapeScreenSize = Size(350.0, 300.0);
await binding.setSurfaceSize(landscapeScreenSize);
addTearDown(() => binding.setSurfaceSize(null));
final Widget child = getChild(width: 500);
await tester.pumpWidget(
MediaQuery(
data: const MediaQueryData(size: landscapeScreenSize),
child: CupertinoApp(
home: Center(
child: CupertinoContextMenu(
actions: <Widget>[
CupertinoContextMenuAction(child: const Text('Test'), onPressed: () {}),
],
child: child,
),
),
),
),
);
expect(find.byWidget(child), findsOneWidget);
final Rect childRect = tester.getRect(find.byWidget(child));
// Start a press on the child.
final TestGesture gesture = await tester.startGesture(childRect.center);
await tester.pump();
await tester.pump(const Duration(milliseconds: 500));
await gesture.up();
await tester.pumpAndSettle();
expect(tester.takeException(), null);
// Verify the child width is constrained correctly.
expect(findStatic(), findsOneWidget);
final Size fittedBoxSize = tester.getSize(findFittedBox());
// availableWidth = 350.0 (screen width) - 2 * 20.0 (padding) = 310.0
// availableWidthForChild = 310.0 - 250.0 (menu width) = 60.0
expect(fittedBoxSize.width, 60.0);
});There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this code from the bot would work, but you might be able to try getting the x an y coordinates of the four corners of the context menu and seeing if they are within the limits you expect and that might be more robust then just checking for exceptions.
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks good, I left two comments
|
|
||
| final double availableHeightForChild = | ||
| screenBounds.height - _ContextMenuRouteStaticState._kPadding; | ||
| final double availableWidth = screenBounds.width - _ContextMenuRouteStaticState._kPadding * 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like elsewhere in the file, _kPadding is used without doubling it, so is multiplying it by two necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the native context menu applies a vertical margin only to the bottom but not the top, but the horizontal margin is applied to the left and right sides.
The other uses of the horizontal _kPadding (that aren't doubled) refer to the padding between the context menu child and the menu sheet.
| testWidgets('CupertinoContextMenu respects available screen width - Portrait', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| const Size portraitScreenSize = Size(300.0, 350.0); | ||
| await binding.setSurfaceSize(portraitScreenSize); | ||
| addTearDown(() => binding.setSurfaceSize(null)); | ||
|
|
||
| final Widget child = getChild(); | ||
| await tester.pumpWidget( | ||
| MediaQuery( | ||
| data: const MediaQueryData(size: portraitScreenSize), | ||
| child: CupertinoApp( | ||
| home: Center( | ||
| child: CupertinoContextMenu( | ||
| actions: <Widget>[ | ||
| CupertinoContextMenuAction(child: const Text('Test'), onPressed: () {}), | ||
| ], | ||
| child: child, | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| expect(find.byWidget(child), findsOneWidget); | ||
| final Rect childRect = tester.getRect(find.byWidget(child)); | ||
|
|
||
| // Start a press on the child. | ||
| final TestGesture gesture = await tester.startGesture(childRect.center); | ||
| await tester.pump(); | ||
| await tester.pump(const Duration(milliseconds: 500)); | ||
| await gesture.up(); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(tester.takeException(), null); | ||
| }); | ||
|
|
||
| testWidgets('CupertinoContextMenu respects available screen width - Landscape', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| const Size landscapeScreenSize = Size(350.0, 300.0); | ||
| await binding.setSurfaceSize(landscapeScreenSize); | ||
| addTearDown(() => binding.setSurfaceSize(null)); | ||
|
|
||
| final Widget child = getChild(width: 500); | ||
| await tester.pumpWidget( | ||
| MediaQuery( | ||
| data: const MediaQueryData(size: landscapeScreenSize), | ||
| child: CupertinoApp( | ||
| home: Center( | ||
| child: CupertinoContextMenu( | ||
| actions: <Widget>[ | ||
| CupertinoContextMenuAction(child: const Text('Test'), onPressed: () {}), | ||
| ], | ||
| child: child, | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| expect(find.byWidget(child), findsOneWidget); | ||
| final Rect childRect = tester.getRect(find.byWidget(child)); | ||
|
|
||
| // Start a press on the child. | ||
| final TestGesture gesture = await tester.startGesture(childRect.center); | ||
| await tester.pump(); | ||
| await tester.pump(const Duration(milliseconds: 500)); | ||
| await gesture.up(); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(tester.takeException(), null); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this code from the bot would work, but you might be able to try getting the x an y coordinates of the four corners of the context menu and seeing if they are within the limits you expect and that might be more robust then just checking for exceptions.
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Roll Flutter from 8f94cb0d8f01 to 9ff2767f3cb6 (56 revisions) flutter/flutter@8f94cb0...9ff2767 2025-09-20 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 0_jKqLGnkILvQ5C8a... to CcCe3HpQtBYhTZscb... (flutter/flutter#175698) 2025-09-20 engine-flutter-autoroll@skia.org Roll Dart SDK from e6e9248aee4f to 9e943fe076c8 (1 revision) (flutter/flutter#175697) 2025-09-20 32538273+ValentinVignal@users.noreply.github.com Add `menuController` to `DropdownMenu` (flutter/flutter#175039) 2025-09-20 engine-flutter-autoroll@skia.org Roll Skia from 1dae085e2f31 to a38a531dec1d (3 revisions) (flutter/flutter#175694) 2025-09-20 bruno.leroux@gmail.com [a11y] TimePicker clock is unnecessarily announced (flutter/flutter#175570) 2025-09-20 engine-flutter-autoroll@skia.org Roll Dart SDK from 78e68d1a7dbf to e6e9248aee4f (4 revisions) (flutter/flutter#175690) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from b56003bf2c20 to 1dae085e2f31 (1 revision) (flutter/flutter#175674) 2025-09-19 matanlurey@users.noreply.github.com Update `CODEOWNERS` (for dev-tooling) (flutter/flutter#175201) 2025-09-19 sokolovskyi.konstantin@gmail.com [a11y-app] Add label to TextFormField in AutoCompleteUseCase. (flutter/flutter#175576) 2025-09-19 sokolovskyi.konstantin@gmail.com Fix RadioGroup single selection check. (flutter/flutter#175654) 2025-09-19 engine-flutter-autoroll@skia.org Roll Packages from f2a65fd to 3d5c419 (2 revisions) (flutter/flutter#175668) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from c74d2bdbd93c to b56003bf2c20 (2 revisions) (flutter/flutter#175665) 2025-09-19 32538273+ValentinVignal@users.noreply.github.com Add `CupertinoLinearActivityIndicator` (flutter/flutter#170108) 2025-09-19 bkonyi@google.com [ Widget Preview ] Don't update filtered preview set when selecting non-source files (flutter/flutter#175596) 2025-09-19 engine-flutter-autoroll@skia.org Roll Dart SDK from 2c79803c97db to 78e68d1a7dbf (3 revisions) (flutter/flutter#175646) 2025-09-19 mdebbar@google.com Delete unused web_unicode library (flutter/flutter#174896) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from 684f3a831216 to c74d2bdbd93c (2 revisions) (flutter/flutter#175644) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from 462bdece17bf to 684f3a831216 (3 revisions) (flutter/flutter#175641) 2025-09-19 engine-flutter-autoroll@skia.org Roll Skia from a2c38aa9df80 to 462bdece17bf (11 revisions) (flutter/flutter#175629) 2025-09-18 1961493+harryterkelsen@users.noreply.github.com fix(tool): Use merge-base for content hash in detached HEAD (flutter/flutter#175554) 2025-09-18 1961493+harryterkelsen@users.noreply.github.com [web] Unskip Cupertino datepicker golden tests in Skwasm (flutter/flutter#174666) 2025-09-18 ryjohn@google.com Update rules to include extension rules (flutter/flutter#175618) 2025-09-18 Breakthrough@users.noreply.github.com [engine] Cleanup Fuchsia FDIO library dependencies (flutter/flutter#174847) 2025-09-18 ahmedsameha1@gmail.com Make sure that a CloseButton doesn't crash in 0x0 environment (flutter/flutter#172902) 2025-09-18 bkonyi@google.com [ Tool ] Serve DevTools from DDS, remove ResidentDevToolsHandler (flutter/flutter#174580) 2025-09-18 Breakthrough@users.noreply.github.com [engine][fuchsia] Update to Fuchsia API level 28 and roll latest GN SDK (flutter/flutter#175425) 2025-09-18 jessiewong401@gmail.com Added a 36 device for Firebase Lab Testing (flutter/flutter#175613) 2025-09-18 engine-flutter-autoroll@skia.org Roll Dart SDK from 09a101793af4 to 2c79803c97db (2 revisions) (flutter/flutter#175608) 2025-09-18 louisehsu@google.com Engine Support for Dynamic View Resizing (flutter/flutter#173610) 2025-09-18 engine-flutter-autoroll@skia.org Roll Packages from fdee698 to f2a65fd (3 revisions) (flutter/flutter#175594) 2025-09-18 15619084+vashworth@users.noreply.github.com Connect the FlutterEngine to the FlutterSceneDelegate (flutter/flutter#174910) 2025-09-18 engine-flutter-autoroll@skia.org Roll Dart SDK from de5dd0f1530f to 09a101793af4 (2 revisions) (flutter/flutter#175583) 2025-09-18 engine-flutter-autoroll@skia.org Roll Skia from 7b9fe91446ee to a2c38aa9df80 (1 revision) (flutter/flutter#175579) 2025-09-18 engine-flutter-autoroll@skia.org Roll Skia from ab1b10547461 to 7b9fe91446ee (4 revisions) (flutter/flutter#175569) 2025-09-18 bruno.leroux@gmail.com [a11y-app] Fix form field label and error message (flutter/flutter#174831) 2025-09-18 bruno.leroux@gmail.com Fix InputDecoration does not apply errorStyle to error (flutter/flutter#174787) 2025-09-18 32538273+ValentinVignal@users.noreply.github.com Migrate to `WidgetPropertyResolver` (flutter/flutter#175397) 2025-09-18 32538273+ValentinVignal@users.noreply.github.com Migrate to WidgetState (flutter/flutter#175396) 2025-09-18 engine-flutter-autoroll@skia.org Roll Skia from 79ec8dfcd9d4 to ab1b10547461 (17 revisions) (flutter/flutter#175561) 2025-09-18 30870216+gaaclarke@users.noreply.github.com Removes NOTICES from licenses input (flutter/flutter#174967) 2025-09-18 engine-flutter-autoroll@skia.org Roll Dart SDK from 116f7fe72839 to de5dd0f1530f (2 revisions) (flutter/flutter#175557) 2025-09-17 1961493+harryterkelsen@users.noreply.github.com [reland][web] Refactor renderers to use the same frontend code #174588 (flutter/flutter#175392) 2025-09-17 erickzanardoo@gmail.com feat: Enable WidgetStateColor to be used in ChipThemeData.deleteIconColor (flutter/flutter#171646) 2025-09-17 15619084+vashworth@users.noreply.github.com Filter out unexpected process logs on iOS with better regex matching. (flutter/flutter#175452) 2025-09-17 victorsanniay@gmail.com CupertinoContextMenu child respects available screen width (flutter/flutter#175300) 2025-09-17 38427679+xVemu@users.noreply.github.com Correct documentation in PredictiveBackFullscreenPageTransitionsBuilder (flutter/flutter#174362) ...
…75300) Missed this in [Correct position for an open CupertinoContextMenu](flutter#170943) Fixes [CupertinoContextMenu wrong position](flutter#175225)
Missed this in Correct position for an open CupertinoContextMenu
Fixes CupertinoContextMenu wrong position