Repository navigation
Conversation
…ncingScrollPhysics
|
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. |
github-actions
Bot
added
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated "rubber band" spring simulation for overscroll recovery in BouncingScrollPhysics to better match iOS native behavior. A new rubberBandSpring is defined, and createBallisticSimulation is updated to use it when the scroll position is out of range and stationary. The logic is sound and well-documented. I have one suggestion to improve code readability. Additionally, while this change is primarily about physics tuning, which can be difficult to test, consider adding a unit test to verify that BouncingScrollPhysics.rubberBandSpring is correctly used when the scroll position is out of range and stationary. This would help ensure the new logic is covered.
There was a problem hiding this comment.
Fantastic change overall! Thank you so much for the investigation, and especially for the figure for demonstration.
I have commented a question.
Regarding tests, do you think the following test is possible?
- Build a scrollable with this physics, overscroll it, release. For every 0.2s elapsed, check the y position of the content.
Thanks for pointing this out, I had previously verified this behavior directly in a native iOS app. There is a clear difference between releasing an overscrolled view from rest and releasing it with residual velocity. With velocity, it behaves like an inertial bounce. From rest, it transitions into a simple exponential snap-back. So the velocity check is used to distinguish between these two physical states, matching the observed iOS behavior. |
@dkwingsmt I’ve added a unit test that verifies the spring selection in createBallisticSimulation. Could you please take a look and let me know if you still think a widget-level test is needed? |
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
LGTM. Thank you for the great work!
|
Assigning @Piinks as a second reviewer, since she reviewed the PR that improved Android overscroll physics. |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Hey @MTtankkeo thanks for contributing this! It looks like it is causing some failures, can you take a look?
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Thanks for your contribution! Since I haven't seen a reply, I'm going to reluctantly close this pull request for now, but if you get time to address the comments, we'd be more than happy to take a look at a new patch. If you do send a new patch, it would be very useful to reference this one in the description in order to help reviewers see the previous context. (Also, in light of the Material/Cupertino pause, merging PRs will be paused until the completion of decoupling. Although, we can still continue the review process and reland the PR to the new package after decoupling. So feel free to reopen this PR if you happen to have time. Thank you!) |
…crollPhysics (flutter#187568) > [!NOTE] > **Relanding** flutter#182426. > > Thank you for your review and guidance on the previous PR. > > I'm relanding this with the fix for the test failures that caused the issue before. *(I've updated the regression test to account for the faster convergence rate of the new physics.)* Fixes flutter#181752 *(for detailed analysis, measurements, and discussion, please refer to the original issue.)* This pull request implements a dedicated “rubber band” physics for overscroll snap-back in BouncingScrollPhysics on iOS to better match the native UIScrollView feel. <img width="1000" alt="546867255-4ad56291-9603-40c1-ae5a-3e319017c3c9" src="https://github.com/user-attachments/assets/8e72ebff-fb8f-4613-b042-7668db15bcf5" /> - - - > [!NOTE] > This change adjusts spring constants and simulation behavior > to improve iOS scroll fidelity. > > Given that this change primarily involves physics parameter tuning, > it may reasonably qualify as test-exempt. However, I am not entirely > certain what level of automated testing would be considered appropriate. > > Please let me know if additional tests are expected, > and I am happy to update the PR accordingly. ## 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. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Tong Mu <[email protected]> Co-authored-by: Victor Sanni <[email protected]> Co-authored-by: Elliott Brooks <[email protected]>
4 participants
Fixes #181752
(for detailed analysis, measurements, and discussion, please refer to the original issue.)
This pull request implements a dedicated “rubber band” physics for overscroll snap-back in BouncingScrollPhysics on iOS to better match the native UIScrollView feel.
Note
This change adjusts spring constants and simulation behavior
to improve iOS scroll fidelity.
Given that this change primarily involves physics parameter tuning,
it may reasonably qualify as test-exempt. However, I am not entirely
certain what level of automated testing would be considered appropriate.
Please let me know if additional tests are expected,
and I am happy to update the PR accordingly.
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.