Skip to content

Conversation

@gnprice
Copy link
Member

@gnprice gnprice commented Mar 7, 2025

Fixes #145592.

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.

@gnprice
Copy link
Member Author

gnprice commented Mar 7, 2025

This is a fresh copy of #163511, in hopes of getting the infra for running Google tests on PRs to successfully run this one; see #163511 (comment) .

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from #163511 (and triggers Google testing) 🤞

@gnprice
Copy link
Member Author

gnprice commented Mar 7, 2025

Well:

[Fri, 07 Mar 2025 21:33:56 UTC] Pull request has been added to the roll queue

[Fri, 07 Mar 2025 21:39:04 UTC] An infra error occurred while running Google testing

(That's the info that's provided on the GitHub check. I appreciate that it does give that information.)

@Piinks
Copy link
Contributor

Piinks commented Mar 7, 2025

Yes, can confirm now, this is an issue not specific to this PR and is currently being worked on. Thanks for your patience @gnprice!

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Mar 7, 2025
@Piinks
Copy link
Contributor

Piinks commented Mar 11, 2025

I think this is resolved now, rebasing to give it another go!

@Piinks Piinks force-pushed the pr-sliver-order-2 branch from 09e5816 to e252330 Compare March 11, 2025 22:29
@justinmc
Copy link
Contributor

@Piinks Google testing is marked as failed here but the CL seemed to have no failures on it? Maybe I'm reading something wrong.

@Piinks
Copy link
Contributor

Piinks commented Mar 19, 2025

Yes, the Google testing shard has been going experiencing multiple infrastructure issues. This did have a failure before that I was not able to investigate before the cl was deleted, which I understand is pretty frustrating. Rebasing again to see if we can get a clean run now.

@Piinks Piinks force-pushed the pr-sliver-order-2 branch from e252330 to 3ff7d01 Compare March 19, 2025 21:25
@Piinks
Copy link
Contributor

Piinks commented Mar 19, 2025

If we aren't able to resolve it this time, I recommend rolling back to the non-breaking commit. Then we can consider pursuing the additional change after as a separate change.

gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Mar 25, 2025
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.)
@gnprice
Copy link
Member Author

gnprice commented Mar 26, 2025

How do the Google testing results look?

If there's still an infra issue getting in the way of clear signal there, I'm not in a rush — in Zulip I already have a version that adds the same functionality by subclassing. (That's the format in which I originally drafted the feature anyway.) So I don't mind this PR waiting to see the infra issue get resolved.

gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Mar 28, 2025
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.)
@Piinks
Copy link
Contributor

Piinks commented Apr 1, 2025

Thanks Greg! We're still having issues with Google testing. Thank you for you patience. 🙏

@justinmc justinmc force-pushed the pr-sliver-order-2 branch from 3ff7d01 to 62391da Compare April 22, 2025 22:24
@Piinks
Copy link
Contributor

Piinks commented Apr 22, 2025

Catching up after being out - Google testing appears to be working now so we rebased.

@Piinks
Copy link
Contributor

Piinks commented Apr 23, 2025

Checking back in here, good news Google testing passed!
Although... there are just some... zulip failures now. 🙃

@gnprice
Copy link
Member Author

gnprice commented Apr 23, 2025

Great!

