Fixes the issue of not being able to stop scrolling by dragging the content again #133750

intonarumori · 2023-08-31T09:52:53Z

Moves the PanGestureRecognizer used to drag the content along both axis to the outer Scrollable subclass.

This solves the issue of the inner Scrollable gestures being disabled while the outer Scrollable is scrolling
Enables the user to stop the scroll movement by dragging the content again

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 this 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].

flutter-dashboard · 2023-08-31T09:52:55Z

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

google-cla · 2023-08-31T09:52:57Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

…ontent again

Piinks

Hi @intonarumori thanks for contributing! This change will need tests to verify that it works as expected. Thank you!

intonarumori · 2023-09-11T20:57:44Z

@Piinks Thanks for taking a look.

For tests my first idea was to look into the CustomScrollView tests and replicate them for the two dimensional version.
However, it looks like there is no concrete subclass of TwoDimensionalScrollView in the Flutter package, so I'm not sure that route would work.

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

Lines 1693 to 1695 in e7058f5

    
           /// It's rare to construct a [TwoDimensionalScrollable] directly. Instead, 
        
           /// consider subclassing [TwoDimensionalScrollView], which combines scrolling, 
        
           /// viewporting, and a layout model in both dimensions.

If you can give me some hints of how to go about testing this I would be happy to look into it.

Piinks · 2023-09-11T21:21:31Z

For sure! There are some concrete classes just for testing, see the following for examples:

jtkeyva · 2023-09-25T06:52:11Z

@intonarumori @Piinks any movement on this? Such a critical feature/bug fix. Does this also allow to scroll again to keep momentum / accelerate like in a regular scrollview?

Thanks

Piinks

Thanks for your patience. I appreciate you taking on a complex fix!

Piinks · 2023-09-25T19:18:59Z

packages/flutter/test/widgets/two_dimensional_scroll_view_test.dart

      expect(scrollable.widget.dragStartBehavior, DragStartBehavior.down);
    }, variant: TargetPlatformVariant.all());
+
+    testWidgets('Fling and tap to stop', (WidgetTester tester) async {


Can you update these tests to check two more things? These should verify they we are actually scrolling when these taps occur. Can you add the following:

check the ScrollActivity.velocity in both axes when the test is about to tap and after.

the activity is available through the ScrollPosition

verifies we went from scrolling to stopping in response

we should also verify the position is actually changing/stops changing as a result of flinging and then stopping it with a tap.

verify the ScrollPosition.pixels before flinging, before tapping, and after tapping

Piinks · 2023-09-25T19:19:46Z

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

      case DiagonalDragBehavior.weightedContinuous:
      case DiagonalDragBehavior.free:
-        verticalScrollable._handleDragEnd(verticalDragDetails);
+        //verticalScrollable._handleDragEnd(verticalDragDetails);


Should this be removed?

Piinks · 2023-09-25T19:20:51Z

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

-    // TwoDimensionalScrollableState.
-    return _configuration.buildOverscrollIndicator(context, child, details);
-  }
+class _TwoDimensionalDragForwarding {


Public docs are not required for private classes (///), but can you add some docs to this for maintainers to better understand what it's for and where ti is used?

Piinks

Hey @intonarumori are you planning to return to this and address the feedback above?

jtkeyva · 2023-10-18T01:07:29Z

almost there!

Piinks · 2023-11-13T21:04:16Z

We have not heard back from @intonarumori on this, so I am going to close it and open a new PR so we can close this bug. Thanks for contributing!

jtkeyva · 2023-11-14T04:50:31Z

@Piinks sounds good thanks. oh hey, nice video btw on this very feature :)

intonarumori · 2023-11-15T12:32:58Z

We have not heard back from @intonarumori on this, so I am going to close it and open a new PR so we can close this bug. Thanks for contributing!

@Piinks Sorry for the late reply, I've been bogged down by work lately and didn't have the time to come back to this.
Many thanks for taking on this. Looking forward to learn from the new one and hopefully land something better formatted next time.

Adopted from #133750 That PR was abandoned. This finishes it up so we can land it. Fixes #133529 Moves the `PanGestureRecognizer` used to drag the content along both axis to the outer vertical `Scrollable` subclass instead of the inner horizontal `Scrollable` subclass. - This solves the issue of the inner `Scrollable` gestures being disabled while the outer `Scrollable` is scrolling - Enables the user to stop the scroll movement by dragging the content again

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Aug 31, 2023

intonarumori added 3 commits September 1, 2023 17:54

fixes the issue of not being able to stop scrolling by dragging the c…

b405633

…ontent again

updated comments

ab2ed51

Added missing type annotations

a9347d4

intonarumori force-pushed the two-dimensional-scroll-dragging-fix branch from 392479a to a9347d4 Compare September 1, 2023 15:54

HansMuller requested a review from Piinks September 1, 2023 21:53

updated comments

345577b

Piinks reviewed Sep 11, 2023

View reviewed changes

Piinks added the p: two_dimensional_scrollables Issues pertaining to the two_dimensional_scrollables package label Sep 11, 2023

intonarumori added 3 commits September 12, 2023 21:43

added tests for two dimensional scrollview fling and stop interaction

767bfcd

Merge branch 'master' into two-dimensional-scroll-dragging-fix

42e1fe6

updated tests

f463e1f

goderbauer requested a review from Piinks September 19, 2023 22:07

Piinks reviewed Sep 25, 2023

View reviewed changes

Piinks mentioned this pull request Sep 25, 2023

[two_dimensional_scrollables] TableView is unable to scroll vertically if horizontal scrolling is impossible and diagonalDragBehavior is not set to none #135384

Closed

2 tasks

Piinks reviewed Oct 17, 2023

View reviewed changes

Piinks added the package flutter/packages repository. See also p: labels. label Oct 19, 2023

goderbauer requested a review from Piinks November 7, 2023 23:06

Piinks closed this Nov 13, 2023

Piinks reopened this Nov 14, 2023

Piinks closed this Nov 14, 2023

Piinks added a commit to Piinks/flutter that referenced this pull request Nov 14, 2023

Finish flutter#133750

b92d1b7

Piinks mentioned this pull request Nov 14, 2023

Fix 2D tap to stop scrolling #138442

Merged

8 tasks

Fixes the issue of not being able to stop scrolling by dragging the content again #133750

Fixes the issue of not being able to stop scrolling by dragging the content again #133750

Uh oh!

Conversation

intonarumori commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Aug 31, 2023

Uh oh!

google-cla bot commented Aug 31, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

intonarumori commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Sep 11, 2023

Uh oh!

jtkeyva commented Sep 25, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

Piinks Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

Piinks Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

jtkeyva commented Oct 18, 2023

Uh oh!

Piinks commented Nov 13, 2023

Uh oh!

jtkeyva commented Nov 14, 2023

Uh oh!

intonarumori commented Nov 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

intonarumori commented Aug 31, 2023 •

edited

Loading

intonarumori commented Sep 11, 2023 •

edited

Loading