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

packages/flutter/lib/src/widgets/nested_scroll_view.dart

@@ -313,10 +313,21 @@ class _NestedScrollViewState extends State<NestedScrollView> {
     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<NestedScrollView> {
       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<NestedScrollView> {
             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();

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<String> _tabs = <String>['Hello', 'World'];
+    const List<String> _tabs = const <String>['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 <Widget>[
                 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()));
   });
 }