Basic scroll semantics support by jonahwilliams · Pull Request #21764 · flutter/flutter

jonahwilliams · 2018-09-12T23:53:13Z

~~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 and scrollChildren 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 with indexInParent. 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 and SliverChildBuilderDelegate 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

…ing_semantics

…c_scrolling_semantics

yjbanov · 2018-09-27T01:17:17Z

  /// if this node represents a text field.
  final TextSelection textSelection;

+  /// The count of children of a scroll node which contribute semantics.


Total count? Visible count? That's the guidance for the case when the child count is virtually unlimited?

yjbanov · 2018-09-27T01:17:51Z

  Rect parentPaintClipRect;

+  /// The index of this node with respect to its parent, including any hidden
+  /// siblings.


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?

yjbanov · 2018-09-27T01:21:24Z

  int _flags = _kEmptyConfig._flags;

-  bool _hasFlag(SemanticsFlag flag) => _flags & flag.index != 0;
+  /// Whether the semantics node currently has a given [SemanticsFlag].


nit: I'd prefer to say "Whether this node currently..."

yjbanov · 2018-09-27T01:22:19Z

  TextSelection _textSelection;

+  /// The count of children of a scroll node which contribute semantics.
+  int get scrollChildren => _scrollChildren;


nit: scrollChildren sounds like the getter returns the children, while in fact it returns count. How about call it scrollChildCount?

yjbanov · 2018-09-27T01:27:17Z

  /// {@macro flutter.rendering.viewport.cacheExtent}
  final double cacheExtent;

+  /// The number of children which will contribute semantic information.


nit: "that will"? I think when you say "which" you refer to "the number", not "children".

yjbanov · 2018-09-27T01:30:01Z

  ///    exclusion.
  final bool excludeFromSemantics;

+  /// The number of children which will contribute semantic information.


nit: "which" => "that"

yjbanov · 2018-09-27T01:33:24Z

+  /// Whether to wrap each child in an [IndexedChildSemantics].
+  ///
+  /// Defaults to true.
+  final bool addSemanticIndexes;


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.

ag -Q indices | wc -l -> 36
ag -Q indexes | wc -l -> 47

Guess we are agnostic

I think this matches the naming of the other config

yjbanov · 2018-09-27T01:34:11Z

+  /// Whether to wrap each child in an [IndexedChildSemantics].
+  ///
+  /// Defaults to true.
+  final bool addSemanticIndexes;


yjbanov · 2018-09-27T01:38:00Z

    this.textSelection,
    this.children = const <TestSemantics>[],
+    this.scrollIndex,
+    this.scrollChildren,


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.

