From 7701127bdd87d22102d076dd6e6626c3605cb290 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Fri, 26 Jan 2018 22:36:00 -0800 Subject: [PATCH] Fix shadows in NestedScrollView (#14279) * Fix shadows in NestedScrollView The previous logic (and especially the previous test) wasn't quite right. * Update nested_scroll_view.dart --- .../lib/src/widgets/nested_scroll_view.dart | 34 +++++++---- .../test/widgets/nested_scroll_view_test.dart | 56 +++++++++++++++++-- 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/packages/flutter/lib/src/widgets/nested_scroll_view.dart b/packages/flutter/lib/src/widgets/nested_scroll_view.dart index 6eb3e97e1b..da04ca695a 100644 --- a/packages/flutter/lib/src/widgets/nested_scroll_view.dart +++ b/packages/flutter/lib/src/widgets/nested_scroll_view.dart @@ -313,10 +313,21 @@ class _NestedScrollViewState extends State { super.dispose(); } + bool _lastHasScrolledBody; + void _handleHasScrolledBodyChanged() { if (!mounted) return; - setState(() { /* _coordinator.hasScrolledBody changed (we use it in the build method) */ }); + final bool newHasScrolledBody = _coordinator.hasScrolledBody; + if (_lastHasScrolledBody != newHasScrolledBody) { + setState(() { + // _coordinator.hasScrolledBody changed (we use it in the build method) + // (We record _lastHasScrolledBody in the build() method, rather than in + // this setState call, because the build() method may be called more + // often than just from here, and we want to only call setState when the + // new value is different than the last built value.) + }); + } } @override @@ -325,6 +336,7 @@ class _NestedScrollViewState extends State { state: this, child: new Builder( builder: (BuildContext context) { + _lastHasScrolledBody = _coordinator.hasScrolledBody; return new _NestedScrollViewCustomScrollView( scrollDirection: widget.scrollDirection, reverse: widget.reverse, @@ -335,7 +347,7 @@ class _NestedScrollViewState extends State { slivers: widget._buildSlivers( context, _coordinator._innerController, - _coordinator.hasScrolledBody, + _lastHasScrolledBody, ), handle: _absorberHandle, ); @@ -461,13 +473,9 @@ class _NestedScrollCoordinator implements ScrollActivityDelegate, ScrollHoldCont return false; } - bool _lastHasScrolledBody; void updateShadow() { - final bool newHasScrolledBody = hasScrolledBody; - if (_lastHasScrolledBody != newHasScrolledBody) { - if (_onHasScrolledBodyChanged != null) - _onHasScrolledBodyChanged(); - } + if (_onHasScrolledBodyChanged != null) + _onHasScrolledBodyChanged(); } ScrollDirection get userScrollDirection => _userScrollDirection; @@ -833,22 +841,24 @@ class _NestedScrollController extends ScrollController { super.attach(position); coordinator.updateParent(); coordinator.updateCanDrag(); + position.addListener(_scheduleUpdateShadow); _scheduleUpdateShadow(); } @override void detach(ScrollPosition position) { assert(position is _NestedScrollPosition); + position.removeListener(_scheduleUpdateShadow); super.detach(position); _scheduleUpdateShadow(); } void _scheduleUpdateShadow() { // We do this asynchronously for attach() so that the new position has had - // time to be initialized, and we do it asynchronously for detach() because - // that happens synchronously during a frame, at a time where it's too late - // to call setState. Since the result is usually animated, the lag incurred - // is no big deal. + // time to be initialized, and we do it asynchronously for detach() and from + // the position change notifications because those happen synchronously + // during a frame, at a time where it's too late to call setState. Since the + // result is usually animated, the lag incurred is no big deal. SchedulerBinding.instance.addPostFrameCallback( (Duration timeStamp) { coordinator.updateShadow(); diff --git a/packages/flutter/test/widgets/nested_scroll_view_test.dart b/packages/flutter/test/widgets/nested_scroll_view_test.dart index 1d40d9567f..aeddbef09c 100644 --- a/packages/flutter/test/widgets/nested_scroll_view_test.dart +++ b/packages/flutter/test/widgets/nested_scroll_view_test.dart @@ -344,14 +344,17 @@ void main() { }); testWidgets('NestedScrollView and internal scrolling', (WidgetTester tester) async { - final List _tabs = ['Hello', 'World']; + const List _tabs = const ['Hello', 'World']; + int buildCount = 0; await tester.pumpWidget( new MaterialApp(home: new Material(child: // THE FOLLOWING SECTION IS FROM THE NestedScrollView DOCUMENTATION + // (EXCEPT FOR THE CHANGES TO THE buildCount COUNTER) new DefaultTabController( length: _tabs.length, // This is the number of tabs. child: new NestedScrollView( headerSliverBuilder: (BuildContext context, bool innerBoxIsScrolled) { + buildCount += 1; // THIS LINE IS NOT IN THE ORIGINAL -- ADDED FOR TEST // These are the slivers that show up in the "outer" scroll view. return [ new SliverOverlapAbsorber( @@ -449,21 +452,46 @@ void main() { // END )), ); + int expectedBuildCount = 0; + expectedBuildCount += 1; + expect(buildCount, expectedBuildCount); expect(find.text('Item 2'), findsOneWidget); + expect(find.text('Item 18'), findsNothing); expect(find.byType(NestedScrollView), isNot(paints..shadow())); // scroll down + final TestGesture gesture0 = await tester.startGesture(tester.getCenter(find.text('Item 2'))); + await gesture0.moveBy(const Offset(0.0, -120.0)); // tiny bit more than the pinned app bar height (56px * 2) + await tester.pump(); + expect(buildCount, expectedBuildCount); + expect(find.text('Item 2'), findsOneWidget); + expect(find.text('Item 18'), findsNothing); + await gesture0.up(); + await tester.pump(const Duration(milliseconds: 1)); // start shadow animation + expectedBuildCount += 1; + expect(buildCount, expectedBuildCount); + await tester.pump(const Duration(milliseconds: 1)); // during shadow animation + expect(buildCount, expectedBuildCount); + expect(find.byType(NestedScrollView), paints..shadow()); + await tester.pump(const Duration(seconds: 1)); // end shadow animation + expect(buildCount, expectedBuildCount); + expect(find.byType(NestedScrollView), paints..shadow()); + // scroll down final TestGesture gesture1 = await tester.startGesture(tester.getCenter(find.text('Item 2'))); await gesture1.moveBy(const Offset(0.0, -800.0)); - await tester.pump(); // start shadow animation + await tester.pump(); + expect(buildCount, expectedBuildCount); + expect(find.byType(NestedScrollView), paints..shadow()); expect(find.text('Item 2'), findsNothing); expect(find.text('Item 18'), findsOneWidget); await gesture1.up(); - await tester.pump(const Duration(seconds: 1)); // end shadow animation + await tester.pump(const Duration(seconds: 1)); + expect(buildCount, expectedBuildCount); expect(find.byType(NestedScrollView), paints..shadow()); // swipe left to bring in tap on the right final TestGesture gesture2 = await tester.startGesture(tester.getCenter(find.byType(NestedScrollView))); await gesture2.moveBy(const Offset(-400.0, 0.0)); await tester.pump(); + expect(buildCount, expectedBuildCount); expect(find.text('Item 18'), findsOneWidget); expect(find.text('Item 2'), findsOneWidget); expect(find.text('Item 0'), findsOneWidget); @@ -472,44 +500,61 @@ void main() { expect(find.byType(NestedScrollView), paints..shadow()); await gesture2.up(); await tester.pump(); // start sideways scroll - await tester.pump(const Duration(seconds: 1)); // end sideways scroll - await tester.pump(); // start shadow going away + await tester.pump(const Duration(seconds: 1)); // end sideways scroll, triggers shadow going away + expect(buildCount, expectedBuildCount); + await tester.pump(const Duration(seconds: 1)); // start shadow going away + expectedBuildCount += 1; + expect(buildCount, expectedBuildCount); await tester.pump(const Duration(seconds: 1)); // end shadow going away + expect(buildCount, expectedBuildCount); expect(find.text('Item 18'), findsNothing); expect(find.text('Item 2'), findsOneWidget); expect(find.byType(NestedScrollView), isNot(paints..shadow())); + await tester.pump(const Duration(seconds: 1)); // just checking we don't rebuild... + expect(buildCount, expectedBuildCount); // peek left to see it's still in the right place final TestGesture gesture3 = await tester.startGesture(tester.getCenter(find.byType(NestedScrollView))); await gesture3.moveBy(const Offset(400.0, 0.0)); await tester.pump(); // bring the left page into view + expect(buildCount, expectedBuildCount); await tester.pump(); // shadow comes back starting here + expectedBuildCount += 1; + expect(buildCount, expectedBuildCount); expect(find.text('Item 18'), findsOneWidget); expect(find.text('Item 2'), findsOneWidget); expect(find.byType(NestedScrollView), isNot(paints..shadow())); await tester.pump(const Duration(seconds: 1)); // shadow finishes coming back + expect(buildCount, expectedBuildCount); expect(find.byType(NestedScrollView), paints..shadow()); await gesture3.moveBy(const Offset(-400.0, 0.0)); await gesture3.up(); await tester.pump(); // left tab view goes away + expect(buildCount, expectedBuildCount); await tester.pump(); // shadow goes away starting here + expectedBuildCount += 1; + expect(buildCount, expectedBuildCount); expect(find.byType(NestedScrollView), paints..shadow()); await tester.pump(const Duration(seconds: 1)); // shadow finishes going away + expect(buildCount, expectedBuildCount); expect(find.byType(NestedScrollView), isNot(paints..shadow())); // scroll back up final TestGesture gesture4 = await tester.startGesture(tester.getCenter(find.byType(NestedScrollView))); await gesture4.moveBy(const Offset(0.0, 200.0)); // expands the appbar again await tester.pump(); + expect(buildCount, expectedBuildCount); expect(find.text('Item 2'), findsOneWidget); expect(find.text('Item 18'), findsNothing); expect(find.byType(NestedScrollView), isNot(paints..shadow())); await gesture4.up(); await tester.pump(const Duration(seconds: 1)); + expect(buildCount, expectedBuildCount); expect(find.byType(NestedScrollView), isNot(paints..shadow())); // peek left to see it's now back at zero final TestGesture gesture5 = await tester.startGesture(tester.getCenter(find.byType(NestedScrollView))); await gesture5.moveBy(const Offset(400.0, 0.0)); await tester.pump(); // bring the left page into view await tester.pump(); // shadow would come back starting here, but there's no shadow to show + expect(buildCount, expectedBuildCount); expect(find.text('Item 18'), findsNothing); expect(find.text('Item 2'), findsNWidgets(2)); expect(find.byType(NestedScrollView), isNot(paints..shadow())); @@ -518,6 +563,7 @@ void main() { await gesture5.up(); await tester.pump(); // right tab view goes away await tester.pumpAndSettle(); + expect(buildCount, expectedBuildCount); expect(find.byType(NestedScrollView), isNot(paints..shadow())); }); }