From ef25052c5162bd913072696e7c5dc420eba0863b Mon Sep 17 00:00:00 2001 From: xster Date: Fri, 18 May 2018 14:40:39 -0700 Subject: [PATCH] Let scrollOffsetCorrection on small lists not leave the list stuck in overscroll (#17599) --- .../flutter/lib/src/cupertino/refresh.dart | 4 +- .../lib/src/rendering/viewport_offset.dart | 10 +++++ .../lib/src/widgets/scroll_position.dart | 38 +++++++++++++++++-- .../scroll_position_with_single_context.dart | 5 --- .../flutter/test/cupertino/refresh_test.dart | 33 ++++++++++++---- 5 files changed, 73 insertions(+), 17 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/refresh.dart b/packages/flutter/lib/src/cupertino/refresh.dart index cfc92351bf..6e50e14e4f 100644 --- a/packages/flutter/lib/src/cupertino/refresh.dart +++ b/packages/flutter/lib/src/cupertino/refresh.dart @@ -282,7 +282,8 @@ class CupertinoRefreshControl extends StatefulWidget { /// 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 @@ class CupertinoRefreshControl extends StatefulWidget { /// /// 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 2624b32e17..fc907be6db 100644 --- a/packages/flutter/lib/src/rendering/viewport_offset.dart +++ b/packages/flutter/lib/src/rendering/viewport_offset.dart @@ -150,10 +150,20 @@ abstract class ViewportOffset extends ChangeNotifier { /// [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 d936b83dd6..e3256136d4 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart @@ -250,13 +250,43 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { /// 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 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { 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 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { 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 4ae9678795..cf0ab5e3e7 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 @@ -84,11 +84,6 @@ class ScrollPositionWithSingleContext extends ScrollPosition implements ScrollAc return super.setPixels(newPixels); } - @override - void correctBy(double correction) { - correctPixels(pixels + correction); - } - @override void absorb(ScrollPosition other) { super.absorb(other); diff --git a/packages/flutter/test/cupertino/refresh_test.dart b/packages/flutter/test/cupertino/refresh_test.dart index ef6317d2d0..3e5877c2c7 100644 --- a/packages/flutter/test/cupertino/refresh_test.dart +++ b/packages/flutter/test/cupertino/refresh_test.dart @@ -62,6 +62,7 @@ void main() { when(mockHelper.refreshTask()).thenAnswer((_) => refreshCompleter.future); }); + int testListLength = 10; SliverList buildAListOfStuff() { return new SliverList( delegate: new SliverChildBuilderDelegate( @@ -71,12 +72,12 @@ void main() { 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 @@ void main() { 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 @@ void main() { 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 @@ void main() { 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 {