goderbauer · 2018-09-27T16:41:41Z

  Widget build(BuildContext context) {
    return CupertinoPageScaffold(
      child: CustomScrollView(
+        semanticChildren: 50,


semanticsChildrenCount? children sounds like I can pass in the actual children?

Not sure about semanticChildrenCount vs semanticsChildrenCount

uber nitty nit: Almost everywhere else we say "childCount" rather than "childrenCount". If we insist on "children", then perhaps we should say "numberOfChildren".

goderbauer · 2018-09-27T16:46:03Z

+  /// 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.


What if the amount is infinity?

goderbauer

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.

goderbauer · 2018-09-27T19:27:58Z

+/// semantics. For example, the [ScrollView] uses the index of the first
+/// visible child semantics node to determine the
+/// [SemanticsConfiguration.scrollIndex].
+class IndexedChildSemantics extends SingleChildRenderObjectWidget {


Can we add an example of where and when somebody should use this?

Can you address this comment, too?

…flutter into basic_scrolling_semantics

goderbauer · 2018-10-01T17:55:12Z

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>


Is this supposed to be included in this PR?

goderbauer · 2018-10-01T17:57:12Z

+/// semantics. For example, the [ScrollView] uses the index of the first
+/// visible child semantics node to determine the
+/// [SemanticsConfiguration.scrollIndex].
+class IndexedChildSemantics extends SingleChildRenderObjectWidget {


Can you address this comment, too?

goderbauer · 2018-10-01T17:58:22Z

  bool get allowImplicitScrolling => true;

+  /// Generates a semantic scroll value, or null if none should be created.
+  String semanticScrollValue({


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?

goderbauer · 2018-10-01T18:03:24Z

  bool get allowImplicitScrolling => true;

+  /// Generates a semantic scroll value, or null if none should be created.
+  String semanticScrollValue({


Is it intentional that this is not overwritten anywhere with an actual implementation? Where is this one used?

jonahwilliams · 2018-10-01T18:16:16Z

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.

…ing_semantics

…flutter into basic_scrolling_semantics

jonahwilliams · 2018-10-04T02:20:07Z

I believe I've addressed all of the comments wrt documentation. Whenever you have a chance PTAL - not tonight of course :)

yjbanov

LGTM if LGT @goderbauer

yjbanov · 2018-10-04T20:03:19Z

             : separatorBuilder(context, itemIndex);
         },
-         childCount: math.max(0, itemCount * 2 - 1),
+         childCount: math.max(0, itemCount * 2 - 1), // same calculation as in semanticChildrenCount below.


nit: semanticChildCount (in the comment)

goderbauer

LGTM with some minor nits.

goderbauer · 2018-10-05T17:09:40Z

 /// )
 /// ```
 ///
+/// ## Accessibility


+1 for this explanation.

…ing_semantics

jonahwilliams · 2018-10-05T21:07:08Z

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]?

goderbauer · 2018-10-05T21:15:49Z

"See other" sounds fine to me.

goderbauer · 2018-10-05T21:17:35Z

❤️ for all the samples!

…ing_semantics

rsnider19 · 2019-03-29T00:07:36Z

@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 scrollIndex. Is this something that we have access to, or is it just meant for the accessibility frameworks?

jonahwilliams · 2019-03-29T00:19:35Z

This is probably not the API you are looking for, since it requires an accessibility service to be running.

rsnider19 · 2019-03-29T00:38:42Z

Gotcha, thanks!

basic scroll semantics support

e548c01

googlebot added the cla: yes label Sep 12, 2018

jonahwilliams mentioned this pull request Sep 12, 2018

Add scrollIndex and scrollChildren to semantics, add Android implementation flutter-team-archive/engine#6239

Merged

jonahwilliams and others added 5 commits September 13, 2018 15:36

Initial implementation of accessibility scrolling semantics

b1c21d5

Delete IDEWorkspaceChecks.plist

63f4203

Merge branch 'master' of github.com:flutter/flutter into basic_scroll…

0cc1332

…ing_semantics

Merge branch 'master' of https://github.com/flutter/flutter into basi…

1b1e6bc

…c_scrolling_semantics

fix tests and move index between keep alive and wrap

9e85c55

jonahwilliams changed the title ~~[WIP] basic scroll semantics support~~ Basic scroll semantics support Sep 26, 2018

jonahwilliams requested review from goderbauer and yjbanov September 26, 2018 19:20

add missing args

a0a062a

yjbanov reviewed Sep 27, 2018

View reviewed changes

goderbauer reviewed Sep 27, 2018

View reviewed changes

Jonah Williams added 4 commits September 27, 2018 10:19

rewrite scroll children doc comment

7330455

rename members and address comments

2f4562b

rename semanticChildren to semanticChildrenCount

fbba3cd

Add missing file

3a2b72f

goderbauer reviewed Sep 27, 2018

View reviewed changes

jonahwilliams added 2 commits September 28, 2018 11:51

Merge branch 'basic_scrolling_semantics' of github.com:jonahwilliams/…

c3eeaa9

…flutter into basic_scrolling_semantics

rename some stuff

3702ab2

goderbauer reviewed Oct 1, 2018

View reviewed changes

jonahwilliams and others added 7 commits October 1, 2018 18:25

add an accessibility section to documentation

4bcadf1

Merge branch 'master' of github.com:flutter/flutter into basic_scroll…

e9bacc5

…ing_semantics

update documentation comments

dd515de

remove scroll announcements

87d2ba3

Delete WorkspaceSettings.xcsettings

fda49be

add see more to render indexed child semantics

1d4c819

Merge branch 'basic_scrolling_semantics' of github.com:jonahwilliams/…

11d1fa4

…flutter into basic_scrolling_semantics

jonahwilliams added 2 commits October 3, 2018 19:14

rename indexedChildSemantics to indexedSemantics

82e53ef

remove last semanticChildCount

77f0bb6

Jonah Williams and others added 2 commits October 4, 2018 10:15

Update basic.dart

dbbfa23

remove semicolon

5e44ed6

yjbanov approved these changes Oct 4, 2018

View reviewed changes

jonahwilliams added 3 commits October 4, 2018 13:22

fix comment

52e560d

add missing comma

e6659e1

maybe this works?

386fe1a

jonahwilliams commented Oct 4, 2018

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/basic.dart

maybe try indenting?

45a81d4

goderbauer approved these changes Oct 5, 2018

View reviewed changes

jonahwilliams added 2 commits October 5, 2018 13:45

Merge branch 'master' of github.com:flutter/flutter into basic_scroll…

ebe1711

…ing_semantics

Address comments and add more sample code

fe10974

refactor ListView.separated constructor to use helper function

e487b8b

see other

8015694

jonahwilliams mentioned this pull request Oct 5, 2018

Implement scrolling announcements for iOS #22747

Closed

Merge branch 'master' of github.com:flutter/flutter into basic_scroll…

aac90f4

…ing_semantics

jonahwilliams merged commit fb7a593 into flutter:master Oct 9, 2018

jonahwilliams deleted the basic_scrolling_semantics branch October 9, 2018 21:16

marcglasberg mentioned this pull request Mar 2, 2019

Return index of the first visible item in ListView #19941

Open

github-actions Bot locked as resolved and limited conversation to collaborators Aug 7, 2021

Conversation

jonahwilliams commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Oct 1, 2018

Uh oh!

jonahwilliams commented Oct 4, 2018

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented Oct 5, 2018

Uh oh!

goderbauer commented Oct 5, 2018

Uh oh!

goderbauer commented Oct 5, 2018

Uh oh!

jonahwilliams commented Sep 12, 2018 •

edited

Loading