Skip to content

Conversation

@vashworth
Copy link
Contributor

@vashworth vashworth commented Feb 16, 2024

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 a cacheExtent (how much space the sliver took up of the Viewport's cacheExtent) greater than 0, otherwise it is excluded.

SliverFillRemaining widgets 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's cacheExtent. This PR sets the cacheExtent for SliverFillRemaining widgets.

In addition, RenderSliverFillRemainingWithScrollable would 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 a maxExtent of the sliver's cacheExtent if it's outside the remaining paint extent but within the viewport's cacheExtent.

Fixes #142065.

Definitions:

  • RenderViewport.cacheExtent:
    /// The viewport has an area before and after the visible area to cache items
    /// that are about to become visible when the user scrolls.
    ///
    /// Items that fall in this cache area are laid out even though they are not
    /// (yet) visible on screen. The [cacheExtent] describes how many pixels
    /// the cache area extends before the leading edge and after the trailing edge
    /// of the viewport.
    ///
    /// The total extent, which the viewport will try to cover with children, is
    /// [cacheExtent] before the leading edge + extent of the main axis +
    /// [cacheExtent] after the trailing edge.
    ///
    /// The cache area is also used to implement implicit accessibility scrolling
    /// on iOS: When the accessibility focus moves from an item in the visible
    /// viewport to an invisible item in the cache area, the framework will bring
    /// that item into view with an (implicit) scroll action.
  • SliverGeometry.cacheExtent:
    /// How many pixels the sliver has consumed in the
    /// [SliverConstraints.remainingCacheExtent].
  • SliverContraints.remainingCacheExtent:
    /// Describes how much content the sliver should provide starting from the
    /// [cacheOrigin].
    ///
    /// Not all content in the [remainingCacheExtent] will be visible as some
    /// of it might fall into the cache area of the viewport.
    ///
    /// Each sliver should start laying out content at the [cacheOrigin] and
    /// try to provide as much content as the [remainingCacheExtent] allows.

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.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Feb 16, 2024
@vashworth vashworth marked this pull request as ready for review February 22, 2024 21:46
@vashworth vashworth requested a review from Piinks February 22, 2024 21:46
@vashworth
Copy link
Contributor Author

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

@Piinks
Copy link
Contributor

Piinks commented Feb 23, 2024

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really clever!

Copy link
Contributor Author

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,
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vashworth vashworth requested a review from Piinks February 27, 2024 16:23
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.

Flutter_LGTM

final double cacheExtent = calculateCacheOffset(
constraints,
from: 0.0,
to: constraints.viewportMainAxisExtent,
Copy link
Contributor

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. 👍

@vashworth
Copy link
Contributor Author

@Piinks I made a g3fix and tests are passing now, could you approve the PR?

@Piinks
Copy link
Contributor

Piinks commented Feb 29, 2024

Oh yeah, sorry, this is the second PR that didn't actually process as approved. So strange.

@vashworth
Copy link
Contributor Author

vashworth commented Mar 11, 2024

Waiting on approval of g3fix before merging (cl/611558507).

@vashworth
Copy link
Contributor Author

Approval given, merging.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App 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.

[iOS] [a11y] Unable to navigate to bottom sliver in CustomScrollView with VoiceOver

2 participants