anchors 3/n: Fix hit-testing when header overflows sliver #1316
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
This is adapted lightly from an example app I made back in the first week of the zulip-flutter project, 2022-12-23, as part of developing the sticky_header library. The example app was extremely helpful then for experimenting with changes and seeing the effects visually and interactively, as well as for print-debugging such experiments. So let's get it into the tree. The main reason I didn't send the example app back then is that it was a whole stand-alone app tree under example/, complete with all the stuff in android/ and ios/ and so on that `flutter create` spits out for setting up a Flutter app. That's pretty voluminous: well over 100 different files totalling about 1.1MB on disk. I did't want to permanently burden the repo with all that, nor have to maintain it all over time. Happily, I realized today that we can skip that, and still have a perfectly good example app, by reusing that infrastructure from the actual Zulip app. That way all we need is a Dart file with a `main` function, corresponding to the old example's `lib/main.dart` which was the only not-from-the-template code in the whole example app. So here it is. Other than moving the Dart file and discarding the rest, the code I wrote back then has been updated to our current formatting style; adjusted slightly for changes in Flutter's Material widgets; and updated for changes I made to the sticky_header API after that first week.
It's already a fact that the header's size in each dimension is non-negative and finite; the framework asserts that in the `layout` implementation (via debugAssertDoesMeetConstraints). So that includes `headerExtent`; and then `paintedHeaderSize` is bounded to between zero and that value.
This is the right thing, as the comment explains. Conveniently it's also simpler.
…y; note a bug The logic below -- particularly in the allowOverflow true case, where it constructs a new SliverGeometry from scratch -- has been implicitly relying on several of these facts already. These wouldn't be true of an arbitrary sliver child; but this sliver knows what type of child it actually has, and they are true of that one. So write down the specific assumptions we can take from that. Reading through [RenderSliverList.performLayout] to see what it can produce as the child geometry also made clear there's another case that this method isn't currently correctly handling at all: the case where scrollOffsetCorrection is non-null. Filed an issue for that; add a todo-comment for it.
Fixes zulip#1309. Fixes zulip#725. This is necessary when scrolling back to the origin over items that grew taller while off screen (and beyond the 250px of near-on-screen cached items). For example that can happen because a message was edited, or because new messages came in that were taller than those previously present.
…sumptions Because the child's hitTestExtent equals its paintExtent, this was producing a new hitTestExtent equal to the new paintExtent. But that's the same behavior the SliverGeometry constructor gives us by default.
This relies on (and expresses) an assumption in the new assertions: that the child's layoutExtent equals paintExtent. That assumption will be helpful in keeping this logic manageable to understand as we add an upcoming further wrinkle.
In particular this will affect the upper sliver (with older messages) in the message list, when we start using two slivers there in earnest.
This is the class we actually use at this point -- not StickyHeaderListView -- so it's good for it to have some docs too.
This way the example can be used to demonstrate the next cluster of bug fixes working correctly, before yet fixing the next issue after those.
This is nearly NFC. The sense in which it isn't is that if any of the `tester.drag` steps touched a header -- which listens for tap gestures -- then this would ensure that step got interpreted as a drag. The old version would (a) interpret it as a tap, not a drag, if it was less than 20px in length; (b) leave those first 20px out of the effective length of the drag. The use of DragStartBehavior.down fixes (b). Then there are some drags of 5px in these tests, subject to (a); TouchSlop fixes those (by reducing the touch slop to 1px, instead of 20px). This change will be needed in order to have the item widgets start recording taps too, like the header widgets do, without messing up the drags in these tests.
This will be useful in a test we'll add for an upcoming change.
…nonzero This fixes a latent bug: this method would give wrong answers if the sliver's paintOrigin were nonzero. See the new comments. The bug is latent because performLayout currently always produces a zero paintOrigin. But we'll start using paintOrigin soon, as part of making hit-testing work correctly when a sticky header is painted by one sliver but needs to encroach on the layout area of another sliver. The framework calls this method as part of hit-testing, so that requires fixing this bug too.
This commit is NFC for the actual app, or at least nearly so. This call to calculatePaintOffset was conceptually wrong: it's asking how much of this sliver's region to be painted is within the range of scroll offsets from zero to headerExtent. That'd be a pertinent question if we were locating something in that range of scroll offsets... but that range is not at all where the header goes, unless by happenstance. So the value returned is meaningless. One reason this buggy line has survived is that the bug is largely latent -- we can remove it entirely, as in this commit, and get exactly the same behavior except in odd circumstances. Specifically: * This paintedHeaderSize variable can only have any effect by being greater than childExtent. * That requires childExtent to be smaller than headerExtent. * The main way that childExtent can be so small is if remainingPaintExtent, which constrains it, is equally small. * But calculatePaintOffset constrains its result, aka paintedHeaderSize, to at most remainingPaintExtent too, so then paintedHeaderSize still won't exceed childExtent. I say "main way" because the alternative is for the child to run out of content before finding as much as headerExtent of content to show. That could happen if the list just has less than that much content; but that means the header's own item is smaller than the header, which is a case that sticky_header doesn't really support well anyway and we don't have in the app. Otherwise, this would have to mean that some of the content was scrolled out of the viewport and then the child ran out of content before filling its allotted remainingPaintExtent of the viewport (and indeed before even reaching a headerExtent amount of content). This is actually not quite impossible, if the scrollable permits overscroll... but making it happen would require piling edge case upon edge case. Anyway, this call never made sense, so remove it. The resulting code still isn't right if headerExtent > childExtent. Removing this wrong logic helps clear the ground for fixing that.
When the sticky header overflows the sliver that provides it -- that is, when the sliver boundary is scrolled to within the area the header covers -- the existing code already got the right visual result, painting the header at its full size. But it didn't work properly for hit-testing: trying to tap the header in the portion where it's overflowing wouldn't work, and would instead go through to whatever's underneath (like the top of the next sliver). That's because the geometry it was reporting from this `performLayout` method didn't reflect the geometry it would actually paint in the `paint` method. When hit-testing, that reported geometry gets interpreted by the framework code before calling this render object's other methods. Fix that by reporting an accurate `paintOrigin` and `paintExtent`. After this fix, sticky headers overflowing into the next sliver seem to work completely correctly... as long as the viewport paints the slivers in the necessary order. We'll take care of that next.
gnprice
force-pushed
the
pr-sticky-hittest
branch
from
90e4b5f to
9ec7683
Compare
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 #1312 (and is stacked atop #1312), fixing another cluster of latent bugs in the sticky_header library to prepare for having two slivers share space back to back in the list.
After #1312, the headers paint correctly in that case. But when the sliver boundary is scrolled to within the header, the hit-testing behavior isn't yet right: trying to tap on the bottom part of the header, where it's overflowing over the bottom sliver, ends up hitting the bottom sliver instead of the header. This PR fixes that.
… Well, it does if the viewport gives the slivers the right paint order (and hit-test order). Making that happen with back-to-back slivers, like the message list needs, requires some more code; and this PR is long enough already. So we'll save that for the next PR.
Selected commit messages
54c2b68 sticky_header example: Enable ink splashes, to demo hit-testing
2c3a6ea sticky_header example: Set allowOverflow true in double-sliver example
421415b sticky_header [nfc]: Fix childMainAxisPosition to handle paintOrigin nonzero
This fixes a latent bug: this method would give wrong answers if the
sliver's paintOrigin were nonzero. See the new comments.
The bug is latent because performLayout currently always produces a
zero paintOrigin. But we'll start using paintOrigin soon, as part of
making hit-testing work correctly when a sticky header is painted by
one sliver but needs to encroach on the layout area of another sliver.
The framework calls this method as part of hit-testing, so that
requires fixing this bug too.
a76ba67 sticky_header: Fix hit-testing when header overflows sliver
When the sticky header overflows the sliver that provides it -- that
is, when the sliver boundary is scrolled to within the area the
header covers -- the existing code already got the right visual
result, painting the header at its full size.
But it didn't work properly for hit-testing: trying to tap the
header in the portion where it's overflowing wouldn't work, and
would instead go through to whatever's underneath (like the top of
the next sliver). That's because the geometry it was reporting from
this
performLayoutmethod didn't reflect the geometry it wouldactually paint in the
paintmethod. When hit-testing, thatreported geometry gets interpreted by the framework code before
calling this render object's other methods.
Fix that by reporting an accurate
paintOriginandpaintExtent.After this fix, sticky headers overflowing into the next sliver
seem to work completely correctly... as long as the viewport paints
the slivers in the necessary order. We'll take care of that next.
90e4b5f sticky_header [nfc]: Doc overflow behavior and paint-order constraints