From 96bdc7dec036bad06c28411cf01f035c9b596751 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 15:35:09 -0800 Subject: [PATCH 01/19] sticky_header [nfc]: Document SliverStickyHeaderList This is the class we actually use at this point -- not StickyHeaderListView -- so it's good for it to have some docs too. --- lib/widgets/sticky_header.dart | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index 24eeb8d518..9dd0183920 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -99,6 +99,18 @@ class RenderStickyHeaderItem extends RenderProxyBox { /// or if [scrollDirection] is horizontal then to the start in the /// reading direction of the ambient [Directionality]. /// It can be controlled with [reverseHeader]. +/// +/// Much like [ListView], a [StickyHeaderListView] is basically +/// a [CustomScrollView] with a single sliver in its [CustomScrollView.slivers] +/// property. +/// For a [StickyHeaderListView], that sliver is a [SliverStickyHeaderList]. +/// +/// If more than one sliver is needed, any code using [StickyHeaderListView] +/// can be ported to use [CustomScrollView] directly, in much the same way +/// as for code using [ListView]. See [ListView] for details. +/// +/// See also: +/// * [SliverStickyHeaderList], which provides the sticky-header behavior. class StickyHeaderListView extends BoxScrollView { // Like ListView, but with sticky headers. StickyHeaderListView({ @@ -296,6 +308,10 @@ enum _HeaderGrowthPlacement { growthEnd } +/// A list sliver with sticky headers. +/// +/// This widget takes most of its behavior from [SliverList], +/// but adds sticky headers as described at [StickyHeaderListView]. class SliverStickyHeaderList extends RenderObjectWidget { SliverStickyHeaderList({ super.key, @@ -306,7 +322,16 @@ class SliverStickyHeaderList extends RenderObjectWidget { delegate: delegate, ); + /// Whether the sticky header appears at the start or the end + /// in the scrolling direction. + /// + /// For example, if the enclosing [Viewport] has [Viewport.axisDirection] + /// of [AxisDirection.down], then + /// [HeaderPlacement.scrollingStart] means the header appears at + /// the top of the viewport, and + /// [HeaderPlacement.scrollingEnd] means it appears at the bottom. final HeaderPlacement headerPlacement; + final _SliverStickyHeaderListInner _child; @override From 41c410cefefe167b0e4cea215473841fbff62f3b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 30 Jan 2025 18:29:54 -0800 Subject: [PATCH 02/19] sticky_header example: Enable ink splashes, to demo hit-testing --- lib/example/sticky_header.dart | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/example/sticky_header.dart b/lib/example/sticky_header.dart index 3fa2185c1a..feeb3181db 100644 --- a/lib/example/sticky_header.dart +++ b/lib/example/sticky_header.dart @@ -197,6 +197,7 @@ class WideHeader extends StatelessWidget { return Material( color: Theme.of(context).colorScheme.primaryContainer, child: ListTile( + onTap: () {}, // nop, but non-null so the ink splash appears title: Text("Section ${i + 1}", style: TextStyle( color: Theme.of(context).colorScheme.onPrimaryContainer)))); @@ -211,7 +212,9 @@ class WideItem extends StatelessWidget { @override Widget build(BuildContext context) { - return ListTile(title: Text("Item ${i + 1}.${j + 1}")); + return ListTile( + onTap: () {}, // nop, but non-null so the ink splash appears + title: Text("Item ${i + 1}.${j + 1}")); } } From 16308e535a72269af798e87c471a14db131ec4df Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 23 Jan 2025 21:43:32 -0800 Subject: [PATCH 03/19] sticky_header example: Set allowOverflow true in double-sliver example --- lib/example/sticky_header.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/example/sticky_header.dart b/lib/example/sticky_header.dart index feeb3181db..a7b6e4167f 100644 --- a/lib/example/sticky_header.dart +++ b/lib/example/sticky_header.dart @@ -152,6 +152,7 @@ class ExampleVerticalDouble extends StatelessWidget { (context, i) { final ii = i + numBottomSections; return StickyHeaderItem( + allowOverflow: true, header: WideHeader(i: ii), child: Column( children: List.generate(numPerSection + 1, (j) { @@ -167,6 +168,7 @@ class ExampleVerticalDouble extends StatelessWidget { (context, i) { final ii = numBottomSections - 1 - i; return StickyHeaderItem( + allowOverflow: true, header: WideHeader(i: ii), child: Column( children: List.generate(numPerSection + 1, (j) { From 4ab8121a7918eba5e9b1d4fe4fdf749b59f02cdc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 16:43:26 -0800 Subject: [PATCH 04/19] sticky_header example: Make double slivers not back-to-back This way the example can be used to demonstrate the next cluster of bug fixes working correctly, before yet fixing the next issue after those. --- lib/example/sticky_header.dart | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/example/sticky_header.dart b/lib/example/sticky_header.dart index a7b6e4167f..1b55d04016 100644 --- a/lib/example/sticky_header.dart +++ b/lib/example/sticky_header.dart @@ -134,23 +134,20 @@ class ExampleVerticalDouble extends StatelessWidget { @override Widget build(BuildContext context) { - const centerSliverKey = ValueKey('center sliver'); - const numSections = 100; + const numSections = 4; const numBottomSections = 2; const numPerSection = 10; return Scaffold( appBar: AppBar(title: Text(title)), body: CustomScrollView( semanticChildCount: numSections, - anchor: 0.5, - center: centerSliverKey, slivers: [ SliverStickyHeaderList( headerPlacement: HeaderPlacement.scrollingStart, delegate: SliverChildBuilderDelegate( childCount: numSections - numBottomSections, (context, i) { - final ii = i + numBottomSections; + final ii = numSections - 1 - i; return StickyHeaderItem( allowOverflow: true, header: WideHeader(i: ii), @@ -161,7 +158,6 @@ class ExampleVerticalDouble extends StatelessWidget { }))); })), SliverStickyHeaderList( - key: centerSliverKey, headerPlacement: HeaderPlacement.scrollingStart, delegate: SliverChildBuilderDelegate( childCount: numBottomSections, From ca394c0fdfaa60342bd55fd1e7968c6ea87b55b9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 13:50:26 -0800 Subject: [PATCH 05/19] sticky_header test: Favor drag gestures over taps, when they compete This is nearly NFC. The sense in which it isn't is that if any of the `tester.drag` steps touched a header -- which listens for tap gestures -- then this would ensure that step got interpreted as a drag. The old version would (a) interpret it as a tap, not a drag, if it was less than 20px in length; (b) leave those first 20px out of the effective length of the drag. The use of DragStartBehavior.down fixes (b). Then there are some drags of 5px in these tests, subject to (a); TouchSlop fixes those (by reducing the touch slop to 1px, instead of 20px). This change will be needed in order to have the item widgets start recording taps too, like the header widgets do, without messing up the drags in these tests. --- test/widgets/sticky_header_test.dart | 52 ++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index c283652ae1..13224fc68d 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -1,6 +1,7 @@ import 'dart:math' as math; import 'package:checks/checks.dart'; +import 'package:flutter/gestures.dart'; import 'package:flutter/widgets.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/widgets/sticky_header.dart'; @@ -8,12 +9,14 @@ import 'package:zulip/widgets/sticky_header.dart'; void main() { testWidgets('sticky headers: scroll up, headers overflow items, explicit version', (tester) async { await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, - child: StickyHeaderListView( - reverse: true, - children: List.generate(100, (i) => StickyHeaderItem( - allowOverflow: true, - header: _Header(i, height: 20), - child: _Item(i, height: 100)))))); + child: TouchSlop(touchSlop: 1, + child: StickyHeaderListView( + dragStartBehavior: DragStartBehavior.down, + reverse: true, + children: List.generate(100, (i) => StickyHeaderItem( + allowOverflow: true, + header: _Header(i, height: 20), + child: _Item(i, height: 100))))))); check(_itemIndexes(tester)).deepEquals([0, 1, 2, 3, 4, 5]); check(_headerIndex(tester)).equals(5); check(tester.getTopLeft(find.byType(_Item).last)).equals(const Offset(0, 0)); @@ -43,11 +46,13 @@ void main() { testWidgets('sticky headers: scroll up, headers bounded by items, semi-explicit version', (tester) async { await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, - child: StickyHeaderListView( - reverse: true, - children: List.generate(100, (i) => StickyHeaderItem( - header: _Header(i, height: 20), - child: _Item(i, height: 100)))))); + child: TouchSlop(touchSlop: 1, + child: StickyHeaderListView( + dragStartBehavior: DragStartBehavior.down, + reverse: true, + children: List.generate(100, (i) => StickyHeaderItem( + header: _Header(i, height: 20), + child: _Item(i, height: 100))))))); void checkState(int index, {required double item, required double header}) => _checkHeader(tester, index, first: false, @@ -108,6 +113,7 @@ void main() { Widget page(Widget Function(BuildContext, int) itemBuilder) { return Directionality(textDirection: TextDirection.ltr, child: StickyHeaderListView.builder( + dragStartBehavior: DragStartBehavior.down, cacheExtent: 0, itemCount: 10, itemBuilder: itemBuilder)); } @@ -386,3 +392,27 @@ class _Item extends StatelessWidget { child: Text("Item $index")); } } + + +/// Sets [DeviceGestureSettings.touchSlop] for the child subtree +/// to the given value, by inserting a [MediaQuery]. +/// +/// For example `TouchSlop(touchSlop: 1, …)` means a touch that moves by even +/// a single pixel will be interpreted as a drag, even if a tap gesture handler +/// is competing for the gesture. For human fingers that'd make it unreasonably +/// difficult to make a tap, but in a test carried out by software it can be +/// convenient for making small drag gestures straightforward. +class TouchSlop extends StatelessWidget { + const TouchSlop({super.key, required this.touchSlop, required this.child}); + + final double touchSlop; + final Widget child; + + @override + Widget build(BuildContext context) { + return MediaQuery( + data: MediaQuery.of(context).copyWith( + gestureSettings: DeviceGestureSettings(touchSlop: touchSlop)), + child: child); + } +} From 63f66d1a88b4a0628db197de7d1458b8f1f99f0f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 13:38:21 -0800 Subject: [PATCH 06/19] sticky_header test [nfc]: Generalize tap-logging from headers --- test/widgets/sticky_header_test.dart | 32 ++++++++++++++++++---------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index 13224fc68d..b4795b1d62 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -1,6 +1,7 @@ import 'dart:math' as math; import 'package:checks/checks.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; import 'package:flutter/widgets.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -304,10 +305,11 @@ Future _checkSequence( // Check the header gets hit when it should, and not when it shouldn't. await tester.tapAt(headerInset(1)); await tester.tapAt(headerInset(expectedHeaderInsetExtent - 1)); - check(_Header.takeTapCount()).equals(2); + check(_TapLogged.takeTapLog())..length.equals(2) + ..every((it) => it.isA<_Header>()); await tester.tapAt(headerInset(extent - 1)); await tester.tapAt(headerInset(extent - (expectedHeaderInsetExtent - 1))); - check(_Header.takeTapCount()).equals(0); + check(_TapLogged.takeTapLog()).isEmpty(); } Future jumpAndCheck(double position) async { @@ -354,28 +356,36 @@ Iterable _itemIndexes(WidgetTester tester) { return tester.widgetList<_Item>(find.byType(_Item)).map((w) => w.index); } -class _Header extends StatelessWidget { +sealed class _TapLogged { + static List<_TapLogged> takeTapLog() { + final result = _tapLog; + _tapLog = []; + return result; + } + static List<_TapLogged> _tapLog = []; +} + +class _Header extends StatelessWidget implements _TapLogged { const _Header(this.index, {required this.height}); final int index; final double height; - static int takeTapCount() { - final result = _tapCount; - _tapCount = 0; - return result; - } - static int _tapCount = 0; - @override Widget build(BuildContext context) { return SizedBox( height: height, width: height, // TODO clean up child: GestureDetector( - onTap: () => _tapCount++, + onTap: () => _TapLogged._tapLog.add(this), child: Text("Header $index"))); } + + @override + void debugFillProperties(DiagnosticPropertiesBuilder properties) { + super.debugFillProperties(properties); + properties.add(IntProperty('index', index)); + } } class _Item extends StatelessWidget { From 8bf8c2476c30dcf86c78a55cec0733333049cbe6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 13:56:27 -0800 Subject: [PATCH 07/19] sticky_header test: Record taps on _Item widgets too This will be useful in a test we'll add for an upcoming change. --- test/widgets/sticky_header_test.dart | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index b4795b1d62..4f6c8ed071 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -309,7 +309,8 @@ Future _checkSequence( ..every((it) => it.isA<_Header>()); await tester.tapAt(headerInset(extent - 1)); await tester.tapAt(headerInset(extent - (expectedHeaderInsetExtent - 1))); - check(_TapLogged.takeTapLog()).isEmpty(); + check(_TapLogged.takeTapLog())..length.equals(2) + ..every((it) => it.isA<_Item>()); } Future jumpAndCheck(double position) async { @@ -388,7 +389,7 @@ class _Header extends StatelessWidget implements _TapLogged { } } -class _Item extends StatelessWidget { +class _Item extends StatelessWidget implements _TapLogged { const _Item(this.index, {required this.height}); final int index; @@ -399,10 +400,17 @@ class _Item extends StatelessWidget { return SizedBox( height: height, width: height, - child: Text("Item $index")); + child: GestureDetector( + onTap: () => _TapLogged._tapLog.add(this), + child: Text("Item $index"))); } -} + @override + void debugFillProperties(DiagnosticPropertiesBuilder properties) { + super.debugFillProperties(properties); + properties.add(IntProperty('index', index)); + } +} /// Sets [DeviceGestureSettings.touchSlop] for the child subtree /// to the given value, by inserting a [MediaQuery]. From b541ea3ad8b5ab7f08221c30cee92d5a4b339090 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 7 Feb 2025 21:17:06 -0800 Subject: [PATCH 08/19] sticky_header example: Add double slivers with header at bottom These changes are NFC for the existing double-sliver example, with the sticky header at the top. --- lib/example/sticky_header.dart | 52 +++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/lib/example/sticky_header.dart b/lib/example/sticky_header.dart index 1b55d04016..b1dadd479e 100644 --- a/lib/example/sticky_header.dart +++ b/lib/example/sticky_header.dart @@ -125,40 +125,61 @@ class ExampleVerticalDouble extends StatelessWidget { super.key, required this.title, // this.reverse = false, - // this.headerDirection = AxisDirection.down, - }); // : assert(axisDirectionToAxis(headerDirection) == Axis.vertical); + required this.headerPlacement, + }); final String title; // final bool reverse; - // final AxisDirection headerDirection; + final HeaderPlacement headerPlacement; @override Widget build(BuildContext context) { const numSections = 4; const numBottomSections = 2; + const numTopSections = numSections - numBottomSections; const numPerSection = 10; + + final headerAtBottom = switch (headerPlacement) { + HeaderPlacement.scrollingStart => false, + HeaderPlacement.scrollingEnd => true, + }; + + // Choose the "center" sliver so that the sliver which might need to paint + // a header overflowing the other header is the sliver that paints last. + final centerKey = headerAtBottom ? + const ValueKey('bottom') : const ValueKey('top'); + + // This is a side effect of our choice of centerKey. + final topSliverGrowsUpward = headerAtBottom; + return Scaffold( appBar: AppBar(title: Text(title)), body: CustomScrollView( semanticChildCount: numSections, + center: centerKey, slivers: [ SliverStickyHeaderList( - headerPlacement: HeaderPlacement.scrollingStart, + key: const ValueKey('top'), + headerPlacement: headerPlacement, delegate: SliverChildBuilderDelegate( childCount: numSections - numBottomSections, (context, i) { - final ii = numSections - 1 - i; + final ii = numBottomSections + + (topSliverGrowsUpward ? i : numTopSections - 1 - i); return StickyHeaderItem( allowOverflow: true, header: WideHeader(i: ii), child: Column( + verticalDirection: headerAtBottom + ? VerticalDirection.up : VerticalDirection.down, children: List.generate(numPerSection + 1, (j) { if (j == 0) return WideHeader(i: ii); return WideItem(i: ii, j: j-1); }))); })), SliverStickyHeaderList( - headerPlacement: HeaderPlacement.scrollingStart, + key: const ValueKey('bottom'), + headerPlacement: headerPlacement, delegate: SliverChildBuilderDelegate( childCount: numBottomSections, (context, i) { @@ -167,10 +188,12 @@ class ExampleVerticalDouble extends StatelessWidget { allowOverflow: true, header: WideHeader(i: ii), child: Column( + verticalDirection: headerAtBottom + ? VerticalDirection.up : VerticalDirection.down, children: List.generate(numPerSection + 1, (j) { - if (j == 0) return WideHeader(i: ii); - return WideItem(i: ii, j: j-1); - }))); + if (j == 0) return WideHeader(i: ii); + return WideItem(i: ii, j: j-1); + }))); })), ])); } @@ -319,8 +342,15 @@ class MainPage extends StatelessWidget { ]; final otherItems = [ _buildButton(context, - title: 'Double slivers', - page: ExampleVerticalDouble(title: 'Double slivers')), + title: 'Double slivers, headers at top', + page: ExampleVerticalDouble( + title: 'Double slivers, headers at top', + headerPlacement: HeaderPlacement.scrollingStart)), + _buildButton(context, + title: 'Double slivers, headers at bottom', + page: ExampleVerticalDouble( + title: 'Double slivers, headers at bottom', + headerPlacement: HeaderPlacement.scrollingEnd)), ]; return Scaffold( appBar: AppBar(title: const Text('Sticky Headers example')), From 628ac15f023a1a5e1cf087f4d98ec1693d6292d2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 10 Feb 2025 22:10:00 -0800 Subject: [PATCH 09/19] sticky_header test [nfc]: Make "first/last item" finders more robust The existing `.first` and `.last` versions rely on the order that children appear in the Flutter element tree. As is, that works because the `_Item` widgets are all children of one list sliver, and it manages its children in a nice straightforward order. But when we add multiple list slivers, the order will depend also on the order those appear as children of the viewport; and in particular the items at the edges of the viewport will no longer always be the first or last items in the tree. So introduce a couple of custom finders to keep finding the first or last items in the sense we mean. For examples of using the APIs these finders use, compare the implementation of [FinderBase.first]. --- test/widgets/sticky_header_test.dart | 63 +++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 2 deletions(-) diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index 4f6c8ed071..422debc825 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -277,9 +277,10 @@ Future _checkSequence( final first = !(reverse ^ reverseHeader ^ reverseGrowth); - final itemFinder = first ? find.byType(_Item).first : find.byType(_Item).last; + final itemFinder = first ? _LeastItemFinder(find.byType(_Item)) + : _GreatestItemFinder(find.byType(_Item)); - double insetExtent(Finder finder) { + double insetExtent(FinderBase finder) { return headerAtCoordinateEnd ? extent - tester.getTopLeft(finder).inDirection(axis.coordinateDirection) : tester.getBottomRight(finder).inDirection(axis.coordinateDirection); @@ -330,6 +331,64 @@ Future _checkSequence( await jumpAndCheck(100); } +abstract class _SelectItemFinder extends FinderBase with ChainedFinderMixin { + bool shouldPrefer(_Item candidate, _Item previous); + + @override + Iterable filter(Iterable parentCandidates) { + Element? result; + _Item? resultWidget; + for (final candidate in parentCandidates) { + if (candidate is! ComponentElement) continue; + final widget = candidate.widget; + if (widget is! _Item) continue; + if (resultWidget == null || shouldPrefer(widget, resultWidget)) { + result = candidate; + resultWidget = widget; + } + } + return [if (result != null) result]; + } +} + +/// Finds the [_Item] with least [_Item.index] +/// out of all elements found by the given parent finder. +class _LeastItemFinder extends _SelectItemFinder { + _LeastItemFinder(this.parent); + + @override + final FinderBase parent; + + @override + String describeMatch(Plurality plurality) { + return 'least-index _Item from ${parent.describeMatch(plurality)}'; + } + + @override + bool shouldPrefer(_Item candidate, _Item previous) { + return candidate.index < previous.index; + } +} + +/// Finds the [_Item] with greatest [_Item.index] +/// out of all elements found by the given parent finder. +class _GreatestItemFinder extends _SelectItemFinder { + _GreatestItemFinder(this.parent); + + @override + final FinderBase parent; + + @override + String describeMatch(Plurality plurality) { + return 'greatest-index _Item from ${parent.describeMatch(plurality)}'; + } + + @override + bool shouldPrefer(_Item candidate, _Item previous) { + return candidate.index > previous.index; + } +} + Future _drag(WidgetTester tester, Offset offset) async { await tester.drag(find.byType(StickyHeaderListView), offset); await tester.pump(); From 488c60c38833027c357294a0aa3d227339644401 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 10 Feb 2025 19:26:17 -0800 Subject: [PATCH 10/19] sticky_header test [nfc]: Prepare generic test for more generality --- test/widgets/sticky_header_test.dart | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index 422debc825..fc9a38c970 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -237,6 +237,15 @@ Future _checkSequence( Axis.vertical => reverseHeader, }; final reverseGrowth = (growthDirection == GrowthDirection.reverse); + final headerPlacement = reverseHeader ^ reverse + ? HeaderPlacement.scrollingEnd : HeaderPlacement.scrollingStart; + + Widget buildItem(int i) { + return StickyHeaderItem( + allowOverflow: allowOverflow, + header: _Header(i, height: 20), + child: _Item(i, height: 100)); + } final controller = ScrollController(); const listKey = ValueKey("list"); @@ -252,13 +261,9 @@ Future _checkSequence( slivers: [ SliverStickyHeaderList( key: listKey, - headerPlacement: (reverseHeader ^ reverse) - ? HeaderPlacement.scrollingEnd : HeaderPlacement.scrollingStart, + headerPlacement: headerPlacement, delegate: SliverChildListDelegate( - List.generate(100, (i) => StickyHeaderItem( - allowOverflow: allowOverflow, - header: _Header(i, height: 20), - child: _Item(i, height: 100))))), + List.generate(100, (i) => buildItem(i)))), const SliverPadding( key: emptyKey, padding: EdgeInsets.zero), @@ -315,7 +320,8 @@ Future _checkSequence( } Future jumpAndCheck(double position) async { - controller.jumpTo(position * (reverseGrowth ? -1 : 1)); + final scrollPosition = position * (reverseGrowth ? -1 : 1); + controller.jumpTo(scrollPosition); await tester.pump(); await checkState(); } From 7df2c378d563f0739ba89316e33eda0ccf207e1a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 10 Feb 2025 19:35:37 -0800 Subject: [PATCH 11/19] sticky_header test [nfc]: Prepare list of slivers more uniformly This will be helpful for keeping complexity down when we add more slivers to this list. --- test/widgets/sticky_header_test.dart | 37 +++++++++++++++++----------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index fc9a38c970..ac7c4270ce 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -1,6 +1,7 @@ import 'dart:math' as math; import 'package:checks/checks.dart'; +import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; import 'package:flutter/widgets.dart'; @@ -247,27 +248,35 @@ Future _checkSequence( child: _Item(i, height: 100)); } + const center = ValueKey("center"); + final slivers = [ + const SliverPadding( + key: center, + padding: EdgeInsets.zero), + SliverStickyHeaderList( + headerPlacement: headerPlacement, + delegate: SliverChildListDelegate( + List.generate(100, (i) => buildItem(i)))), + ]; + + final double anchor; + if (reverseGrowth) { + slivers.reverseRange(0, slivers.length); + anchor = 1.0; + } else { + anchor = 0.0; + } + final controller = ScrollController(); - const listKey = ValueKey("list"); - const emptyKey = ValueKey("empty"); await tester.pumpWidget(Directionality( textDirection: textDirection ?? TextDirection.rtl, child: CustomScrollView( controller: controller, scrollDirection: axis, reverse: reverse, - anchor: reverseGrowth ? 1.0 : 0.0, - center: reverseGrowth ? emptyKey : listKey, - slivers: [ - SliverStickyHeaderList( - key: listKey, - headerPlacement: headerPlacement, - delegate: SliverChildListDelegate( - List.generate(100, (i) => buildItem(i)))), - const SliverPadding( - key: emptyKey, - padding: EdgeInsets.zero), - ]))); + anchor: anchor, + center: center, + slivers: slivers))); final overallSize = tester.getSize(find.byType(CustomScrollView)); final extent = overallSize.onAxis(axis); From fba97d8d5cf1247152e7b10545ae99467cb78d8b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 10 Feb 2025 19:42:46 -0800 Subject: [PATCH 12/19] sticky_header test: Use 10 items instead of 100 This is still enough to fill more than the viewport, and will be more helpful for scrolling past an entire sliver when we add more slivers. --- test/widgets/sticky_header_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index ac7c4270ce..82c020a861 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -256,7 +256,7 @@ Future _checkSequence( SliverStickyHeaderList( headerPlacement: headerPlacement, delegate: SliverChildListDelegate( - List.generate(100, (i) => buildItem(i)))), + List.generate(10, (i) => buildItem(i)))), ]; final double anchor; From f53521be182efc5602b5bd0e4831344734129705 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 10 Feb 2025 19:49:13 -0800 Subject: [PATCH 13/19] sticky_header test: Test slivers splitting viewport There are still some bugs affecting the sticky_header library when a SliverStickyHeaderList occupies only part of the viewport (which is a configuration we'll need for letting the message list grow in both directions, for #82). I sent a PR which aimed to fix a cluster of those, in which I tried to get away without writing these systematic test cases for them. It worked for the cases I did test -- including the cases that would actually arise for the Zulip message list -- and I believed the changes were correct when I sent the PR. But that version was still conceptually confused, as evidenced by the fact that it turned out to break other cases: https://github.com/zulip/zulip-flutter/pull/1316#discussion_r1947339710 So that seems like a sign that this really should get systematic all-cases tests. Some of these new test cases don't yet work properly, because they exercise the aforementioned bugs. The "header overflowing sliver" skip condition will be removed later in this series. The "paint order" skips will be addressed in an upcoming PR. --- test/widgets/sticky_header_test.dart | 169 ++++++++++++++++++++++----- 1 file changed, 139 insertions(+), 30 deletions(-) diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index 82c020a861..e4bfdca515 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -4,6 +4,7 @@ import 'package:checks/checks.dart'; import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; +import 'package:flutter/rendering.dart'; import 'package:flutter/widgets.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/widgets/sticky_header.dart'; @@ -75,36 +76,42 @@ void main() { for (final reverse in [true, false]) { for (final reverseHeader in [true, false]) { for (final growthDirection in GrowthDirection.values) { - for (final allowOverflow in [true, false]) { - final name = 'sticky headers: ' - 'scroll ${reverse ? 'up' : 'down'}, ' - 'header at ${reverseHeader ? 'bottom' : 'top'}, ' - '$growthDirection, ' - 'headers ${allowOverflow ? 'overflow' : 'bounded'}'; - testWidgets(name, (tester) => - _checkSequence(tester, - Axis.vertical, - reverse: reverse, - reverseHeader: reverseHeader, - growthDirection: growthDirection, - allowOverflow: allowOverflow, - )); - - for (final textDirection in TextDirection.values) { + for (final sliverConfig in _SliverConfig.values) { + for (final allowOverflow in [true, false]) { final name = 'sticky headers: ' - '${textDirection.name.toUpperCase()} ' - 'scroll ${reverse ? 'backward' : 'forward'}, ' - 'header at ${reverseHeader ? 'end' : 'start'}, ' + 'scroll ${reverse ? 'up' : 'down'}, ' + 'header at ${reverseHeader ? 'bottom' : 'top'}, ' '$growthDirection, ' - 'headers ${allowOverflow ? 'overflow' : 'bounded'}'; + 'headers ${allowOverflow ? 'overflow' : 'bounded'}, ' + 'slivers ${sliverConfig.name}'; testWidgets(name, (tester) => _checkSequence(tester, - Axis.horizontal, textDirection: textDirection, + Axis.vertical, reverse: reverse, reverseHeader: reverseHeader, growthDirection: growthDirection, allowOverflow: allowOverflow, + sliverConfig: sliverConfig, )); + + for (final textDirection in TextDirection.values) { + final name = 'sticky headers: ' + '${textDirection.name.toUpperCase()} ' + 'scroll ${reverse ? 'backward' : 'forward'}, ' + 'header at ${reverseHeader ? 'end' : 'start'}, ' + '$growthDirection, ' + 'headers ${allowOverflow ? 'overflow' : 'bounded'}, ' + 'slivers ${sliverConfig.name}'; + testWidgets(name, (tester) => + _checkSequence(tester, + Axis.horizontal, textDirection: textDirection, + reverse: reverse, + reverseHeader: reverseHeader, + growthDirection: growthDirection, + allowOverflow: allowOverflow, + sliverConfig: sliverConfig, + )); + } } } } @@ -223,6 +230,12 @@ void main() { }); } +enum _SliverConfig { + single, + backToBack, + followed, +} + Future _checkSequence( WidgetTester tester, Axis axis, { @@ -231,6 +244,7 @@ Future _checkSequence( bool reverseHeader = false, GrowthDirection growthDirection = GrowthDirection.forward, required bool allowOverflow, + _SliverConfig sliverConfig = _SliverConfig.single, }) async { assert(textDirection != null || axis == Axis.vertical); final headerAtCoordinateEnd = switch (axis) { @@ -241,6 +255,19 @@ Future _checkSequence( final headerPlacement = reverseHeader ^ reverse ? HeaderPlacement.scrollingEnd : HeaderPlacement.scrollingStart; + if (allowOverflow + && ((sliverConfig == _SliverConfig.backToBack + && (reverse ^ reverseHeader)) + || (sliverConfig == _SliverConfig.followed + && (reverse ^ reverseHeader ^ !reverseGrowth)))) { + // (The condition for this skip is pretty complicated; it's just the + // conditions where the bug gets triggered, and I haven't tried to + // work through why this exact set of cases is what's affected. + // The important thing is they all get fixed in an upcoming commit.) + markTestSkipped('bug in header overflowing sliver'); // TODO fix + return; + } + Widget buildItem(int i) { return StickyHeaderItem( allowOverflow: allowOverflow, @@ -248,8 +275,14 @@ Future _checkSequence( child: _Item(i, height: 100)); } + const sliverScrollExtent = 1000; const center = ValueKey("center"); final slivers = [ + if (sliverConfig == _SliverConfig.backToBack) + SliverStickyHeaderList( + headerPlacement: headerPlacement, + delegate: SliverChildListDelegate( + List.generate(10, (i) => buildItem(-i - 1)))), const SliverPadding( key: center, padding: EdgeInsets.zero), @@ -257,16 +290,45 @@ Future _checkSequence( headerPlacement: headerPlacement, delegate: SliverChildListDelegate( List.generate(10, (i) => buildItem(i)))), + if (sliverConfig == _SliverConfig.followed) + SliverStickyHeaderList( + headerPlacement: headerPlacement, + delegate: SliverChildListDelegate( + List.generate(10, (i) => buildItem(i + 10)))), ]; final double anchor; + bool paintOrderGood; if (reverseGrowth) { slivers.reverseRange(0, slivers.length); anchor = 1.0; + paintOrderGood = switch (sliverConfig) { + _SliverConfig.single => true, + // The last sliver will paint last. + _SliverConfig.backToBack => headerPlacement == HeaderPlacement.scrollingEnd, + // The last sliver will paint last. + _SliverConfig.followed => headerPlacement == HeaderPlacement.scrollingEnd, + }; } else { anchor = 0.0; + paintOrderGood = switch (sliverConfig) { + _SliverConfig.single => true, + // The last sliver will paint last. + _SliverConfig.backToBack => headerPlacement == HeaderPlacement.scrollingEnd, + // The first sliver will paint last. + _SliverConfig.followed => headerPlacement == HeaderPlacement.scrollingStart, + }; + } + + final skipBecausePaintOrder = allowOverflow && !paintOrderGood; + if (skipBecausePaintOrder) { + // TODO need to control paint order of slivers within viewport in order to + // make some configurations behave properly when headers overflow slivers + markTestSkipped('sliver paint order'); + // Don't return yet; we'll still check layout, and skip specific affected checks below. } + final controller = ScrollController(); await tester.pumpWidget(Directionality( textDirection: textDirection ?? TextDirection.rtl, @@ -281,6 +343,7 @@ Future _checkSequence( final overallSize = tester.getSize(find.byType(CustomScrollView)); final extent = overallSize.onAxis(axis); assert(extent % 100 == 0); + assert(sliverScrollExtent - extent > 100); // A position `inset` from the center of the edge the header is found on. Offset headerInset(double inset) { @@ -318,6 +381,7 @@ Future _checkSequence( check(insetExtent(find.byType(_Header))).equals(expectedHeaderInsetExtent); // Check the header gets hit when it should, and not when it shouldn't. + if (skipBecausePaintOrder) return; await tester.tapAt(headerInset(1)); await tester.tapAt(headerInset(expectedHeaderInsetExtent - 1)); check(_TapLogged.takeTapLog())..length.equals(2) @@ -335,15 +399,60 @@ Future _checkSequence( await checkState(); } - await checkState(); - await jumpAndCheck(5); - await jumpAndCheck(10); - await jumpAndCheck(20); - await jumpAndCheck(50); - await jumpAndCheck(80); - await jumpAndCheck(90); - await jumpAndCheck(95); - await jumpAndCheck(100); + Future checkLocally() async { + final scrollOffset = controller.position.pixels * (reverseGrowth ? -1 : 1); + await checkState(); + await jumpAndCheck(scrollOffset + 5); + await jumpAndCheck(scrollOffset + 10); + await jumpAndCheck(scrollOffset + 20); + await jumpAndCheck(scrollOffset + 50); + await jumpAndCheck(scrollOffset + 80); + await jumpAndCheck(scrollOffset + 90); + await jumpAndCheck(scrollOffset + 95); + await jumpAndCheck(scrollOffset + 100); + } + + Iterable listExtents() { + final result = tester.renderObjectList(find.byType(SliverStickyHeaderList, skipOffstage: false)) + .map((renderObject) => (renderObject as RenderSliver) + .geometry!.layoutExtent); + return reverseGrowth ? result.toList().reversed : result; + } + + switch (sliverConfig) { + case _SliverConfig.single: + // Just check the first header, at a variety of offsets, + // and check it hands off to the next header. + await checkLocally(); + + case _SliverConfig.followed: + // Check behavior as the next sliver scrolls into view. + await jumpAndCheck(sliverScrollExtent - extent); + check(listExtents()).deepEquals([extent, 0]); + await checkLocally(); + check(listExtents()).deepEquals([extent - 100, 100]); + + // Check behavior as the original sliver scrolls out of view. + await jumpAndCheck(sliverScrollExtent - 100); + check(listExtents()).deepEquals([100, extent - 100]); + await checkLocally(); + check(listExtents()).deepEquals([0, extent]); + + case _SliverConfig.backToBack: + // Scroll the other sliver into view; + // check behavior as it scrolls back out. + await jumpAndCheck(-100); + check(listExtents()).deepEquals([100, extent - 100]); + await checkLocally(); + check(listExtents()).deepEquals([0, extent]); + + // Scroll the original sliver out of view; + // check behavior as it scrolls back in. + await jumpAndCheck(-extent); + check(listExtents()).deepEquals([extent, 0]); + await checkLocally(); + check(listExtents()).deepEquals([extent - 100, 100]); + } } abstract class _SelectItemFinder extends FinderBase with ChainedFinderMixin { From d98b67b26fa682b70cf378d7295ba0b9ca6331d9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 7 Feb 2025 22:00:21 -0800 Subject: [PATCH 14/19] sticky_header [nfc]: Fix childMainAxisPosition to properly use paintExtent This fixes a latent bug: this method would give wrong answers if the sliver's paintExtent differed from its layoutExtent. The bug is latent because performLayout currently always produces a layoutExtent equal to paintExtent. But we'll start making them differ soon, as part of making hit-testing work correctly when a sticky header is painted by one sliver but needs to encroach on the layout area of another sliver. The framework calls this method as part of hit-testing, so that requires fixing this bug too. --- lib/widgets/sticky_header.dart | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index 9dd0183920..44b61fbcf1 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -691,16 +691,21 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper double childMainAxisPosition(RenderObject child) { if (child == this.child) return 0.0; assert(child == header); + + final headerParentData = (header!.parentData as SliverPhysicalParentData); + final paintOffset = headerParentData.paintOffset; + // We use Sliver*Physical*ParentData, so the header's position is stored in // physical coordinates. To meet the spec of `childMainAxisPosition`, we // need to convert to the sliver's coordinate system. - final headerParentData = (header!.parentData as SliverPhysicalParentData); - final paintOffset = headerParentData.paintOffset; + // This is all a bit silly because callers like [hitTestBoxChild] are just + // going to do the same things in reverse to get physical coordinates. + // Ah well; that's the API. return switch (constraints.growthAxisDirection) { AxisDirection.right => paintOffset.dx, - AxisDirection.left => geometry!.layoutExtent - header!.size.width - paintOffset.dx, + AxisDirection.left => geometry!.paintExtent - header!.size.width - paintOffset.dx, AxisDirection.down => paintOffset.dy, - AxisDirection.up => geometry!.layoutExtent - header!.size.height - paintOffset.dy, + AxisDirection.up => geometry!.paintExtent - header!.size.height - paintOffset.dy, }; } From 4cc3cb167d77925eb5de29d12120453c9cd720c4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 11 Feb 2025 13:52:59 -0800 Subject: [PATCH 15/19] sticky_header [nfc]: Split header-overflows-sliver condition explicitly This makes for fewer situations to think about at a given point in the code, and will make the logic a bit easier to follow when we make some corrections to the overflow case. --- lib/widgets/sticky_header.dart | 50 +++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index 44b61fbcf1..7e8464bca1 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -617,26 +617,38 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper // even if the (visible part of the) item is smaller than the header, // and even if the whole child sliver is smaller than the header. - final paintedHeaderSize = calculatePaintOffset(constraints, from: 0, to: headerExtent); - geometry = SliverGeometry( // TODO review interaction with other slivers - scrollExtent: geometry.scrollExtent, - layoutExtent: childExtent, - paintExtent: math.max(childExtent, paintedHeaderSize), - maxPaintExtent: math.max(geometry.maxPaintExtent, headerExtent), - hasVisualOverflow: geometry.hasVisualOverflow - || headerExtent > constraints.remainingPaintExtent, - - // The cache extent is an extension of layout, not paint; it controls - // where the next sliver should start laying out content. (See - // [SliverConstraints.remainingCacheExtent].) The header isn't meant - // to affect where the next sliver gets laid out, so it shouldn't - // affect the cache extent. - cacheExtent: geometry.cacheExtent, - ); + if (headerExtent <= childExtent) { + // The header fits within the child sliver. + // So it doesn't affect this sliver's overall geometry. - headerOffset = _headerAtCoordinateEnd() - ? childExtent - headerExtent - : 0.0; + headerOffset = _headerAtCoordinateEnd() + ? childExtent - headerExtent + : 0.0; + } else { + // The header will overflow the child sliver. + // That makes this sliver's geometry a bit more complicated. + + final paintedHeaderSize = calculatePaintOffset(constraints, from: 0, to: headerExtent); + geometry = SliverGeometry( // TODO review interaction with other slivers + scrollExtent: geometry.scrollExtent, + layoutExtent: childExtent, + paintExtent: math.max(childExtent, paintedHeaderSize), + maxPaintExtent: math.max(geometry.maxPaintExtent, headerExtent), + hasVisualOverflow: geometry.hasVisualOverflow + || headerExtent > constraints.remainingPaintExtent, + + // The cache extent is an extension of layout, not paint; it controls + // where the next sliver should start laying out content. (See + // [SliverConstraints.remainingCacheExtent].) The header isn't meant + // to affect where the next sliver gets laid out, so it shouldn't + // affect the cache extent. + cacheExtent: geometry.cacheExtent, + ); + + headerOffset = _headerAtCoordinateEnd() + ? childExtent - headerExtent + : 0.0; + } } else { // The header's item has [StickyHeaderItem.allowOverflow] false. // Keep the header within the item, pushing the header partly out of From afeacd5a65af39893b2e4fbd89183a2a1e8c4540 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 14:15:11 -0800 Subject: [PATCH 16/19] sticky_header: Cut wrong use of calculatePaintOffset This commit is NFC for the actual app, or at least nearly so. This call to calculatePaintOffset was conceptually wrong: it's asking how much of this sliver's region to be painted is within the range of scroll offsets from zero to headerExtent. That'd be a pertinent question if we were locating something in that range of scroll offsets... but that range is not at all where the header goes, unless by happenstance. So the value returned is meaningless. One reason this buggy line has survived is that the bug is largely latent -- we can remove it entirely, as in this commit, and get exactly the same behavior except in odd circumstances. Specifically: * This paintedHeaderSize variable can only have any effect by being greater than childExtent. * In this case childExtent is smaller than headerExtent, too. * The main way that childExtent can be so small is if remainingPaintExtent, which constrains it, is equally small. * But calculatePaintOffset constrains its result, aka paintedHeaderSize, to at most remainingPaintExtent too, so then paintedHeaderSize still won't exceed childExtent. I say "main way" because the alternative is for the child to run out of content before finding as much as headerExtent of content to show. That could happen if the list just has less than that much content; but that means the header's own item is smaller than the header, which is a case that sticky_header doesn't really support well anyway and we don't have in the app. Otherwise, this would have to mean that some of the content was scrolled out of the viewport and then the child ran out of content before filling its allotted remainingPaintExtent of the viewport (and indeed before even reaching a headerExtent amount of content). This is actually not quite impossible, if the scrollable permits overscroll... but making it happen would require piling edge case upon edge case. Anyway, this call never made sense, so remove it. The resulting code in this headerExtent > childExtent case still isn't right. Removing this wrong logic helps clear the ground for fixing that. --- lib/widgets/sticky_header.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index 7e8464bca1..d2516fb02c 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -628,11 +628,10 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper // The header will overflow the child sliver. // That makes this sliver's geometry a bit more complicated. - final paintedHeaderSize = calculatePaintOffset(constraints, from: 0, to: headerExtent); geometry = SliverGeometry( // TODO review interaction with other slivers scrollExtent: geometry.scrollExtent, layoutExtent: childExtent, - paintExtent: math.max(childExtent, paintedHeaderSize), + paintExtent: childExtent, maxPaintExtent: math.max(geometry.maxPaintExtent, headerExtent), hasVisualOverflow: geometry.hasVisualOverflow || headerExtent > constraints.remainingPaintExtent, From 732761f6da2d0fc54de69fa59a882eb60d878727 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 7 Feb 2025 21:45:55 -0800 Subject: [PATCH 17/19] sticky_header [nfc]: Expand on the header-overflows-sliver case This case has several bugs in it. Not coincidentally, it's tricky to think through: there are several sub-cases and variables involved (the growth direction, vs. the header-placement direction, vs. the coordinate direction, ...). And in fact my original PR revision which fixed the cases that would affect the Zulip message list was still conceptually confused, as evidenced by the fact that it turned out to break other cases: https://github.com/zulip/zulip-flutter/pull/1316#discussion_r1947339710 One step in sorting that out was the preceding commit which split this overflows-sliver case from the alternative. As a next step, let's expand on the reasoning here a bit, with named variables and comments. In doing so, it becomes more apparent that several points in this calculation are wrong; for this NFC commit, mark those with TODO-comments. We'll fix them shortly. --- lib/widgets/sticky_header.dart | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index d2516fb02c..e5bb29b4a7 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -628,13 +628,28 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper // The header will overflow the child sliver. // That makes this sliver's geometry a bit more complicated. + // This sliver's paint region consists entirely of the header. + final paintExtent = headerExtent; + headerOffset = _headerAtCoordinateEnd() + ? childExtent - headerExtent // TODO buggy, should be zero + : 0.0; + + // Its layout region (affecting where the next sliver begins layout) + // is that given by the child sliver. + final layoutExtent = childExtent; + + // The paint origin places this sliver's paint region relative to its + // layout region. + final paintOrigin = 0.0; // TODO buggy + geometry = SliverGeometry( // TODO review interaction with other slivers scrollExtent: geometry.scrollExtent, - layoutExtent: childExtent, - paintExtent: childExtent, - maxPaintExtent: math.max(geometry.maxPaintExtent, headerExtent), + layoutExtent: layoutExtent, + paintExtent: childExtent, // TODO buggy + paintOrigin: paintOrigin, + maxPaintExtent: math.max(geometry.maxPaintExtent, paintExtent), hasVisualOverflow: geometry.hasVisualOverflow - || headerExtent > constraints.remainingPaintExtent, + || paintExtent > constraints.remainingPaintExtent, // The cache extent is an extension of layout, not paint; it controls // where the next sliver should start laying out content. (See @@ -643,10 +658,6 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper // affect the cache extent. cacheExtent: geometry.cacheExtent, ); - - headerOffset = _headerAtCoordinateEnd() - ? childExtent - headerExtent - : 0.0; } } else { // The header's item has [StickyHeaderItem.allowOverflow] false. From dc2c9f038d1e8d944a10b596a39a5d91a6246ef8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 15:16:00 -0800 Subject: [PATCH 18/19] sticky_header: Fix hit-testing when header overflows sliver When the sticky header overflows the sliver that provides it -- that is, when the sliver boundary is scrolled to within the area the header covers -- the existing code already got the right visual result, painting the header at its full size. But it didn't work properly for hit-testing: trying to tap the header in the portion where it's overflowing wouldn't work, and would instead go through to whatever's underneath (like the top of the next sliver). That's because the geometry it was reporting from this `performLayout` method didn't reflect the geometry it would actually paint in the `paint` method. When hit-testing, that reported geometry gets interpreted by the framework code before calling this render object's other methods. Specifically, this sliver was reporting to its parent that its paint region (described by `paintOrigin` and `paintExtent`) was the same as the child sliver's paint region. In reality, this sliver in this case will paint a larger region than that. Fix by accurately reporting this sliver's paint region (via `paintOrigin` and `paintExtent`), while adjusting `headerOffset` to be relative to the new truthful paint region rather than the old inaccurate one. This fix lets us mark a swath of test cases as no longer skipped. On the other hand, this change introduces a different bug in this overflow case: the child sliver is now painted in the wrong place if _headerAtCoordinateEnd(). (It gets lined up with the inner edge of the header, even though it's too short to reach the viewport edge from there.) That bug is latent as far as the Zulip app is concerned, so we leave it for now with a TODO. Other than that, after this fix, sticky headers overflowing into the next sliver seem to work completely correctly... as long as the viewport paints the slivers in the necessary order. We'll take care of that in an upcoming PR. --- lib/widgets/sticky_header.dart | 20 +++++--- test/widgets/sticky_header_test.dart | 73 ++++++++++++++++++++++------ 2 files changed, 73 insertions(+), 20 deletions(-) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index e5bb29b4a7..5cfe923bc8 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -630,22 +630,30 @@ class _RenderSliverStickyHeaderList extends RenderSliver with RenderSliverHelper // This sliver's paint region consists entirely of the header. final paintExtent = headerExtent; - headerOffset = _headerAtCoordinateEnd() - ? childExtent - headerExtent // TODO buggy, should be zero - : 0.0; + headerOffset = 0.0; // Its layout region (affecting where the next sliver begins layout) // is that given by the child sliver. final layoutExtent = childExtent; // The paint origin places this sliver's paint region relative to its - // layout region. - final paintOrigin = 0.0; // TODO buggy + // layout region so that they share the edge the header appears at + // (which should be the edge of the viewport). + final headerGrowthPlacement = + _widget.headerPlacement._byGrowth(constraints.growthDirection); + final paintOrigin = switch (headerGrowthPlacement) { + _HeaderGrowthPlacement.growthStart => 0.0, + _HeaderGrowthPlacement.growthEnd => layoutExtent - paintExtent, + }; + // TODO the child sliver should be painted at offset -paintOrigin + // (This bug doesn't matter so long as the header is opaque, + // because the header covers the child in that case. + // For that reason the Zulip message list isn't affected.) geometry = SliverGeometry( // TODO review interaction with other slivers scrollExtent: geometry.scrollExtent, layoutExtent: layoutExtent, - paintExtent: childExtent, // TODO buggy + paintExtent: paintExtent, paintOrigin: paintOrigin, maxPaintExtent: math.max(geometry.maxPaintExtent, paintExtent), hasVisualOverflow: geometry.hasVisualOverflow diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index e4bfdca515..affe75e8c6 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -228,6 +228,48 @@ void main() { await tester.pump(); checkState(103, item: 0, header: 0); }); + + testWidgets('hit-testing for header overflowing sliver', (tester) async { + final controller = ScrollController(); + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomScrollView( + controller: controller, + slivers: [ + SliverStickyHeaderList( + headerPlacement: HeaderPlacement.scrollingStart, + delegate: SliverChildListDelegate( + List.generate(100, (i) => StickyHeaderItem( + allowOverflow: true, + header: _Header(i, height: 20), + child: _Item(i, height: 100))))), + SliverStickyHeaderList( + headerPlacement: HeaderPlacement.scrollingStart, + delegate: SliverChildListDelegate( + List.generate(100, (i) => StickyHeaderItem( + allowOverflow: true, + header: _Header(100 + i, height: 20), + child: _Item(100 + i, height: 100))))), + ]))); + + const topExtent = 100 * 100; + for (double topHeight in [5, 10, 15, 20]) { + controller.jumpTo(topExtent - topHeight); + await tester.pump(); + // The top sliver occupies height [topHeight]. + // Its header overhangs by `20 - topHeight`. + + final expected = >[]; + for (int y = 1; y < 20; y++) { + await tester.tapAt(Offset(400, y.toDouble())); + expected.add((it) => it.isA<_Header>().index.equals(99)); + } + for (int y = 21; y < 40; y += 2) { + await tester.tapAt(Offset(400, y.toDouble())); + expected.add((it) => it.isA<_Item>().index.equals(100)); + } + check(_TapLogged.takeTapLog()).deepEquals(expected); + } + }); } enum _SliverConfig { @@ -255,19 +297,6 @@ Future _checkSequence( final headerPlacement = reverseHeader ^ reverse ? HeaderPlacement.scrollingEnd : HeaderPlacement.scrollingStart; - if (allowOverflow - && ((sliverConfig == _SliverConfig.backToBack - && (reverse ^ reverseHeader)) - || (sliverConfig == _SliverConfig.followed - && (reverse ^ reverseHeader ^ !reverseGrowth)))) { - // (The condition for this skip is pretty complicated; it's just the - // conditions where the bug gets triggered, and I haven't tried to - // work through why this exact set of cases is what's affected. - // The important thing is they all get fixed in an upcoming commit.) - markTestSkipped('bug in header overflowing sliver'); // TODO fix - return; - } - Widget buildItem(int i) { return StickyHeaderItem( allowOverflow: allowOverflow, @@ -377,7 +406,15 @@ Future _checkSequence( 100 - (first ? scrollOffset % 100 : (-scrollOffset) % 100); final double expectedHeaderInsetExtent = allowOverflow ? 20 : math.min(20, expectedItemInsetExtent); - check(insetExtent(itemFinder)).equals(expectedItemInsetExtent); + if (expectedItemInsetExtent < expectedHeaderInsetExtent) { + // TODO there's a bug here if the header isn't opaque; + // this check would exercise the bug: + // check(insetExtent(itemFinder)).equals(expectedItemInsetExtent); + // Instead, check that things will be fine if the header is opaque. + check(insetExtent(itemFinder)).isLessOrEqual(expectedHeaderInsetExtent); + } else { + check(insetExtent(itemFinder)).equals(expectedItemInsetExtent); + } check(insetExtent(find.byType(_Header))).equals(expectedHeaderInsetExtent); // Check the header gets hit when it should, and not when it shouldn't. @@ -572,6 +609,10 @@ class _Header extends StatelessWidget implements _TapLogged { } } +extension _HeaderChecks on Subject<_Header> { + Subject get index => has((x) => x.index, 'index'); +} + class _Item extends StatelessWidget implements _TapLogged { const _Item(this.index, {required this.height}); @@ -595,6 +636,10 @@ class _Item extends StatelessWidget implements _TapLogged { } } +extension _ItemChecks on Subject<_Item> { + Subject get index => has((x) => x.index, 'index'); +} + /// Sets [DeviceGestureSettings.touchSlop] for the child subtree /// to the given value, by inserting a [MediaQuery]. /// From 043ae105c0d5c419230d632d371f5416e248011d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 15:49:17 -0800 Subject: [PATCH 19/19] sticky_header [nfc]: Doc overflow behavior and paint-order constraints --- lib/widgets/sticky_header.dart | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index 5cfe923bc8..d9d9a738dc 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -312,6 +312,27 @@ enum _HeaderGrowthPlacement { /// /// This widget takes most of its behavior from [SliverList], /// but adds sticky headers as described at [StickyHeaderListView]. +/// +/// ## Overflow across slivers +/// +/// When the list item that controls the sticky header has +/// [StickyHeaderItem.allowOverflow] true, the header will be permitted +/// to overflow not only the item but this whole sliver. +/// +/// The caller is responsible for arranging the paint order between slivers +/// so that this works correctly: a sliver that might overflow must be painted +/// after any sliver it might overflow onto. +/// For example if [headerPlacement] puts headers at the left of the viewport +/// (and any items with [StickyHeaderItem.allowOverflow] true are present), +/// then this [SliverStickyHeaderList] must paint after any slivers that appear +/// to the right of this sliver. +/// +/// At present there's no off-the-shelf way to fully control the paint order +/// between slivers. +/// See the implementation of [RenderViewport.childrenInPaintOrder] for the +/// paint order provided by [CustomScrollView]; it meets the above needs +/// for some arrangements of slivers and values of [headerPlacement], +/// but not others. class SliverStickyHeaderList extends RenderObjectWidget { SliverStickyHeaderList({ super.key,