Let scrollOffsetCorrection on small lists not leave the list stuck in overscroll (#17599)
diff --git a/packages/flutter/lib/src/cupertino/refresh.dart b/packages/flutter/lib/src/cupertino/refresh.dart index cfc9235..6e50e14 100644 --- a/packages/flutter/lib/src/cupertino/refresh.dart +++ b/packages/flutter/lib/src/cupertino/refresh.dart
@@ -282,7 +282,8 @@ /// The amount of overscroll the scrollable must be dragged to trigger a reload. /// - /// Must not be null, must be larger than 0.0 and larger than [refreshIndicatorExtent]. + /// Must not be null, must be larger than 0.0 and larger than + /// [refreshIndicatorExtent]. Defaults to 100px when not specified. /// /// When overscrolled past this distance, [onRefresh] will be called if not /// null and the [builder] will build in the [RefreshIndicatorMode.armed] state. @@ -293,6 +294,7 @@ /// /// Must not be null and must be positive, but can be 0.0, in which case the /// sliver will start retracting back to 0.0 as soon as the refresh is started. + /// Defaults to 60px when not specified. /// /// Must be smaller than [refreshTriggerPullDistance], since the sliver /// shouldn't grow further after triggering the refresh.
diff --git a/packages/flutter/lib/src/rendering/viewport_offset.dart b/packages/flutter/lib/src/rendering/viewport_offset.dart index 2624b32..fc907be 100644 --- a/packages/flutter/lib/src/rendering/viewport_offset.dart +++ b/packages/flutter/lib/src/rendering/viewport_offset.dart
@@ -150,10 +150,20 @@ /// [RenderViewport], before [applyContentDimensions]. After this method is /// called, the layout will be recomputed and that may result in this method /// being called again, though this should be very rare. + /// + /// See also: + /// + /// * [jumpTo], for also changing the scroll position when not in layout. + /// [jumpTo] applies the change immediately and notifies its listeners. void correctBy(double correction); /// Jumps the scroll position from its current value to the given value, /// without animation, and without checking if the new value is in range. + /// + /// See also: + /// + /// * [correctBy], for changing the current offset in the middle of layout + /// and that defers the notification of its listeners until after layout. void jumpTo(double pixels); /// The direction in which the user is trying to change [pixels], relative to
diff --git a/packages/flutter/lib/src/widgets/scroll_position.dart b/packages/flutter/lib/src/widgets/scroll_position.dart index d936b83..e325613 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart
@@ -250,13 +250,43 @@ /// To force the [pixels] to a particular value without honoring the normal /// conventions for changing the scroll offset, consider [forcePixels]. (But /// see the discussion there for why that might still be a bad idea.) + /// + /// See also: + /// + /// * The method [correctBy], which is a method of [ViewportOffset] used + /// by viewport render objects to correct the offset during layout + /// without notifying its listeners. + /// * The method [jumpTo], for making changes to position while not in the + /// middle of layout and applying the new position immediately. + /// * The method [animateTo], which is like [jumpTo] but animating to the + /// distination offset. void correctPixels(double value) { _pixels = value; } + /// Apply a layout-time correction to the scroll offset. + /// + /// This method should change the [pixels] value by `correction`, but without + /// calling [notifyListeners]. It is called during layout by the + /// [RenderViewport], before [applyContentDimensions]. After this method is + /// called, the layout will be recomputed and that may result in this method + /// being called again, though this should be very rare. + /// + /// See also: + /// + /// * [jumpTo], for also changing the scroll position when not in layout. + /// [jumpTo] applies the change immediately and notifies its listeners. + /// * [correctPixels], which is used by the [ScrollPosition] itself to + /// set the offset initially during construction or after + /// [applyViewportDimension] or [applyContentDimensions] is called. @override void correctBy(double correction) { + assert( + _pixels != null, + 'An initial pixels value must exist by caling correctPixels on the ScrollPosition', + ); _pixels += correction; + _didChangeViewportDimensionOrReceiveCorrection = true; } /// Change the value of [pixels] to the new value, and notify any customers, @@ -360,13 +390,13 @@ return result; } - bool _didChangeViewportDimension = true; + bool _didChangeViewportDimensionOrReceiveCorrection = true; @override bool applyViewportDimension(double viewportDimension) { if (_viewportDimension != viewportDimension) { _viewportDimension = viewportDimension; - _didChangeViewportDimension = true; + _didChangeViewportDimensionOrReceiveCorrection = true; // If this is called, you can rely on applyContentDimensions being called // soon afterwards in the same layout phase. So we put all the logic that // relies on both values being computed into applyContentDimensions. @@ -419,12 +449,12 @@ bool applyContentDimensions(double minScrollExtent, double maxScrollExtent) { if (!nearEqual(_minScrollExtent, minScrollExtent, Tolerance.defaultTolerance.distance) || !nearEqual(_maxScrollExtent, maxScrollExtent, Tolerance.defaultTolerance.distance) || - _didChangeViewportDimension) { + _didChangeViewportDimensionOrReceiveCorrection) { _minScrollExtent = minScrollExtent; _maxScrollExtent = maxScrollExtent; _haveDimensions = true; applyNewDimensions(); - _didChangeViewportDimension = false; + _didChangeViewportDimensionOrReceiveCorrection = false; } return true; }
diff --git a/packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart b/packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart index 4ae9678..cf0ab5e 100644 --- a/packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart +++ b/packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart
@@ -85,11 +85,6 @@ } @override - void correctBy(double correction) { - correctPixels(pixels + correction); - } - - @override void absorb(ScrollPosition other) { super.absorb(other); if (other is! ScrollPositionWithSingleContext) {
diff --git a/packages/flutter/test/cupertino/refresh_test.dart b/packages/flutter/test/cupertino/refresh_test.dart index ef6317d..3e5877c 100644 --- a/packages/flutter/test/cupertino/refresh_test.dart +++ b/packages/flutter/test/cupertino/refresh_test.dart
@@ -62,6 +62,7 @@ when(mockHelper.refreshTask()).thenAnswer((_) => refreshCompleter.future); }); + int testListLength = 10; SliverList buildAListOfStuff() { return new SliverList( delegate: new SliverChildBuilderDelegate( @@ -71,12 +72,12 @@ child: new Center(child: new Text(index.toString())), ); }, - childCount: 10, + childCount: testListLength, ), ); } - group('UI tests', () { + final Function uiTestGroup = () { testWidgets("doesn't invoke anything without user interaction", (WidgetTester tester) async { debugDefaultTargetPlatformOverride = TargetPlatform.iOS; @@ -682,6 +683,11 @@ testWidgets( 'sliver scrolled away when task completes properly removes itself', (WidgetTester tester) async { + if (testListLength < 4) { + // This test only makes sense when the list is long enough that + // the indicator can be scrolled away while refreshing. + return; + } debugDefaultTargetPlatformOverride = TargetPlatform.iOS; refreshIndicator = const Center(child: const Text('-1')); @@ -849,11 +855,9 @@ debugDefaultTargetPlatformOverride = null; } ); - }); + }; - // Test the internal state machine directly to make sure the UI aren't just - // correct by coincidence. - group('state machine test', () { + final Function stateMachineTestGroup = () { testWidgets('starts in inactive state', (WidgetTester tester) async { debugDefaultTargetPlatformOverride = TargetPlatform.iOS; @@ -1228,7 +1232,22 @@ debugDefaultTargetPlatformOverride = null; } ); - }); + }; + + group('UI tests long list', uiTestGroup); + + // Test the internal state machine directly to make sure the UI aren't just + // correct by coincidence. + group('state machine test long list', stateMachineTestGroup); + + // Retest everything and make sure that it still works when the whole list + // is smaller than the viewport size. + testListLength = 2; + group('UI tests short list', uiTestGroup); + + // Test the internal state machine directly to make sure the UI aren't just + // correct by coincidence. + group('state machine test short list', stateMachineTestGroup); } class MockHelper extends Mock {