scroll: Keep short list pinned at bottom of viewport, not past bottom

This is NFC as to the real message list, because so far the bottom
sliver there always has height 0.

Before this change, the user could always scroll up (moving the
content down) so that the bottom sliver was entirely off the bottom
of the viewport, even if that exposed blank space at the top of the
viewport because the top sliver was shorter than the viewport.
After this change, it's never in bounds to have part of the viewport
be blank for lack of content while there's content scrolled out of
the viewport at the other end.

This is a step toward letting us move part of the message list into
the bottom sliver, because it fixes a bug that would otherwise create
in the case where the top sliver fits entirely on the screen.

Author: gnprice

lib/widgets/scrolling.dart

@@ -281,9 +281,42 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext {
   /// 0.0 and 60.0, or -10.0 and 10.0, or -40.0 and 0.0, or other values,
   /// depending on the value of [Viewport.anchor].
   bool applyContentDimensionsRaw(double wholeMinScrollExtent, double wholeMaxScrollExtent) {
-    // This makes the simplifying assumption that `anchor` is 1.0.
-    final effectiveMin = math.min(0.0, wholeMinScrollExtent + viewportDimension);
+    // The origin point of these scroll coordinates, scroll extent 0.0,
+    // is that the boundary between slivers is the bottom edge of the viewport.
+    // (That's expressed by setting `anchor` to 1.0, consulted in
+    // `_attemptLayout` below.)
+
+    // The farthest the list can scroll down (moving the content up)
+    // is to the point where the bottom end of the list
+    // touches the bottom edge of the viewport.
     final effectiveMax = wholeMaxScrollExtent;
+
+    // The farthest the list can scroll up (moving the content down)
+    // is either:
+    //   * the same as the farthest it can scroll down,
+    //   * or the point where the top end of the list
+    //     touches the top edge of the viewport,
+    // whichever is farther up.
+    final effectiveMin = math.min(effectiveMax,
+      wholeMinScrollExtent + viewportDimension);
+
+    // The first point comes into effect when the list is short,
+    // so the whole thing fits into the viewport.  In that case,
+    // the only scroll position allowed is with the bottom end of the list
+    // at the bottom edge of the viewport.
+
+    // The upstream answer (with no `applyContentDimensionsRaw`) would
+    // effectively say:
+    //   final effectiveMin = math.min(0.0,
+    //     wholeMinScrollExtent + viewportDimension);
+    //
+    // In other words, the farthest the list can scroll up might be farther up
+    // than the answer here: it could always scroll up to 0.0, meaning that the
+    // boundary between slivers is at the bottom edge of the viewport.
+    // Whenever the top sliver is shorter than the viewport (and the bottom
+    // sliver isn't empty), this would mean one can scroll up past
+    // the top of the list, even though that scrolls other content offscreen.
+
     return applyContentDimensions(effectiveMin, effectiveMax);
   }
 
@@ -297,13 +330,21 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext {
     if (!_hasEverCompletedLayout) {
       // The list is being laid out for the first time (its first performLayout).
       // Start out scrolled to the end.
+      // This also brings [pixels] within bounds, which
+      // the initial value of 0.0 might not have been.
       final target = maxScrollExtent;
       if (!hasPixels || pixels != target) {
         correctPixels(target);
         changed = true;
       }
     }
 
+    // This step must come after the first-time correction above.
+    // Otherwise, if the initial [pixels] value of 0.0 was out of bounds
+    // (which happens if the top slivers are shorter than the viewport),
+    // then the base implementation of [applyContentDimensions] would
+    // bring it in bounds via a scrolling animation, which isn't right when
+    // starting from the meaningless initial 0.0 value.
     if (!super.applyContentDimensions(minScrollExtent, maxScrollExtent)) {
       changed = true;
     }

test/widgets/scrolling_test.dart

@@ -163,7 +163,7 @@ void main() {
     final findTop = find.text('top', skipOffstage: false);
     final findBottom = find.text('bottom', skipOffstage: false);
 
-    testWidgets('short/short -> starts scrolled to bottom', (tester) async {
+    testWidgets('short/short -> pinned at bottom', (tester) async {
       // Starts out with items at bottom of viewport.
       await prepare(tester, topHeight: 100, bottomHeight: 100);
       check(tester.getRect(findBottom)).bottom.equals(600);
@@ -172,9 +172,14 @@ void main() {
       await tester.drag(findTop, Offset(0, -100));
       await tester.pump();
       check(tester.getRect(findBottom)).bottom.equals(600);
+
+      // Try scrolling up (by dragging down); doesn't move.
+      await tester.drag(findTop, Offset(0, 100));
+      await tester.pump();
+      check(tester.getRect(findBottom)).bottom.equals(600);
     });
 
-    testWidgets('short/long -> starts scrolled to bottom', (tester) async {
+    testWidgets('short/long -> scrolls to ends and no farther', (tester) async {
       // Starts out scrolled to bottom.
       await prepare(tester, topHeight: 100, bottomHeight: 800);
       check(tester.getRect(findBottom)).bottom.equals(600);
@@ -183,6 +188,34 @@ void main() {
       await tester.drag(findBottom, Offset(0, -100));
       await tester.pump();
       check(tester.getRect(findBottom)).bottom.equals(600);
+
+      // Try scrolling up (by dragging down); moves only as far as top of list.
+      await tester.drag(findBottom, Offset(0, 400));
+      await tester.pump();
+      check(tester.getRect(findBottom)).bottom.equals(900);
+      check(tester.getRect(findTop)).top.equals(0);
+    });
+
+    testWidgets('short/short -> starts at bottom, immediately without animation', (tester) async {
+      await prepare(tester, topHeight: 100, bottomHeight: 100);
+
+      final ys = <double>[];
+      for (int i = 0; i < 10; i++) {
+        ys.add(tester.getRect(findBottom).bottom - 600);
+        await tester.pump(Duration(milliseconds: 15));
+      }
+      check(ys).deepEquals(List.generate(10, (_) => 0.0));
+    });
+
+    testWidgets('short/long -> starts at bottom, immediately without animation', (tester) async {
+      await prepare(tester, topHeight: 100, bottomHeight: 800);
+
+      final ys = <double>[];
+      for (int i = 0; i < 10; i++) {
+        ys.add(tester.getRect(findBottom).bottom - 600);
+        await tester.pump(Duration(milliseconds: 15));
+      }
+      check(ys).deepEquals(List.generate(10, (_) => 0.0));
     });
 
     testWidgets('starts at bottom, even when bottom underestimated at first', (tester) async {