Send ScrollMetricsNotification when SingleChildScrollView is scrolled. #179133
Conversation
|
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 modifies the notifyListeners method in ScrollPosition to update the _lastMetrics field with the current scroll pixels value. While this correctly addresses the issue of stale pixel data, I have a concern regarding the performance impact of creating a new ScrollMetrics object on every frame during scroll animations. I've provided a comment suggesting a more performant alternative.
| _lastMetrics = _lastMetrics?.copyWith( | ||
| pixels: pixels, | ||
| ); |
There was a problem hiding this comment.
While this change correctly updates the pixels value, it does so on every frame during a scroll animation by creating a new FixedScrollMetrics object. This could have performance implications in complex scrolling scenarios.
A more performant approach would be to update the metrics only when they are about to be dispatched. For example, if _lastMetrics is used in didEndScroll, the copyWith call could be moved there. This would avoid object creation on every frame of a scroll.
Consider removing this change and applying the fix closer to where _lastMetrics is consumed, for instance:
// In didEndScroll()
if (_lastMetrics != null) {
final ScrollMetrics endMetrics = _lastMetrics!.copyWith(pixels: pixels);
ScrollMetricsNotification(metrics: endMetrics).dispatch(context.notificationContext);
}This would achieve the same goal with less overhead during active scrolling.
yiiim
force-pushed
the
fixes/scroll_position
branch
3 times, most recently
from
67b7bc8 to
3e85619
Compare
yiiim
changed the title
| expect(events[2] is ScrollUpdateNotification, true); | ||
| expect(events[3] is ScrollEndNotification, true); | ||
| expect(events[4] is UserScrollNotification, true); | ||
| expect(events[3] is ScrollMetricsNotification, true); |
There was a problem hiding this comment.
I just catch this now, but if only pixel (scroll position) is changing shouldn't this be ScrollUpdateNotification ? in that case line 51 has already included that, do we need to fire another ScrollMetricsNotification?
There was a problem hiding this comment.
This is the tricky part. For a CustomScrollView, every scroll triggers a relayout, so it dispatches a ScrollMetricsNotification, but this is not the case for a SingleChildScrollView. My idea is to make them consistent. I think the most ideal way is to not dispatch ScrollMetricsNotification on scroll, but this would be a breaking change for CustomScrollView. Alternatively, we could maintain the status quo and use the reopened #102339 to update _lastMetrics. @Piinks, do you remember why this PR was closed at the time?
The solution in this PR is to have SingleChildScrollView also dispatch a ScrollMetricsNotification on every scroll. As the PR description says, this also breaks some tests. I'd like to know which solution you think we should adopt? @chunhtai @Piinks.
Is this always true? Or only when the metrics have changed? |
Why do we need to unify the behavior? What I am wondering here considering this change is, what is the problem this is trying to solve? Looking at the older linked issues, these are not meant to behave the same way. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. 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. |
yiiim
force-pushed
the
fixes/scroll_position
branch
from
3e85619 to
6a37e6a
Compare
When a CustomScrollView scrolls, it inevitably triggers a layout pass, which calls applyContentDimensions. In applyContentDimensions, as shown here: , since pixels has already changed, _isMetricsChanged() is always true. Therefore, scrolling a CustomScrollView will always trigger a ScrollMetricsNotification. |
The issue we need to address is updating _lastMetrics for SingleChildScrollView, which is a prerequisite for resolving #173857. I considered keeping the current inconsistent behavior. However, if we update _lastMetrics, it will inevitably trigger a ScrollMetricsNotification. |
yiiim
force-pushed
the
fixes/scroll_position
branch
from
a9b771e to
286ef4f
Compare
yiiim
force-pushed
the
fixes/scroll_position
branch
from
373f708 to
9568a62
Compare
|
Reading the API doc of ScrollMetricsNotification https://api.flutter.dev/flutter/widgets/ScrollMetricsNotification-class.html
kind of implying user scrolling shouldn't send a ScrollMetricsNotification
Implies ScrollMetricsNotification is related to content size changes. It seems to me the CustomScrollView's behavior is incorrect unless the scrolling causes content size changes. Maybe what we want to do is that we should make sure it is not only scroll position change when the metrics changes before sending the notification |
Understood. I previously tried preventing CustomScrollView from dispatching ScrollMetricsNotification while scrolling, but that caused many issues and broke a lot of tests (e.g., #87076). I’ll likely need to address those failures incrementally before proceeding. |
|
Hmm, I think maybe the behavior drifted and now correcting it one way or the other is proving difficult due to how breaking it is. @yiiim you have done a lot of work here and in the other PRs related to this - which is super appreciated by the way! WDYT about writing down your findings from all of this in the doc template so you and @chunhtai and I can figure out the best course of action here? That way we have all the info in one place and can discuss more easily what the right solution is. I think this will also be more respectful of your time 💙 |
|
Agree with @Piinks , discuss this in a doc seems better so that you don't have to go back and forth with the implementation until we figure out a way forward. Also appreciate all the work you have been put into this so far! |
Thank you so much for your comment! I think this is great. Due to language barriers, I do find the documentation a bit challenging, but I believe I will be able to complete it.
I’ve always enjoyed the time and effort I’ve put into Flutter, and I really appreciate working together with all of you. |
3 participants
Background: #173857.
ScrollMetricsNotificationwas originally not intended to be triggered during scrolling, butCustomScrollViewtriggers a layout during scrolling, which also triggersScrollMetricsNotification.SingleChildScrollViewdoes not trigger a layout when scrolling, resulting in different behaviors betweenSingleChildScrollViewandCustomScrollView. This PR unifies their behavior so that both triggerScrollMetricsNotificationwhen scrolling.@chunhtai, this breaks some of your tests in #87076, mainly because triggering
ScrollMetricsNotificationduring scrolling happens one frame earlier than during layout, resulting in one less frame. Is this correct?The original purpose was to update
_lastMetrics. I tried three approaches:As shown in this PR, trigger
ScrollMetricsNotificationand update_lastMetricson every scroll;Cancel triggering
ScrollMetricsNotificationduringCustomScrollViewscrolling, keepapplyContentDimensionsunchanged, and update only thepixelsproperty in_lastMetricsduring scrolling without sending notifications. Since pixels is already updated,applyContentDimensionswill not trigger notifications either. This will cause more test failures.Maintain the current
ScrollMetricsNotificationtriggering, similar to Fix ScrollMetricsNotifiation for SingleChildScrollView #102339.I prefer the first approach, which keeps the behavior of
SingleChildScrollViewandCustomScrollViewconsistent.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.