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
Basic scroll semantics support #21764
Basic scroll semantics support #21764
Conversation
…c_scrolling_semantics
@@ -249,6 +251,12 @@ class SemanticsData extends Diagnosticable { | |||
/// if this node represents a text field. | |||
final TextSelection textSelection; | |||
|
|||
/// The count of children of a scroll node which contribute semantics. |
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.
Total count? Visible count? That's the guidance for the case when the child count is virtually unlimited?
@@ -1125,6 +1139,10 @@ class SemanticsNode extends AbstractNode with DiagnosticableTreeMixin { | |||
/// If this rect is null [parentSemanticsClipRect] also has to be null. | |||
Rect parentPaintClipRect; | |||
|
|||
/// The index of this node with respect to its parent, including any hidden | |||
/// siblings. |
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.
nit: I do not understand this dartdoc :)
What does "with respect to" mean when talking about an index? And how can siblings be included with respect to the parent? 😕
Is being hidden determined by some property? Could you link to that property from the doc?
@@ -1415,7 +1434,8 @@ class SemanticsNode extends AbstractNode with DiagnosticableTreeMixin { | |||
|
|||
int _flags = _kEmptyConfig._flags; | |||
|
|||
bool _hasFlag(SemanticsFlag flag) => _flags & flag.index != 0; | |||
/// Whether the semantics node currently has a given [SemanticsFlag]. |
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.
nit: I'd prefer to say "Whether this node currently..."
@@ -1479,6 +1499,14 @@ class SemanticsNode extends AbstractNode with DiagnosticableTreeMixin { | |||
TextSelection get textSelection => _textSelection; | |||
TextSelection _textSelection; | |||
|
|||
/// The count of children of a scroll node which contribute semantics. | |||
int get scrollChildren => _scrollChildren; |
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.
nit: scrollChildren
sounds like the getter returns the children, while in fact it returns count. How about call it scrollChildCount
?
@@ -172,6 +173,20 @@ abstract class ScrollView extends StatelessWidget { | |||
/// {@macro flutter.rendering.viewport.cacheExtent} | |||
final double cacheExtent; | |||
|
|||
/// The number of children which will contribute semantic information. |
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.
nit: "that will"? I think when you say "which" you refer to "the number", not "children".
@@ -161,6 +163,20 @@ class Scrollable extends StatefulWidget { | |||
/// exclusion. | |||
final bool excludeFromSemantics; | |||
|
|||
/// The number of children which will contribute semantic information. |
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.
nit: "which" => "that"
/// Whether to wrap each child in an [IndexedChildSemantics]. | ||
/// | ||
/// Defaults to true. | ||
final bool addSemanticIndexes; |
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 property reads like a method name. isAddSemanticIndexes
? Also, do we prefer "indexes" or "indices"? We should probably be consistent with the rest of the API.
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.
ag -Q indices | wc -l
-> 36
ag -Q indexes | wc -l
-> 47
Guess we are agnostic
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.
I think this matches the naming of the other config
/// Whether to wrap each child in an [IndexedChildSemantics]. | ||
/// | ||
/// Defaults to true. | ||
final bool addSemanticIndexes; |
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.
ditto
@@ -48,6 +48,8 @@ class TestSemantics { | |||
this.transform, | |||
this.textSelection, | |||
this.children = const <TestSemantics>[], | |||
this.scrollIndex, | |||
this.scrollChildren, |
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.
One idea I had in the past, but never got to implement, was to make this more flexible by taking Matcher
as a value. Then you could use an any
matcher to indicate that you don't care about the value. I don't know if it's helpful in this case.
@@ -135,6 +135,7 @@ class CupertinoDemoTab1 extends StatelessWidget { | |||
Widget build(BuildContext context) { | |||
return CupertinoPageScaffold( | |||
child: CustomScrollView( | |||
semanticChildren: 50, |
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.
semanticsChildrenCount? children sounds like I can pass in the actual children?
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.
Not sure about semanticChildrenCount vs semanticsChildrenCount
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.
uber nitty nit: Almost everywhere else we say "childCount" rather than "childrenCount". If we insist on "children", then perhaps we should say "numberOfChildren".
/// while the [new ListView.separated] constructor will use half that amount. | ||
/// | ||
/// For [CustomScrollView] and other types which do not receive a builder | ||
/// or list of widgets, the child count must be explicitly provided. |
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.
What if the amount is infinity?
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.
Somewhere (maybe on the CutomScrollWidget) there should be a section # Accessibility
documenting how to make a custom scroller accessible. Currently, that documentation is kinda all over the place.
/// semantics. For example, the [ScrollView] uses the index of the first | ||
/// visible child semantics node to determine the | ||
/// [SemanticsConfiguration.scrollIndex]. | ||
class IndexedChildSemantics extends SingleChildRenderObjectWidget { |
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.
Can we add an example of where and when somebody should use this?
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.
Can you address this comment, too?
…flutter into basic_scrolling_semantics
@@ -0,0 +1,8 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
Is this supposed to be included in this PR?
/// semantics. For example, the [ScrollView] uses the index of the first | ||
/// visible child semantics node to determine the | ||
/// [SemanticsConfiguration.scrollIndex]. | ||
class IndexedChildSemantics extends SingleChildRenderObjectWidget { |
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.
Can you address this comment, too?
@@ -234,6 +234,12 @@ class ScrollPhysics { | |||
/// scroll position to fulfill such a request. | |||
bool get allowImplicitScrolling => true; | |||
|
|||
/// Generates a semantic scroll value, or null if none should be created. | |||
String semanticScrollValue({ |
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 probably should have more documentation. In what cases should this return a non-null value? What's an example value this should return in that case?
@@ -234,6 +234,12 @@ class ScrollPhysics { | |||
/// scroll position to fulfill such a request. | |||
bool get allowImplicitScrolling => true; | |||
|
|||
/// Generates a semantic scroll value, or null if none should be created. | |||
String semanticScrollValue({ |
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.
Is it intentional that this is not overwritten anywhere with an actual implementation? Where is this one used?
Don't worry I'm still working on this! Will update when it is ready for review again... need to remember what I was doing. |
…flutter into basic_scrolling_semantics
I believe I've addressed all of the comments wrt documentation. Whenever you have a chance PTAL - not tonight of course :) |
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 if LGT @goderbauer
@@ -816,9 +882,13 @@ class ListView extends BoxScrollView { | |||
? itemBuilder(context, itemIndex) | |||
: separatorBuilder(context, itemIndex); | |||
}, | |||
childCount: math.max(0, itemCount * 2 - 1), | |||
childCount: math.max(0, itemCount * 2 - 1), // same calculation as in semanticChildrenCount below. |
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.
nit: semanticChildCount (in the 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 with some minor nits.
examples/flutter_gallery/lib/demo/cupertino/cupertino_navigation_demo.dart
Outdated
Show resolved
Hide resolved
@@ -317,6 +334,40 @@ abstract class ScrollView extends StatelessWidget { | |||
/// ) | |||
/// ``` | |||
/// | |||
/// ## Accessibility |
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.
+1 for this explanation.
I just added some more sample code sections to the SliverDelegates - should I add identical code + comments to each or just a note that says |
"See other" sounds fine to me. |
❤️ for all the samples! |
@jonahwilliams I am trying to determine when an item is scrolled off of the screen, and this seems like what I need, but I don't understand how get |
This is probably not the API you are looking for, since it requires an accessibility service to be running. |
Gotcha, thanks! |
This change requires an engine roll that hasn't happened yet and updates to several dozen tests.Note: The concept of "List" as given here is what a user of an app would think of a list.
Given scrollIndex, the first visible child of a list and scrollChildren, the total number of items in a list, TalkBack and VoiceOver will provide additional context to the user:
Showing items ${scrollIndex} to ${scrollIndex + visibleCount} of ${scrollChildren}
Or
Showing item ${scrollIndex} of ${scrollChildren}
This change requires a corresponding engine change which adds
scrollIndex
andscrollChildren
to the semantics node. Having done that, what remains is to correctly pass this information through.So given the three pieces of information needed above, we can start by having Scrollable/ScrollView take "scrollChildren" as a parameter. I called this field "semanticChildren" to distinguish it. In the case of ListView and other subclasses, we can usually infer this value from the length of children or item count passed - but we always allow it to be specified just in case they are different. For the CustomScrollView, the value always needs to be provided.
The next piece of information we need to pipe through is the first visible index. We already know what semantic node is visible by the time assembleSemanticNodes is called in the Scrollable, but at this point we have no to figure out what the index of this was, since slivers are lazily rendered. To solve this, I added
IndexedChildSemantics/RenderIndexedChildSemantics
which allows tagging a semantics node withindexInParent
. Then we can simply read this value off of the semantic node.It is the responsibility of specific subclasses to handle tagging correctly. For the
SliverChildListDelegate
andSliverChildBuilderDelegate
classes I added three parameters to control this:addSemanticIndexes - defaults to true, whether to apply indexedChildSemantics. This would allow users to provide their own if necessary.
semanticIndexOffset - defaults to 0, an offset to apply to each index added to a given child. For example, if you have a CustomScrollView with two SliverLists, you can give the second a semanticIndexOffset equal to the length of the first.
semanticIndexCallback - defaults to always returning an index. A callback which takes a widget and the local index and provides a new index, with null based short circuit. This allows ListView.separated ti ignore odd indexes.
Together all of these additions allow the delegates to pass their builder's index to the child semantics.
The final piece of information, the number of visible children, is handled automatically in the engine by checking for the last child semantics node that isn't hidden.
For simple cases, like ListView/ListView.builder/ListView.separated no change needs to be made.
Fixes #10595