From 7efed85b353aa5b147fbe5d23d9630a4f0b1b442 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Hn=C3=ADzdo?= Date: Wed, 10 Jan 2024 01:28:09 +0100 Subject: [PATCH] `NestedScrollView`'s outer scrollable jumping with `BouncingScrollPhysics` due to `double` precision errors (#138319) This PR fixes scrolling issues with `NestedScrollView` using the `BouncingScrollPhysics`. In one of the steps of the calculation, we can reach a state where the position of the inner scrollable is set to a `double` value that falls within `precisionErrorTolerance` of `0` but we were using `==` with `0` rather than checking for a precision. My posts in the linked issue show the current behavior, and how I reached to conclusion (the code in this PR). This PR only addresses the "jumping" of the outer scrollable. Fixes #136199 I have not finished a test for this since I have never done so and therefore have 0 experience writing tests in Flutter, so any help there would be appreciated. I am also not sure how to test double precision errors in general. I did run all the nested_scroll_view_tests.dart locally and there are no failures. --- .../lib/src/widgets/nested_scroll_view.dart | 6 +- .../test/widgets/nested_scroll_view_test.dart | 112 ++++++++++++++++++ 2 files changed, 115 insertions(+), 3 deletions(-) diff --git a/packages/flutter/lib/src/widgets/nested_scroll_view.dart b/packages/flutter/lib/src/widgets/nested_scroll_view.dart index f2226888b4..6df3d917d8 100644 --- a/packages/flutter/lib/src/widgets/nested_scroll_view.dart +++ b/packages/flutter/lib/src/widgets/nested_scroll_view.dart @@ -1065,7 +1065,7 @@ class _NestedScrollCoordinator implements ScrollActivityDelegate, ScrollHoldCont outerDelta = math.max(outerDelta, potentialOuterDelta); } } - if (outerDelta != 0.0) { + if (outerDelta.abs() > precisionErrorTolerance) { final double innerDelta = _outerPosition!.applyClampedDragUpdate( outerDelta, ); @@ -1302,8 +1302,8 @@ class _NestedScrollPosition extends ScrollPosition implements ScrollActivityDele this, delta, ); - if (oldPixels == newPixels) { - // Delta must have been so small we dropped it during floating point addition. + if ((oldPixels - newPixels).abs() < precisionErrorTolerance) { + // Delta is so small we can drop it. return 0.0; } // Check for overscroll: diff --git a/packages/flutter/test/widgets/nested_scroll_view_test.dart b/packages/flutter/test/widgets/nested_scroll_view_test.dart index 9c8c4afaa7..e56874a220 100644 --- a/packages/flutter/test/widgets/nested_scroll_view_test.dart +++ b/packages/flutter/test/widgets/nested_scroll_view_test.dart @@ -212,6 +212,118 @@ void main() { expect(context.clipBehavior, equals(Clip.antiAlias)); }); + testWidgets('NestedScrollView always scrolls outer scrollable first', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/136199 + final Key innerKey = UniqueKey(); + final GlobalKey outerKey = GlobalKey(); + + final ScrollController outerController = ScrollController(); + + Widget build() { + return Directionality( + textDirection: TextDirection.ltr, + child: Scaffold( + body: NestedScrollView( + key: outerKey, + controller: outerController, + physics: const BouncingScrollPhysics(), + headerSliverBuilder: (BuildContext context, bool innerBoxIsScrolled) => [ + SliverToBoxAdapter( + child: Container(color: Colors.green, height: 300), + ), + SliverOverlapAbsorber( + handle: NestedScrollView.sliverOverlapAbsorberHandleFor(context), + sliver: SliverToBoxAdapter( + child: Container( + color: Colors.blue, + height: 64, + ), + ), + ), + ], + body: SingleChildScrollView( + key: innerKey, + physics: const BouncingScrollPhysics(), + child: Container( + decoration: const BoxDecoration( + gradient: LinearGradient( + begin: Alignment.topCenter, + end: Alignment.bottomCenter, + colors: [Colors.black, Colors.blue], + stops: [0, 1], + ), + ), + height: 800, + ), + ), + ), + ), + ); + } + + await tester.pumpWidget(build()); + + final ScrollController outer = outerKey.currentState!.outerController; + final ScrollController inner = outerKey.currentState!.innerController; + + // Assert the initial positions + expect(outer.offset, 0.0); + expect(inner.offset, 0.0); + + outerController.addListener(() { + fail('Outer controller should not be scrolling'); // This should never be called + }); + + await tester.drag(find.byKey(innerKey), const Offset(0, 2000)); // Over-scroll the inner Scrollable to the bottom + + // Using a precise value to make addition/subtraction possible later in the test + // Which better conveys the intent of the test + // The value is not equal to 2000 due to BouncingScrollPhysics of the inner Scrollable + const double endPosition = -1974.0862087158384; + const Duration nextFrame = Duration(microseconds: 16666); + + // Assert positions after over-scrolling + expect(outer.offset, 0.0); + expect(inner.offset, endPosition); + + await tester.fling(find.byKey(innerKey), const Offset(0, -600), 2000); // Fling the inner Scrollable to the top + + // Assert positions after fling + expect(outer.offset, 0.0); + expect(inner.offset, endPosition + 600); + + await tester.pump(nextFrame); + + // Assert positions after pump + expect(outer.offset, 0.0); + expect(inner.offset, endPosition + 600); + + double currentOffset = inner.offset; + int maxNumberOfSteps = 100; + + while (inner.offset < 0) { + maxNumberOfSteps--; + if (maxNumberOfSteps <= 0) { + fail('Scrolling did not settle in an expected number of steps.'); + } + await tester.pump(nextFrame); + expect(inner.offset, greaterThanOrEqualTo(currentOffset)); + expect(outer.offset, 0.0); + + currentOffset = inner.offset; + } + + // Assert positions returned to/stayed at 0.0 + expect(outer.offset, 0.0); + expect(inner.offset, 0.0); + + await tester.pumpAndSettle(); + + // Assert values settle at 0.0 + expect(outer.offset, 0.0); + expect(inner.offset, 0.0); + }); + testWidgets('NestedScrollView overscroll and release and hold', (WidgetTester tester) async { await tester.pumpWidget(buildTest()); expect(find.text('aaa2'), findsOneWidget);