From c92c71e96cd3b278f31c7fb11c6fa956eb551f76 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 31 Jan 2025 16:43:29 -0800 Subject: [PATCH 1/6] sticky_header example: Add back-to-back list with headers at top This example demonstrates a bug we'll shortly fix: an overflowing sliver getting painted before the sliver it's meant to overflow onto. --- lib/example/sticky_header.dart | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/example/sticky_header.dart b/lib/example/sticky_header.dart index b1dadd479e..97c0bb3b1e 100644 --- a/lib/example/sticky_header.dart +++ b/lib/example/sticky_header.dart @@ -126,11 +126,13 @@ class ExampleVerticalDouble extends StatelessWidget { required this.title, // this.reverse = false, required this.headerPlacement, + required this.topSliverGrowsUpward, }); final String title; // final bool reverse; final HeaderPlacement headerPlacement; + final bool topSliverGrowsUpward; @override Widget build(BuildContext context) { @@ -144,14 +146,9 @@ class ExampleVerticalDouble extends StatelessWidget { 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 ? + final centerKey = topSliverGrowsUpward ? 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( @@ -345,11 +342,19 @@ class MainPage extends StatelessWidget { title: 'Double slivers, headers at top', page: ExampleVerticalDouble( title: 'Double slivers, headers at top', + topSliverGrowsUpward: false, + headerPlacement: HeaderPlacement.scrollingStart)), + _buildButton(context, + title: 'Split slivers, headers at top', + page: ExampleVerticalDouble( + title: 'Split slivers, headers at top', + topSliverGrowsUpward: true, headerPlacement: HeaderPlacement.scrollingStart)), _buildButton(context, - title: 'Double slivers, headers at bottom', + title: 'Split slivers, headers at bottom', page: ExampleVerticalDouble( - title: 'Double slivers, headers at bottom', + title: 'Split slivers, headers at bottom', + topSliverGrowsUpward: true, headerPlacement: HeaderPlacement.scrollingEnd)), ]; return Scaffold( From 8833bea19f1bb07e22b70d827547a2a51a687d2b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 26 Jan 2024 15:47:24 -0800 Subject: [PATCH 2/6] scroll [nfc]: Give SingleChildScrollViewWithScrollbar docs and a new home --- lib/widgets/content.dart | 28 +-------------------------- lib/widgets/scrolling.dart | 39 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 27 deletions(-) create mode 100644 lib/widgets/scrolling.dart diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index c949602824..5f2a059a69 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -22,6 +22,7 @@ import 'inset_shadow.dart'; import 'lightbox.dart'; import 'message_list.dart'; import 'poll.dart'; +import 'scrolling.dart'; import 'store.dart'; import 'text.dart'; @@ -796,33 +797,6 @@ class _CodeBlockContainer extends StatelessWidget { } } -class SingleChildScrollViewWithScrollbar extends StatefulWidget { - const SingleChildScrollViewWithScrollbar( - {super.key, required this.scrollDirection, required this.child}); - - final Axis scrollDirection; - final Widget child; - - @override - State createState() => - _SingleChildScrollViewWithScrollbarState(); -} - -class _SingleChildScrollViewWithScrollbarState - extends State { - final ScrollController controller = ScrollController(); - - @override - Widget build(BuildContext context) { - return Scrollbar( - controller: controller, - child: SingleChildScrollView( - controller: controller, - scrollDirection: widget.scrollDirection, - child: widget.child)); - } -} - class MathBlock extends StatelessWidget { const MathBlock({super.key, required this.node}); diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart new file mode 100644 index 0000000000..665659aefa --- /dev/null +++ b/lib/widgets/scrolling.dart @@ -0,0 +1,39 @@ +import 'package:flutter/material.dart'; + +/// A [SingleChildScrollView] that always shows a Material [Scrollbar]. +/// +/// This differs from the behavior provided by [MaterialScrollBehavior] in that +/// (a) the scrollbar appears even when [scrollDirection] is [Axis.horizontal], +/// and (b) the scrollbar appears on all platforms, rather than only on +/// desktop platforms. +// TODO(upstream): SingleChildScrollView should have a scrollBehavior field +// and pass it on to Scrollable, just like ScrollView does; then this would +// be covered by using that. +// TODO: Maybe show scrollbar only on mobile platforms, like MaterialScrollBehavior +// and the base ScrollBehavior do? +class SingleChildScrollViewWithScrollbar extends StatefulWidget { + const SingleChildScrollViewWithScrollbar( + {super.key, required this.scrollDirection, required this.child}); + + final Axis scrollDirection; + final Widget child; + + @override + State createState() => + _SingleChildScrollViewWithScrollbarState(); +} + +class _SingleChildScrollViewWithScrollbarState + extends State { + final ScrollController controller = ScrollController(); + + @override + Widget build(BuildContext context) { + return Scrollbar( + controller: controller, + child: SingleChildScrollView( + controller: controller, + scrollDirection: widget.scrollDirection, + child: widget.child)); + } +} From 05ad4fd61dae1cd28d33ee9bd411cefa3a3b6d9b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 6 Feb 2024 20:45:54 -0800 Subject: [PATCH 3/6] msglist test [nfc]: Find scroll view more generically This lets these tests stop caring which specific ScrollView subclass the implementation of MessageList uses. --- test/widgets/message_list_test.dart | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index bd54412a46..6d2fdeef27 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -103,9 +103,11 @@ void main() { .findsOne(); } + ScrollView findScrollView(WidgetTester tester) => + tester.widget(find.bySubtype()); + ScrollController? findMessageListScrollController(WidgetTester tester) { - final scrollView = tester.widget(find.byType(CustomScrollView)); - return scrollView.controller; + return findScrollView(tester).controller; } group('MessageListPage', () { @@ -368,7 +370,7 @@ void main() { group('fetch older messages on scroll', () { int? itemCount(WidgetTester tester) => - tester.widget(find.byType(CustomScrollView)).semanticChildCount; + findScrollView(tester).semanticChildCount; testWidgets('basic', (tester) async { await setupMessageListPage(tester, foundOldest: false, From 5ab54d8eacb414970a9d0f66939c37ac246b3b46 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 16 Feb 2025 22:21:04 -0800 Subject: [PATCH 4/6] scroll: Add CustomPaintOrderScrollView This lets us control the paint order between the slivers, so that the top sliver is able to overflow over the bottom sliver when a sticky header calls for that. I've sent a PR upstream to add the same feature to CustomScrollView itself: https://github.com/flutter/flutter/pull/164818 That PR has been delayed by infra issues in Google testing, though. So doing this in our tree lets us move ahead without waiting. (I actually wrote this version first, then adapted it to produce the upstream PR. So landing this version isn't significant extra work.) --- lib/widgets/scrolling.dart | 207 +++++++++++++++++++++++++++++++ test/widgets/scrolling_test.dart | 191 ++++++++++++++++++++++++++++ 2 files changed, 398 insertions(+) create mode 100644 test/widgets/scrolling_test.dart diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index 665659aefa..7ceff8b745 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -1,4 +1,5 @@ import 'package:flutter/material.dart'; +import 'package:flutter/rendering.dart'; /// A [SingleChildScrollView] that always shows a Material [Scrollbar]. /// @@ -37,3 +38,209 @@ class _SingleChildScrollViewWithScrollbarState child: widget.child)); } } + +/// Specifies an order in which to paint the slivers of a [CustomScrollView]. +/// +/// Whichever order the slivers are painted in, +/// they will be hit-tested in the opposite order. +/// +/// This can also be thought of as an ordering in the z-direction: +/// whichever sliver is painted last (and hit-tested first) is on top, +/// because it will paint over other slivers if there is overlap. +/// Similarly, whichever sliver is painted first (and hit-tested last) +/// is on the bottom. +enum SliverPaintOrder { + /// The first sliver paints on top, and the last sliver on bottom. + /// + /// The slivers are painted in the reverse order of [CustomScrollView.slivers], + /// and hit-tested in the same order as [CustomScrollView.slivers]. + firstIsTop, + + /// The last sliver paints on top, and the first sliver on bottom. + /// + /// The slivers are painted in the same order as [CustomScrollView.slivers], + /// and hit-tested in the reverse order. + lastIsTop, + + /// The default order for [CustomScrollView]: the center sliver paints on top, + /// and the first sliver paints on bottom. + /// + /// If [CustomScrollView.center] is null or corresponds to the first sliver + /// in [CustomScrollView.slivers], this order is equivalent to [firstIsTop]. + /// Otherwise, the [CustomScrollView.center] sliver paints on top; + /// it's followed in the z-order by the slivers after it to the end + /// of the list, then the slivers before the center in reverse order, + /// with the first sliver in the list at the bottom in the z-direction. + centerTopFirstBottom, +} + +/// A [CustomScrollView] with control over the paint order, or z-order, +/// between slivers. +/// +/// This is just like [CustomScrollView] except it adds the [paintOrder_] field. +/// +/// (Actually there's one [CustomScrollView] feature this doesn't implement: +/// [shrinkWrap] always has its default value of false. That feature would be +/// easy to add if desired.) +// TODO(upstream): Pending PR: https://github.com/flutter/flutter/pull/164818 +// Notes from before sending that PR: +// Add an option [ScrollView.zOrder]? (An enum, or possibly +// a delegate.) Or at minimum document on [ScrollView.center] the +// existing behavior, which is counterintuitive. +// Nearest related upstream feature requests I find are for a "z-index", +// for CustomScrollView, Column, Row, and Stack respectively: +// https://github.com/flutter/flutter/issues/121173#issuecomment-1712825747 +// https://github.com/flutter/flutter/issues/121173 +// https://github.com/flutter/flutter/issues/121173#issuecomment-1914959184 +// https://github.com/flutter/flutter/issues/70836 +// A delegate would give enough flexibility for that and much else, +// but I'm not sure how many use cases wouldn't be covered by a small enum. +// +// Ah, and here's a more on-point issue (more recently): +// https://github.com/flutter/flutter/issues/145592 +// +// TODO: perhaps sticky_header should configure a CustomPaintOrderScrollView automatically? +class CustomPaintOrderScrollView extends CustomScrollView { + const CustomPaintOrderScrollView({ + 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, + SliverPaintOrder paintOrder = SliverPaintOrder.centerTopFirstBottom, + }) : paintOrder_ = paintOrder; + + /// The order in which to paint the slivers; + /// equivalently, the order in which to arrange them in the z-direction. + /// + /// Whichever order the slivers are painted in, + /// they will be hit-tested in the opposite order. + /// + /// To think of this as an ordering in the z-direction: + /// whichever sliver is painted last (and hit-tested first) is on top, + /// because it will paint over other slivers if there is overlap. + /// Similarly, whichever sliver is painted first (and hit-tested last) + /// is on the bottom. + /// + /// This defaults to [SliverPaintOrder.centerTopFirstBottom], + /// the behavior of the [CustomScrollView] base class. + final SliverPaintOrder paintOrder_; + + @override + Widget buildViewport(BuildContext context, ViewportOffset offset, + AxisDirection axisDirection, List slivers) { + return CustomPaintOrderViewport( + axisDirection: axisDirection, + offset: offset, + slivers: slivers, + cacheExtent: cacheExtent, + center: center, + anchor: anchor, + clipBehavior: clipBehavior, + paintOrder_: paintOrder_, + ); + } +} + +/// The viewport configured by a [CustomPaintOrderScrollView]. +class CustomPaintOrderViewport extends Viewport { + CustomPaintOrderViewport({ + super.key, + super.axisDirection, + super.crossAxisDirection, + super.anchor, + required super.offset, + super.center, + super.cacheExtent, + super.cacheExtentStyle, + super.slivers, + super.clipBehavior, + required this.paintOrder_, + }); + + final SliverPaintOrder paintOrder_; + + @override + RenderViewport createRenderObject(BuildContext context) { + return RenderCustomPaintOrderViewport( + axisDirection: axisDirection, + crossAxisDirection: crossAxisDirection + ?? Viewport.getDefaultCrossAxisDirection(context, axisDirection), + anchor: anchor, + offset: offset, + cacheExtent: cacheExtent, + cacheExtentStyle: cacheExtentStyle, + clipBehavior: clipBehavior, + paintOrder_: paintOrder_, + ); + } +} + +/// The render object configured by a [CustomPaintOrderViewport]. +class RenderCustomPaintOrderViewport extends RenderViewport { + RenderCustomPaintOrderViewport({ + super.axisDirection, + required super.crossAxisDirection, + required super.offset, + super.anchor, + super.children, + super.center, + super.cacheExtent, + super.cacheExtentStyle, + super.clipBehavior, + required this.paintOrder_, + }); + + final SliverPaintOrder paintOrder_; + + Iterable get _lastToFirst { + final List children = []; + RenderSliver? child = lastChild; + while (child != null) { + children.add(child); + child = childBefore(child); + } + return children; + } + + Iterable get _firstToLast { + final List children = []; + RenderSliver? child = firstChild; + while (child != null) { + children.add(child); + child = childAfter(child); + } + return children; + } + + @override + Iterable get childrenInPaintOrder { + return switch (paintOrder_) { + SliverPaintOrder.firstIsTop => _lastToFirst, + SliverPaintOrder.lastIsTop => _firstToLast, + SliverPaintOrder.centerTopFirstBottom => super.childrenInPaintOrder, + }; + } + + @override + Iterable get childrenInHitTestOrder { + return switch (paintOrder_) { + SliverPaintOrder.firstIsTop => _firstToLast, + SliverPaintOrder.lastIsTop => _lastToFirst, + SliverPaintOrder.centerTopFirstBottom => super.childrenInHitTestOrder, + }; + } +} diff --git a/test/widgets/scrolling_test.dart b/test/widgets/scrolling_test.dart new file mode 100644 index 0000000000..bfb010ccf0 --- /dev/null +++ b/test/widgets/scrolling_test.dart @@ -0,0 +1,191 @@ +import 'package:checks/checks.dart'; +// ignore: undefined_hidden_name // anticipates https://github.com/flutter/flutter/pull/164818 +import 'package:flutter/rendering.dart' hide SliverPaintOrder; +// ignore: undefined_hidden_name // anticipates https://github.com/flutter/flutter/pull/164818 +import 'package:flutter/widgets.dart' hide SliverPaintOrder; +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/widgets/scrolling.dart'; + +void main() { + group('CustomPaintOrderScrollView paint order', () { + final paintLog = []; + + Widget makeSliver(int i) { + return SliverToBoxAdapter( + key: ValueKey(i), + child: CustomPaint( + painter: TestCustomPainter() + ..onPaint = (_, _) => paintLog.add(i), + child: Text('Item $i'))); + } + + testWidgets('firstIsTop', (tester) async { + addTearDown(paintLog.clear); + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomPaintOrderScrollView( + paintOrder: SliverPaintOrder.firstIsTop, + center: ValueKey(2), anchor: 0.5, + slivers: List.generate(5, makeSliver)))); + + // First sliver paints last, over other slivers; last sliver paints first. + check(paintLog).deepEquals([4, 3, 2, 1, 0]); + }); + + testWidgets('lastIsTop', (tester) async { + addTearDown(paintLog.clear); + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomPaintOrderScrollView( + paintOrder: SliverPaintOrder.lastIsTop, + center: ValueKey(2), anchor: 0.5, + slivers: List.generate(5, makeSliver)))); + + // Last sliver paints last, over other slivers; first sliver paints first. + check(paintLog).deepEquals([0, 1, 2, 3, 4]); + }); + + // This test will fail if a corresponding upstream PR lands: + // https://github.com/flutter/flutter/pull/164818 + // because that eliminates the quirky centerTopFirstBottom behavior. + // In that case, skip this test for a quick fix; or go ahead and + // rip out CustomPaintOrderScrollView in favor of CustomScrollView. + // (Greg has a draft commit ready which does the latter.) + testWidgets('centerTopFirstBottom', (tester) async { + addTearDown(paintLog.clear); + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomPaintOrderScrollView( + paintOrder: SliverPaintOrder.centerTopFirstBottom, + center: ValueKey(2), anchor: 0.5, + slivers: List.generate(5, makeSliver)))); + + // The particular order CustomScrollView paints in. + check(paintLog).deepEquals([0, 1, 4, 3, 2]); + + // Check that CustomScrollView indeed paints in the same order. + final result = paintLog.toList(); + paintLog.clear(); + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomScrollView( + center: ValueKey(2), anchor: 0.5, + slivers: List.generate(5, makeSliver)))); + check(paintLog).deepEquals(result); + }); + }); + + group('CustomPaintOrderScrollView hit-test order', () { + Widget makeSliver(int i) { + return _AllOverlapSliver(key: ValueKey(i), id: i); + } + + List sliverIds(Iterable path) => [ + for (final e in path) + if (e.target case _RenderAllOverlapSliver(:final id)) + id, + ]; + + testWidgets('firstIsTop', (WidgetTester tester) async { + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomPaintOrderScrollView( + paintOrder: SliverPaintOrder.firstIsTop, + center: const ValueKey(2), anchor: 0.5, + slivers: List.generate(5, makeSliver)))); + + final result = tester.hitTestOnBinding(const Offset(400, 300)); + check(sliverIds(result.path)).deepEquals([0, 1, 2, 3, 4]); + }); + + testWidgets('lastIsTop', (WidgetTester tester) async { + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomPaintOrderScrollView( + paintOrder: SliverPaintOrder.lastIsTop, + center: const ValueKey(2), anchor: 0.5, + slivers: List.generate(5, makeSliver)))); + + final result = tester.hitTestOnBinding(const Offset(400, 300)); + check(sliverIds(result.path)).deepEquals([4, 3, 2, 1, 0]); + }); + + // This test will fail if the upstream PR 164818 lands. + // In that case the test is no longer needed and we'll take it out; + // see comment on other centerTopFirstBottom test above. + testWidgets('centerTopFirstBottom', (tester) async { + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomPaintOrderScrollView( + paintOrder: SliverPaintOrder.centerTopFirstBottom, + center: const ValueKey(2), anchor: 0.5, + slivers: List.generate(5, makeSliver)))); + + final result = tester.hitTestOnBinding(const Offset(400, 300)); + // The particular order CustomScrollView hit-tests in. + check(sliverIds(result.path)).deepEquals([2, 3, 4, 1, 0]); + + // Check that CustomScrollView indeed hit-tests in the same order. + await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, + child: CustomScrollView( + center: const ValueKey(2), anchor: 0.5, + slivers: List.generate(5, makeSliver)))); + check(sliverIds(tester.hitTestOnBinding(const Offset(400, 300)).path)) + .deepEquals(sliverIds(result.path)); + }); + }); +} + +class TestCustomPainter extends CustomPainter { + void Function(Canvas canvas, Size size)? onPaint; + + @override + void paint(Canvas canvas, Size size) { + if (onPaint != null) onPaint!(canvas, size); + } + + @override + bool shouldRepaint(covariant CustomPainter oldDelegate) { + return true; + } +} + +/// A sliver that overlaps with other slivers as far as possible, +/// and does nothing else. +class _AllOverlapSliver extends LeafRenderObjectWidget { + const _AllOverlapSliver({super.key, required this.id}); + + final int id; + + @override + RenderObject createRenderObject(BuildContext context) => _RenderAllOverlapSliver(id); +} + +class _RenderAllOverlapSliver extends RenderSliver { + _RenderAllOverlapSliver(this.id); + + final int id; + + @override + void performLayout() { + geometry = SliverGeometry( + paintExtent: constraints.remainingPaintExtent, + maxPaintExtent: constraints.remainingPaintExtent, + layoutExtent: 0.0, + ); + } + + @override + bool hitTest( + SliverHitTestResult result, { + required double mainAxisPosition, + required double crossAxisPosition, + }) { + if (mainAxisPosition >= 0.0 && + mainAxisPosition < geometry!.hitTestExtent && + crossAxisPosition >= 0.0 && + crossAxisPosition < constraints.crossAxisExtent) { + result.add( + SliverHitTestEntry( + this, + mainAxisPosition: mainAxisPosition, + crossAxisPosition: crossAxisPosition, + ), + ); + } + return false; + } +} From 49eff8bb366827ee258b0383a7f25fd157219148 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 16 Feb 2025 22:28:25 -0800 Subject: [PATCH 5/6] sticky_header test: Use CustomPaintOrderScrollView in test, example, and doc By letting us control the paint order between slivers, this fixes all the skipped tests in this test file. --- lib/example/sticky_header.dart | 8 +++- lib/widgets/sticky_header.dart | 13 ++++--- test/widgets/sticky_header_test.dart | 55 ++++++++++++---------------- 3 files changed, 36 insertions(+), 40 deletions(-) diff --git a/lib/example/sticky_header.dart b/lib/example/sticky_header.dart index 97c0bb3b1e..57c7022123 100644 --- a/lib/example/sticky_header.dart +++ b/lib/example/sticky_header.dart @@ -17,8 +17,10 @@ /// so as to set the app ID differently. library; -import 'package:flutter/material.dart'; +// ignore: undefined_hidden_name // anticipates https://github.com/flutter/flutter/pull/164818 +import 'package:flutter/material.dart' hide SliverPaintOrder; +import '../widgets/scrolling.dart'; import '../widgets/sticky_header.dart'; /// Example page using [StickyHeaderListView] and [StickyHeaderItem] in a @@ -151,9 +153,11 @@ class ExampleVerticalDouble extends StatelessWidget { return Scaffold( appBar: AppBar(title: Text(title)), - body: CustomScrollView( + body: CustomPaintOrderScrollView( semanticChildCount: numSections, center: centerKey, + paintOrder: headerAtBottom ? + SliverPaintOrder.lastIsTop : SliverPaintOrder.firstIsTop, slivers: [ SliverStickyHeaderList( key: const ValueKey('top'), diff --git a/lib/widgets/sticky_header.dart b/lib/widgets/sticky_header.dart index d9d9a738dc..8547229030 100644 --- a/lib/widgets/sticky_header.dart +++ b/lib/widgets/sticky_header.dart @@ -318,6 +318,8 @@ enum _HeaderGrowthPlacement { /// 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. +/// (This provides seamless behavior if, for example, two back-to-back slivers +/// are used for implementing a double-ended scrollable list.) /// /// The caller is responsible for arranging the paint order between slivers /// so that this works correctly: a sliver that might overflow must be painted @@ -327,12 +329,11 @@ enum _HeaderGrowthPlacement { /// 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. +/// To control the viewport's paint order, +/// consider using [CustomPaintOrderScrollView] instead of [CustomScrollView]. +/// Then [SliverPaintOrder.firstIsTop] for [HeaderPlacement.scrollingStart], +/// or [SliverPaintOrder.lastIsTop] for [HeaderPlacement.scrollingEnd], +/// suffices for meeting the needs above. class SliverStickyHeaderList extends RenderObjectWidget { SliverStickyHeaderList({ super.key, diff --git a/test/widgets/sticky_header_test.dart b/test/widgets/sticky_header_test.dart index affe75e8c6..6ff94c02d5 100644 --- a/test/widgets/sticky_header_test.dart +++ b/test/widgets/sticky_header_test.dart @@ -4,9 +4,12 @@ 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'; +// ignore: undefined_hidden_name // anticipates https://github.com/flutter/flutter/pull/164818 +import 'package:flutter/rendering.dart' hide SliverPaintOrder; +// ignore: undefined_hidden_name // anticipates https://github.com/flutter/flutter/pull/164818 +import 'package:flutter/widgets.dart' hide SliverPaintOrder; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/widgets/scrolling.dart'; import 'package:zulip/widgets/sticky_header.dart'; void main() { @@ -230,19 +233,24 @@ void main() { }); testWidgets('hit-testing for header overflowing sliver', (tester) async { + const centerKey = ValueKey('center'); final controller = ScrollController(); await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, - child: CustomScrollView( + child: CustomPaintOrderScrollView( controller: controller, + anchor: 0.0, + center: centerKey, + paintOrder: SliverPaintOrder.firstIsTop, slivers: [ SliverStickyHeaderList( headerPlacement: HeaderPlacement.scrollingStart, delegate: SliverChildListDelegate( List.generate(100, (i) => StickyHeaderItem( allowOverflow: true, - header: _Header(i, height: 20), - child: _Item(i, height: 100))))), + header: _Header(99 - i, height: 20), + child: _Item(99 - i, height: 100))))), SliverStickyHeaderList( + key: centerKey, headerPlacement: HeaderPlacement.scrollingStart, delegate: SliverChildListDelegate( List.generate(100, (i) => StickyHeaderItem( @@ -251,9 +259,8 @@ void main() { child: _Item(100 + i, height: 100))))), ]))); - const topExtent = 100 * 100; for (double topHeight in [5, 10, 15, 20]) { - controller.jumpTo(topExtent - topHeight); + controller.jumpTo(-topHeight); await tester.pump(); // The top sliver occupies height [topHeight]. // Its header overhangs by `20 - topHeight`. @@ -327,49 +334,34 @@ Future _checkSequence( ]; 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. + SliverPaintOrder paintOrder = SliverPaintOrder.centerTopFirstBottom; + if (!allowOverflow || (sliverConfig == _SliverConfig.single)) { + // The paint order doesn't matter. + } else { + paintOrder = headerPlacement == HeaderPlacement.scrollingStart + ? SliverPaintOrder.firstIsTop : SliverPaintOrder.lastIsTop; } - final controller = ScrollController(); await tester.pumpWidget(Directionality( textDirection: textDirection ?? TextDirection.rtl, - child: CustomScrollView( + child: CustomPaintOrderScrollView( controller: controller, scrollDirection: axis, reverse: reverse, anchor: anchor, center: center, + paintOrder: paintOrder, slivers: slivers))); - final overallSize = tester.getSize(find.byType(CustomScrollView)); + final overallSize = tester.getSize(find.bySubtype()); final extent = overallSize.onAxis(axis); assert(extent % 100 == 0); assert(sliverScrollExtent - extent > 100); @@ -418,7 +410,6 @@ 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) From e932a05d8e0de1270dc3639e7098317d211e4fde Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sun, 16 Feb 2025 22:28:25 -0800 Subject: [PATCH 6/6] msglist [nfc]: Use SliverPaintOrder.firstIsTop This changes the paint order of these slivers. Right now that has no observable effect, because the second sliver has zero height and paints nothing. In the future when we put the messages in back-to-back slivers, this is the paint order we'll need so that the top sliver can have a sticky header overflow properly over the bottom sliver. --- lib/widgets/message_list.dart | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 7a95d10177..1e95a1be49 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1,7 +1,8 @@ import 'dart:math'; import 'package:collection/collection.dart'; -import 'package:flutter/material.dart'; +// ignore: undefined_hidden_name // anticipates https://github.com/flutter/flutter/pull/164818 +import 'package:flutter/material.dart' hide SliverPaintOrder; import 'package:flutter_color_models/flutter_color_models.dart'; import 'package:intl/intl.dart' hide TextDirection; @@ -21,6 +22,7 @@ import 'emoji_reaction.dart'; import 'icons.dart'; import 'page.dart'; import 'profile.dart'; +import 'scrolling.dart'; import 'sticky_header.dart'; import 'store.dart'; import 'text.dart'; @@ -629,7 +631,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat sliver = SliverSafeArea(sliver: sliver); } - return CustomScrollView( + return CustomPaintOrderScrollView( // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or // similar) if that is ever offered: // https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849 @@ -645,6 +647,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat semanticChildCount: length + 2, anchor: 1.0, center: centerSliverKey, + paintOrder: SliverPaintOrder.firstIsTop, slivers: [ sliver,