DraggableScrollableSheet & NestedScrollView should respect NeverScrollableScrollPhysics #123109

xu-baolin · 2023-03-21T12:57:07Z

Adding a ScrollPhysics.allowUserScrolling to indicate whether the physics allowed the user scrolling.

Piinks · 2023-03-22T22:01:46Z

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

@@ -969,4 +972,7 @@ class NeverScrollableScrollPhysics extends ScrollPhysics {

  @override
  bool get allowImplicitScrolling => false;
+
+  @override
+  bool get allowUserScrolling => false;


Doesn't shouldAcceptUserOffset above provide the same thing?

Hm, the default implementation of shouldAcceptUserOffset might not work as this is intended. However, I think if adding this allowUserScrolling is the right way to resolve this, it should work with shouldAcceptUserOffset, or at least document what the difference between them is. Should shouldAcceptUserOffset also consider allowUserScrolling?

@Piinks Nice point! Let us review the original issue again : )
1, The framework warp a gesture recognizer only if physics.shouldAcceptUserOffset(this) returns true.

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

Lines 645 to 649 in f266ccc

void setCanDrag(bool value) {

if (value == _lastCanDrag && (!value || widget.axis == _lastAxisDirection)) {

return;

}

if (!value) {

So, If the DSS's extent becomes large enough, the scrollable will become unscrollable and the gesture will be removed (The extent's size is actually the scrollable viewport). Then the DSS is stuck and cannot be pulled back. This is the #31739 root cause.

2, Here is _DraggableScrollableSheetScrollPosition.applyUserOffset 's implement

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

Lines 877 to 882 in f266ccc

void applyUserOffset(double delta) {

if (!listShouldScroll &&

(!(extent.isAtMin || extent.isAtMax) ||

(extent.isAtMin && delta < 0) ||

(extent.isAtMax && delta > 0))) {

extent.addPixelDelta(-delta, context.notificationContext!);

It set the extent's size by the user scrolling. So, if we want fix the original issue we must let the scrollable always scrollable, right?

Regardless of whether #31832 or #109081, they essentially make the scrollable to be always scrollable, but #31832 is a bit hack and introduces issue #98314.

Unfortunately, #109081 introduces issue #121021 because it ignores the ‘NeverScrollableScrollPhysics’ case. This is why I make this change to introduce a method to distinguish the NeverScrollableScrollPhysics and others.

In addition, I tried other solutions at the same time, such as caching the old physics, and then applying it to the user scrolling, but I gave up for it is a bit complicated.

Glad to here your thoughts : )

Ah thank you for this! I really appreciate the context. I came across #113753 which is a similar-ish case of the explicit physics being overridden by a widget applying other physics.

This makes me think, what do you think about some way for users to specify that physics should not be meddled with? And then maybe in applyTo we check that? I dunno, just brainstorming..
However, this is slightly different from #113753 in that in the nested scroll view, we see userSuppliedPhysics.applyTo(extraImplementationPhysics), while here we see extraImplementationPhysics.applyTo(userSuppliedPhysics).. so this idea may not actually work...

Since it appears there is a pattern where the user may not want us to to change the scroll physics (I am sure there are other places where we do this, overriding the value explicitly set by the user), what are you thoughts on approaching this in a way that lets users preserve their preferred scroll physics? (not exclusive to just NeverScrollable and allowUserScrolling)

Ah thank you for this! I really appreciate the context. I came across https://github.com/flutter/flutter/issues/113753 which is a similar-ish case of the explicit physics being overridden by a widget applying other physics.

It seems like physics overridden is not the root cause of that issue but this patch may help to fix that : )

This makes me think, what do you think about some way for users to specify that physics should not be meddled with?

Good idea but I'm concerned that doing this would be a breaking change?

Piinks

I don't know that this is the right approach, I actually wonder if the original fix for #98314 was not the right solution. Applying AlwaysScrollableScrollPhysics may have other side effects like we are seeing here with NeverScrollableScrollPhysics.
I am not sure what the right change is, but I'd like to see if we can revisit the original issue and check to see if there is a better solution there first. What do you think @xu-baolin?

xu-baolin · 2023-04-10T03:37:55Z

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

@@ -781,7 +781,7 @@ class _DraggableScrollableSheetScrollController extends ScrollController {
    ScrollPosition? oldPosition,
  ) {
    return _DraggableScrollableSheetScrollPosition(
-      physics: const AlwaysScrollableScrollPhysics().applyTo(physics),
+      physics: physics.applyTo(const AlwaysScrollableScrollPhysics()),


userSuppliedPhysics.applyTo(extraImplementationPhysics) can fix DDS's issue simply : )

xu-baolin · 2023-04-10T03:44:51Z

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

@@ -1439,7 +1439,8 @@ class _NestedScrollPosition extends ScrollPosition implements ScrollActivityDele
  }

  void updateCanDrag(double totalExtent) {
-    context.setCanDrag(totalExtent > (viewportDimension - maxScrollExtent) || minScrollExtent != maxScrollExtent);
+    context.setCanDrag(physics.allowUserScrolling &&


Fixes #113753

Here seems to be more suitable use physics.allowUserScrolling

Ah yes, thank you!

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

Piinks

Thanks for your patience on this one. I really appreciate you taking a look at that other issue and including a full fix! LGTM!

auto-submit · 2023-04-11T21:35:06Z

auto label is removed for flutter/flutter, pr: 123109, due to - The status or check suite Mac customer_testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

aqrc · 2023-04-12T13:49:14Z

Thank you very much for fixing that! Could you please estimate when this fix is going to be officially released?

…NeverScrollableScrollPhysics (flutter/flutter#123109)

…lableScrollPhysics (flutter#123109) DraggableScrollableSheet & NestedScrollView should respect NeverScrollableScrollPhysics

flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Mar 21, 2023

xu-baolin force-pushed the xbl230320 branch from 493ad79 to 725ab52 Compare March 21, 2023 12:59

xu-baolin requested a review from Piinks March 21, 2023 13:00

Piinks reviewed Mar 22, 2023

View reviewed changes

xu-baolin force-pushed the xbl230320 branch 2 times, most recently from 56c008d to d33b342 Compare March 23, 2023 02:54

xu-baolin requested a review from Piinks March 23, 2023 12:42

Piinks reviewed Mar 29, 2023

View reviewed changes

Piinks mentioned this pull request Apr 4, 2023

NestedScrollView physics not working #113753

Closed

xu-baolin force-pushed the xbl230320 branch 2 times, most recently from 9e448a8 to 8fd6b36 Compare April 10, 2023 03:13

xu-baolin changed the title ~~DraggableScrollableSheet should respect NeverScrollableScrollPhysics~~ DraggableScrollableSheet & NestedScrollView should respect NeverScrollableScrollPhysics Apr 10, 2023

xu-baolin commented Apr 10, 2023

View reviewed changes

Piinks reviewed Apr 11, 2023

View reviewed changes

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

Piinks reviewed Apr 11, 2023

View reviewed changes

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

Piinks approved these changes Apr 11, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 11, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 11, 2023

xu-baolin and others added 2 commits April 11, 2023 16:42

fix some bugs

d58b812

Apply suggestions from code review

9b14a7d

Piinks force-pushed the xbl230320 branch from 1619938 to 9b14a7d Compare April 11, 2023 21:42

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 11, 2023

auto-submit bot merged commit 328635b into flutter:master Apr 12, 2023
72 checks passed

engine-flutter-autoroll mentioned this pull request Apr 12, 2023

Roll Flutter from 535b4d5ec3f1 to 3b4b14919173 (44 revisions) flutter/packages#3694

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 12, 2023

328635b3d DraggableScrollableSheet & NestedScrollView should respect …

09c4b78

…NeverScrollableScrollPhysics (flutter/flutter#123109)

HansMuller mentioned this pull request Apr 20, 2023

Flutter Framework Team: Notable Commits #121415

Open

huycozy mentioned this pull request Jun 14, 2023

NeverScrollableScrollPhysics doesn't disable scroll in body of NestedScrollView #128798

Closed

dam-ease mentioned this pull request Aug 8, 2023

NestedScrollView doesn't react to NeverScrollableScrollPhysics on device or emulator #132075

Closed

2 tasks

huycozy mentioned this pull request Oct 9, 2023

NestedScrollView allows dragging when there is not enough content to scroll #136069

Open

2 tasks

zmtzawqlp added a commit to fluttercandies/extended_nested_scroll_view that referenced this pull request Dec 22, 2023

Merge code from flutter/flutter#123109 (fix #148)

6d66ce3

zmtzawqlp mentioned this pull request Dec 22, 2023

[Bug report] fluttercandies/extended_nested_scroll_view#148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DraggableScrollableSheet & NestedScrollView should respect NeverScrollableScrollPhysics #123109

DraggableScrollableSheet & NestedScrollView should respect NeverScrollableScrollPhysics #123109

xu-baolin commented Mar 21, 2023 •

edited

Piinks Mar 22, 2023

Piinks Mar 22, 2023

xu-baolin Mar 30, 2023

Piinks Apr 4, 2023

xu-baolin Apr 10, 2023

Piinks left a comment

xu-baolin Apr 10, 2023

Piinks Apr 11, 2023

xu-baolin Apr 10, 2023

Piinks Apr 11, 2023

Piinks left a comment

auto-submit bot commented Apr 11, 2023

aqrc commented Apr 12, 2023

	void setCanDrag(bool value) {
	if (value == _lastCanDrag && (!value \|\| widget.axis == _lastAxisDirection)) {
	return;
	}
	if (!value) {

	void applyUserOffset(double delta) {
	if (!listShouldScroll &&
	(!(extent.isAtMin \|\| extent.isAtMax) \|\|
	(extent.isAtMin && delta < 0) \|\|
	(extent.isAtMax && delta > 0))) {
	extent.addPixelDelta(-delta, context.notificationContext!);

DraggableScrollableSheet & NestedScrollView should respect NeverScrollableScrollPhysics #123109

DraggableScrollableSheet & NestedScrollView should respect NeverScrollableScrollPhysics #123109

Conversation

xu-baolin commented Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

auto-submit bot commented Apr 11, 2023

aqrc commented Apr 12, 2023

xu-baolin commented Mar 21, 2023 •

edited