fix mouse wheel scroll miscontrol of ScrollPosition. by YeungKC · Pull Request #66039 · flutter/flutter

YeungKC · 2020-09-17T16:20:23Z

Description

Trying to solve #55362 and #64266.

The handling of PointerSignalEvent in Scrollable is directly derived from the current ScrollPosition.pixels + delta, while NestedScrollView contains multiple ScrollPositions, and NestedScrollView contains multiple ScrollPosition. It doesn't make sense to calculate the target value alone.
So I've changed it to pass the delta directly to ScrollPosition and let ScrollPosition handle it internally.

About #64266, The original PointerSignalEvent was handled by call ScrollPosition.jumpTo which did not update ScrollDirection.forward or ScrollDirection.reverse, now it is handled by call ScrollPosition.jumpTo which does not update ScrollDirection.forward or ScrollDirection.reverse. call beginActivity, now it will update ScrollDirection.forward normally.

Related Discord: https://discordapp.com/channels/608014603317936148/608018585025118217/755515518294687844

Related Issues

Fixes #55362
#64266

Replace this paragraph with a list of issues related to this PR from our issue database. Indicate, which of these issues are resolved or fixed by this PR. There should be at least one issue listed here.

Tests

I added the following tests:

scrollable_test.dart
- Holding scroll and Scroll pointer signal will update ScrollDirection.forward / ScrollDirection.reverse
nested_scroll_view_test.dart
- Holding scroll and Scroll pointer signal will update ScrollDirection.forward / ScrollDirection.reverse
- Scroll Pointer signals should not cause overscroll

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.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-09-17T16:20:26Z

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.

Piinks · 2020-09-22T17:24:26Z

It looks like @dkwingsmt may be more familiar with the failing pointer tests here.

Regarding writing a test for this, this test checks the pointer scroll event, we can modify this test to use a NestedScrollView and validate the behavior:

flutter/packages/flutter/test/widgets/scrollable_test.dart

