Skip to content

Commit bf3af70

Browse files
committed
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.
1 parent 2541a10 commit bf3af70

File tree

2 files changed

+160
-0
lines changed

2 files changed

+160
-0
lines changed

lib/widgets/scrolling.dart

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,14 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext {
260260
super.debugLabel,
261261
});
262262

263+
// TODO(upstream): is the lack of [absorb] a bug in [_TabBarScrollPosition]?
264+
@override
265+
void absorb(ScrollPosition other) {
266+
super.absorb(other);
267+
if (other is! MessageListScrollPosition) return;
268+
_hasEverCompletedLayout = other._hasEverCompletedLayout;
269+
}
270+
263271
/// Like [applyContentDimensions], but called without adjusting
264272
/// the arguments to subtract the viewport dimension.
265273
///
@@ -278,6 +286,36 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext {
278286
final effectiveMax = wholeMaxScrollExtent;
279287
return applyContentDimensions(effectiveMin, effectiveMax);
280288
}
289+
290+
bool _hasEverCompletedLayout = false;
291+
292+
@override
293+
bool applyContentDimensions(double minScrollExtent, double maxScrollExtent) {
294+
// Inspired by _TabBarScrollPosition.applyContentDimensions upstream.
295+
bool changed = false;
296+
297+
if (!_hasEverCompletedLayout) {
298+
// The list is being laid out for the first time (its first performLayout).
299+
// Start out scrolled to the end.
300+
final target = maxScrollExtent;
301+
if (!hasPixels || pixels != target) {
302+
correctPixels(target);
303+
changed = true;
304+
}
305+
}
306+
307+
if (!super.applyContentDimensions(minScrollExtent, maxScrollExtent)) {
308+
changed = true;
309+
}
310+
311+
if (!changed) {
312+
// Because this method is about to return true,
313+
// this will be the last round of this layout.
314+
_hasEverCompletedLayout = true;
315+
}
316+
317+
return !changed;
318+
}
281319
}
282320

283321
/// A version of [ScrollController] adapted for the Zulip message list.

test/widgets/scrolling_test.dart

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import 'package:flutter/widgets.dart' hide SliverPaintOrder;
66
import 'package:flutter_test/flutter_test.dart';
77
import 'package:zulip/widgets/scrolling.dart';
88

9+
import '../flutter_checks.dart';
10+
911
void main() {
1012
group('CustomPaintOrderScrollView paint order', () {
1113
final paintLog = <int>[];
@@ -127,6 +129,126 @@ void main() {
127129
.deepEquals(sliverIds(result.path));
128130
});
129131
});
132+
133+
group('MessageListScrollView', () {
134+
Widget buildList({
135+
MessageListScrollController? controller,
136+
required double topHeight,
137+
required double bottomHeight,
138+
}) {
139+
return MessageListScrollView(
140+
controller: controller ?? MessageListScrollController(),
141+
center: const ValueKey('center'),
142+
slivers: [
143+
SliverToBoxAdapter(
144+
child: SizedBox(height: topHeight, child: Text('top'))),
145+
SliverToBoxAdapter(key: const ValueKey('center'),
146+
child: SizedBox(height: bottomHeight, child: Text('bottom'))),
147+
]);
148+
}
149+
150+
Future<void> prepare(WidgetTester tester, {
151+
MessageListScrollController? controller,
152+
required double topHeight,
153+
required double bottomHeight,
154+
}) async {
155+
await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr,
156+
child: buildList(controller: controller,
157+
topHeight: topHeight, bottomHeight: bottomHeight)));
158+
await tester.pump();
159+
}
160+
161+
// The `skipOffstage: false` produces more informative output
162+
// when a test fails because one of the slivers is just offscreen.
163+
final findTop = find.text('top', skipOffstage: false);
164+
final findBottom = find.text('bottom', skipOffstage: false);
165+
166+
testWidgets('short/short -> starts scrolled to bottom', (tester) async {
167+
// Starts out with items at bottom of viewport.
168+
await prepare(tester, topHeight: 100, bottomHeight: 100);
169+
check(tester.getRect(findBottom)).bottom.equals(600);
170+
171+
// Try scrolling down (by dragging up); doesn't move.
172+
await tester.drag(findTop, Offset(0, -100));
173+
await tester.pump();
174+
check(tester.getRect(findBottom)).bottom.equals(600);
175+
});
176+
177+
testWidgets('short/long -> starts scrolled to bottom', (tester) async {
178+
// Starts out scrolled to bottom.
179+
await prepare(tester, topHeight: 100, bottomHeight: 800);
180+
check(tester.getRect(findBottom)).bottom.equals(600);
181+
182+
// Try scrolling down (by dragging up); doesn't move.
183+
await tester.drag(findBottom, Offset(0, -100));
184+
await tester.pump();
185+
check(tester.getRect(findBottom)).bottom.equals(600);
186+
});
187+
188+
testWidgets('starts at bottom, even when bottom underestimated at first', (tester) async {
189+
const numItems = 10;
190+
const itemHeight = 300.0;
191+
192+
// A list where the bottom sliver takes several rounds of layout
193+
// to see how long it really is.
194+
final controller = MessageListScrollController();
195+
await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr,
196+
child: MessageListScrollView(
197+
controller: controller,
198+
center: const ValueKey('center'),
199+
slivers: [
200+
SliverToBoxAdapter(
201+
child: SizedBox(height: 100, child: Text('top'))),
202+
SliverList.list(key: const ValueKey('center'),
203+
children: List.generate(numItems, (i) =>
204+
SizedBox(height: (i+1) * itemHeight, child: Text('item $i')))),
205+
])));
206+
await tester.pump();
207+
208+
// Starts out scrolled all the way to the bottom,
209+
// even though it must have taken several rounds of layout to find that.
210+
check(controller.position.pixels)
211+
.equals(itemHeight * numItems * (numItems + 1)/2);
212+
check(tester.getRect(find.text('item ${numItems-1}', skipOffstage: false)))
213+
.bottom.equals(600);
214+
});
215+
216+
testWidgets('position preserved when scrollable rebuilds', (tester) async {
217+
// Tests that [MessageListScrollPosition.absorb] does its job.
218+
//
219+
// In the app, this situation can be triggered by changing the device's
220+
// theme between light and dark. For this simplified example for a test,
221+
// go for devicePixelRatio (which ScrollableState directly depends on).
222+
223+
final controller = MessageListScrollController();
224+
final widget = Directionality(textDirection: TextDirection.ltr,
225+
child: buildList(controller: controller,
226+
topHeight: 400, bottomHeight: 400));
227+
await tester.pumpWidget(
228+
MediaQuery(data: MediaQueryData(devicePixelRatio: 1.0),
229+
child: widget));
230+
check(tester.getRect(findTop)).bottom.equals(200);
231+
final position = controller.position;
232+
check(position).isA<MessageListScrollPosition>();
233+
234+
// Drag away from the initial scroll position.
235+
await tester.drag(findBottom, Offset(0, 200));
236+
await tester.pump();
237+
check(tester.getRect(findTop)).bottom.equals(400);
238+
check(controller.position).identicalTo(position);
239+
240+
// Then cause the ScrollableState to have didChangeDependencies called…
241+
await tester.pumpWidget(
242+
MediaQuery(data: MediaQueryData(devicePixelRatio: 2.0),
243+
child: widget));
244+
// … so that it constructs a new MessageListScrollPosition…
245+
check(controller.position)
246+
..not((it) => it.identicalTo(position))
247+
..isA<MessageListScrollPosition>();
248+
// … and check the scroll position is preserved, not reset to initial.
249+
check(tester.getRect(findTop)).bottom.equals(400);
250+
});
251+
});
130252
}
131253

132254
class TestCustomPainter extends CustomPainter {

0 commit comments

Comments
 (0)