Control paint order / z-order of slivers, take 1 #163511

gnprice · 2025-02-18T04:07:49Z

When slivers in a CustomScrollView overlap, this makes it possible to control which of the slivers will sit in front of the other one, meaning that it paints last and gets hit-tested first.

In this version there are two choices: the first sliver paints in front, or the last sliver paints in front. The first sliver in front is the default; it's the behavior needed for SliverAppBar, or other situations where the first sliver is some sort of header.

The existing behavior is actually quite a bit more complicated than that, with the "center" sliver painting in front, plus it varies depending on shrinkWrap. As far as I can tell this behavior isn't documented anywhere. It's equivalent to first-sliver-in-front whenever CustomScrollView.center isn't being used; and in fact there don't seem to be any tests in the tree that notice the difference between the existing behavior and first-sliver-in-front.

So although I actually have an implementation that preserves the more complex existing defaults, I'm hopeful we won't need it: if neither google3 nor customer tests are relying on that undocumented distinction either, then we can go ahead and simplify this behavior without a breaking change.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

gnprice · 2025-02-18T04:25:30Z

So although I actually have an implementation that preserves the more complex existing defaults, I'm hopeful we won't need it

In the PR, the first commit is the implementation that preserves existing behavior unchanged. The second commit removes the legacy behavior:

 packages/flutter/lib/src/rendering/viewport.dart   | 115 +++++---------------------------
 packages/flutter/lib/src/widgets/scroll_view.dart  |  15 ++---
 packages/flutter/lib/src/widgets/viewport.dart     |   7 +-
 packages/flutter/test/rendering/viewport_test.dart |   6 ++
 4 files changed, 32 insertions(+), 111 deletions(-)

So if we end up needing to keep the legacy behavior around, that first commit is the draft of how to do so.

gnprice · 2025-02-18T04:28:23Z

Results of some code archeology:

The legacy behavior dates to when slivers were first introduced, in e82b18d / #7370 in 2017. There was a bit of discussion then about the choice of paint order: #7370 (comment)

The reasoning there explains why it has the current behavior instead of painting the last sliver in front — which would be the behavior that matches Flex and most other widgets. But it doesn't appear that the simpler option of painting the first sliver in front came up at the time.

