From cdc0f15f273558a3a669afd3f1ee660d5fb663e7 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Tue, 14 Feb 2017 18:49:11 -0800 Subject: [PATCH] SingleChildScrollView can assert in LayoutBuilder (#8162) We can't read our size in the offset setter because we might be in the middle of layout. This issue also occured with the sliver-based viewport. --- .../flutter/lib/src/widgets/scrollable.dart | 2 +- .../src/widgets/single_child_scroll_view.dart | 19 +--- .../single_child_scroll_view_test.dart | 93 +++++++++++++++++++ 3 files changed, 97 insertions(+), 17 deletions(-) diff --git a/packages/flutter/lib/src/widgets/scrollable.dart b/packages/flutter/lib/src/widgets/scrollable.dart index d81742726a0..a6554708b1c 100644 --- a/packages/flutter/lib/src/widgets/scrollable.dart +++ b/packages/flutter/lib/src/widgets/scrollable.dart @@ -157,7 +157,7 @@ class Scrollable2State extends State with TickerProviderStateMixin bool _shouldUpdatePosition(Scrollable2 oldConfig) { return config.physics?.runtimeType != oldConfig.physics?.runtimeType - || config.controller?.runtimeType != config.controller?.runtimeType; + || config.controller?.runtimeType != oldConfig.controller?.runtimeType; } @override diff --git a/packages/flutter/lib/src/widgets/single_child_scroll_view.dart b/packages/flutter/lib/src/widgets/single_child_scroll_view.dart index 42eaedc3c16..32291ca70b4 100644 --- a/packages/flutter/lib/src/widgets/single_child_scroll_view.dart +++ b/packages/flutter/lib/src/widgets/single_child_scroll_view.dart @@ -154,24 +154,11 @@ class _RenderSingleChildViewport extends RenderBox with RenderObjectWithChildMix if (value == _offset) return; if (attached) - _offset.removeListener(markNeedsLayout); - if (_offset.pixels != value.pixels) - markNeedsLayout(); + _offset.removeListener(markNeedsPaint); _offset = value; if (attached) - _offset.addListener(markNeedsLayout); - // If we already have a size, then we should re-report the dimensions - // to the new ViewportOffset. If we don't then we'll report them when - // we establish the dimensions later, so don't worry about it now. - if (hasSize) { - assert(_minScrollExtent != null); - assert(_maxScrollExtent != null); - assert(_effectiveExtent != null); - final bool didAcceptViewportDimensions = offset.applyViewportDimension(_effectiveExtent); - final bool didAcceptContentDimensions = offset.applyContentDimensions(_minScrollExtent, _maxScrollExtent); - if (didAcceptViewportDimensions || didAcceptContentDimensions) - markNeedsPaint(); - } + _offset.addListener(markNeedsPaint); + markNeedsLayout(); } @override diff --git a/packages/flutter/test/widgets/single_child_scroll_view_test.dart b/packages/flutter/test/widgets/single_child_scroll_view_test.dart index eaedc628564..76c236e03aa 100644 --- a/packages/flutter/test/widgets/single_child_scroll_view_test.dart +++ b/packages/flutter/test/widgets/single_child_scroll_view_test.dart @@ -5,6 +5,32 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:flutter/widgets.dart'; +class TestScrollPosition extends ScrollPosition { + TestScrollPosition({ + ScrollPhysics physics, + AbstractScrollState state, + double initialPixels: 0.0, + ScrollPosition oldPosition, + }) : super( + physics: physics, + state: state, + initialPixels: initialPixels, + oldPosition: oldPosition, + ); +} + +class TestScrollController extends ScrollController { + @override + ScrollPosition createScrollPosition(ScrollPhysics physics, AbstractScrollState state, ScrollPosition oldPosition) { + return new TestScrollPosition( + physics: physics, + state: state, + initialPixels: initialScrollOffset, + oldPosition: oldPosition, + ); + } +} + void main() { testWidgets('SingleChildScrollView control test', (WidgetTester tester) async { await tester.pumpWidget(new SingleChildScrollView( @@ -23,4 +49,71 @@ void main() { expect(box.localToGlobal(Point.origin), equals(const Point(0.0, -200.0))); }); + + testWidgets('Changing controllers changes scroll position', (WidgetTester tester) async { + TestScrollController controller = new TestScrollController(); + + await tester.pumpWidget(new SingleChildScrollView( + child: new Container( + height: 2000.0, + decoration: const BoxDecoration( + backgroundColor: const Color(0xFF00FF00), + ), + ), + )); + + await tester.pumpWidget(new SingleChildScrollView( + controller: controller, + child: new Container( + height: 2000.0, + decoration: const BoxDecoration( + backgroundColor: const Color(0xFF00FF00), + ), + ), + )); + + Scrollable2State scrollable = tester.state(find.byType(Scrollable2)); + expect(scrollable.position, const isInstanceOf()); + }); + + testWidgets('Changing scroll controller inside dirty layout builder does not assert', (WidgetTester tester) async { + ScrollController controller = new ScrollController(); + + await tester.pumpWidget(new Center( + child: new SizedBox( + width: 750.0, + child: new LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return new SingleChildScrollView( + child: new Container( + height: 2000.0, + decoration: const BoxDecoration( + backgroundColor: const Color(0xFF00FF00), + ), + ), + ); + }, + ), + ), + )); + + await tester.pumpWidget(new Center( + child: new SizedBox( + width: 700.0, + child: new LayoutBuilder( + builder: (BuildContext context, BoxConstraints constraints) { + return new SingleChildScrollView( + controller: controller, + child: new Container( + height: 2000.0, + decoration: const BoxDecoration( + backgroundColor: const Color(0xFF00FF00), + ), + ), + ); + }, + ), + ), + )); + }); }