Fix ScrollMetricsNotifiation for SingleChildScrollView #102339
Conversation
flutter-dashboard
bot
added
f: scrolling
darrenaustin
left a comment
darrenaustin
left a comment
There was a problem hiding this comment.
LGTM. Just a small question about the test.
| // Scroll a bit | ||
| await tester.drag(find.byType(SingleChildScrollView), const Offset(0.0, -100.0)); | ||
|
|
||
| // We should have received a ScrollMetricsNotification |
There was a problem hiding this comment.
If there is only one notification, why is the counter 3 instead of 2?
There was a problem hiding this comment.
This surprised me, TIL that tester.drag actually moves more than once. I can change this to a manual gesture so it's clearer. There are some other test updates I need to confirm as well.
There was a problem hiding this comment.
No worries, you could also just document that in this case there are two new notifications from the tester.drag call.
|
@xu-baolin can you take a look at this and let me know what you think? It looks like when you designed ScrollMetricsNotification, leaving out the SingleChildScrollView was intentional. |
| _didChangeViewportDimensionOrReceiveCorrection = false; | ||
| _pendingDimensions = true; | ||
| if (haveDimensions && !correctForNewDimensions(_lastMetrics!, currentMetrics!)) { | ||
| _maybeDispatchScrollMetricsNotification(); |
There was a problem hiding this comment.
Why not let _RenderSingleChildViewport.performLayout response the return value like what RenderViewport.performLayout does?
It should do multi times otherwise the ScrollPosition has a wrong state, such as the _lastMetrics does not updated.
| // Content dimensions do not change as SingleChildScrollView only lays out | ||
| // once, but the metrics can change based on scrolling and we may want to | ||
| // dispatch a ScrollMetricsNotification. | ||
| offset.applyContentDimensions(_minScrollExtent, _maxScrollExtent); |
There was a problem hiding this comment.
According to the original design intent, ScrollMetricsNotification is to complement ScrollNotification, not to replace ScrollNotification. As the documentation says
/// For example, when the content of a scrollable is altered, making it larger
/// or smaller, this notification will be dispatched. Similarly, if the size
/// of the window or parent changes, the scrollable can notify of these
/// changes in dimensions.
///
/// The above behaviors usually do not trigger [ScrollNotification] events,
/// so this is useful for listening to [ScrollMetrics] changes that are not
/// caused by the user scrolling.
But would love to explore further how to revamp it. : ）
|
Thanks for taking a look @xu-baolin! You gave me an idea for a different solution. Much appreciated! |
3 participants
Fixes #102181
Necessary to re-land #101460
SingleChildScrollViewhas its own viewport render object, and only performs layout once (since it only has one child), so it never callsapplyContentDimensionsafter the initial layout.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.