Repository navigation
Fix Cupertino sheet covered top gap transition - #187058
huahua8893 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Cupertino sheet transition in sheet.dart to include a black background behind the sheet during secondary transitions by utilizing a Stack and a FadeTransition. A new widget test has been added to verify that this opaque background is correctly rendered when multiple sheets are pushed. I have no feedback to provide.
victorsanni
left a comment
There was a problem hiding this comment.
Thanks for the PR. Can you add a video of native iOS in the same circumstances to the video description? and also what the previous (wrong) behavior this PR is fixing too? While the latter is in the issue it may be helpful to do a side-by-side comparison here also.
a1ed79d to
d02a69b
Compare
|
I've marked this PR as not ready to port to flutter/packages yet. |
Thanks for the update! Sounds good. I'll wait for the instructions and move this change over to material_ui/cupertino_ui once they are ready to receive PRs. |
d02a69b to
f8d3c0a
Compare
There was a problem hiding this comment.
Thanks for the investigation! The fix looks good to me, however I'm not a fan of this unit test. Checking pixels from a screenshot is unsupported on Web, less performative, and might even be flaky, and should only be used if a regular coordinate test isn't possible.
What do you think of a test like this?
testWidgets('covered sheet does not reveal the root route through its top gap', (
WidgetTester tester,
) async {
final GlobalKey scaffoldKey = GlobalKey();
await tester.pumpWidget(
CupertinoApp(
home: CupertinoPageScaffold(
key: scaffoldKey,
child: Column(
children: <Widget>[
const Text('Page 1'),
CupertinoButton(
onPressed: () {
Navigator.push<void>(
scaffoldKey.currentContext!,
CupertinoSheetRoute<void>(
builder: (BuildContext context) {
return CupertinoPageScaffold(
child: Column(
children: <Widget>[
const Text('Page 2'),
CupertinoButton(
onPressed: () {
Navigator.push<void>(
context,
CupertinoSheetRoute<void>(
builder: (BuildContext context) {
return const CupertinoPageScaffold(child: Text('Page 3'));
},
),
);
},
child: const Text('Push Page 3'),
),
],
),
);
},
),
);
},
child: const Text('Push Page 2'),
),
],
),
),
),
);
await tester.tap(find.text('Push Page 2'));
await tester.pumpAndSettle();
await tester.tap(find.text('Push Page 3'));
await tester.pumpAndSettle();
final double page1Top = tester
.getTopLeft(
find.ancestor(of: find.text('Page 1'), matching: find.byType(CupertinoPageScaffold)),
)
.dy;
final double page2Top = tester
.getTopLeft(
find.ancestor(of: find.text('Page 2'), matching: find.byType(CupertinoPageScaffold)),
)
.dy;
final double page3Top = tester
.getTopLeft(
find.ancestor(of: find.text('Page 3'), matching: find.byType(CupertinoPageScaffold)),
)
.dy;
// Sheet 2 moves up above Page 1 so that the root route (Page 1) is completely hidden behind Sheet 2.
expect(page2Top, lessThanOrEqualTo(page1Top));
// Sheet 3 is stacked on top of Sheet 2, with Sheet 2 peeking out above Sheet 3.
expect(page2Top, lessThanOrEqualTo(page3Top));
});f8d3c0a to
d82946c
Compare
|
This pull request contains changes to Material or Cupertino, which are currently frozen in this repository. Changes should be made in Please refer to #188444 for instructions. |
|
@dkwingsmt Sorry for the delayed. This approach is more robust and broadly applicable than the previous test. I have updated the regression test to use the suggested coordinate-based assertions and verified that it fails with the previous implementation and passes with the fix, confirming that it accurately captures the issue. I also learned a valuable testing approach from your suggestion. Thank you very much for the thoughtful feedback! PS:I have noted the recent migration to the new flutter/packages repository, and I will port this PR accordingly by following the provided instructions. |
|
@huahua8893 Yes, can you port the changes to the packages, request the same reviewers, and close this PR? Thank you! |
|
@dkwingsmt @victorsanni Thank you for the reviews and guidance. I have ported this change to flutter/packages#12530 and requested the same reviewers there. I’m closing this PR in favor of the migrated PR. |
…#12530) Ports flutter/flutter#187058 to `cupertino_ui` following flutter/flutter#188444. Fixes flutter/flutter#187057. When multiple `CupertinoSheetRoute`s are stacked, the covered sheet's top gap can reveal the root route because the top-gap padding sits outside the secondary route transition. This change applies the covered sheet's secondary transition outside the top-gap padding, so the sheet and its gap move together. It also adds a coordinate-based regression test that verifies the root route remains fully covered. ## Tests - `flutter test test/sheet_test.dart --no-pub` - `dart run script/tool/bin/flutter_plugin_tools.dart analyze --packages cupertino_ui` - `dart run script/tool/bin/flutter_plugin_tools.dart validate --packages cupertino_ui --base-sha=252bb33ad3666c7d28621c87edccafff86e210fd --check-for-missing-changes` - `dart run script/tool/bin/flutter_plugin_tools.dart publish-check --packages cupertino_ui` ## Pre-Review Checklist
…flutter#12530) Ports flutter/flutter#187058 to `cupertino_ui` following flutter/flutter#188444. Fixes flutter/flutter#187057. When multiple `CupertinoSheetRoute`s are stacked, the covered sheet's top gap can reveal the root route because the top-gap padding sits outside the secondary route transition. This change applies the covered sheet's secondary transition outside the top-gap padding, so the sheet and its gap move together. It also adds a coordinate-based regression test that verifies the root route remains fully covered. ## Tests - `flutter test test/sheet_test.dart --no-pub` - `dart run script/tool/bin/flutter_plugin_tools.dart analyze --packages cupertino_ui` - `dart run script/tool/bin/flutter_plugin_tools.dart validate --packages cupertino_ui --base-sha=252bb33ad3666c7d28621c87edccafff86e210fd --check-for-missing-changes` - `dart run script/tool/bin/flutter_plugin_tools.dart publish-check --packages cupertino_ui` ## Pre-Review Checklist
…flutter#12530) Ports flutter/flutter#187058 to `cupertino_ui` following flutter/flutter#188444. Fixes flutter/flutter#187057. When multiple `CupertinoSheetRoute`s are stacked, the covered sheet's top gap can reveal the root route because the top-gap padding sits outside the secondary route transition. This change applies the covered sheet's secondary transition outside the top-gap padding, so the sheet and its gap move together. It also adds a coordinate-based regression test that verifies the root route remains fully covered. ## Tests - `flutter test test/sheet_test.dart --no-pub` - `dart run script/tool/bin/flutter_plugin_tools.dart analyze --packages cupertino_ui` - `dart run script/tool/bin/flutter_plugin_tools.dart validate --packages cupertino_ui --base-sha=252bb33ad3666c7d28621c87edccafff86e210fd --check-for-missing-changes` - `dart run script/tool/bin/flutter_plugin_tools.dart publish-check --packages cupertino_ui` ## Pre-Review Checklist
Fixes #187057
This fixes a visual issue in stacked
CupertinoSheetRoutes where the top gap of a covered sheet can reveal a lower route.When a sheet is covered by another sheet, the covered sheet is translated and scaled by its secondary route animation. However, its top gap was outside that secondary transition. In a stack such as:
Root route -> Sheet 2 -> Sheet 3
the top gap could remain visually separate from the covered sheet's transition, allowing the root route background to show through at the top of the sheet stack.
This change applies the covered sheet's secondary transition outside its top gap padding, so the covered sheet and its top gap move and scale together.
A regression test was added to verify that a covered sheet applies its secondary transition outside the top gap.
Tests
flutter test packages/flutter/test/cupertino/sheet_test.dart dart analyze packages/flutter/lib/src/cupertino/sheet.dart packages/flutter/test/cupertino/sheet_test.dartPre-launch Checklist
///).Problem
When multiple
CupertinoSheetRoutes are stacked, the covered sheet's top gap can reveal a lower route.For example:
Root route -> Sheet 2 -> Sheet 3
If the root route has a distinct background color, that color can become visible above the covered sheet. This makes the sheet stack look incorrect because the root route should remain hidden behind the sheet stack.
Videos
Before this PR:
2026-05-25.22.32.33.mov
After this PR:
2026-05-25.22.28.59.mov