Line 287 in 4299dd7

    
           testWidgets('Scroll pointer signals are handled on Fuchsia', (WidgetTester tester) async {

Piinks · 2020-09-22T17:27:50Z

It doesn't make sense to calculate the target value alone.

Can you clarify? I am wondering if what we may need here is to deploy some of the unnesting methods in the NestedScrollView to calculate the 'true' position. I am not sure if that would work, but it may be a simpler approach that isn't breaking.

YeungKC · 2020-09-22T17:46:25Z

@Piinks

Do you mean to leave Scrollable unchanged and have _NestedScrollPosition return 'true' pixels?

I think this may be difficult.

because ScrollPosition.pixels involves more code.
if we go to Scrollable and additionally determine Position as _NestedScrollPosition, this may not be very elegant.
Originally Scrollable used Position.jumpTo, which caused [web][desktop] Scrolling with 2 fingers (not press&hold style) categorized as ScrollDirection.idle (instead of ScrollDirection.forward or ScrollDirection.reverse) #64266 to occur.

Piinks · 2020-09-22T17:48:57Z

Ah ok. Do you think it is possible to separate this out into two pull requests? This appears to be trying to solve two different issues.

YeungKC · 2020-09-22T17:54:53Z

I don't think so, as pointerScroll instead of jumpTo is now the core code for the solution to both issues.

Piinks · 2020-09-22T18:10:34Z

I don't think so, as pointerScroll instead of jumpTo is now the core code for the solution to both issues.

Is it possible you could add pointerScroll in one change, and then implement it additional changes to solve each issue? This will be much easier to review if it is only trying to solve one problem.

YeungKC · 2020-09-22T18:24:50Z

I don't think so, I don't think there's any other way, after adding pointerScroll, it actually solves two problems...

YeungKC · 2020-09-22T19:40:34Z

I may not have described it clearly before, but now I'll explain why not.

added pointerScroll to fix Unexpected behaviour of SliverAppBar in NestedScrollView on Mouse Scroll #55362 calculation error pixels, and now it's handled internally by position.
having also given it to internal handling, it also solves [web][desktop] Scrolling with 2 fingers (not press&hold style) categorized as ScrollDirection.idle (instead of ScrollDirection.forward or ScrollDirection.reverse) #64266, otherwise I would need to change the jumpTo code to add updateUserScrollDirection, as changing the jumpTo code would affect the expected effect of calling jumpTo properly.

So it can't be split into two PR. I hope I've made it clear to you this time.

cc @Piinks

dkwingsmt · 2020-09-22T21:12:02Z

I'm pretty sure the test failure is because of the design that relies on scheduleMicrotask in the constructor. The test does not clear microtasks, therefore the jump is not executed.

I'm not an expert in scrollables but I don't think it's a good design. I suppose ScrollActivity is designed to be passive, only triggers side effects when certain events come, instead of during constructor and starts its own microtask.

YeungKC · 2020-09-23T10:52:39Z

Thanks for the review, but regarding the scheduleMicrotask not executing, I debug 'Scroll pointer signals are handled on Fuchsia' test and adding the scheduleMicrotask.callback breakpoint, which I'm pretty sure has already been executed.

I may have a different opinion about ScrollActivity, because there is BallisticScrollActivity, DrivenScrollActivity, and also the callback to init async in the constructor, which shows that ScrollActivity It's not necessary or recommended to do it passively.

But I agree that scheduleMicrotask is probably not the best way to go.

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

…miscontrol_of_ScrollPosition # Conflicts: # packages/flutter/lib/src/widgets/scrollable.dart

…y is now abandoned.

…/ ScrollDirection.reverse" test.

…rollDirection.forward / ScrollDirection.reverse test.

YeungKC · 2020-09-29T21:18:26Z

Now it' s been updated and improved for testing.
I apologize for the delay of a few days.

Piinks

This LGTM! Thank you for your contribution! This is a really great change.

There are some merge conflicts that will need to be resolved before we can merge it in.

…miscontrol_of_ScrollPosition # Conflicts: # packages/flutter/test/widgets/nested_scroll_view_test.dart

…miscontrol_of_ScrollPosition

Piinks · 2020-10-30T16:07:24Z

Hey @YeungKC can you check your branch? It looks like there was an issue when you rebased.

…miscontrol_of_ScrollPosition

…crollPosition' into Fix_Mouse_Scroll_miscontrol_of_ScrollPosition

Piinks · 2020-10-30T19:02:22Z

Hey @YeungKC I'm sorry to ask again, but can you update once more? It looks like your last update picked up a broken build that was fixed in #69426

…miscontrol_of_ScrollPosition

YeungKC · 2020-10-30T19:06:31Z

It seems like I had already noticed it at the time, but at the time I wasn't sure if I was the only one experiencing the problem.

https://discord.com/channels/608014603317936148/613398423093116959/771433253529059438

It's updated now. I'm watching ci work.

Out of date

fluttergithubbot · 2020-10-30T20:08:04Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux web_long_running_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite framework_tests-widgets-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite framework_tests-libraries-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite framework_tests-misc-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

…miscontrol_of_ScrollPosition

YeungKC · 2020-11-01T15:50:42Z

There were still some problems before, but now it can be merged.

cc @Piinks

YeungKC added 2 commits September 18, 2020 00:02

feat: fix mouse scroll miscontrol of ScrollPosition.

13edc23

refactor: remove dev log

1c471e5

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Sep 17, 2020

googlebot added the cla: yes label Sep 17, 2020

HansMuller requested review from Piinks and dkwingsmt September 17, 2020 23:54

Piinks added a: mouse Issues related to using a mouse or mouse support f: scrolling Viewports, list views, slivers, etc. a: desktop Running on desktop labels Sep 22, 2020

stuartmorgan-g reviewed Sep 23, 2020

View reviewed changes

packages/flutter/lib/src/widgets/scroll_activity.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/scroll_activity.dart Outdated Show resolved Hide resolved

YeungKC added 8 commits September 28, 2020 23:21

Merge remote-tracking branch 'upstream/master' into Fix_Mouse_Scroll_…

7c15f9c

…miscontrol_of_ScrollPosition # Conflicts: # packages/flutter/lib/src/widgets/scrollable.dart

refactor: fix null safety.

08257fd

refactor: The pointerScroll is refactored and the use of beginActivit…

c9d3049

…y is now abandoned.

feat: Add "Scroll pointer signal will update ScrollDirection.forward …

4eef477

…/ ScrollDirection.reverse" test.

refactor: Fix update mouse scrolling error UserScrollDirection.

c2e4105

refactor: Add Holding scroll and Scroll pointer signal will update Sc…

7a922c6

…rollDirection.forward / ScrollDirection.reverse test.

refactor: Improve pointer signal testing.

9415fdf

refactor: Delete superfluous punctuation.

9670778

YeungKC added 3 commits October 20, 2020 16:29

refactor: Improve comment for pointerScroll.

f0451ab

refactor: Improve nested_scroll_view_test.dart.

7c91de1

refactor: Improve nested_scroll_view_test.dart.

910c11a

YeungKC requested a review from Piinks October 21, 2020 14:37

Piinks approved these changes Oct 28, 2020

View reviewed changes

YeungKC added 4 commits October 30, 2020 00:07

Merge remote-tracking branch 'upstream/master' into Fix_Mouse_Scroll_…

da63093

…miscontrol_of_ScrollPosition # Conflicts: # packages/flutter/test/widgets/nested_scroll_view_test.dart

Merge remote-tracking branch 'upstream/master' into Fix_Mouse_Scroll_…

29bae54

…miscontrol_of_ScrollPosition

Merge remote-tracking branch 'upstream/master' into Fix_Mouse_Scroll_…

e5d524a

…miscontrol_of_ScrollPosition

Merge remote-tracking branch 'upstream/master' into Fix_Mouse_Scroll_…

381235a

…miscontrol_of_ScrollPosition

YeungKC added 3 commits October 31, 2020 02:18

Merge remote-tracking branch 'upstream/master' into Fix_Mouse_Scroll_…

5ee6aec

…miscontrol_of_ScrollPosition

refactor: soft reset.

56bd0b2

Merge remote-tracking branch 'origin/Fix_Mouse_Scroll_miscontrol_of_S…

2f83814

…crollPosition' into Fix_Mouse_Scroll_miscontrol_of_ScrollPosition

Merge remote-tracking branch 'upstream/master' into Fix_Mouse_Scroll_…

7c9c71b

…miscontrol_of_ScrollPosition

Piinks added the waiting for tree to go green label Oct 30, 2020

fluttergithubbot removed the waiting for tree to go green label Oct 30, 2020

Merge remote-tracking branch 'upstream/master' into Fix_Mouse_Scroll_…

c83b998

…miscontrol_of_ScrollPosition

Piinks added the waiting for tree to go green label Nov 2, 2020

fluttergithubbot merged commit 60a8b33 into flutter:master Nov 2, 2020

YeungKC mentioned this pull request Nov 4, 2020

SliverAppBar with floating: true does not reappear while scrolling up on desktop #60670

Closed

This was referenced Nov 20, 2020

Pointer scroll not handled correctly for nested NeverScrollable ScrollView #70948

Closed

Fix pointer scroll for nested NeverScrollables #70953

Merged

pedromassangocode mentioned this pull request Nov 25, 2020

Scrolling in web stopped working in latest beta and dev #71112

Closed

This was referenced Jan 29, 2021

[Flutter Web] floating sliver app bar behavior on mouse scrolling #40430

Closed

Mouse scrolling does not trigger showing SliverPersistentHeader when floating is set to true #75059

Closed

Conversation

YeungKC commented Sep 17, 2020 • edited by Piinks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Sep 17, 2020

Uh oh!

Piinks commented Sep 22, 2020

Uh oh!

Piinks commented Sep 22, 2020

Uh oh!

YeungKC commented Sep 22, 2020

Uh oh!

Piinks commented Sep 22, 2020

Uh oh!

YeungKC commented Sep 22, 2020

Uh oh!

Piinks commented Sep 22, 2020

Uh oh!

YeungKC commented Sep 22, 2020

Uh oh!

YeungKC commented Sep 22, 2020

Uh oh!

dkwingsmt commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YeungKC commented Sep 23, 2020

Uh oh!

Uh oh!

Uh oh!

YeungKC commented Sep 29, 2020

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Oct 30, 2020

Uh oh!

Piinks commented Oct 30, 2020

Uh oh!

YeungKC commented Oct 30, 2020

Uh oh!

fluttergithubbot commented Oct 30, 2020

Uh oh!

YeungKC commented Nov 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

YeungKC commented Sep 17, 2020 •

edited by Piinks

Loading

dkwingsmt commented Sep 22, 2020 •

edited

Loading