flutter icon indicating copy to clipboard operation
flutter copied to clipboard

Add snapAnimationCurve to DraggableScrollableSheet

Open Kal-Elx opened this issue 8 months ago • 10 comments

This pull request adds an optional parameter snapAnimationCurve to DraggableScrollableSheet which controls the curve of the snapping animation when the sheet is released after drag.

This fixes https://github.com/flutter/flutter/issues/121996 where the current snap animation of DraggableScrollableSheet is described as linear/mechanic.

The reason why it feels mechanic is because the animation is calculated position + velocity * time. This pull request changes this calculation to instead calculate the position as position + displacement * snapAnimationCurve!.transform(progress).

Here's an example of the result with snapAnimationCurve: Curves.easeOutQuad: https://github.com/user-attachments/assets/b54a5821-f2cf-4280-b30d-8138082da3c8

And another with

snapAnimationCurve: Curves.bounceOut,
snapAnimationDuration: const Duration(milliseconds: 1200),

https://github.com/user-attachments/assets/363837c6-0de8-493e-83df-768064a74be0

Pre-launch Checklist

  • [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • [X] I signed the [CLA].
  • [X] I listed at least one issue that this PR fixes in the description above.
  • [X] I updated/added relevant documentation (doc comments with ///).
  • [X] I added new tests to check the change I am making, or this PR is [test-exempt].
  • [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • [X] All existing and new tests are passing.

Discussion

The provided solution does not fully support curves that can take values > 1 or < 0, e.g. Curves.easeOutBack. Using one of these curves could try to move the sheet outside of its minChildSize and maxChildSize which the current implementation does not support so the sheet would just halt at its min and max positions. I think this should be addressed but I would argue that it's outside of the scope of this PR since it's also an existing problem when using animateTo on DraggableScrollableController and any solution in this area would likely solve both problems.

Kal-Elx avatar Mar 28 '25 14:03 Kal-Elx

Thanks for taking the time to look at this @justinmc!

I added a test that verifies that the sheet follows the given snapAnimationCurve during snapping.

Let me know if I can do anything else.

Kal-Elx avatar Apr 16 '25 10:04 Kal-Elx

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #166134 at sha ed40580188170ef8a431ef4cce02d9df08155e1e

flutter-dashboard[bot] avatar Apr 16 '25 10:04 flutter-dashboard[bot]

Thanks for the review @justinmc! I've fixed the comment. Let me know if I should do something more to get this merged.

Kal-Elx avatar Apr 20 '25 11:04 Kal-Elx

Let's wait for a secondary review from @gnprice who I think has been OOO.

justinmc avatar Apr 22 '25 22:04 justinmc

Yeah, I still feel like adding this API doesn't solve most of the problem — a much bigger improvement would be if we change the default from Curves.linear to whatever is our best estimate of what the curve should be. I think everyone's agreed that Curves.linear isn't the best default.

@Kal-Elx What curve are you planning to use for this argument in your app or apps? How did you choose that curve? Potentially that curve should be the default.

gnprice avatar Apr 30 '25 06:04 gnprice

The Material spec doesn't seem to say what the curve should be. I only see this predictive back gif, which happens to show the snap animation too. It's definitely not linear. https://m3.material.io/components/bottom-sheets/guidelines

Cool, good find. (Down at the bottom of that page.)

One thing I notice there is that it seems like there are two different curves involved:

  • When the sheet is entering, it uses a curve that starts quickly and ends slowly (something like Curves.easeOut).
  • When the sheet is exiting, it uses a curve that starts slowly and ends quickly (something like Curves.easeIn).

Would both of those be controlled by the curve parameter here? (I'm not sure if they would — I don't have my head around all the interactions involved in DraggableScrollableSheet.) If so, then it seems like the API in this PR wouldn't yet be enough to enable an app to match the Material behavior.

gnprice avatar Apr 30 '25 06:04 gnprice

I just went and experimented with Google Maps on an Android phone, since that's an example of an app that uses a bottom sheet with several snap sizes and that generally seems to have a good Material implementation.

Dragging and flinging it between the snap sizes, it's definitely using some sort of ease-out curve (one that starts quickly and ends slowly).

I'm also realizing now that the gif we discussed above may not be quite on point: it shows the sheet entering, and exiting, but not moving between two snap sizes where both the start and end size have it still existing. I think the latter interaction is what this code is most about, and I'm not sure this code covers either the entrance or exit animations.

gnprice avatar Apr 30 '25 06:04 gnprice

Here's a screen recording of the Google Maps behavior: https://github.com/user-attachments/assets/eb2d854f-2734-4cd9-af6e-e6250170d23f

gnprice avatar Apr 30 '25 06:04 gnprice

Dragging and flinging it between the snap sizes, it's definitely using some sort of ease-out curve (one that starts quickly and ends slowly).

More precisely, as I look more closely: if you drag the sheet to an intermediate point but then let go without flinging (i.e. with your finger not moving, or at least not moving quickly), then it (a) starts from a standstill, just like it was when you let go, but (b) accelerates quickly early on, and then (c) slows down more gradually to a stop. Roughly like Curves.ease.

There's an example of this around 00:13 in my screen recording above.

I think the behavior here can't be captured by a single curve (or a single Curve, anyway), because of the way it depends on the initial velocity of the fling:

  • If you let go from a standstill, then it first accelerates and then decelerates, as I described just now.
  • If you let go while moving quickly, then it starts at that rapid speed and decelerates from there.

The Simulation concept is well suited for this sort of thing, though.

gnprice avatar Apr 30 '25 07:04 gnprice

Hey @gnprice — sorry for the late response and thanks for taking the time to compare snapping behaviors!

In the app I’m working on right now we ended up using Curves.easeOut and it's a major improvement.

You’re absolutely right that inserting a curve means the simulation’s actual initial velocity no longer equals the user’s fling velocity but that mismatch already exists in the current api if you set snapAnimationDuration since then the sheet animates for a fixed time regardless of how hard the user flings. In my opinion allowing a curve doesn’t worsen the status quo; it just lets us make the motion feel better within the same constraint.

I definitely agree this isn’t the ideal long-term solution, and I’d love to see an implementation that truly matches the Material behavior when there's time to implement it. In the meantime, merging this feels like a practical win for developers without introducing new risk.

Of course the final call is yours and the framework team’s. Thanks again for the thoughtful review! 🙏

Kal-Elx avatar May 23 '25 12:05 Kal-Elx

Following up on where we left off here, @gnprice did you see @Kal-Elx's response to your feedback?

Piinks avatar Jun 18 '25 18:06 Piinks

FYI @gnprice

Piinks avatar Aug 18 '25 22:08 Piinks

Touched base with @justinmc about this to review @gnprice's feedback. This does not appear to be the right change to make here. Ideally, instead of a Curve, a Simulation would be more accurate to address the issue. Is this a change you would like to make to this PR @Kal-Elx?

Piinks avatar Sep 26 '25 22:09 Piinks

Thanks for coming back to this PR. I agree that a simulation would be the ideal solution. Unfortunately I won’t have time in the near future to implement that. Feel free to continue as you see fit (including closing this PR).

Kal-Elx avatar Sep 30 '25 14:09 Kal-Elx

Ok, thanks @Kal-Elx for letting us know. I will close this then.

Piinks avatar Sep 30 '25 20:09 Piinks