From 90385914b6c3971b9272c08b561bfe46c6960a31 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 25 Mar 2025 14:15:14 -0700 Subject: [PATCH 1/9] msglist [nfc]: Cut already-broken logic for slightly less padding at bottom This `i == 1` condition was never true, because in that situation the caller would build a MarkAsReadWidget instead of calling this method. This 8px vs. 11px distinction dates back to the prototype: 731b1990c made it 8px instead of 0px, and the distinction itself goes back to the commit 9916194ea msglist: Start on rendering messages in the prototype's first hours. The logic that drove it, though, became fragile with e7fe06cbc which changed it from "i == 0" (the end of the list, OK that's fairly canonical as a special value) to "i == 1" (more arbitrary). So then it naturally got broken a little later, in 56ab39576 in 2023-11, and it's been broken ever since: we just always show 11px of padding here. We might further change the layout in the future, but if we do we'll fix it forward starting from the behavior the app has already had for over a year. --- lib/widgets/message_list.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index f00f873b1a..4a5990593a 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -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)) { @@ -659,7 +659,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 +685,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); } } From 41351f93fefa6390fadd3472b8c527df9ce6e97d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 22 Mar 2025 23:29:24 -0700 Subject: [PATCH 2/9] msglist [nfc]: Introduce MessageListScrollView, not yet doing anything different --- lib/widgets/message_list.dart | 2 +- lib/widgets/scrolling.dart | 92 +++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 4a5990593a..23c821f572 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -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 diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index 7ceff8b745..ec345dbbdd 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -244,3 +244,95 @@ class RenderCustomPaintOrderViewport extends RenderViewport { }; } } + +/// 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.anchor, + 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, + anchor: anchor, + clipBehavior: clipBehavior, + paintOrder_: paintOrder_, + ); + } +} + +/// The version of [Viewport] that underlies [MessageListScrollView]. +class MessageListViewport extends CustomPaintOrderViewport { + MessageListViewport({ + super.key, + super.axisDirection, + super.crossAxisDirection, + super.anchor, + 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), + anchor: anchor, + offset: offset, + cacheExtent: cacheExtent, + cacheExtentStyle: cacheExtentStyle, + clipBehavior: clipBehavior, + paintOrder_: paintOrder_, + ); + } +} + +/// The version of [RenderViewport] that underlies [MessageListViewport] +/// and [MessageListScrollView]. +class RenderMessageListViewport extends RenderCustomPaintOrderViewport { + RenderMessageListViewport({ + super.axisDirection, + required super.crossAxisDirection, + required super.offset, + super.anchor, + super.children, + super.center, + super.cacheExtent, + super.cacheExtentStyle, + super.clipBehavior, + required super.paintOrder_, + }); +} From 019ae883f39a446fff8b7a8df5ae9c9964024d0a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 22 Mar 2025 23:36:01 -0700 Subject: [PATCH 3/9] scroll [nfc]: Copy RenderViewport.performLayout and friends from upstream Some of the behavior we'd like to customize isn't currently cleanly exposed to subclasses any more than it is to parent widgets passing constructor arguments. In particular, we'll want to change a few bits of logic in [RenderViewport.performLayout], replacing the handling of the `anchor` field with something more flexible. In order to do that, we'll start from a copy of that method, so that we can edit the copy. Then the base class's `performLayout` refers to a private helper method `_attemptLayout`, so we need a copy of that too; and they each refer to a number of private fields, so we need copies of those too; and to make those work correctly, we need copies of all the other members that refer to those fields, so that they're all referring correctly to the same version of those fields (namely the one on the subclass) rather than to a mix of the versions on the base class and those on the subclass. Fortunately, flood-filling that graph of members which refer to private members, which are referred to by other members, etc., terminates with a connected component which is... not small, but a lot smaller and less unwieldy than if we had to copy the whole upstream file these are defined in. --- lib/widgets/scrolling.dart | 212 +++++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index ec345dbbdd..cb140b8b8a 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -1,3 +1,6 @@ +import 'dart:math' as math; + +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; @@ -322,6 +325,8 @@ class MessageListViewport extends CustomPaintOrderViewport { /// 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, @@ -335,4 +340,211 @@ class RenderMessageListViewport extends RenderCustomPaintOrderViewport { super.clipBehavior, required super.paintOrder_, }); + + 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 { + if (offset.applyContentDimensions( + math.min(0.0, _minScrollExtent + mainAxisExtent * anchor), + math.max(0.0, _maxScrollExtent - mainAxisExtent * (1.0 - anchor)), + )) { + 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). + 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; + } + } + } From a51ad1c5afc40aefa17b46629228fee0461fb855 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 23 Mar 2025 00:57:41 -0700 Subject: [PATCH 4/9] msglist [nfc]: Introduce MessageListScrollPosition --- lib/widgets/message_list.dart | 2 +- lib/widgets/scrolling.dart | 37 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 23c821f572..b3fd3db535 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 diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index cb140b8b8a..984f0c7825 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -248,6 +248,43 @@ 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, + }); +} + +/// 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 From 91f7b6919400b181b1266c2ae30582d762a7b05b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 24 Mar 2025 16:41:37 -0700 Subject: [PATCH 5/9] msglist [nfc]: Force anchor to 1.0 We'll rely on this assumption to simplify some upcoming customizations. --- lib/widgets/message_list.dart | 1 - lib/widgets/scrolling.dart | 9 ++++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index b3fd3db535..473c96c91d 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -645,7 +645,6 @@ class _MessageListState extends State with PerAccountStoreAwareStat controller: scrollController, semanticChildCount: length + 2, - anchor: 1.0, center: centerSliverKey, paintOrder: SliverPaintOrder.firstIsTop, diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index 984f0c7825..ea0201e975 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -300,7 +300,6 @@ class MessageListScrollView extends CustomPaintOrderScrollView { super.scrollBehavior, // super.shrinkWrap, // omitted, always false super.center, - super.anchor, super.cacheExtent, super.slivers, super.semanticChildCount, @@ -321,7 +320,6 @@ class MessageListScrollView extends CustomPaintOrderScrollView { slivers: slivers, cacheExtent: cacheExtent, center: center, - anchor: anchor, clipBehavior: clipBehavior, paintOrder_: paintOrder_, ); @@ -334,7 +332,6 @@ class MessageListViewport extends CustomPaintOrderViewport { super.key, super.axisDirection, super.crossAxisDirection, - super.anchor, required super.offset, super.center, super.cacheExtent, @@ -350,7 +347,6 @@ class MessageListViewport extends CustomPaintOrderViewport { axisDirection: axisDirection, crossAxisDirection: crossAxisDirection ?? Viewport.getDefaultCrossAxisDirection(context, axisDirection), - anchor: anchor, offset: offset, cacheExtent: cacheExtent, cacheExtentStyle: cacheExtentStyle, @@ -369,7 +365,6 @@ class RenderMessageListViewport extends RenderCustomPaintOrderViewport { super.axisDirection, required super.crossAxisDirection, required super.offset, - super.anchor, super.children, super.center, super.cacheExtent, @@ -378,6 +373,9 @@ class RenderMessageListViewport extends RenderCustomPaintOrderViewport { required super.paintOrder_, }); + @override + double get anchor => 1.0; + double? _calculatedCacheExtent; @override @@ -499,6 +497,7 @@ class RenderMessageListViewport extends RenderCustomPaintOrderViewport { // 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, From 5b32b586c94c6956ec9e5f6a60646da77427819b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 24 Mar 2025 16:41:50 -0700 Subject: [PATCH 6/9] scroll [nfc]: Introduce applyContentDimensionsRaw on scroll position --- lib/widgets/scrolling.dart | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index ea0201e975..d5ac60073e 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -259,6 +259,25 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext { 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) { + // This makes the simplifying assumption that `anchor` is 1.0. + final effectiveMin = math.min(0.0, wholeMinScrollExtent + viewportDimension); + final effectiveMax = wholeMaxScrollExtent; + return applyContentDimensions(effectiveMin, effectiveMax); + } } /// A version of [ScrollController] adapted for the Zulip message list. @@ -449,10 +468,12 @@ class RenderMessageListViewport extends RenderCustomPaintOrderViewport { if (correction != 0.0) { offset.correctBy(correction); } else { - if (offset.applyContentDimensions( - math.min(0.0, _minScrollExtent + mainAxisExtent * anchor), - math.max(0.0, _maxScrollExtent - mainAxisExtent * (1.0 - anchor)), - )) { + // 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; } } From 60fae9974a6028f2a6e9d129c0af94b19d37e121 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 3 Apr 2025 16:55:25 -0700 Subject: [PATCH 7/9] scroll: Start out scrolled to bottom of list This is NFC as to the real message list, because so far the bottom sliver there always has height 0, so that maxScrollExtent is always 0. This is a step toward letting us move part of the message list into the bottom sliver, because it means that doing so would preserve the list's current behavior of starting out scrolled to the end. --- lib/widgets/scrolling.dart | 38 ++++++++++ test/widgets/scrolling_test.dart | 122 +++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+) diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index d5ac60073e..e55b74ca0b 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -260,6 +260,14 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext { super.debugLabel, }); + // TODO(upstream): is the lack of [absorb] a bug in [_TabBarScrollPosition]? + @override + void absorb(ScrollPosition other) { + super.absorb(other); + if (other is! MessageListScrollPosition) return; + _hasEverCompletedLayout = other._hasEverCompletedLayout; + } + /// Like [applyContentDimensions], but called without adjusting /// the arguments to subtract the viewport dimension. /// @@ -278,6 +286,36 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext { final effectiveMax = wholeMaxScrollExtent; return applyContentDimensions(effectiveMin, effectiveMax); } + + 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. + final target = maxScrollExtent; + if (!hasPixels || pixels != target) { + correctPixels(target); + changed = true; + } + } + + 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. diff --git a/test/widgets/scrolling_test.dart b/test/widgets/scrolling_test.dart index 2fb606d1dd..197b19b7f4 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 = []; @@ -124,6 +126,126 @@ void main() { // the former matched the latter's old default behavior. ); }); + + group('MessageListScrollView', () { + Widget buildList({ + MessageListScrollController? controller, + required double topHeight, + required double bottomHeight, + }) { + return 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'))), + ]); + } + + Future prepare(WidgetTester tester, { + MessageListScrollController? controller, + required double topHeight, + required double bottomHeight, + }) async { + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: buildList(controller: controller, + topHeight: topHeight, bottomHeight: bottomHeight))); + 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 -> starts scrolled to 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); + }); + + testWidgets('short/long -> starts scrolled to bottom', (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); + }); + + 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('position preserved when scrollable rebuilds', (tester) async { + // Tests that [MessageListScrollPosition.absorb] does its job. + // + // In the app, this situation can be triggered by changing the device's + // theme between light and dark. For this simplified example for a test, + // go for devicePixelRatio (which ScrollableState directly depends on). + + final controller = MessageListScrollController(); + final widget = Directionality(textDirection: TextDirection.ltr, + child: buildList(controller: controller, + topHeight: 400, bottomHeight: 400)); + await tester.pumpWidget( + MediaQuery(data: MediaQueryData(devicePixelRatio: 1.0), + child: widget)); + check(tester.getRect(findTop)).bottom.equals(200); + final position = controller.position; + check(position).isA(); + + // Drag away from the initial scroll position. + await tester.drag(findBottom, Offset(0, 200)); + await tester.pump(); + check(tester.getRect(findTop)).bottom.equals(400); + check(controller.position).identicalTo(position); + + // Then cause the ScrollableState to have didChangeDependencies called… + await tester.pumpWidget( + MediaQuery(data: MediaQueryData(devicePixelRatio: 2.0), + child: widget)); + // … so that it constructs a new MessageListScrollPosition… + check(controller.position) + ..not((it) => it.identicalTo(position)) + ..isA(); + // … and check the scroll position is preserved, not reset to initial. + check(tester.getRect(findTop)).bottom.equals(400); + }); + }); } class TestCustomPainter extends CustomPainter { From eec002dc7bfa000122a3332fcf2afa7b0de1130f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 24 Mar 2025 15:19:04 -0700 Subject: [PATCH 8/9] 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. --- lib/widgets/scrolling.dart | 45 ++++++++++++++++++++++++++++++-- test/widgets/scrolling_test.dart | 37 ++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index e55b74ca0b..388cb4671f 100644 --- a/lib/widgets/scrolling.dart +++ b/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,6 +330,8 @@ 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); @@ -304,6 +339,12 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext { } } + // 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; } diff --git a/test/widgets/scrolling_test.dart b/test/widgets/scrolling_test.dart index 197b19b7f4..74a07b3ce4 100644 --- a/test/widgets/scrolling_test.dart +++ b/test/widgets/scrolling_test.dart @@ -160,7 +160,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); @@ -169,9 +169,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); @@ -180,6 +185,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 = []; + 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 { From f20dfb7f4ee8413c10e58bb79bc433436b498091 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 24 Mar 2025 16:45:38 -0700 Subject: [PATCH 9/9] scroll: Stay at end once there This is NFC as to the real message list, because so far the bottom sliver there always has height 0, so that both maxScrollExtent and this.maxScrollExtent are always 0. This is a step toward letting us move part of the message list into the bottom sliver, because it means that doing so would preserve the list's current behavior of remaining scrolled to the end once there as e.g. new messages arrive. --- lib/widgets/scrolling.dart | 16 ++++++++++++++++ test/widgets/scrolling_test.dart | 29 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index 388cb4671f..d5a1447763 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -2,6 +2,7 @@ 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]. @@ -320,6 +321,9 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext { return applyContentDimensions(effectiveMin, effectiveMax); } + bool _nearEqual(double a, double b) => + nearEqual(a, b, Tolerance.defaultTolerance.distance); + bool _hasEverCompletedLayout = false; @override @@ -337,6 +341,13 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext { 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. @@ -345,6 +356,11 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext { // 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; } diff --git a/test/widgets/scrolling_test.dart b/test/widgets/scrolling_test.dart index 74a07b3ce4..9454ef4fe3 100644 --- a/test/widgets/scrolling_test.dart +++ b/test/widgets/scrolling_test.dart @@ -243,6 +243,35 @@ void main() { .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); + }); + testWidgets('position preserved when scrollable rebuilds', (tester) async { // Tests that [MessageListScrollPosition.absorb] does its job. //