Make ClampingScrollSimulation ballistic and more like Android #120420
Conversation
Fixes flutter#120338. Fixes flutter#119875. Fixes flutter#113424. Fixes most of flutter#16371, as the combined symptom of those three. (There's still flutter#120345, and possibly other contributing factors, remaining.) This replaces the implementation of ClampingScrollSimulation with a new version that better matches the Android scroll physics it's intended to match. The new implementation is a version of the Android curve which has been adjusted so that it behaves in a physically reasonable way, in order to satisfy an invariant which the Flutter scroll protocol relies on. Because the old ClampingScrollSimulation didn't satisfy that invariant either, it suffered from a bug (flutter#120338) where restarting the simulation with new metrics would have the side effect of adding friction, making a fling go about 15% less far. So this fixes flutter#120338. The new implementation also goes the same total distance as the Android scroll physics, whereas the existing version would go about 7% less far (stacking with the 15% reduction when the other issue was triggered.) That fixes flutter#119875. Finally, the new implementation comes to a smooth stop, like Android does, whereas the existing version would speed slightly up before abruptly stopping. That fixes flutter#113424. The new implementation's curve is the unique curve that both goes the same total distance as Android's for every possible starting velocity, and meets the Flutter scroll protocol's ballistic invariant. So in that sense this is the canonical way of fixing up the Android behavior to something physically reasonable, or something that works correctly in Flutter.
A few tests encoded precise expectations for how far a fling-scroll would go, and need small updates.
flutter-dashboard
bot
added
a: accessibility
|
Here are before/after screen recordings showing the effect of this PR. The recipe is very similar to the repro recipes for #119875 and #120338:
The first pair of recordings are with The "before" version therefore shows the effects of #120338 as well as #119875, so it suffers much too much drag; but the restarts mask the effect of #113424. The "after" version matches Android much more closely, and ends at almost exactly the same place.
The second pair of recordings are with The "before" version therefore shows the effect of #113424, speeding up at the end. And it still displays #119875, suffering too much drag but not as much as when #120338 is also present. The "after" version again matches Android much more closely; the behavior is the same as in the version with restarts, to within the precision of my finger. There's no speeding up at the end.
|
|
This PR does not use Android's exact scrolling curve, as the previous PRs #77497 and #107735 did. As described in #120338, the Android curve unfortunately does not satisfy the Flutter scroll protocol's invariant of physical reasonableness. Using the Android curve without changes to the scroll protocol causes a loss of friction, #83632 — the effect is in the opposite direction from the existing curve's #119875, and is worse. Revising the protocol to remove that invariant would be possible, but complicated, and probably requires a breaking change: This PR fixes the major user-visible issues with the current implementation and makes it much more similar to Android's. When I try it out, either in So I think it would be reasonable to declare that remaining discrepancy a polish issue in Android, not in Flutter, and stick with this PR's "like Android, but fixed" physics indefinitely. Alternatively, even if we do intend to pursue changing the scroll protocol in order to use the Android curve, I think it would make sense to merge this implementation first, for two reasons:
|
| @@ -23,4 +25,98 @@ void main() { | |||
| checkInitialConditions(75.0, 614.2093); | |||
| checkInitialConditions(5469.0, 182.114534); | |||
| }); | |||
|
|
|||
| test('ClampingScrollSimulation only decelerates, never speeds up', () { | |||
There was a problem hiding this comment.
Can we add a test that the velocity eventually goes to zero?
There was a problem hiding this comment.
Sure, will do.
I think that follows as a consequence of being ballistic/restartable: if the simulation still has significant velocity in the last moments before the end (i.e., if the limit as time goes to the ending time isn't zero), then restarting it at that point would mean it goes on for longer than the few moments that were otherwise remaining.
But it would still be sensible to have a test for it directly.
|
What a wonderful set of write ups. I'm truly impressed and thank you! I don't own any of this infrastructure so I can't provide a proper review but I did leave a suggestion for another test. |
|
Thank you so much for all of this work! I have this and the other PR in scroll_overlay on my review queue, just wanted to acknowledge since it may take me a moment to review all of this. |
Also fix formatting nits in the tests.
|
Are these curves specific to flutter apps running on Android? I find that the scrolling curve on Linux also jumps a little towards the end. |
This scroll physics is the one Flutter uses by default on all platforms other than iOS and macOS. So you're seeing #113424, which is one of the issues this PR fixes. |
|
I've fired off some additional internal testing, will report back. |
|
So I've tested this manually against some of the regressions we saw in the past from trying to resolve this issue, so far all is well. 👍 |
|
Great! Thanks for all the testing. Hoping those internal tests all come back green. |
|
Still following up on tests, there is an infra issue with triggering and returning the results here |
|
So there are some test failures, but they look to be expected. For example, if a customer has a screenshot test that scrolled before taking the picture, it would look different now. I will need a day or two to check all of the tests and communicate with customers. |
|
Makes sense. Thanks for your help! |
|
Update: Still working with customers to ensure all of the changes we can see are expected for this PR. Almost there. 👍 |
|
Only 1 customer approval remains. Thanks for your patience. |
|
Customers are set. 👍 |
| /// | ||
| /// The default value causes the particle to travel the same total distance | ||
| /// as in the Android scroll physics. | ||
| // See mFlingFriction. |
There was a problem hiding this comment.
This should take the form of a See also and a breadcrumb [] like in other docs.
There was a problem hiding this comment.
Oh I see this is private, // v ///. You can disregard, unless this would be helpful as a public comment.
There was a problem hiding this comment.
Cool. Yeah, this is a reference to the corresponding Android source (like some other private comments in this class), so I think it's useful only for someone actually reading the implementation.
Fixes #120338.
Fixes #119875.
Fixes #113424.
Fixes most of #16371, as the combined symptom of those three. (There's still #120345, and possibly other contributing factors, remaining.)
This replaces the implementation of ClampingScrollSimulation with a new version that better matches the Android scroll physics it's intended to match.
The new implementation is a version of the Android curve which has been adjusted so that it behaves in a physically reasonable way, in order to satisfy an invariant which the Flutter scroll protocol relies on.
Because the old ClampingScrollSimulation didn't satisfy that invariant either, it suffered from a bug (#120338) where restarting the simulation with new metrics would have the side effect of adding friction, making a fling go about 15% less far. So this fixes #120338.
The new implementation also goes the same total distance as the Android scroll physics, whereas the existing version would go about 7% less far (stacking with the 15% reduction when the other issue was triggered.) That fixes #119875.
Finally, the new implementation comes to a smooth stop, like Android does, whereas the existing version would speed slightly up before abruptly stopping. That fixes #113424.
The new implementation's curve is the unique curve that both goes the same total distance as Android's for every possible starting velocity, and meets the Flutter scroll protocol's ballistic invariant. So in that sense this is the canonical way of fixing up the Android behavior to something physically reasonable, or something that works correctly in Flutter.
Cc: @Piinks @grouma @goderbauer @nt4f04uNd @MxSoul who've previously been interested in this problem.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.