Skip to content

Conversation

@goderbauer
Copy link
Member

@goderbauer goderbauer commented Apr 26, 2018

Viewport Cache

Adds a cacheExtent to viewports, which defines an area before and after the visible part of the viewport in axis direction. Elements, that fall into this cache area, will be layouted and included in the render tree even though they are not visible (yet). They are pre-cached because they will appear in the viewport next when the user starts scrolling.
Slivers are informed about the cacheExtent in their incoming SliverConstraints via the new cacheOrigin and remainingCacheExtent properties and they should compute a value for the new cacheExtent property in their SliverGeometry (see the corresponding doc comments for details).

Fixes #170.

Implicit a11y scrolling on iOS

The elements falling into the cache area are also included in the semantics tree, where they are marked as hidden. These are used on iOS to implement implicit a11y scrolling: When the user moves the focus from the last visible element in the viewport to one of these hidden elements via linear navigation (right/left swipes) the framework is asked to bring this hidden element into view and an implicit scroll action is performed (see flutter/engine#5052).
To figure out which SemanticsNodes should be marked as hidden in the tree the viewport defines a new semanticsClipRect in addition to the already present paintClipRect. While the paintClipRect has the size of the viewport (the visible part), the semanticsClipRect is extended by the cache areas at the beginning and end of the viewport. If a SemanticsNode falls outside of the semanticsClipRect it is not included in the semantics tree at all because it's currently unreachable by the user. If it is inside semanticsClipRect, but outside of paintClipRect it is included in the tree and marked as hidden to enable implicit a11y scrolling. If it is inside paintClipRect it is included like a regular SemanticsNode.

Fixes #11983.
Fixes #17023.

Also in this PR

RenderObjects that are currently not visible in the viewport are now considered offstage in tests. In order to find them with the Finders provided by the Flutter test framework you'll have to set skipOffstage: false on the Finder.

@goderbauer goderbauer requested a review from Hixie April 26, 2018 21:48
/// is different from the rect returned by [describeApproximatePaintClip].
/// If the semantics clip rect and the paint clip rect are the same, this
/// method returns null.
Rect describeSemanticsClip(covariant RenderObject child) => null;
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth adding a paragraph about the motivation and typical use

///
/// 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, th framework will bring
Copy link
Contributor

Choose a reason for hiding this comment

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

th->the

break;
}

return parentData.layoutOffset < renderObject.constraints.scrollOffset + renderObject.constraints.remainingPaintExtent && parentData.layoutOffset + itemExtent > renderObject.constraints.scrollOffset;
Copy link
Contributor

Choose a reason for hiding this comment

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

this line could benefit from some wrapping


@override
Rect describeSemanticsClip(RenderObject child) {
switch(axis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after switch

semanticBounds.bottom,
);
}
assert(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

the assert (!= null) should be before the switch

///
/// 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, th framework will bring
Copy link
Contributor

Choose a reason for hiding this comment

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

th->the here too

you might want to use a template rather than duplicating this

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL DartDoc templates. So cool.

///
/// 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, th framework will bring
Copy link
Contributor

Choose a reason for hiding this comment

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

th->the

/// The scroll view 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 rendered even though they are not
Copy link
Contributor

Choose a reason for hiding this comment

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

(here and everywhere else this text is included):
"rendered" means "shown on screen".
Maybe use the term "laid out"? Or "instantiated"?


@override
Rect describeSemanticsClip(RenderSliver child) {
switch(axis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same nits as before

maybe factor out this method into something where it can be reused directly?

case Axis.vertical:
return new Rect.fromLTRB(
semanticBounds.left,
semanticBounds.top - cacheExtent,
Copy link
Contributor

Choose a reason for hiding this comment

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

(same comment in the other instance of this method):

Why a fixed extend out in the scrolling axis, instead of near-infinity?

Copy link
Member Author

Choose a reason for hiding this comment

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

To keep the semantics tree compact.

In the context of the SingleChildViewport: Imagine a thousands of pixels long column inside the viewport with many child SemanticsNodes (one per cell). Only the semantics nodes that fall inside the visible range plus a handful of objects before and after the viewport are relevant. Everything else is overhead. This ensures we cut those other child semantic nodes out.

Same is true for the other Sliver-based viewport: Imagine a loooong sliver with many child semantics nodes. Again, only some of these are relevant.


// we made it without a correction, whee!
return 0.0;
return 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unexpected extra space

@Hixie
Copy link
Contributor

Hixie commented May 2, 2018

LGTM

@goderbauer goderbauer merged commit 7984f6e into flutter:master May 4, 2018
@goderbauer goderbauer deleted the ios-scroll-merge branch May 4, 2018 17:48
@passsy
Copy link
Contributor

passsy commented May 7, 2018

Great addition!

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
@goderbauer goderbauer added framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: scrolling Viewports, list views, slivers, etc. labels Feb 22, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

4 participants