Add a hook for scroll position to notify scrolling context when dimen… #87076

chunhtai · 2021-07-26T23:42:58Z

The scrollable needs to know if the content dimensions in the scroll position has changed in order to update its semantics node correctly. This pr adds an API to ScrollingContext to listen to the change.

fixes #40419

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Piinks

This looks similar to some of #85499, what do you think @chunhtai?

chunhtai · 2021-07-27T18:40:39Z

This looks similar to some of #85499, what do you think @chunhtai?

I think i can make use of the scroll notification to do the job, but I more toward to add this as a call back because the scrollable is strictly tied to a scroll position and not any arbitrary scroll position below its widget sub tree, plus there is one frame delay for it

If the strategy is to avoid adding new api to scroll context and use notification system when it is possible. I can switch to use the notification instead

Piinks · 2021-07-29T20:21:17Z

#85499 landed, do you need to update here @chunhtai? Or is this ready for review as is? What do you think?

chunhtai · 2021-07-29T21:25:23Z

I think we still have to figure out whether to add the new api or use the scrollmetrics notification, I currently more toward adding the api See my previous comment.

Piinks

with doc nit :)

Piinks · 2021-07-30T22:21:34Z

packages/flutter/lib/src/widgets/scroll_context.dart

Can we just add documentation for ScrollMetricsNotification here and vice versa - add documentation for this in ScrollMetricsNotification. I think having two separate APIs is ok, because they have very different uses. We may want to also say when they may be preferred, like if you want to listen for this change, choose the other one.

xu-baolin · 2021-07-30T22:29:10Z

packages/flutter/lib/src/widgets/scroll_position.dart

I'm just a little worried, because this api will be called in the layout stage, which will greatly restrict the application scenario, such as the tree cannot be rebuilt, and in the case of lazy loading, it will be called multiple times in the same frame.

That is actually a good point, let me think about this more

Will review again after potential updates

chunhtai

updated to use notification, PTAL @Piinks @xu-baolin

chunhtai · 2021-07-30T23:12:14Z

packages/flutter/lib/src/widgets/scroll_position.dart

@xu-baolin @Piinks
any reason we don't want to send when it initialize for the first time?

My use case here is the render object listens to the scrollable metrics before it applies content dimensions for the first time, it wants to be notified when the scrollable metrics initialize.

If this doesn't make sense, we probably need a hook, something like didInitializeScrollMetrics

There is no special reason, it's just that we didn't find the necessary scene at the time. If you need it, I think we can do it.

I post #87421 to land this change.

xu-baolin · 2021-07-30T23:26:40Z

packages/flutter/lib/src/widgets/scrollable.dart

Maybe we should check the depth of the events.

can you elaborate?

ah I see what you meant

Oh,you already checked the context, I think it have the same effect.

I think use depth is safer since another scrolling context can target the same notification context, that is a weird case if that happens though

xu-baolin · 2021-07-30T23:57:35Z

packages/flutter/test/widgets/scrollable_test.dart

This is intended change, right?

yes, now it takes one more frame to update the semantics

You just need updates the semantics at the first initial metrics?

no and subsequence changes as well, we just lucky that scroll metrics change usually come with scroll position changes (which will usuaully cause semantics update) so it wasn't so obvious before.

without the this pr, the semantics for the first frame is always wrong, but we are lucky that usually gets update due to some other semantics changes from the modal route.

Got it.thanks for your explanation.

chunhtai · 2021-07-31T00:21:30Z

packages/flutter/test/cupertino/scrollbar_test.dart

@Piinks Since now the first scroll metrics initialize will call fadeoutanimationcontroller.forward(), the error may throw multiple times

We probably won't need the first empty scroll event request now.

chunhtai · 2021-08-02T18:11:10Z

blocked on https://github.com/flutter/flutter/pull/87421/files

chunhtai · 2021-08-03T20:18:20Z

@Piinks @xu-baolin PTAL 🍡

Piinks · 2021-08-03T21:36:32Z

Just waiting on the scroll metrics change to roll, there were some failing tests but I think I have them all fixed. Also, CI is unhappy here.

xu-baolin · 2021-08-04T01:22:50Z

Just waiting on the scroll metrics change to roll, there were some failing tests but I think I have them all fixed. Also, CI is unhappy here.

Thanks 😄

goderbauer · 2021-08-17T17:17:35Z

Update: we are still waiting on the scroll metrics PR to roll.

…sions have changed

Piinks

LGTM!

flutter-dashboard bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Jul 26, 2021

google-cla bot added the cla: yes label Jul 26, 2021

chunhtai force-pushed the issues/40419 branch from cfd9d37 to 3b7aa78 Compare July 26, 2021 23:47

chunhtai requested review from Piinks and goderbauer July 26, 2021 23:47

Piinks reviewed Jul 27, 2021

View reviewed changes

Piinks previously approved these changes Jul 30, 2021

View reviewed changes

xu-baolin reviewed Jul 30, 2021

View reviewed changes

chunhtai force-pushed the issues/40419 branch from e1255e8 to 9c364aa Compare July 30, 2021 23:10

chunhtai commented Jul 30, 2021

View reviewed changes

xu-baolin reviewed Jul 30, 2021

View reviewed changes

chunhtai commented Jul 31, 2021

View reviewed changes

chunhtai force-pushed the issues/40419 branch from 63e5d07 to 5326f65 Compare August 3, 2021 20:12

chunhtai force-pushed the issues/40419 branch from af5ceb9 to 3117082 Compare August 10, 2021 17:35

Piinks added the f: scrolling Viewports, list views, slivers, etc. label Aug 11, 2021

Piinks added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Aug 17, 2021

chunhtai added 3 commits August 23, 2021 13:43

Add a hook for scroll position to notify scrolling context when dimen…

71a8985

…sions have changed

update

5ae777d

fix test

df154b4

chunhtai force-pushed the issues/40419 branch from 3117082 to df154b4 Compare August 23, 2021 20:44

Piinks approved these changes Aug 23, 2021

View reviewed changes

chunhtai added the waiting for tree to go green label Aug 23, 2021

fluttergithubbot merged commit 93a1484 into flutter:master Aug 23, 2021

yiiim mentioned this pull request Nov 29, 2025

Send ScrollMetricsNotification when SingleChildScrollView is scrolled. #179133

Draft

9 tasks

Add a hook for scroll position to notify scrolling context when dimen… #87076

Add a hook for scroll position to notify scrolling context when dimen… #87076

Uh oh!

Conversation

chunhtai commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Jul 29, 2021

Uh oh!

chunhtai commented Jul 29, 2021

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Jul 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Aug 2, 2021

Uh oh!

chunhtai commented Aug 3, 2021

Uh oh!

Piinks commented Aug 3, 2021

Uh oh!

xu-baolin commented Aug 4, 2021

Uh oh!

goderbauer commented Aug 17, 2021

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

chunhtai commented Jul 26, 2021 •

edited

Loading

chunhtai commented Jul 27, 2021 •

edited

Loading

chunhtai Jul 30, 2021 •

edited

Loading

chunhtai Jul 31, 2021 •

edited

Loading