A few weeks later in 2017, when shrink-wrapping viewports were implemented (5928d22 / #7790), those painted the last sliver in front, just like most other widgets. Naturally that caused a series of bug reports over the years, because it turned out some people wanted app bars in shrink-wrapped viewports, just like in other viewports. Those issues were fixed in 2021, in b65a235 / #73195, by just flipping the order around to paint the first sliver in front, which is nearly but not quite the same thing that non-shrink-wrapped viewports do.

With this PR, all viewports by default paint their slivers in the same order that shrink-wrapped viewports have since 2021; and there's also an option to paint in the opposite order, as shrink-wrapped viewports did from 2017 to 2021, and as requested in #145592.

gnprice · 2025-02-21T00:29:16Z

Looks like customer_testing indeed passed — so nothing there was relying on that center-related quirk in CustomScrollView's paint order, just like nothing in the Flutter tree's own tests was doing so.

Hopefully the same thing will be true of the Google internal tests. If not (and if it isn't easy to just fix them forward), I can remove the second commit from this PR to make a version that preserves the legacy default behavior exactly.

Piinks

@chunhtai could this have implications for semantic traversal? IIRC visitChildrenForSemantics always visits children in paint order.

Piinks · 2025-02-24T20:48:58Z

packages/flutter/lib/src/rendering/viewport.dart

  viewport,
 }

+/// Specifies an order in which to paint the slivers of a [ScrollView].


Suggested change

/// Specifies an order in which to paint the slivers of a [ScrollView].

/// Specifies an order in which to paint the slivers of a [Viewport].

Sure, done.

Piinks · 2025-02-24T20:57:07Z

packages/flutter/lib/src/rendering/viewport.dart

-    if (firstChild == null) {
-      return children;
-    }
-    RenderSliver? child = center;


so nothing there was relying on that center-related quirk in CustomScrollView's paint order, just like nothing in the Flutter tree's own tests was doing so.

Can you say more about this? I wondered if this would impact cases with center.. I think scrollable_positioned_list relies heavily on the center property to drive their scroll to index logic. What quirk are you referring to?

I was referring to the quirk that the PR's first commit describes as SliverPaintOrder.centerTopFirstBottom, and documents here:
cb389bd#diff-46c7759851537103b628677eeaab4081a4abf52f213b92953703360ccb8a5534R66-R77

where the paint order is almost like the firstIsTop behavior, but gets more complicated when center is in use.

This PR doesn't change anything about the layout of the slivers or the handling of scroll positions — it's strictly about the paint order (and the semantics order which stays in sync with paint order, and the hit-test order which stays in sync as the reverse of paint order). So scroll-to-index logic shouldn't be affected.

chunhtai · 2025-02-24T21:29:47Z

Regarding to the semantics ordering, this change looks fine because the visiChildrenForSemantics is implemented based on the childrenInPaintOrder getter

flutter/packages/flutter/lib/src/rendering/viewport.dart

Line 314 in a8d1b62

void visitChildrenForSemantics(RenderObjectVisitor visitor) {

Piinks

Tentative LGTM to fire off the Google testing shard 😜

FYI for @Renzo-Olivares, we were chatting today about some strange semantics traversal behavior seen in single child scroll view, but not in the scroll view subclasses. Made me think of this since single child scroll view is implemented differently, and this will make it consistent across those classes.

gnprice · 2025-02-26T04:57:49Z

Hmm, Google testing failed, alas. So I guess the next question is what those failures look like — whether it looks like they'd be trivial to just fix.

It's probably not worth going through the breaking-change process for this: the existing behavior is a bit quirky, but I think for most applications the quirks will never come up. (They only come up if you (a) use center in a scroll-view/viewport that also (b) has slivers overlapping each other.) So the cost of leaving the existing default behavior unchanged is mostly some extra complexity in the implementation and docs, and no effect on most applications' code.

If we decide the current version of this PR would be a breaking change, then I can revise it to a non-breaking-change version by basically reverting the second commit:
b7ef445 Drop legacy sliver paint order, simplifying
(and adjusting the tests to match).

Piinks · 2025-03-05T21:07:41Z

Rebasing to trigger a fresh run of Google tests, I need to verify if the failures were unrelated. From what I saw it was not clear.

gnprice · 2025-03-06T23:28:15Z

Looks like the Google tests failed on this run too. I'll be curious to hear how the failures look.

Piinks · 2025-03-07T15:52:40Z

Unfortunately we had an infra issue here, I am re-triggering the tests because the results were deleted. 😮‍💨

Instead of the legacy behavior where the center sliver paints on top, just have the first sliver paint on top. This is quite a bit simpler to document and simpler to implement, and it removes variation between a shrink-wrapped and non-shrink-wrapped viewport. Moreover the new behavior is the same as the old whenever not using the `center` field; and it turns out there are no tests in the tree that notice the difference. So hopefully this won't even be a breaking change.

Held back for this last commit to avoid writing two versions of the tests. Hopefully we can go ahead with this cleaned-up behavior; if we end up needing the legacy behavior after all, the tests can be adapted to test that too.

I took these out as a last-minute simplifying change... and I guess I must not have actually rerun the tests after doing so, oops. Turns out I put them there in the first place for a good reason.

This keeps things a bit cleaner for code that uses this feature.

Piinks · 2025-03-07T15:53:00Z

I've set an alert to check back here at lunch 😉

Piinks · 2025-03-07T20:03:38Z

Checking in, infra seems to be struggling with getting this tested. I pressed some buttons manually from the other side to try and get it going. Will check back at the end of the day.

Piinks · 2025-03-07T20:58:07Z

Hrmph. It appears that our infra tooling to copy and test PRs in Google is confounded on this one. I've seen this happen sometimes when it has been tested a number of times or grows in age. @gnprice can you push a fresh commit to see if it helps? If that does not help, I recommend one of two possible next steps - creating a new PR, -or- reverting the potentially breaking commit here, landing this and putting up that second commit in a separate change.

gnprice · 2025-03-07T21:10:40Z

OK, sent a fresh PR #164818, just rebasing this one. Hopefully that does it 🤞

gnprice · 2025-04-07T22:18:57Z

Closing as superseded by #164818.

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Feb 18, 2025

gnprice mentioned this pull request Feb 18, 2025

Add feature to change z-order of slivers (paint order, hit-test order) #145592

Closed

justinmc requested a review from Piinks February 18, 2025 23:04

Piinks reviewed Feb 24, 2025

View reviewed changes

Piinks approved these changes Feb 26, 2025

View reviewed changes

Piinks force-pushed the pr-sliver-order branch from db0fd01 to 1600e42 Compare March 5, 2025 21:07

gnprice added 7 commits March 7, 2025 07:52

Add ScrollView.paintOrder, preserving default behavior unchanged

8ff55e4

Add tests

173cc4a

Held back for this last commit to avoid writing two versions of the tests. Hopefully we can go ahead with this cleaned-up behavior; if we end up needing the legacy behavior after all, the tests can be adapted to test that too.

Fix an import

d44820d

Fix tests: use anchor

d31586f

I took these out as a last-minute simplifying change... and I guess I must not have actually rerun the tests after doing so, oops. Turns out I put them there in the first place for a good reason.

Re-export SliverPaintOrder from widgets library

faa60df

This keeps things a bit cleaner for code that uses this feature.

Refer to Viewport instead of ScrollView in enum docs

ce6a74c

Piinks force-pushed the pr-sliver-order branch from 1600e42 to ce6a74c Compare March 7, 2025 15:52

gnprice changed the title ~~Control paint order / z-order of slivers~~ Control paint order / z-order of slivers, take 1 Mar 7, 2025

gnprice mentioned this pull request Mar 7, 2025

Control paint order / z-order of slivers #164818

Merged

gnprice closed this Apr 7, 2025

gnprice deleted the pr-sliver-order branch April 25, 2025 22:07

	/// Specifies an order in which to paint the slivers of a [ScrollView].
	/// Specifies an order in which to paint the slivers of a [Viewport].

Control paint order / z-order of slivers, take 1 #163511

Control paint order / z-order of slivers, take 1 #163511

Uh oh!

Conversation

gnprice commented Feb 18, 2025

Pre-launch Checklist

Uh oh!

gnprice commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Feb 18, 2025

Uh oh!

gnprice commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

Piinks Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice commented Feb 26, 2025

Uh oh!

Piinks commented Mar 5, 2025

Uh oh!

gnprice commented Mar 6, 2025

Uh oh!

Piinks commented Mar 7, 2025

Uh oh!

Piinks commented Mar 7, 2025

Uh oh!

Piinks commented Mar 7, 2025

Uh oh!

Piinks commented Mar 7, 2025

Uh oh!

gnprice commented Mar 7, 2025

Uh oh!

gnprice commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gnprice commented Feb 18, 2025 •

edited

Loading

gnprice commented Feb 21, 2025 •

edited

Loading

chunhtai commented Feb 24, 2025 •

edited

Loading