Those Zulip tests are part of the same work as this PR (from zulip/zulip-flutter@f53521b, in zulip/zulip-flutter#1316). I'll go add some skips, then update the Zulip pin in flutter/tests. After this merges I can update our tests (and app code) to use the new API 🙂

(Specifically, I added those Zulip tests because I wanted to exercise the sticky-header code I was working on in as wide a variety of configurations as possible, and that code requires controlling the z-order in order to work correctly. When I sent the original version of this PR (#163511) the week after merging those tests, I knew it would break those tests but my plan was to get this merged and switch the tests to the new API before the customer_testing pin advanced past them. But while this was waiting for Google testing, the pin ended up getting bumped for unrelated reasons, so my clever plan didn't work.)

gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Apr 23, 2025
These two tests on CustomPaintOrderScrollView compare its behavior
to the default z-order / paint order behavior of CustomScrollView.
They'd therefore be broken by my upstream PR that gives
CustomScrollView the functionality of CustomPaintOrderScrollView
and simplifies its default behavior:
  flutter/flutter#164818

So skip the tests.  Once that PR lands, we can update past it and
then remove CustomPaintOrderScrollView and its tests entirely.

(The customer_tests failures currently seen on the upstream PR are
different, because the Zulip pin in flutter/tests is currently at
d302584.  The failures there are in the sticky_header tests, which
want to exercise a variety of sliver configurations and need to
control the z-order when they do so.  As of d302584, they did so
by relying on the specifics of the existing CustomScrollView
behavior.  Since then we merged CustomPaintOrderScrollView, and in
49eff8b adapted those tests to use it, making them immune to
changes in the default z-order; but that also meant adding
CustomPaintOrderScrollView's own tests, including these two.)
chrisbobbe pushed a commit to zulip/zulip-flutter that referenced this pull request Apr 23, 2025
These two tests on CustomPaintOrderScrollView compare its behavior
to the default z-order / paint order behavior of CustomScrollView.
They'd therefore be broken by my upstream PR that gives
CustomScrollView the functionality of CustomPaintOrderScrollView
and simplifies its default behavior:
  flutter/flutter#164818

So skip the tests.  Once that PR lands, we can update past it and
then remove CustomPaintOrderScrollView and its tests entirely.

(The customer_tests failures currently seen on the upstream PR are
different, because the Zulip pin in flutter/tests is currently at
d302584.  The failures there are in the sticky_header tests, which
want to exercise a variety of sliver configurations and need to
control the z-order when they do so.  As of d302584, they did so
by relying on the specifics of the existing CustomScrollView
behavior.  Since then we merged CustomPaintOrderScrollView, and in
49eff8b adapted those tests to use it, making them immune to
changes in the default z-order; but that also meant adding
CustomPaintOrderScrollView's own tests, including these two.)
gnprice added a commit to gnprice/flutter_customer_testing that referenced this pull request Apr 23, 2025
This updates to the following change in Zulip:
  zulip/zulip-flutter#1480
  zulip/zulip-flutter@88e7bcc

in order to unblock this PR:
  flutter/flutter#164818
auto-submit bot pushed a commit to flutter/tests that referenced this pull request Apr 24, 2025
This updates to the following change in Zulip:
  zulip/zulip-flutter#1480
  zulip/zulip-flutter@88e7bcc

in order to unblock this PR:
  flutter/flutter#164818

> [!IMPORTANT]
> This repository is a testing suite that contains references to tests (in the `registry` directory) that are run with every commit to Flutter
> to verify that no breaking changes have been introduced (in the "customer_testing" shards). Your merged PR is _not_ automatically run in the
> Flutter CI tree.
>
> After merging this PR, you must send another PR to flutter/flutter updating `dev/customer_testing/tests.version` in that repo with the latest git commit SHA of the flutter/test repo.
>
> See <flutter/flutter#162048> for details.
@@ -1 +1 @@
55deebbfa3f3b85e63b839fa3aa5a3ad0cf51607
c9cbc600e3974fb5f2fe505def53f1dc11cefda7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping this should be done separately. 🙏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK. I'd seen some other PRs do it this way, but I'll send a separate PR instead.

This does amplify the degree to which it'd be great to get the flutter/tests tree merged into the flutter/flutter monorepo to simplify this workflow 🙂 (cf #162041 (comment)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sent #167754.

And in writing it I discovered one reason why it's a good idea to do the bump in a separate PR: it looks like the last PR that intended such a bump, earlier today, accidentally ended up reverting a previous bump instead. I think the way that happened is that the version got independently updated while that PR was still pending, and then at merge time the conflict got resolved in the wrong direction.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
romanejaquez pushed a commit to romanejaquez/flutter that referenced this pull request Aug 14, 2025
Fixes flutter#145592.

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.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants