-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Control paint order / z-order of slivers #164818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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) . |
Piinks
left a comment
There was a problem hiding this 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) 🤞
|
Well: (That's the info that's provided on the GitHub check. I appreciate that it does give that information.) |
|
Yes, can confirm now, this is an issue not specific to this PR and is currently being worked on. Thanks for your patience @gnprice! |
|
I think this is resolved now, rebasing to give it another go! |
09e5816 to
e252330
Compare
|
@Piinks Google testing is marked as failed here but the CL seemed to have no failures on it? Maybe I'm reading something wrong. |
|
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. |
e252330 to
3ff7d01
Compare
|
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. |
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.)
|
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. |
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.)
|
Thanks Greg! We're still having issues with Google testing. Thank you for you patience. 🙏 |
3ff7d01 to
62391da
Compare
|
Catching up after being out - Google testing appears to be working now so we rebased. |
|
Checking back in here, good news Google testing passed! |
|
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.) |
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.)
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.)
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
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 | |||
There was a problem hiding this comment.
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. 🙏
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
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.
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.
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 wheneverCustomScrollView.centerisn'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.