diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 1e95a1be49..d9e410dc06 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -466,7 +466,7 @@ class MessageList extends StatefulWidget { class _MessageListState extends State with PerAccountStoreAwareStateMixin { MessageListView? model; - final ScrollController scrollController = ScrollController(); + final ScrollController scrollController = MessageListScrollController(); final ValueNotifier _scrollToBottomVisibleValue = ValueNotifier(false); @override @@ -622,7 +622,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat if (i == 2) return TypingStatusWidget(narrow: widget.narrow); final data = model!.items[length - 1 - (i - 3)]; - return _buildItem(zulipLocalizations, data, i); + return _buildItem(zulipLocalizations, data); })); if (!ComposeBox.hasComposeBox(widget.narrow)) { @@ -631,7 +631,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat sliver = SliverSafeArea(sliver: sliver); } - return CustomPaintOrderScrollView( + return MessageListScrollView( // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or // similar) if that is ever offered: // https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849 @@ -645,7 +645,6 @@ class _MessageListState extends State with PerAccountStoreAwareStat controller: scrollController, semanticChildCount: length + 2, - anchor: 1.0, center: centerSliverKey, paintOrder: SliverPaintOrder.firstIsTop, @@ -659,7 +658,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat ]); } - Widget _buildItem(ZulipLocalizations zulipLocalizations, MessageListItem data, int i) { + Widget _buildItem(ZulipLocalizations zulipLocalizations, MessageListItem data) { switch (data) { case MessageListHistoryStartItem(): return Center( @@ -685,7 +684,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat return MessageItem( key: ValueKey(data.message.id), header: header, - trailingWhitespace: i == 1 ? 8 : 11, + trailingWhitespace: 11, item: data); } } diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index 7ceff8b745..b8ea5c77af 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -1,4 +1,8 @@ +import 'dart:math' as math; + +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; +import 'package:flutter/physics.dart'; import 'package:flutter/rendering.dart'; /// A [SingleChildScrollView] that always shows a Material [Scrollbar]. @@ -244,3 +248,447 @@ class RenderCustomPaintOrderViewport extends RenderViewport { }; } } + +/// A version of [ScrollPosition] adapted for the Zulip message list, +/// used by [MessageListScrollController]. +class MessageListScrollPosition extends ScrollPositionWithSingleContext { + MessageListScrollPosition({ + required super.physics, + required super.context, + super.initialPixels, + super.keepScrollOffset, + super.oldPosition, + super.debugLabel, + }); + + /// Like [applyContentDimensions], but called without adjusting + /// the arguments to subtract the viewport dimension. + /// + /// For instance, if there is 100.0 pixels of scrollable content + /// of which 40.0 pixels is in the reverse-growing slivers and + /// 60.0 pixels in the forward-growing slivers, then the arguments + /// will be -40.0 and 60.0, regardless of the viewport dimension. + /// + /// By contrast in a call to [applyContentDimensions], in this example and + /// if the viewport dimension is 80.0, then the arguments might be + /// 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) { + // 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); + } + + bool _nearEqual(double a, double b) => + nearEqual(a, b, Tolerance.defaultTolerance.distance); + + bool _hasEverCompletedLayout = false; + + @override + bool applyContentDimensions(double minScrollExtent, double maxScrollExtent) { + // Inspired by _TabBarScrollPosition.applyContentDimensions upstream. + bool changed = false; + + 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; + } + } else if (_nearEqual(pixels, this.maxScrollExtent) + && !_nearEqual(pixels, maxScrollExtent)) { + // The list was scrolled to the end before this layout round. + // Make sure it stays at the end. + // (For example, show the new message that just arrived.) + correctPixels(maxScrollExtent); + 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. + // + // For the "stays at the end" correction, it's not clear if the order + // matters in practice. But the doc on [applyNewDimensions], called by + // the base [applyContentDimensions], says it should come after any + // calls to [correctPixels]; so OK, do this after the [correctPixels]. + if (!super.applyContentDimensions(minScrollExtent, maxScrollExtent)) { + changed = true; + } + + if (!changed) { + // Because this method is about to return true, + // this will be the last round of this layout. + _hasEverCompletedLayout = true; + } + + return !changed; + } +} + +/// A version of [ScrollController] adapted for the Zulip message list. +class MessageListScrollController extends ScrollController { + MessageListScrollController({ + super.initialScrollOffset, + super.keepScrollOffset, + super.debugLabel, + super.onAttach, + super.onDetach, + }); + + @override + ScrollPosition createScrollPosition(ScrollPhysics physics, + ScrollContext context, ScrollPosition? oldPosition) { + return MessageListScrollPosition( + physics: physics, + context: context, + initialPixels: initialScrollOffset, + keepScrollOffset: keepScrollOffset, + oldPosition: oldPosition, + debugLabel: debugLabel, + ); + } +} + +/// A version of [CustomScrollView] adapted for the Zulip message list. +/// +/// This lets us customize behavior in ways that aren't currently supported +/// by the fields of [CustomScrollView] itself. +class MessageListScrollView extends CustomPaintOrderScrollView { + const MessageListScrollView({ + super.key, + super.scrollDirection, + super.reverse, + super.controller, + super.primary, + super.physics, + super.scrollBehavior, + // super.shrinkWrap, // omitted, always false + super.center, + super.cacheExtent, + super.slivers, + super.semanticChildCount, + super.dragStartBehavior, + super.keyboardDismissBehavior, + super.restorationId, + super.clipBehavior, + super.hitTestBehavior, + super.paintOrder, + }); + + @override + Widget buildViewport(BuildContext context, ViewportOffset offset, + AxisDirection axisDirection, List slivers) { + return MessageListViewport( + axisDirection: axisDirection, + offset: offset, + slivers: slivers, + cacheExtent: cacheExtent, + center: center, + clipBehavior: clipBehavior, + paintOrder_: paintOrder_, + ); + } +} + +/// The version of [Viewport] that underlies [MessageListScrollView]. +class MessageListViewport extends CustomPaintOrderViewport { + MessageListViewport({ + super.key, + super.axisDirection, + super.crossAxisDirection, + required super.offset, + super.center, + super.cacheExtent, + super.cacheExtentStyle, + super.slivers, + super.clipBehavior, + required super.paintOrder_, + }); + + @override + RenderViewport createRenderObject(BuildContext context) { + return RenderMessageListViewport( + axisDirection: axisDirection, + crossAxisDirection: crossAxisDirection + ?? Viewport.getDefaultCrossAxisDirection(context, axisDirection), + offset: offset, + cacheExtent: cacheExtent, + cacheExtentStyle: cacheExtentStyle, + clipBehavior: clipBehavior, + paintOrder_: paintOrder_, + ); + } +} + +/// The version of [RenderViewport] that underlies [MessageListViewport] +/// and [MessageListScrollView]. +// TODO(upstream): Devise upstream APIs to obviate the duplicated code here; +// use `git log -L` to see what edits we've made locally. +class RenderMessageListViewport extends RenderCustomPaintOrderViewport { + RenderMessageListViewport({ + super.axisDirection, + required super.crossAxisDirection, + required super.offset, + super.children, + super.center, + super.cacheExtent, + super.cacheExtentStyle, + super.clipBehavior, + required super.paintOrder_, + }); + + @override + double get anchor => 1.0; + + double? _calculatedCacheExtent; + + @override + Rect describeSemanticsClip(RenderSliver? child) { + if (_calculatedCacheExtent == null) { + return semanticBounds; + } + + switch (axis) { + case Axis.vertical: + return Rect.fromLTRB( + semanticBounds.left, + semanticBounds.top - _calculatedCacheExtent!, + semanticBounds.right, + semanticBounds.bottom + _calculatedCacheExtent!, + ); + case Axis.horizontal: + return Rect.fromLTRB( + semanticBounds.left - _calculatedCacheExtent!, + semanticBounds.top, + semanticBounds.right + _calculatedCacheExtent!, + semanticBounds.bottom, + ); + } + } + + static const int _maxLayoutCyclesPerChild = 10; + + // Out-of-band data computed during layout. + late double _minScrollExtent; + late double _maxScrollExtent; + bool _hasVisualOverflow = false; + + @override + void performLayout() { + // Ignore the return value of applyViewportDimension because we are + // doing a layout regardless. + switch (axis) { + case Axis.vertical: + offset.applyViewportDimension(size.height); + case Axis.horizontal: + offset.applyViewportDimension(size.width); + } + + if (center == null) { + assert(firstChild == null); + _minScrollExtent = 0.0; + _maxScrollExtent = 0.0; + _hasVisualOverflow = false; + offset.applyContentDimensions(0.0, 0.0); + return; + } + assert(center!.parent == this); + + final (double mainAxisExtent, double crossAxisExtent) = switch (axis) { + Axis.vertical => (size.height, size.width), + Axis.horizontal => (size.width, size.height), + }; + + final double centerOffsetAdjustment = center!.centerOffsetAdjustment; + final int maxLayoutCycles = _maxLayoutCyclesPerChild * childCount; + + double correction; + int count = 0; + do { + correction = _attemptLayout( + mainAxisExtent, + crossAxisExtent, + offset.pixels + centerOffsetAdjustment, + ); + if (correction != 0.0) { + offset.correctBy(correction); + } else { + // TODO(upstream): Move applyContentDimensionsRaw to ViewportOffset + // (possibly with an API change to tell it [anchor]?); + // give it a default implementation calling applyContentDimensions; + // have RenderViewport.performLayout call it. + if ((offset as MessageListScrollPosition) + .applyContentDimensionsRaw(_minScrollExtent, _maxScrollExtent)) { + break; + } + } + count += 1; + } while (count < maxLayoutCycles); + assert(() { + if (count >= maxLayoutCycles) { + assert(count != 1); + throw FlutterError( + 'A RenderViewport exceeded its maximum number of layout cycles.\n' + 'RenderViewport render objects, during layout, can retry if either their ' + 'slivers or their ViewportOffset decide that the offset should be corrected ' + 'to take into account information collected during that layout.\n' + 'In the case of this RenderViewport object, however, this happened $count ' + 'times and still there was no consensus on the scroll offset. This usually ' + 'indicates a bug. Specifically, it means that one of the following three ' + 'problems is being experienced by the RenderViewport object:\n' + ' * One of the RenderSliver children or the ViewportOffset have a bug such' + ' that they always think that they need to correct the offset regardless.\n' + ' * Some combination of the RenderSliver children and the ViewportOffset' + ' have a bad interaction such that one applies a correction then another' + ' applies a reverse correction, leading to an infinite loop of corrections.\n' + ' * There is a pathological case that would eventually resolve, but it is' + ' so complicated that it cannot be resolved in any reasonable number of' + ' layout passes.', + ); + } + return true; + }()); + } + + double _attemptLayout(double mainAxisExtent, double crossAxisExtent, double correctedOffset) { + assert(!mainAxisExtent.isNaN); + assert(mainAxisExtent >= 0.0); + assert(crossAxisExtent.isFinite); + assert(crossAxisExtent >= 0.0); + assert(correctedOffset.isFinite); + _minScrollExtent = 0.0; + _maxScrollExtent = 0.0; + _hasVisualOverflow = false; + + // centerOffset is the offset from the leading edge of the RenderViewport + // to the zero scroll offset (the line between the forward slivers and the + // reverse slivers). + assert(anchor == 1.0); + final double centerOffset = mainAxisExtent * anchor - correctedOffset; + final double reverseDirectionRemainingPaintExtent = clampDouble( + centerOffset, + 0.0, + mainAxisExtent, + ); + final double forwardDirectionRemainingPaintExtent = clampDouble( + mainAxisExtent - centerOffset, + 0.0, + mainAxisExtent, + ); + + _calculatedCacheExtent = switch (cacheExtentStyle) { + CacheExtentStyle.pixel => cacheExtent, + CacheExtentStyle.viewport => mainAxisExtent * cacheExtent!, + }; + + final double fullCacheExtent = mainAxisExtent + 2 * _calculatedCacheExtent!; + final double centerCacheOffset = centerOffset + _calculatedCacheExtent!; + final double reverseDirectionRemainingCacheExtent = clampDouble( + centerCacheOffset, + 0.0, + fullCacheExtent, + ); + final double forwardDirectionRemainingCacheExtent = clampDouble( + fullCacheExtent - centerCacheOffset, + 0.0, + fullCacheExtent, + ); + + final RenderSliver? leadingNegativeChild = childBefore(center!); + + if (leadingNegativeChild != null) { + // negative scroll offsets + final double result = layoutChildSequence( + child: leadingNegativeChild, + scrollOffset: math.max(mainAxisExtent, centerOffset) - mainAxisExtent, + overlap: 0.0, + layoutOffset: forwardDirectionRemainingPaintExtent, + remainingPaintExtent: reverseDirectionRemainingPaintExtent, + mainAxisExtent: mainAxisExtent, + crossAxisExtent: crossAxisExtent, + growthDirection: GrowthDirection.reverse, + advance: childBefore, + remainingCacheExtent: reverseDirectionRemainingCacheExtent, + cacheOrigin: clampDouble(mainAxisExtent - centerOffset, -_calculatedCacheExtent!, 0.0), + ); + if (result != 0.0) { + return -result; + } + } + + // positive scroll offsets + return layoutChildSequence( + child: center, + scrollOffset: math.max(0.0, -centerOffset), + overlap: leadingNegativeChild == null ? math.min(0.0, -centerOffset) : 0.0, + layoutOffset: + centerOffset >= mainAxisExtent ? centerOffset : reverseDirectionRemainingPaintExtent, + remainingPaintExtent: forwardDirectionRemainingPaintExtent, + mainAxisExtent: mainAxisExtent, + crossAxisExtent: crossAxisExtent, + growthDirection: GrowthDirection.forward, + advance: childAfter, + remainingCacheExtent: forwardDirectionRemainingCacheExtent, + cacheOrigin: clampDouble(centerOffset, -_calculatedCacheExtent!, 0.0), + ); + } + + @override + bool get hasVisualOverflow => _hasVisualOverflow; + + @override + void updateOutOfBandData(GrowthDirection growthDirection, SliverGeometry childLayoutGeometry) { + switch (growthDirection) { + case GrowthDirection.forward: + _maxScrollExtent += childLayoutGeometry.scrollExtent; + case GrowthDirection.reverse: + _minScrollExtent -= childLayoutGeometry.scrollExtent; + } + if (childLayoutGeometry.hasVisualOverflow) { + _hasVisualOverflow = true; + } + } + +} diff --git a/test/widgets/scrolling_test.dart b/test/widgets/scrolling_test.dart index bfb010ccf0..2b74bd7fb5 100644 --- a/test/widgets/scrolling_test.dart +++ b/test/widgets/scrolling_test.dart @@ -6,6 +6,8 @@ import 'package:flutter/widgets.dart' hide SliverPaintOrder; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/widgets/scrolling.dart'; +import '../flutter_checks.dart'; + void main() { group('CustomPaintOrderScrollView paint order', () { final paintLog = []; @@ -127,6 +129,143 @@ void main() { .deepEquals(sliverIds(result.path)); }); }); + + group('MessageListScrollView', () { + Future prepare(WidgetTester tester, { + MessageListScrollController? controller, + required double topHeight, + required double bottomHeight, + }) async { + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: MessageListScrollView( + controller: controller ?? MessageListScrollController(), + center: const ValueKey('center'), + slivers: [ + SliverToBoxAdapter( + child: SizedBox(height: topHeight, child: Text('top'))), + SliverToBoxAdapter(key: const ValueKey('center'), + child: SizedBox(height: bottomHeight, child: Text('bottom'))), + ]))); + await tester.pump(); + } + + // The `skipOffstage: false` produces more informative output + // when a test fails because one of the slivers is just offscreen. + final findTop = find.text('top', skipOffstage: false); + final findBottom = find.text('bottom', skipOffstage: false); + + 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); + + // Try scrolling down (by dragging up); doesn't move. + 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 -> 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); + + // Try scrolling down (by dragging up); doesn't move. + 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 = []; + 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 = []; + 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 { + const numItems = 10; + const itemHeight = 300.0; + + // A list where the bottom sliver takes several rounds of layout + // to see how long it really is. + final controller = MessageListScrollController(); + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: MessageListScrollView( + controller: controller, + center: const ValueKey('center'), + slivers: [ + SliverToBoxAdapter( + child: SizedBox(height: 100, child: Text('top'))), + SliverList.list(key: const ValueKey('center'), + children: List.generate(numItems, (i) => + SizedBox(height: (i+1) * itemHeight, child: Text('item $i')))), + ]))); + await tester.pump(); + + // Starts out scrolled all the way to the bottom, + // even though it must have taken several rounds of layout to find that. + check(controller.position.pixels) + .equals(itemHeight * numItems * (numItems + 1)/2); + check(tester.getRect(find.text('item ${numItems-1}', skipOffstage: false))) + .bottom.equals(600); + }); + + testWidgets('stick to end of list when it grows', (tester) async { + final controller = MessageListScrollController(); + await prepare(tester, controller: controller, + topHeight: 400, bottomHeight: 400); + check(tester.getRect(findBottom))..top.equals(200)..bottom.equals(600); + + // Bottom sliver grows; remain scrolled to (new) bottom. + await prepare(tester, controller: controller, + topHeight: 400, bottomHeight: 500); + check(tester.getRect(findBottom))..top.equals(100)..bottom.equals(600); + }); + + testWidgets('when not at end, let it grow without following', (tester) async { + final controller = MessageListScrollController(); + await prepare(tester, controller: controller, + topHeight: 400, bottomHeight: 400); + check(tester.getRect(findBottom))..top.equals(200)..bottom.equals(600); + + // Scroll up (by dragging down) to detach from end of list. + await tester.drag(findBottom, Offset(0, 100)); + await tester.pump(); + check(tester.getRect(findBottom))..top.equals(300)..bottom.equals(700); + + // Bottom sliver grows; remain at existing position, now farther from end. + await prepare(tester, controller: controller, + topHeight: 400, bottomHeight: 500); + check(tester.getRect(findBottom))..top.equals(300)..bottom.equals(800); + }); + }); } class TestCustomPainter extends CustomPainter {