Fix RangeMaintainingScrollPhysics (#63146)

It now uses the scroll metrics as they stood at the end of the last frame.

It previously used a weird combination of the old extents and the newish position, which led to some weird effects when the position had been changed in expectation of a viewport or content dimension change.

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

@@ -412,6 +412,45 @@ class ScrollPhysics {
 /// These physics should be combined with other scroll physics, e.g.
 /// [BouncingScrollPhysics] or [ClampingScrollPhysics], to obtain a complete
 /// description of typical scroll physics. See [applyTo].
+///
+/// ## Implementation details
+///
+/// Specifically, these physics perform two adjustments.
+///
+/// The first is to maintain overscroll when the position is out of range.
+///
+/// The second is to enforce the boundary when the position is in range.
+///
+/// If the current velocity is zero, neither adjustment is made. The
+/// assumption is that there is an ongoing animation and therefore
+/// further changing the scroll position would disrupt the experience.
+///
+/// If the extents haven't changed, then the overscroll adjustment is
+/// not made. The assumption is that if the position is overscrolled,
+/// it is intentional, otherwise the position could not have reached
+/// that position. (Consider [ClampingScrollPhysics] vs
+/// [BouncingScrollPhysics] for example.)
+///
+/// If the position itself changed since the last animation frame,
+/// then the overscroll is not maintained. The assumption is similar
+/// to the previous case: the position would not have been placed out
+/// of range unless it was intentional.
+///
+/// In addition, if the position changed and the boundaries were and
+/// still are finite, then the boundary isn't enforced either, for
+/// the same reason. However, if any of the boundaries were or are
+/// now infinite, the boundary _is_ enforced, on the assumption that
+/// infinite boundaries indicate a lazy-loading scroll view, which
+/// cannot enforce boundaries while the full list has not loaded.
+///
+/// If the range was out of range, then the boundary is not enforced
+/// even if the range is not maintained. If the range is maintained,
+/// then the distance between the old position and the old boundary is
+/// applied to the new boundary to obtain the new position.
+///
+/// If the range was in range, and the boundary is to be enforced,
+/// then the new position is obtained by deferring to the other physics,
+/// if any, and then clamped to the new range.
 class RangeMaintainingScrollPhysics extends ScrollPhysics {
   /// Creates scroll physics that maintain the scroll position in range.
   const RangeMaintainingScrollPhysics({ ScrollPhysics parent }) : super(parent: parent);
@@ -428,18 +467,60 @@ class RangeMaintainingScrollPhysics extends ScrollPhysics {
     @required bool isScrolling,
     @required double velocity,
   }) {
-    if (velocity != 0.0 || ((oldPosition.minScrollExtent == newPosition.minScrollExtent) && (oldPosition.maxScrollExtent == newPosition.maxScrollExtent))) {
-      return super.adjustPositionForNewDimensions(oldPosition: oldPosition, newPosition: newPosition, isScrolling: isScrolling, velocity: velocity);
+    bool maintainOverscroll = true;
+    bool enforceBoundary = true;
+    if (velocity != 0.0) {
+      // Don't try to adjust an animating position, the jumping around
+      // would be distracting.
+      maintainOverscroll = false;
+      enforceBoundary = false;
     }
-    if (oldPosition.pixels < oldPosition.minScrollExtent) {
-      final double oldDelta = oldPosition.minScrollExtent - oldPosition.pixels;
-      return newPosition.minScrollExtent - oldDelta;
+    if ((oldPosition.minScrollExtent == newPosition.minScrollExtent) &&
+        (oldPosition.maxScrollExtent == newPosition.maxScrollExtent)) {
+      // If the extents haven't changed then ignore overscroll.
+      maintainOverscroll = false;
     }
-    if (oldPosition.pixels > oldPosition.maxScrollExtent) {
-      final double oldDelta = oldPosition.pixels - oldPosition.maxScrollExtent;
-      return newPosition.maxScrollExtent + oldDelta;
+    if (oldPosition.pixels != newPosition.pixels) {
+      // If the position has been changed already, then it might have
+      // been adjusted to expect new overscroll, so don't try to
+      // maintain the relative overscroll.
+      maintainOverscroll = false;
+      if (oldPosition.minScrollExtent.isFinite && oldPosition.maxScrollExtent.isFinite &&
+          newPosition.minScrollExtent.isFinite && newPosition.maxScrollExtent.isFinite) {
+        // In addition, if the position changed then we only enforce
+        // the new boundary if the previous boundary was not entirely
+        // finite. A common case where the position changes while one
+        // of the extents is infinite is a lazily-loaded list. (If the
+        // boundaries were finite, and the position changed, then we
+        // assume it was intentional.)
+        enforceBoundary = false;
+      }
     }
-    return newPosition.pixels.clamp(newPosition.minScrollExtent, newPosition.maxScrollExtent) as double;
+    if ((oldPosition.pixels < oldPosition.minScrollExtent) &&
+        (oldPosition.pixels > oldPosition.maxScrollExtent)) {
+      // If the old position was out of range, then we should
+      // not try to keep the new position in range.
+      enforceBoundary = false;
+    }
+    if (maintainOverscroll) {
+      // Force the new position to be no more out of range
+      // than it was before, if it was overscrolled.
+      if (oldPosition.pixels < oldPosition.minScrollExtent) {
+        final double oldDelta = oldPosition.minScrollExtent - oldPosition.pixels;
+        return newPosition.minScrollExtent - oldDelta;
+      }
+      if (oldPosition.pixels > oldPosition.maxScrollExtent) {
+        final double oldDelta = oldPosition.pixels - oldPosition.maxScrollExtent;
+        return newPosition.maxScrollExtent + oldDelta;
+      }
+    }
+    // If we're not forcing the overscroll, defer to other physics.
+    double result = super.adjustPositionForNewDimensions(oldPosition: oldPosition, newPosition: newPosition, isScrolling: isScrolling, velocity: velocity);
+    if (enforceBoundary) {
+      // ...but if they put us out of range then reinforce the boundary.
+      result = result.clamp(newPosition.minScrollExtent, newPosition.maxScrollExtent) as double;
+    }
+    return result;
   }
 }
 

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

@@ -226,7 +226,7 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
   /// If there is any overscroll, it is reported using [didOverscrollBy].
   double setPixels(double newPixels) {
     assert(_pixels != null);
-    assert(SchedulerBinding.instance.schedulerPhase.index <= SchedulerPhase.transientCallbacks.index);
+    assert(SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks, 'A scrollable\'s position should not change during the build, layout, and paint phases, otherwise the rendering will be confused.');
     if (newPixels != pixels) {
       final double overscroll = applyBoundaryConditions(newPixels);
       assert(() {
@@ -471,27 +471,37 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
     return true;
   }
 
+  bool _pendingDimensions = false;
+  ScrollMetrics _lastMetrics;
+
   @override
   bool applyContentDimensions(double minScrollExtent, double maxScrollExtent) {
     assert(minScrollExtent != null);
     assert(maxScrollExtent != null);
+    assert(haveDimensions == (_lastMetrics != null));
     if (!nearEqual(_minScrollExtent, minScrollExtent, Tolerance.defaultTolerance.distance) ||
         !nearEqual(_maxScrollExtent, maxScrollExtent, Tolerance.defaultTolerance.distance) ||
         _didChangeViewportDimensionOrReceiveCorrection) {
       assert(minScrollExtent != null);
       assert(maxScrollExtent != null);
       assert(minScrollExtent <= maxScrollExtent);
-      final ScrollMetrics oldPosition = haveDimensions ? copyWith() : null;
       _minScrollExtent = minScrollExtent;
       _maxScrollExtent = maxScrollExtent;
-      final ScrollMetrics newPosition = haveDimensions ? copyWith() : null;
+      final ScrollMetrics currentMetrics = haveDimensions ? copyWith() : null;
       _didChangeViewportDimensionOrReceiveCorrection = false;
-      if (haveDimensions && !correctForNewDimensions(oldPosition, newPosition))
+      _pendingDimensions = true;
+      if (haveDimensions && !correctForNewDimensions(_lastMetrics, currentMetrics)) {
         return false;
+      }
       _haveDimensions = true;
+    }
+    assert(haveDimensions);
+    if (_pendingDimensions) {
       applyNewDimensions();
+      _pendingDimensions = false;
     }
     assert(!_didChangeViewportDimensionOrReceiveCorrection, 'Use correctForNewDimensions() (and return true) to change the scroll offset during applyContentDimensions().');
+    _lastMetrics = copyWith();
     return true;
   }
 
@@ -546,6 +556,7 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
   @mustCallSuper
   void applyNewDimensions() {
     assert(pixels != null);
+    assert(_pendingDimensions);
     activity.applyNewDimensions();
     _updateSemanticActions(); // will potentially request a semantics update.
   }

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

@@ -692,7 +692,7 @@ class ScrollableState extends State<Scrollable> with TickerProviderStateMixin, R
         key: _scrollSemanticsKey,
         child: result,
         position: position,
-        allowImplicitScrolling: widget?.physics?.allowImplicitScrolling ?? _physics.allowImplicitScrolling,
+        allowImplicitScrolling: _physics.allowImplicitScrolling,
         semanticChildCount: widget.semanticChildCount,
       );
     }
@@ -704,6 +704,7 @@ class ScrollableState extends State<Scrollable> with TickerProviderStateMixin, R
   void debugFillProperties(DiagnosticPropertiesBuilder properties) {
     super.debugFillProperties(properties);
     properties.add(DiagnosticsProperty<ScrollPosition>('position', position));
+    properties.add(DiagnosticsProperty<ScrollPhysics>('effective physics', _physics));
   }
 
   @override
@@ -971,7 +972,7 @@ class ScrollAction extends Action<ScrollIntent> {
     assert(state.position.viewportDimension != null);
     assert(state.position.maxScrollExtent != null);
     assert(state.position.minScrollExtent != null);
-    assert(state.widget.physics == null || state.widget.physics.shouldAcceptUserOffset(state.position));
+    assert(state._physics == null || state._physics.shouldAcceptUserOffset(state.position));
     if (state.widget.incrementCalculator != null) {
       return state.widget.incrementCalculator(
         ScrollIncrementDetails(
@@ -1060,7 +1061,7 @@ class ScrollAction extends Action<ScrollIntent> {
     assert(state.position.minScrollExtent != null);
 
     // Don't do anything if the user isn't allowed to scroll.
-    if (state.widget.physics != null && !state.widget.physics.shouldAcceptUserOffset(state.position)) {
+    if (state._physics != null && !state._physics.shouldAcceptUserOffset(state.position)) {
       return;
     }
     final double increment = _getIncrement(state, intent);

packages/flutter/test/widgets/range_maintaining_scroll_physics_test.dart

@@ -219,4 +219,45 @@ void main() {
     expect(position.maxScrollExtent, 100.0);
     expect(position.pixels, 0.0);
   });
+
+  testWidgets('expanding page views', (WidgetTester tester) async {
+    await tester.pumpWidget(Padding(padding: const EdgeInsets.only(right: 200.0), child: TabBarDemo()));
+    await tester.tap(find.text('bike'));
+    await tester.pump();
+    await tester.pump(const Duration(seconds: 1));
+    final Rect bike1 = tester.getRect(find.byIcon(Icons.directions_bike));
+    await tester.pumpWidget(Padding(padding: EdgeInsets.zero, child: TabBarDemo()));
+    final Rect bike2 = tester.getRect(find.byIcon(Icons.directions_bike));
+    expect(bike2.center, bike1.shift(const Offset(100.0, 0.0)).center);
+  });
 }
+
+class TabBarDemo extends StatelessWidget {
+  @override
+  Widget build(BuildContext context) {
+    return MaterialApp(
+      home: DefaultTabController(
+        length: 3,
+        child: Scaffold(
+          appBar: AppBar(
+            bottom: const TabBar(
+              tabs: <Widget>[
+                Tab(text: 'car'),
+                Tab(text: 'transit'),
+                Tab(text: 'bike'),
+              ],
+            ),
+            title: const Text('Tabs Demo'),
+          ),
+          body: const TabBarView(
+            children: <Widget>[
+              Icon(Icons.directions_car),
+              Icon(Icons.directions_transit),
+              Icon(Icons.directions_bike),
+            ],
+          ),
+        ),
+      ),
+    );
+  }
+}

packages/flutter/test/widgets/scroll_activity_test.dart

@@ -0,0 +1,264 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// @dart = 2.8
+
+import 'package:flutter/material.dart';
+import 'package:flutter/scheduler.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+List<Widget> children(int n) {
+  return List<Widget>.generate(n, (int i) {
+    return Container(height: 100.0, child: Text('$i'));
+  });
+}
+
+void main() {
+  testWidgets('Scrolling with list view changes', (WidgetTester tester) async {
+    final ScrollController controller = ScrollController();
+    await tester.pumpWidget(MaterialApp(home: ListView(children: children(30), controller: controller)));
+    final double thirty = controller.position.maxScrollExtent;
+    controller.jumpTo(thirty);
+    await tester.pump();
+    controller.jumpTo(thirty + 100.0); // past the end
+    await tester.pump();
+    await tester.pumpWidget(MaterialApp(home: ListView(children: children(31), controller: controller)));
+    expect(controller.position.pixels, thirty + 200.0); // same distance past the end
+    expect(await tester.pumpAndSettle(), 7); // now it goes ballistic...
+    expect(controller.position.pixels, thirty + 100.0); // and ends up at the end
+  });
+
+  testWidgets('Ability to keep a PageView at the end manually (issue 62209)', (WidgetTester tester) async {
+    await tester.pumpWidget(const MaterialApp(home: PageView62209()));
+    expect(find.text('Page 1'), findsOneWidget);
+    expect(find.text('Page 100'), findsNothing);
+    await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 2'), findsOneWidget);
+    expect(find.text('Page 100'), findsNothing);
+    await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 3'), findsOneWidget);
+    expect(find.text('Page 100'), findsNothing);
+    await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 4'), findsOneWidget);
+    expect(find.text('Page 100'), findsNothing);
+    await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 5'), findsOneWidget);
+    expect(find.text('Page 100'), findsNothing);
+    await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 5'), findsNothing);
+    expect(find.text('Page 100'), findsOneWidget);
+    await tester.tap(find.byType(FlatButton)); // 6
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 6'), findsNothing);
+    expect(find.text('Page 5'), findsNothing);
+    expect(find.text('Page 100'), findsOneWidget);
+    await tester.tap(find.byType(FlatButton)); // 7
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 6'), findsNothing);
+    expect(find.text('Page 7'), findsNothing);
+    expect(find.text('Page 5'), findsNothing);
+    expect(find.text('Page 100'), findsOneWidget);
+    await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 5'), findsOneWidget);
+    expect(find.text('Page 100'), findsNothing);
+    await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 4'), findsOneWidget);
+    expect(find.text('Page 5'), findsNothing);
+    expect(find.text('Page 100'), findsNothing);
+    await tester.tap(find.byType(FlatButton)); // 8
+    await tester.pump();
+    expect(find.text('Page 1'), findsNothing);
+    expect(find.text('Page 8'), findsNothing);
+    expect(find.text('Page 4'), findsOneWidget);
+    expect(find.text('Page 5'), findsNothing);
+    expect(find.text('Page 100'), findsNothing);
+    await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 3'), findsOneWidget);
+    await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 2'), findsOneWidget);
+    await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 6'), findsOneWidget);
+    await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 7'), findsOneWidget);
+    await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 8'), findsOneWidget);
+    await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 1'), findsOneWidget);
+    await tester.tap(find.byType(FlatButton)); // 9
+    await tester.pump();
+    expect(find.text('Page 1'), findsOneWidget);
+    expect(find.text('Page 9'), findsNothing);
+    await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0));
+    await tester.pump();
+    expect(find.text('Page 9'), findsOneWidget);
+  });
+}
+
+class PageView62209 extends StatefulWidget {
+  const PageView62209();
+
+  @override
+  _PageView62209State createState() => _PageView62209State();
+}
+
+class _PageView62209State extends State<PageView62209> {
+  int _nextPageNum = 1;
+  final List<Carousel62209Page> _pages = <Carousel62209Page>[];
+
+  @override
+  void initState() {
+    super.initState();
+    for (int i = 0; i < 5; i++) {
+      _pages.add(Carousel62209Page(
+        key: Key('$_nextPageNum'),
+        number: _nextPageNum++,
+      ));
+    }
+    _pages.add(const Carousel62209Page(number: 100));
+  }
+
+  @override
+  Widget build(BuildContext context) {
+    return Scaffold(
+      body: Column(
+        children: <Widget>[
+          Expanded(child: Carousel62209(pages: _pages)),
+          FlatButton(
+            child: const Text('ADD PAGE'),
+            onPressed: () {
+              setState(() {
+                _pages.insert(
+                  1,
+                  Carousel62209Page(
+                    key: Key('$_nextPageNum'),
+                    number: _nextPageNum++,
+                  ),
+                );
+              });
+            },
+          )
+        ],
+      ),
+    );
+  }
+}
+
+class Carousel62209Page extends StatelessWidget {
+  const Carousel62209Page({this.number, Key key}) : super(key: key);
+
+  final int number;
+
+  @override
+  Widget build(BuildContext context) {
+    return Center(child: Text('Page $number'));
+  }
+}
+
+class Carousel62209 extends StatefulWidget {
+  const Carousel62209({Key key, this.pages}) : super(key: key);
+
+  final List<Carousel62209Page> pages;
+
+  @override
+  _Carousel62209State createState() => _Carousel62209State();
+}
+
+class _Carousel62209State extends State<Carousel62209> {
+  // page variables
+  PageController _pageController;
+  int _currentPage = 0;
+
+  // controls updates outside of user interaction
+  List<Carousel62209Page> _pages;
+  bool _jumpingToPage = false;
+
+  @override
+  void initState() {
+    super.initState();
+    _pages = widget.pages.toList();
+    _pageController = PageController(initialPage: 0, keepPage: false);
+  }
+
+  @override
+  void didUpdateWidget(Carousel62209 oldWidget) {
+    super.didUpdateWidget(oldWidget);
+    if (!_jumpingToPage) {
+      int newPage = -1;
+      for (int i = 0; i < widget.pages.length; i++) {
+        if (widget.pages[i].number == _pages[_currentPage].number) {
+          newPage = i;
+        }
+      }
+      if (newPage == _currentPage) {
+        _pages = widget.pages.toList();
+      } else {
+        _jumpingToPage = true;
+        SchedulerBinding.instance.addPostFrameCallback((_) {
+          if (mounted) {
+            setState(() {
+              _pages = widget.pages.toList();
+              _currentPage = newPage;
+              _pageController.jumpToPage(_currentPage);
+              SchedulerBinding.instance.addPostFrameCallback((_) {
+                _jumpingToPage = false;
+              });
+            });
+          }
+        });
+      }
+    }
+  }
+
+  @override
+  void dispose() {
+    _pageController.dispose();
+    super.dispose();
+  }
+
+  bool _handleScrollNotification(ScrollNotification notification) {
+    if (notification is ScrollUpdateNotification) {
+      final int page = _pageController.page.round();
+      if (!_jumpingToPage && _currentPage != page) {
+        _currentPage = page;
+      }
+    }
+    return true;
+  }
+
+  @override
+  Widget build(BuildContext context) {
+    return NotificationListener<ScrollNotification>(
+      onNotification: _handleScrollNotification,
+      child: PageView.builder(
+        controller: _pageController,
+        itemCount: _pages.length,
+        itemBuilder: (BuildContext context, int index) {
+          return _pages[index];
+        },
+      ),
+    );
+  }
+}