Reduce iOS scroll damping for lists with differently sized items #59623
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
f7e4e6f to
74924c1
Compare
There was a problem hiding this comment.
Thank you for fixing this! And sorry for the delay, it took me longer than expected to do the research. I think we might want to keep using the 0.135 drag coefficient, but the other changes look good!
FYI the definition of breaking changes has been updated just recently, changing tests located in the test folder alone is no longer considered breaking.
I have yet to find out what caused the difference in flinging velocity, will post my updates in #11772.
There was a problem hiding this comment.
Thank you for updating this!
There was a problem hiding this comment.
The 0.135 coefficient is actually accurate. You can verify this in a native UIScrollView (or UITableView) by adding the following code to its delegate and flinging the scroll view:
func scrollViewWillEndDragging(_ scrollView: UIScrollView, withVelocity velocity: CGPoint, targetContentOffset: UnsafeMutablePointer<CGPoint>) {
let actualTargetVerticalOffset = targetContentOffset.pointee.y
let computedVerticalOffset = scrollView.contentOffset.y - (velocity.y - 0.01)/(log(scrollView.decelerationRate.rawValue))
print("actual target vertical offset: \(actualTargetVerticalOffset), computed from velocity: \(computedVerticalOffset)")
}The computed value is going to be very close to the actual content offset (but not exactly the same, I guess there's rounding/flooring happening in the actual implementation, so the final value snaps to physical pixels).
The default scrollView.decelerationRate value is 0.998 (.normal), but the velocity is in a different unit here in scrollViewWillEndDragging (pixel per millisecond I guess), thus the 0.135 (0.998^1000).
There was a problem hiding this comment.
The velocity we use to create the ballistic activity is responsible for the most content offset error here.
|
(PR triage) @LongCatIsLooong @passsy What is the status of this PR? Should it be closed in favor of #60501? |
…items As pointed out in flutter#32448, the velocity shouldn't be decreased when creating the simulation. The current solution only works for lists where all items have the same size. Lists with different sized items recreate the simulation over and over again (because the maxScrollExtent slightly changes), reducing the velocity with each call by 9%. The velocity should not depend on how often the simulation gets created! Instead I tweaked the damping value of the FrictionSimulation to match the native behavior more closely. For testing, I updated the [scroll_overlay](flutter/platform_tests#2) test project to use different sized cells. The new damping isn't pixel-perfect. I'm not even sure it is possible at all. But at least lists with equally and unequally sized cells now behave the same. Related flutter#11772
0047a8f to
72bb989
Compare
|
@LongCatIsLooong I reverted the drag from
|
|
@goderbauer @passsy I didn't add tests in #60501 to cover the case where the viewport needs to apply a new content dimension. I was hoping that we can wait for both PRs to be reviewed and then merge them together. |
There was a problem hiding this comment.
LGTM. Thank you for fixing this long-standing issue (and updating the scroll_overlay app)! I'll try to merge this PR and #60501 together to prevent potential regression.
You may need to merge the latest upstream/master to the PR branch to fix the "docs-linux" CI failure.
Description
On iOS, lists with differently sized items have bigger damping than lists with equally sized items.
During a fling the
maxScrollExtentcontinuously changes (slightly) when the items aren't equally sized. This causes a new simulation to be created. TheFastBouncingScrollPhysicsused for iOS, decreases the velocity by 9% every time it is created.This problem wasn't tested in the existing scroll_overlay platform test. All items there have the same size causing the simulation to only be created once.
This PR removes the velocity change when creating the simulation and tweaks the friction of the
FrictionSimulationto match the native iOS behavior closely for both scenarios.The new damping isn't pixel-perfect (if that's even possible) but close. The focus of this PR is that lists with equally and unequally sized cells now behave the same on iOS.
Related Issues
Related #11772
Fixes #32448
Tests
For testing, I updated the scroll_overlay PR#2 test project to use different sized cells.
I tested with iPhone XR, iPhone SE and Pixel 4
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change