-
Notifications
You must be signed in to change notification settings - Fork 29.7k
CupertinoSheetRoute with scrolling and dragging #177337
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
base: master
Are you sure you want to change the base?
Conversation
|
missing one minor bit of fidelity. if you are scrolled down and fling to the top it should spring at the top instead of slam to a halt |
|
The physics when scrolling to the top is definitely not there yet. It either stops suddenly or bounces when it shouldn't when the drag to dismiss happens. I opened this draft PR to get some feedback on the overall approach. |
| return Navigator.of(context, rootNavigator: true).push<T>(route); | ||
| } else { | ||
| widgetBuilder = (BuildContext context) { | ||
| Widget nestedNavigationContent(Widget child) { |
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.
Since this is changed to define an inline function, is the else necessary?
| context, | ||
| rootNavigator: true, | ||
| ).push<T>(CupertinoSheetRoute<T>(builder: widgetBuilder, enableDrag: enableDrag)); | ||
| final PageRoute<T> route = effectiveBuilder != 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.
Can you simplify the logic to de-dupe and call on your new wrapper method based on useNestedNavigation?
| } | ||
| } | ||
|
|
||
| class _NeverUserScrollableScrollPhysics extends ScrollPhysics { |
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.
Why is this needed over using NeverScrollableScrollPhysics?
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 didn't want to set allowImplicitScrolling to false.
7eae9f1 to
f165a18
Compare
| child: _CupertinoSheetScope(child: builder(context)), | ||
| child: Builder( | ||
| builder: (BuildContext context) { | ||
| return _CupertinoSheetScope(sheetContext: context, child: _effectiveBuilder(context)); |
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.
Why do we need to pass around context? That's an anti-pattern.
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.
Ultimately it's so that _CupertinoDragGestureDetector is able to figure out the height of the sheet for knowing how much to adjust the page transition in response to a user drag.
It gets there through CupertinoSheetDragArea looking up the _CupertinoSheetScope above it and passing along the context to _CupertinoDragGestureDetector.
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.
Yeah, it's really not something we should do. Passing around build context is not safe. DraggableScrollableSheet keeps track of the sheet's extent. Are we able to do that 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.
I think I can change it so that _CupertinoSheetScope tracks the height/extent rather than the context and that'll work pretty smoothly.
chunhtai
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.
For Accessibility, we would need to focus on two things
- dismissable
in talkback this is fine as user can do gesture down and left to issue a android back.
For voiceOver though, it is two fingers Z gesture and it calls Semantics(onDismiss). Looking at the semantics tree, I didnt see the semantics action is set.
- the draghandle
In talkback, tap should be able to dismiss the sheet similar to
| onSemanticsTap: widget.onClosing, |
For voicerOver, we may need to check the iOS behavior, do you know whether there is builtin app that uses the native draggable sheet?
| bool get enableDrag; | ||
|
|
||
| /// Determines whether the content can be scrolled. | ||
| bool get enableScroll; |
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.
can this be private?
|
also, while not yet wired up in the engine, can you also apply SemanticsRole.dragHandle to the handle so that we won't forget about this in the future when it is wired up |
You can find the native sheet in the Contacts app, when you add a new contact. You can also find another one in the Settings app through General -> Language & Region -> Add Language |
CupertinoSheet currently does not add a drag handle. For the native sheet widget, the drag handle is normally added when the sheet is resizable, which CupertinoSheet does not do by default, nor is this PR adding that. However, CupertinoSheetDragArea could be wrapped around a drag handle type widget manually added by a dev. So should we either
|
|
I tested on a device both the native behavior and this code. On native the two finger Z-scrub gesture does not dismiss the sheet, it seems. Double tap, then drag does dismiss it however. For CupertinoSheet, currently the Z-scrub gesture does dismiss it, the same with double tap and drag. Should we block the two finger Z gesture? |
@MitchellGoodwin have you checked the native behavior with accessibility controls enabled? Does SwiftUI expose the ability to add drag handles? |
@Piinks I have checked the native behavior with accessibility controls, using a physical device. For the drag handle on native, they do expose a way to add it through prefersGrabberVisibile. They refer to the drag handle as a "grabber". From the HIGs documentation on sheets, they say: "Include a grabber in a resizable sheet. A grabber shows people that they can drag the sheet to resize it; they can also tap it to cycle through the detents. In addition to providing a visual indicator of resizability, a grabber also works with VoiceOver so people can resize the sheet without seeing the screen." I filled an issue for the drag handle #179358. I think that can be done in a separate PR from this one. |
943dd41 to
b0d7ced
Compare
justinmc
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.
Just trying to think through an alternative approach... Rather than a new constructor for CupertinoSheetRoute that passes a ScrollController, could you use PrimaryScrollController.of inside of CupertinoSheetRoute and ask users to do the same and pass that to their relevant scroll view? I'm no expert in this area but it seems kind of aligned with the intention of PrimaryScrollController.
If not PrimaryScrollController, then would a new similar InheritedWidget make sense? ...Or would it just make this more complicated and less discoverable?
Otherwise, do you know how this kind of thing works in SwiftUI? Is it always the entire CupertinoSheet that is scrollable?
Assuming none of these drive-by ideas is viable, and that this covers the main use cases from SwiftUI, then this approach looks good to me!
| /// This is a convenience method for displaying [CupertinoSheetRoute] for common, | ||
| /// straightforward use cases. The Widget returned from `pageBuilder` will be | ||
| /// used to display the content on the [CupertinoSheetRoute]. | ||
| /// straightforward use cases. There are two different build methods that are provided | ||
| /// by the method for building the content of the [CupertinoSheetRoute]. Use `builder` | ||
| /// if there is no content within the sheet that needs to scroll. This is good | ||
| /// for sheets with a simple display, however scrolling gestures will conflict | ||
| /// with the drag to dismiss gesture. `scrollableBuilder` will enable scrollable | ||
| /// content within the sheet. See [CupertinoSheetRoute.scrollable] for more | ||
| /// information. |
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.
Nit: Should you mention that this is for vertical scrolling?
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.
Good callout
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return CupertinoSheetDragArea(child: child); |
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.
Could this be automatically built into CupertinoNavigationBar?
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's possible, and it would be convenient as this would theoretically be a common use case. I'd just worry about this behavior being discoverable.
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.
This does feel a bit kludgy from the developer experience side.
What native behavior are we trying to emulate by exposing this drag wrapper for non-scrolling parts? If I had a header that I wanted to have drag the sheet, why not put it in a pinned sliver in a CustomScrollView as part of the scrollable? Would that achieve the same effect without adding more API for the developer to have to wire up?
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.
What native behavior are we trying to emulate by exposing this drag wrapper for non-scrolling parts?
We want to be able to trigger the drag-to-dismiss behavior when a drag originates outside of the scroll area, and only the drag-to-dismiss, not a scroll. For example, if you start a drag on the header of a native sheet, it will not scroll the content below it, no matter what direction you drag. This is also the only way to trigger the stretch upwards drag effect on a sheet. If you start the drag from within the scrolling area then the draggable/scrollable behavior would happen.
If I had a header that I wanted to have drag the sheet, why not put it in a pinned sliver in a CustomScrollView as part of the scrollable? Would that achieve the same effect without adding more API for the developer to have to wire up?
I believe in that case then a scroll would be triggered on a drag upwards originating in the pinned sliver, instead of the stretch animation.
From native: a drag starting within the navbar will not scroll and will stretch upwards. A drag starting below the navbar will scroll depending on the direction, and will not scroll upwards.

Basically outside of the scrolling area we want the sheet to work as if it's a non-scrolling sheet. This wrapper lets a developer wrap the original vertical drag gesture recognizer only around those areas.
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.
Would it be easier for the developer if, in the case of a scrollable sheet, we provide a header parameter and we can manage the gesture resolution ourselves internally?
What is the native experience like for developers building the UI you described above?
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 believe in that case then a scroll would be triggered on a drag upwards originating in the pinned sliver, instead of the stretch animation.
Did you try it?
We have control over how the scrolling offset is applied here with the custom scroll position class in the scrollable sheet, could we divert it in this case?
@justinmc The only issue with that is that we would need to know that the sheet content is intended to be scrollable. So, we could provide the controller with
I don't think a new widget would be needed. As far the user of this API would be concerned, the scroll controller isn't unique in how they implement it. This would make this API further different from DraggableScrollableSheet, so there's a difference in consistency there.
You can just put a scroll view within the sheet and it works as expected. Scrollable areas in Swift know when they are the child of a resizable area and handle things accordingly. Similarly they are able to have priority over the drag gesture that might be on the rest of the sheet. The most common use case is that the majority of the sheet, except for the nav bar is scrollable. So the situation in the example. |
| /// This is a convenience method for displaying [CupertinoSheetRoute] for common, | ||
| /// straightforward use cases. The Widget returned from `pageBuilder` will be | ||
| /// used to display the content on the [CupertinoSheetRoute]. | ||
| /// straightforward use cases. There are two different build methods that are provided |
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 know this is not exactly part of this change, but lil nit:
| /// This is a convenience method for displaying [CupertinoSheetRoute] for common, | |
| /// straightforward use cases. The Widget returned from `pageBuilder` will be | |
| /// used to display the content on the [CupertinoSheetRoute]. | |
| /// straightforward use cases. There are two different build methods that are provided | |
| /// This is a convenience method for displaying [CupertinoSheetRoute] for | |
| /// most use cases. There are two different build methods that are provided |
Follow-up question - what are the less common or straightforward sue bases, and what should people use in those scenarios?
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.
Follow-up question - what are the less common or straightforward sue bases, and what should people use in those scenarios?
showCupertinoSheet adds some boilerplate around nested navigation, and making sure the route is pushed to the right Navigator, which should work for most use cases of both. But for nested navigation in particular there might be configurations that our boilerplate might not fit, so in those cases they would use CupertinoSheetRoute directly and set things up themselves. This includes add-to-app situations with just the sheet, restorable state, custom Navigators, etc.
| /// Creates a page route that displays an iOS styled sheet that is expected to | ||
| /// have scrollable content that spans the whole sheet. | ||
| /// | ||
| /// When this constructor is used, the content of the sheet will not watch for |
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.
Can you explain this a bit more? If the content is a scrollable, won't it actually be watching for those drag gestures?
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.
What this means is that unlike the non-scrolling version, a vertical drag gesture recognizer won't be placed over the whole sheet, and instead the vertical drags would be watched for by the scrolling content.
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.
Except for things not part of the scrollable?
| /// | ||
| /// When this constructor is used, the content of the sheet will not watch for | ||
| /// drag gestures to trigger the drag downwards to dismiss behavior. Instead, | ||
| /// if a vertical happens within a scrollable widget that uses the [ScrollController] |
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 think a word is missing here.
I would focus this paragraph on the expected behavior. Some of the implementation details are not super relevant to the user.
| /// This example shows how to show a Cupertino Sheet with scrollable content, | ||
| /// and a navbar that will trigger the drag only on a gesture started within |
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.
Breadcrumbs would help here, what is the navbar?
Can you talk more about this? does this mean for non scrollable cupertino sheet user can start dragging anywhere in the sheet to start popping gesture? This sounds a bit unexpected. Besides this I also + 1 to what justin suggested, Ideally if we can have a way to not passing controller from constructor, we don't need to change the API |
Yes, that it is what it means, and that's how the native sheet works. Screen.Recording.2025-12-09.at.3.41.55.PM.movYou can disable that with the |
| if (didPop) { | ||
| return; | ||
| } | ||
| Navigator.of(context, rootNavigator: true).pop(result); |
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 know this is not introduced by this pr, but what if there is multiple navigator above the nested navigator? I thought we may want to pop just one level above this nested navigator
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.
showCupertinoSheet always pushes to the root navigator, especially as it needs to slide the whole page down. So popping the sheet also targets the root navigator, as the last route in the stack should theoretically be the sheet you are meaning to pop, in the case of nested navigation.
If there is a situation where that does not work for their app, then we direct people to use CupertinoSheetRoute directly instead of showCupertinoSheet.
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.
Ah ok, makes sense
| WidgetBuilder? pageBuilder, | ||
| WidgetBuilder? builder, |
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.
Just curious since I've been learning a bit more about this area, what is the difference between pageBuilder and builder?
| _dragGestureController!.dragUpdate(details.primaryDelta!, _stretchDragController!.controller); | ||
| _dragGestureController!.dragUpdate( | ||
| // Divide by size of the sheet. | ||
| details.primaryDelta! / sheetHeight, |
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.
Do we need to check for sheetHeight == 0?
| static VelocityTracker _cupertinoVelocityBuilder(PointerEvent event) => | ||
| IOSScrollViewFlingVelocityTracker(event.kind); | ||
|
|
||
| double get sheetHeight => widget.sheetHeight ?? context.size!.height; |
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.
We should make sure the fallback is safe to access, has error message.
| return; | ||
| } | ||
| _dragGestureController!.dragEnd( | ||
| details.velocity.pixelsPerSecond.dy / context.size!.height, |
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.
What about this change requires that sheet height has to be passed all the way down here instead of continuing to use context.size!.height?
Piinks
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.
It would be best to update the design document about this change. In particular, some diagrams illustrating how gestures come in and are handled would be ideal. There are a lot of classes working together here, but it looks like there is some unfortunate leaking of information across boundaries we typically try to avoid. Updating the design document will help a lot with to see the whole picture here.
|
|
||
| @override | ||
| Widget buildContent(BuildContext context) { | ||
| final double sheetHeight = |
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.
There should probably be a call to debugCheckHasMediaQuery before accessing it.
| final double sheetHeight; | ||
|
|
||
| final CupertinoSheetRoute<dynamic> route; |
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.
Maybe something for the design doc to explain, if the CupertinoSheetRoute's role is to see if another sheet is in the tree already, why does it need the sheetHeight and the route itself?
| } | ||
| } | ||
|
|
||
| /// A scroll position that manages scroll activities for |
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 docs looks like leftover copy paste from the widgets class.
These should be updated to reflect the right context. To avoid confusion, it might be best to give this class a unique name.
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.
Also something for the doc - since this was copied from the widgets layer and then changed to suit the use case, the document should talk about what is different and why.
Fixes #161687
Enables scrollable content within the sheet to work together with the dragging animation.
Flutter:
Screen.Recording.2025-11-25.at.2.14.02.PM.mov
Native:
Screen.Recording.2025-10-16.at.1.42.10.PM.mov
Fling when scrolling to the top vs fling when the scrollable content is already at the top
Screen.Recording.2025-11-25.at.2.16.13.PM.mov
When scrolling is enabled, then the sheet will no longer have a drag gesture recognizer over the sheet content, and will instead rely on the scrollable content to trigger the drag. A non-scrolling area can be wrapped with
CupertinoSheetDragAreato put a drag gesture recognizer only on that area, convenient for navbars. See cupertino_sheet.3.dart for a full example.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.