anchors 4/n: Control which sliver paints over which #1435
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gnprice
force-pushed
the
pr-sliver-paint-order
branch
from
8f72cbd to
6a50072
Compare
|
I've now also sent the next step, #1436. That PR is a draft because it still needs tests. |
chrisbobbe
reviewed
Mar 27, 2025
test/widgets/message_list_test.dart
Outdated
Comment on lines
106
to
107
| ScrollView findScrollView(WidgetTester tester) => | ||
| tester.widget<ScrollView>(find.byWidgetPredicate((w) => w is ScrollView)); |
There was a problem hiding this comment.
Would find.bySubtype<ScrollView>() be simpler/better?
lib/widgets/scrolling.dart
Outdated
| // Ah, and here's a more on-point issue (more recently): | ||
| // https://github.com/flutter/flutter/issues/145592 | ||
| // | ||
| // TODO: perh sticky_header should configure a CustomPaintOrderScrollView automatically? |
There was a problem hiding this comment.
nit: "perh" for "perhaps", right?
There was a problem hiding this comment.
Ha, yeah — that's a bit of shorthand I use in notes to myself, which I guess this started as and then I didn't catch that before turning it into a PR.
This example demonstrates a bug we'll shortly fix: an overflowing sliver getting painted before the sliver it's meant to overflow onto.
This lets these tests stop caring which specific ScrollView subclass the implementation of MessageList uses.
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: flutter/flutter#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.)
…and doc By letting us control the paint order between slivers, this fixes all the skipped tests in this test file.
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.
gnprice
force-pushed
the
pr-sliver-paint-order
branch
from
6a50072 to
e932a05
Compare
|
Thanks for the review! Merged, with those fixes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the next round after #1316, toward #82.
Since that PR, the sticky header has been properly laid out and painted even in cases where it needs to overflow on top of another sliver… so long as the sliver with the header gets painted after the sliver it needs to overflow upon. This PR subclasses CustomScrollView and related classes (most pertinently RenderViewport) so that it's always possible to arrange for that to happen.
One welcome consequence is to clear out all the skipped tests that #1316 left in
sticky_header_test.dart.I've also sent a PR upstream to add this same functionality to CustomScrollView (and RenderViewport etc.) itself:
flutter/flutter#164818
Hopefully that will land and let us cut out a lot of this code. In the meantime this lets us move forward without waiting; and this custom local version is actually how I drafted this feature in the first place, before adapting it into an upstream PR, so it wasn't much work to polish it up to land here.
Selected commit messages
a1cd62a 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.
f35926d scrolling [nfc]: Give SingleChildScrollViewWithScrollbar docs and a new home
83cdc11 scrolling: 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:
flutter/flutter#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.)
1ce40cf 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.
8f72cbd 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.