Feat: Add top gap for cupertino sheet #171348
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
I'm not sure the top gap should be set as a pixel value. Instead it might be better as a ratio of the screen height, from 0 to 1.0. So 0.5 would be half screen. That would be safer with looking correct across different screen sizes. |
3154d0a to
cd81ee1
Compare
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Thank you for putting this together. Overall I like the approach, but this causes a problem with stacking multiple sheets, that I mention below. I'm not sure how best to approach that.
In the future, we'll likely want to add detents to the sheet: #169832. I was wondering if we should not add a topGap value and leave that to be handled by the more complicated detents, but I think it would both be good to have a simple value to set, and this could function as an initial sheet size to open on when using detents. So I think we should continue with this for now.
| final bool enableDrag; | ||
|
|
||
| @override | ||
| final double? topGap; |
There was a problem hiding this comment.
I think we could make this a little friendlier. We can add a private _topGap value. The constructor sets _topGap to whatever user defined property, or leaves it null. Then this override only sets a getter topGap that returns _topGap ?? _kTopGapRatio. Then we can both just use topGap everywhere and if another bit of code looks at topGap than it will be a reliable source of truth.
Also, I think we don't want to expose a setter for now. In the future we will probably want to enable programmatically changing the top gap while the sheet is open and having it animate the difference, and leaving no open setter for now makes that easier.
There was a problem hiding this comment.
Pushed the change.
7dac230 to
92dec8d
Compare
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Looks like the test is still failing.
But it looks like in SwiftUI the stacked view only happens with the default sheet height, or .large for them. All others have the previous page shown normally.
I think that simplifies what we need to do. If a user provides a topGap, then we don't want the previous route to transition at all, even if it matches our default. So I think we can do this:
- Create a private double? _topGap. The constructor sets _topGap to the user provided topGap, if provided. This lets us track if it's left as null.
- Add a topGap getter for providing programmatically which topGap to use.
double get topGap => _topGap ?? _kTopGapRatio- Add a
canTransitionFromoverride and have both it andcanTransitionToreturn false if _topGap != null
I think if we do all that, we can revert delegatedTransition to what it was before and not worry about it. It won't be used at all if exit transitions are blocked from the code above.
b92aa43 to
91fe520
Compare
|
@MitchellGoodwin Reverted the delegatedTransition Change and updated canTransition methods with topGap, but i think test are still failing, I tested it on device, looks fine but might have missed any edge case. |
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Thank you for the update, I left some comments.
| Animation<double> secondaryAnimation, | ||
| bool allowSnapshotting, | ||
| Widget? child, | ||
| ) => CupertinoSheetTransition.delegateTransition( |
There was a problem hiding this comment.
I think this can be simplified to
_topGap != null ? null : CupertinoSheetTransition.delegateTransition;| if (this is CupertinoSheetRoute<dynamic>) { | ||
| return (this as CupertinoSheetRoute<dynamic>)._topGap == null; | ||
| } |
There was a problem hiding this comment.
Can we get this logic without the type casting?
| Widget? child, | ||
| ) { | ||
| Widget? child, { | ||
| double? topGap, |
There was a problem hiding this comment.
Since we won't use this transition if there is a topGap, then we can remove this and not handle it.
91fe520 to
6797926
Compare
|
Are there any updates on this? Could we get this merged at this point? |
|
@erkinovalim We still need review. |
aa3f128 to
a4f1b27
Compare
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Sorry it took a while to get back to this. Approach looks good, I have some nitpick comments on the canTransitionTo and canTransitionFrom methods. Main thing left is adding a test.
a4f1b27 to
40a4bc9
Compare
|
@MitchellGoodwin could you review this please? |
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Thank you for the updates. It looks like the top gap value isn't actually being used for the sheet itself. The topGap needs to be applied to the _stretchDragAnimation in order to be used. However, I'm not sure how stretching should work with a custom top gap. Either the value for the max stretch needs to adjust if a custom top gap is given, or stretching needs to be disabled with a custom top gap.
| bool get enableDrag; | ||
|
|
||
| /// The gap between the top of the screen and the top of the sheet as a ratio | ||
| /// of the screen height (0.0 to 1.0). Defaults to [_kTopGapRatio]. |
There was a problem hiding this comment.
Private variables won't be able to be seen by documentation.
| /// of the screen height (0.0 to 1.0). Defaults to [_kTopGapRatio]. | |
| /// of the screen height (0.0 to 1.0). Defaults to a value of 0.08. |
There was a problem hiding this comment.
Sure, will update.
| double get topGap => _topGap ?? _kTopGapRatio; | ||
|
|
||
| @override | ||
| bool get hasCustomTopGap => _topGap != null; |
There was a problem hiding this comment.
This maybe should be a private method. I'm not sure if it's useful outside of these classes.
There was a problem hiding this comment.
This is used inside mixin. Here, we are just providing value. So, updated to private.
2c027e4 to
5d263d7
Compare
|
@MitchellGoodwin could you review this please? |
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Apologies for the late review. Tested this locally and it lgtm. Left two comments, but other than that, I think this is good to go.
| if (this is CupertinoSheetRoute<dynamic> && _hasCustomTopGap) { | ||
| return false; | ||
| } | ||
| return nextRoute is _CupertinoSheetRouteTransitionMixin && !_hasCustomTopGap; |
There was a problem hiding this comment.
This check for !_hasCustomTopGap is redundant, because it already happens above.
5d263d7 to
c8f49ae
Compare
| this.enableDrag = true, | ||
| double? topGap, | ||
| }) : assert( | ||
| topGap == null || (topGap >= 0.0 && topGap <= 1.0), |
There was a problem hiding this comment.
I wonder if we should limit the topGap so that it is not able to be 1.0, or the gap is the whole screen height. That would mean that none of the sheet is visible. I don't know why somebody would do that, but if they did it would mean that the top most route is active, but not on the screen at all, which would be hacky and also seems like it would mess with accessibility. Maybe it should be a max of 0.9, or 0.95?
If they set the topgap to 0.0 though, so that the sheet is fullscreen, I think that's perfectly ok.
There was a problem hiding this comment.
Yaa, we can do 0.9 because even 10% will be too small.
There was a problem hiding this comment.
What is the behavior like on swiftui?
Manual roll Flutter from d81baab to 57c3f8b (38 revisions) Manual roll requested by stuartmorgan@google.com flutter/flutter@d81baab...57c3f8b 2025-12-17 bruno.leroux@gmail.com Add FloatingActionButtonTheme (flutter/flutter#179736) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from b1569739f431 to fb576bd6827a (1 revision) (flutter/flutter#179989) 2025-12-17 bruno.leroux@gmail.com Update more comments related to theme normalization (flutter/flutter#179682) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from cce9b86bda7d to b1569739f431 (1 revision) (flutter/flutter#179979) 2025-12-17 jobguldemeester@gmail.com Adds property passthrough for CheckboxListTile, SwitchListTile and RadioListTile (flutter/flutter#178098) 2025-12-17 engine-flutter-autoroll@skia.org Roll Dart SDK from ca949b915846 to 3793f3d2d0c4 (2 revisions) (flutter/flutter#179973) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from 318400199beb to cce9b86bda7d (1 revision) (flutter/flutter#179976) 2025-12-17 ahmedsameha1@gmail.com Make sure that a CupertinoTextFormFieldRow doesn't crash in 0x0 envir… (flutter/flutter#179932) 2025-12-17 ahmedsameha1@gmail.com Make sure that a CupertinoTabView doesn't crash in 0x0 environment (flutter/flutter#179845) 2025-12-17 ahmedsameha1@gmail.com Make sure that a CupertinoTextField doesn't crash in 0x0 environment (flutter/flutter#179865) 2025-12-17 ahmedsameha1@gmail.com Make sure that a CupertinoSwitch doesn't crash in 0x0 environment (flutter/flutter#179748) 2025-12-17 116356835+AbdeMohlbi@users.noreply.github.com Update `BuildContext` docs to make it easier to understand (flutter/flutter#178616) 2025-12-17 41930132+hellohuanlin@users.noreply.github.com [ios][pv] quick fix to enable and re-enable web view's gesture recognizer (flutter/flutter#179908) 2025-12-17 biggs0125@gmail.com Deduplicate wasm dry run entries in analytics. (flutter/flutter#179970) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from 99899cbb415b to 318400199beb (1 revision) (flutter/flutter#179969) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from 2ac4a8709bc9 to 99899cbb415b (1 revision) (flutter/flutter#179968) 2025-12-17 engine-flutter-autoroll@skia.org Roll Dart SDK from 95a92bc705d3 to ca949b915846 (6 revisions) (flutter/flutter#179967) 2025-12-17 biggs0125@gmail.com Add package info to wasm dry run events. (flutter/flutter#179826) 2025-12-17 matt.boetger@gmail.com Platform View Hide/Show Integration test (flutter/flutter#179902) 2025-12-16 engine-flutter-autoroll@skia.org Roll Skia from 61162d72343f to 2ac4a8709bc9 (14 revisions) (flutter/flutter#179961) 2025-12-16 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 433KtmJvbMyaDMJvD... to fAoyBAT99XxwPE5hL... (flutter/flutter#179960) 2025-12-16 bkonyi@google.com [ Tool ] Fix update-packages not accounting for path dependencies (flutter/flutter#179951) 2025-12-16 45459898+RamonFarizel@users.noreply.github.com ListTile fix MinIntrinsicHeight calculation (flutter/flutter#179515) 2025-12-16 108678139+manu-sncf@users.noreply.github.com Fix pinned header in NestedScrollView (flutter/flutter#179210) 2025-12-16 bruno.leroux@gmail.com Update some comments related to theme normalization (flutter/flutter#179624) 2025-12-16 iliyazelenkog@gmail.com Add Cyrillic keyboard layout support for flutter_tools terminal commands (flutter/flutter#177855) 2025-12-16 lauren@selfisekai.rocks Minor fixes for libstdc++ 15 (flutter/flutter#178601) 2025-12-16 34465683+rkishan516@users.noreply.github.com Feat: Add top gap for cupertino sheet (flutter/flutter#171348) 2025-12-16 116356835+AbdeMohlbi@users.noreply.github.com Align `Build.API_LEVELS` usage in `FlutterImageDecoder.java` with existing usage (flutter/flutter#179868) 2025-12-16 41930132+hellohuanlin@users.noreply.github.com Revert "[ios][pv] accept/reject gesture based on hitTest (with new wi… (flutter/flutter#179895) 2025-12-16 codefu@google.com fix: line endings for flutter/dart/flutter-dev (flutter/flutter#179912) 2025-12-16 22373191+Hari-07@users.noreply.github.com Platform view blur clipping - Rounded Rect (iOS) (flutter/flutter#177551) 2025-12-16 engine-flutter-autoroll@skia.org Roll Dart SDK from 20d114f951db to 95a92bc705d3 (1 revision) (flutter/flutter#179909) 2025-12-16 34871572+gmackall@users.noreply.github.com [Reland] Unmodified android sdk bundle (flutter/flutter#179920) 2025-12-16 engine-flutter-autoroll@skia.org Roll Skia from 6903a4e65c3f to 61162d72343f (2 revisions) (flutter/flutter#179915) 2025-12-16 engine-flutter-autoroll@skia.org Roll Packages from 2cd921c to 57725eb (1 revision) (flutter/flutter#179942) 2025-12-16 45490440+shindonghwi@users.noreply.github.com Filter out FrameEvents/updateAcquireFence log spam from adb logcat (flutter/flutter#179884) 2025-12-16 30870216+gaaclarke@users.noreply.github.com `flutter update-packages --force-upgrade --update-hashes` (flutter/flutter#179950) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. ...
Feat: Add top gap for cupertino sheet
fixes: #169465
Pre-launch Checklist
///).