diff --git a/packages/stream_chat_flutter/CHANGELOG.md b/packages/stream_chat_flutter/CHANGELOG.md index b24ef222b3..2220c44b44 100644 --- a/packages/stream_chat_flutter/CHANGELOG.md +++ b/packages/stream_chat_flutter/CHANGELOG.md @@ -23,6 +23,7 @@ - Fixed `StreamTypingIndicator` briefly showing typing users from a different context (main channel vs. thread) on its first frame. - Fixed the attachment picker throwing a `Tooltip` assertion error when a custom `TabbedAttachmentPickerOption` is added without a `title`; the tooltip is now only shown when a title is provided. - Fixed the `StreamBackButton` unread badge including the currently open channel in its total count. +- Fixed `StreamMessageListView` jumping several screens when selecting text in a message on desktop or web. The `ScrollablePositionedList` viewports now account for their `anchor` in `getOffsetToReveal`, so implicit reveals (`Scrollable.ensureVisible`, `RenderObject.showOnScreen`) no longer overshoot. [#2862](https://github.com/GetStream/stream-chat-flutter/issues/2862) ## 10.2.0 diff --git a/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/positioned_list.dart b/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/positioned_list.dart index 45d97036a8..a38fd0023e 100644 --- a/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/positioned_list.dart +++ b/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/positioned_list.dart @@ -13,7 +13,6 @@ import 'package:stream_chat_flutter/scrollable_positioned_list/src/indexed_key.d import 'package:stream_chat_flutter/scrollable_positioned_list/src/item_positions_listener.dart'; import 'package:stream_chat_flutter/scrollable_positioned_list/src/item_positions_notifier.dart'; import 'package:stream_chat_flutter/scrollable_positioned_list/src/scroll_view.dart'; -import 'package:stream_chat_flutter/scrollable_positioned_list/src/wrapping.dart'; /// A list of widgets similar to [ListView], except scroll control /// and position reporting is based on index rather than pixel offset. @@ -532,14 +531,6 @@ class _PositionedListState extends State { for (final element in elements) { final box = element.renderObject! as RenderBox; viewport ??= RenderAbstractViewport.of(box) as RenderViewportBase?; - var anchor = 0.0; - if (viewport is RenderViewport) { - anchor = viewport.anchor; - } - - if (viewport is CustomRenderViewport) { - anchor = viewport.anchor; - } final key = element.widget.key! as IndexedKey; // Skip this element if `box` has never been laid out, isn't @@ -560,9 +551,13 @@ class _PositionedListState extends State { if (!box.attached) continue; try { if (widget.scrollDirection == Axis.vertical) { + // `getOffsetToReveal` is anchor-aware on both viewports used + // here (see `UnboundedRenderViewport.getOffsetToReveal`), so + // the delta against the current pixels is already the item's + // painted offset from the viewport's leading edge. final reveal = viewport!.getOffsetToReveal(box, 0).offset; if (!reveal.isFinite) continue; - final itemOffset = reveal - viewport.offset.pixels + anchor * viewport.size.height; + final itemOffset = reveal - viewport.offset.pixels; positions.add( ItemPosition( index: key.index, diff --git a/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/viewport.dart b/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/viewport.dart index 54804738bc..db95b11893 100644 --- a/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/viewport.dart +++ b/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/viewport.dart @@ -341,6 +341,48 @@ class UnboundedRenderViewport extends RenderViewport { ); } + /// [RenderViewportBase.getOffsetToReveal] converts the target into the + /// viewport's *scroll offset* space and returns that value directly as the + /// `offset.pixels` to move to. That conversion is anchor-blind: it assumes + /// scroll offset 0 sits at the viewport's leading edge, whereas + /// [_attemptLayout] puts it at `mainAxisExtent * anchor - pixels`. Revealing + /// a target therefore lands it `anchor * mainAxisExtent` past where it + /// should be. + /// + /// Stock [RenderViewport] clamps `anchor` to `[0, 1]`, so the error is at + /// most one viewport. Here `anchor` is unbounded — the anchor-preservation + /// path in `ScrollablePositionedList` folds accumulated scroll pixels into + /// it, so it grows without limit as the list paginates. Any implicit reveal + /// (`Scrollable.ensureVisible`, or `RenderEditable.showOnScreen` when text + /// selection moves inside a selectable message) would then teleport the list + /// by many screens. + /// + /// Shift the result back into `pixels` space so a reveal is a no-op for a + /// target that is already at the requested alignment. + @override + RevealedOffset getOffsetToReveal( + RenderObject target, + double alignment, { + Rect? rect, + Axis? axis, + }) { + final revealed = super.getOffsetToReveal(target, alignment, rect: rect, axis: axis); + final correction = anchor * (this.axis == Axis.vertical ? size.height : size.width); + if (correction == 0 || !revealed.offset.isFinite) return revealed; + + // `super` derived `rect` from `offset.pixels - targetOffset`; moving + // `targetOffset` by `correction` moves the revealed rect by the same + // amount against the axis direction. + final revealedRect = switch (axisDirection) { + AxisDirection.up => revealed.rect.translate(0, correction), + AxisDirection.down => revealed.rect.translate(0, -correction), + AxisDirection.left => revealed.rect.translate(correction, 0), + AxisDirection.right => revealed.rect.translate(-correction, 0), + }; + + return RevealedOffset(offset: revealed.offset + correction, rect: revealedRect); + } + @override bool get hasVisualOverflow => _hasVisualOverflow; diff --git a/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/wrapping.dart b/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/wrapping.dart index e746813cae..7164fb6dfd 100644 --- a/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/wrapping.dart +++ b/packages/stream_chat_flutter/lib/scrollable_positioned_list/src/wrapping.dart @@ -142,6 +142,39 @@ class CustomRenderShrinkWrappingViewport extends CustomRenderViewport { late double _shrinkWrapExtent; + /// The main axis extent the last layout pass resolved the anchor origin + /// against. Set by [_attemptLayout]; null until the first layout. + double? _layoutMainAxisExtent; + + /// [RenderViewportBase]'s implementation resolves the target in + /// scroll-offset space and ignores [anchor], so a reveal on a list with a + /// non-zero anchor scrolls `anchor * mainAxisExtent` too far. Shift it back + /// into `pixels` space. + /// + /// `UnboundedRenderViewport.getOffsetToReveal` in `viewport.dart` carries + /// the full explanation; this is the shrink-wrapping twin. + @override + RevealedOffset getOffsetToReveal( + RenderObject target, + double alignment, { + Rect? rect, + Axis? axis, + }) { + final revealed = super.getOffsetToReveal(target, alignment, rect: rect, axis: axis); + final mainAxisExtent = _layoutMainAxisExtent ?? (this.axis == Axis.vertical ? size.height : size.width); + final correction = anchor * mainAxisExtent; + if (correction == 0 || !correction.isFinite || !revealed.offset.isFinite) return revealed; + + final revealedRect = switch (axisDirection) { + AxisDirection.up => revealed.rect.translate(0, correction), + AxisDirection.down => revealed.rect.translate(0, -correction), + AxisDirection.left => revealed.rect.translate(correction, 0), + AxisDirection.right => revealed.rect.translate(-correction, 0), + }; + + return RevealedOffset(offset: revealed.offset + correction, rect: revealedRect); + } + /// This value is set during layout based on the [CacheExtentStyle]. /// /// When the style is [CacheExtentStyle.viewport], it is the main axis extent @@ -289,6 +322,11 @@ class CustomRenderShrinkWrappingViewport extends CustomRenderViewport { ) { assert(!mainAxisExtent.isNaN, 'The maxExtent of $this has not been set.'); assert(mainAxisExtent >= 0.0, 'The maxExtent of $this is negative.'); + // Anchor origin is `mainAxisExtent * anchor`, and in a shrink-wrapping + // viewport this extent is the incoming constraint — not `size`, which + // ends up as the (smaller) shrink-wrapped content extent. Record it for + // [getOffsetToReveal]. + _layoutMainAxisExtent = mainAxisExtent; assert( crossAxisExtent.isFinite, 'The crossAxisExtent of $this is not finite.', diff --git a/packages/stream_chat_flutter/test/scrollable_positioned_list/reveal_anchor_test.dart b/packages/stream_chat_flutter/test/scrollable_positioned_list/reveal_anchor_test.dart new file mode 100644 index 0000000000..2407fcdcb7 --- /dev/null +++ b/packages/stream_chat_flutter/test/scrollable_positioned_list/reveal_anchor_test.dart @@ -0,0 +1,232 @@ +// Tests for anchor-aware `getOffsetToReveal` on the SPL viewports. +// +// `RenderViewportBase.getOffsetToReveal` resolves a target into the +// viewport's scroll-offset space and hands that value back as the +// `offset.pixels` to move to. That conversion assumes scroll offset 0 +// sits at the viewport's leading edge — true only when `anchor` is 0. +// The SPL viewports place it at `mainAxisExtent * anchor - pixels`, and +// their `anchor` is deliberately unbounded (anchor preservation folds +// accumulated scroll pixels into it as the list paginates). +// +// Anything that reveals a descendant therefore used to jump the list by +// `anchor * mainAxisExtent`. The user-visible symptom was a channel +// teleporting several screens when a message's selectable text moved the +// selection and `RenderEditable.showOnScreen` fired. +// +// See https://github.com/GetStream/stream-chat-flutter/issues/2862. + +import 'package:flutter/foundation.dart'; +import 'package:flutter/gestures.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:stream_chat_flutter/scrollable_positioned_list/scrollable_positioned_list.dart'; +// Not exported from the barrel file; imported directly to assert which +// viewport implementation `shrinkWrap: true` actually builds. +import 'package:stream_chat_flutter/scrollable_positioned_list/src/wrapping.dart'; + +const _viewportHeight = 600.0; +const _viewportWidth = 400.0; +const _itemHeight = 40.0; +const _itemCount = 500; +const _positionedIndex = 100; + +/// A large, out-of-`[0, 1]` anchor — the state the list reaches once anchor +/// preservation has folded scroll pixels into the alignment a few times. +const _anchor = 3.0; + +void main() { + Future pump( + WidgetTester tester, { + required bool reverse, + bool selectableItems = false, + bool shrinkWrap = false, + }) async { + tester.view.devicePixelRatio = 1.0; + tester.view.physicalSize = const Size(_viewportWidth, _viewportHeight); + addTearDown(tester.view.resetPhysicalSize); + addTearDown(tester.view.resetDevicePixelRatio); + + final controller = ItemScrollController(); + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: ScrollablePositionedList.builder( + itemCount: _itemCount, + reverse: reverse, + shrinkWrap: shrinkWrap, + itemScrollController: controller, + itemBuilder: (context, i) => SizedBox( + key: ValueKey('item-$i'), + height: _itemHeight, + child: selectableItems ? SelectableText('item-$i') : Text('item-$i'), + ), + ), + ), + ), + ); + await tester.pumpAndSettle(); + + controller.jumpTo(index: _positionedIndex, alignment: _anchor); + await tester.pumpAndSettle(); + } + + ScrollPosition positionOf(WidgetTester tester) => + tester.state(find.byType(Scrollable).first).position; + + /// Distance from the viewport's leading edge to [item]'s leading edge, + /// along the scroll axis. + double leadingEdgeOf(WidgetTester tester, Finder item, {required bool reverse}) { + final rect = tester.getRect(item); + return reverse ? _viewportHeight - rect.bottom : rect.top; + } + + /// An on-screen item that is a few rows in from the leading edge, so a + /// reveal is expected to scroll by a known, non-zero amount. + Finder itemInsideViewport(WidgetTester tester, {required bool reverse}) { + final candidates = + tester + .widgetList(find.byType(SizedBox)) + .map((w) => w.key) + .whereType>() + .map(find.byKey) + .where((f) { + final edge = leadingEdgeOf(tester, f, reverse: reverse); + return edge > 0 && edge < _viewportHeight - _itemHeight; + }) + .toList() + ..sort( + (a, b) => leadingEdgeOf(tester, a, reverse: reverse).compareTo(leadingEdgeOf(tester, b, reverse: reverse)), + ); + expect(candidates, isNotEmpty, reason: 'need an on-screen item to reveal'); + return candidates[candidates.length ~/ 2]; + } + + for (final reverse in [false, true]) { + group('reverse: $reverse', () { + testWidgets('ensureVisible moves by exactly the on-screen offset', (tester) async { + await pump(tester, reverse: reverse); + + final target = itemInsideViewport(tester, reverse: reverse); + final expectedDelta = leadingEdgeOf(tester, target, reverse: reverse); + final before = positionOf(tester).pixels; + + await Scrollable.ensureVisible(tester.element(target)); + await tester.pumpAndSettle(); + + expect( + positionOf(tester).pixels - before, + // `pixels` grows along the axis direction, so revealing an item + // that sits `expectedDelta` past the leading edge always moves it + // forward by that much — regardless of `reverse`. + closeTo(expectedDelta, 1), + reason: + 'reveal must not add anchor * viewportDimension ' + '(${_anchor * _viewportHeight}px) to the scroll offset', + ); + expect( + leadingEdgeOf(tester, target, reverse: reverse), + closeTo(0, 1), + reason: 'the revealed item should sit at the leading edge', + ); + }); + + testWidgets('a second ensureVisible on the same item is a no-op', (tester) async { + await pump(tester, reverse: reverse); + + final target = itemInsideViewport(tester, reverse: reverse); + await Scrollable.ensureVisible(tester.element(target)); + await tester.pumpAndSettle(); + + final settled = positionOf(tester).pixels; + await Scrollable.ensureVisible(tester.element(target)); + await tester.pumpAndSettle(); + + expect(positionOf(tester).pixels, closeTo(settled, 1)); + }); + }); + } + + // `shrinkWrap: true` swaps `UnboundedViewport` for + // `CustomShrinkWrappingViewport`, so the reveal correction runs through + // `CustomRenderShrinkWrappingViewport.getOffsetToReveal` — a separate + // implementation from the one the groups above cover. + group('shrinkWrap', () { + testWidgets('the shrink-wrapping viewport is the one under test', (tester) async { + await pump(tester, reverse: true, shrinkWrap: true); + + expect( + tester.allRenderObjects.whereType(), + isNotEmpty, + reason: 'shrinkWrap: true must build the shrink-wrapping viewport', + ); + }); + + for (final reverse in [false, true]) { + testWidgets('ensureVisible moves by exactly the on-screen offset (reverse: $reverse)', (tester) async { + await pump(tester, reverse: reverse, shrinkWrap: true); + + final target = itemInsideViewport(tester, reverse: reverse); + final expectedDelta = leadingEdgeOf(tester, target, reverse: reverse); + final before = positionOf(tester).pixels; + + await Scrollable.ensureVisible(tester.element(target)); + await tester.pumpAndSettle(); + + expect( + positionOf(tester).pixels - before, + closeTo(expectedDelta, 1), + reason: + 'reveal must not add anchor * mainAxisExtent ' + '(${_anchor * _viewportHeight}px) to the scroll offset', + ); + expect( + leadingEdgeOf(tester, target, reverse: reverse), + closeTo(0, 1), + reason: 'the revealed item should sit at the leading edge', + ); + }); + + testWidgets('a second ensureVisible on the same item is a no-op (reverse: $reverse)', (tester) async { + await pump(tester, reverse: reverse, shrinkWrap: true); + + final target = itemInsideViewport(tester, reverse: reverse); + await Scrollable.ensureVisible(tester.element(target)); + await tester.pumpAndSettle(); + + final settled = positionOf(tester).pixels; + await Scrollable.ensureVisible(tester.element(target)); + await tester.pumpAndSettle(); + + expect(positionOf(tester).pixels, closeTo(settled, 1)); + }); + } + }); + + testWidgets('selecting text in a message does not scroll the list', (tester) async { + // Reset inside the body rather than via `addTearDown`: `flutter_test` + // asserts the foundation debug vars are unset at the end of the test + // body, which runs before any tear-down. + debugDefaultTargetPlatformOverride = TargetPlatform.macOS; + try { + await pump(tester, reverse: true, selectableItems: true); + + final target = itemInsideViewport(tester, reverse: true); + final before = positionOf(tester).pixels; + + // Drag-select across the text. On macOS this reports + // `SelectionChangedCause.drag`, which makes `EditableText` call + // `bringIntoView` -> `RenderEditable.showOnScreen`. + final start = tester.getCenter(target) - const Offset(30, 0); + final gesture = await tester.startGesture(start, kind: PointerDeviceKind.mouse); + await tester.pump(const Duration(milliseconds: 50)); + await gesture.moveTo(start + const Offset(50, 0)); + await tester.pumpAndSettle(); + await gesture.up(); + await tester.pumpAndSettle(); + + expect(positionOf(tester).pixels, closeTo(before, 1)); + } finally { + debugDefaultTargetPlatformOverride = null; + } + }); +}