-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Set cacheExtent for SliverFillRemaining widget #143612
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
Set cacheExtent for SliverFillRemaining widget #143612
Conversation
|
I'm aware of the Google testing failures. They're failing because of semantics golden text comparisons, which I think is expected and just needs to updated since this PR creates more semantic nodes than previously. @ChristianEdwardPadilla perhaps you could advise on that |
|
Yeah, the google failures all look expected and normal. They can be fixed and set up to roll in with this change by creating a g3 fix in the frob tool. |
| final double cacheExtent = calculateCacheOffset( | ||
| constraints, | ||
| from: 0.0, | ||
| to: constraints.viewportMainAxisExtent, |
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.
Should this be contraints.remainingCacheExtent?
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.
So calculateCacheOffset will use whichever is smaller - either the constraints.viewportMainAxisExtent or the contraints.remainingCacheExtent. The reason I use constraints.viewportMainAxisExtent is because that's the sliver's scrollExtent.
So the way I thought about it is that the RenderSliverFillRemainingWithScrollable widget has the max potential size of the viewportMainAxisExtent, even when part of it isn't visible and the child is the size of the remaining space in the viewport. So it's always taking up that much of the cacheExtent (or whatever is left of it). Also, remainingCacheExtent could be larger than the viewportMainAxisExtent, so it wouldn't make sense for it to use more cacheExtent than it's max potential size.
Does that make sense?
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.
Right! Yes, thank you for explaining that. I forget SliverFillRemaining is trickier then other slivers because it never actually extends into the cache extent, since it only fills the remaining space and no more than that. Gotcha. 👍
| if (child != null) { | ||
| double maxExtent = extent; | ||
|
|
||
| // If sliver has no extent, but is within viewport's cacheExtent, use the |
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.
This is really clever!
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.
Thanks!
| paintExtent: paintedChildSize, | ||
| maxPaintExtent: paintedChildSize, | ||
| hasVisualOverflow: extent > constraints.remainingPaintExtent || constraints.scrollOffset > 0.0, | ||
| cacheExtent: cacheExtent, |
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.
Should cacheExtent be updated to reflect what extent the child ended up being after laying out?
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.
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.
| final double cacheExtent = calculateCacheOffset( | ||
| constraints, | ||
| from: 0.0, | ||
| to: constraints.viewportMainAxisExtent, |
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.
Right! Yes, thank you for explaining that. I forget SliverFillRemaining is trickier then other slivers because it never actually extends into the cache extent, since it only fills the remaining space and no more than that. Gotcha. 👍
|
@Piinks I made a g3fix and tests are passing now, could you approve the PR? |
|
Oh yeah, sorry, this is the second PR that didn't actually process as approved. So strange. |
|
Waiting on approval of g3fix before merging (cl/611558507). |
|
Approval given, merging. |

When a Sliver with items is outside of the Viewport, but within the Viewport's
cacheExtent, the framework should create SemanticNodes for the items even though they are out of view. However, for this to work, the Sliver's geometry must have acacheExtent(how much space the sliver took up of the Viewport'scacheExtent) greater than 0, otherwise it is excluded.SliverFillRemainingwidgets that fall outside the viewport did not have this set and therefore were being excluded when SemanticNodes were created, even if they were within the Viewport'scacheExtent. This PR sets thecacheExtentforSliverFillRemainingwidgets.In addition,
RenderSliverFillRemainingWithScrollablewould get dropped from the semantic tree because it's child had a size of 0 when outside the remaining paint extent. To fix, we give the child amaxExtentof the sliver'scacheExtentif it's outside the remaining paint extent but within the viewport's cacheExtent.Fixes #142065.
Definitions:
RenderViewport.cacheExtent:SliverGeometry.cacheExtent:SliverContraints.remainingCacheExtent:Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.