AccessibilityBridge support for edge triggered semantics (iOS + Android) #4901

jonahwilliams · 2018-03-29T20:40:44Z

Engine support for flutter/flutter#13704, still requires another PR which adds support to the semantics object.

FIXED: ~~Currently the iOS version loses device connection on the simulator during some transitions~~

…lity

jonahwilliams · 2018-03-29T20:52:56Z

CC @yjbanov and @cbracken for expert level objective c++ readability.

yjbanov · 2018-03-29T21:04:34Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+    } else  {
+      NSString* latestRoute = routes[[routes count] - 1];
+      if (![latestRoute isEqualToString:_previous_route_]) {
+        _previous_route_ = latestRoute;


@cbracken to the rescue! I think here you should explicitly retain the object as you are assigning to a C++ class field, which won't do any ref counting for you. Similarly, when you nil it out, you should explicitly release it.

This fixed the crash!

yjbanov · 2018-03-29T21:06:10Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

         ([self node].actions & ~blink::kScrollableSemanticsActions) != 0;
 }

+- (void)latestRoutes:(NSMutableArray*)routes {


The name of this method does not convey the fact that it populates the argument.

renamed to collectRoutes

…alteRoutes

yjbanov · 2018-03-29T21:24:28Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

  [accessibility_channel_.get() setMessageHandler:^(id message, FlutterReply reply) {
    HandleEvent((NSDictionary*)message);
  }];
+  _previous_route_ = nil;


Release before setting to nil?

I think this is actually just wrong, moved to destructor

yjbanov · 2018-03-29T21:25:09Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+    [root collectRoutes:routes];
+    if ([routes count] == 0) {
+      NSString* oldRoute = _previous_route_;
+      [oldRoute release];


I think the two lines above are the same as [_previous_route_ release];.

yjbanov · 2018-03-29T21:26:26Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

  fml::scoped_nsobject<NSMutableDictionary<NSNumber*, SemanticsObject*>> objects_;
  fml::scoped_nsprotocol<FlutterBasicMessageChannel*> accessibility_channel_;
  fml::WeakPtrFactory<AccessibilityBridge> weak_factory_;
+  NSString* _previous_route_;


I believe we only use the postfix _

yjbanov · 2018-03-29T21:36:12Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

  [accessibility_channel_.get() setMessageHandler:^(id message, FlutterReply reply) {
    HandleEvent((NSDictionary*)message);
  }];
-  _previous_route_ = nil;


You should probably still initialize it to nil so it does not contain garbage (C++ is this way), but for better readability, do so in the initializer list.

yjbanov · 2018-03-29T21:37:19Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm


 AccessibilityBridge::~AccessibilityBridge() {
  view_.accessibilityElements = nil;
+  [previous_route_ release];


If you just release and not nil it out, it will continue holding onto a dangling pointer (or worse, a live pointer with a random lifecycle).

yjbanov · 2018-03-29T21:37:34Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

-      NSString* oldRoute = _previous_route_;
-      [oldRoute release];
-      _previous_route_ = nil;
+      [previous_route_ release];


And here. Setting to nil is likely still necessary.

Hixie · 2018-03-29T22:10:16Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

            assert globalRect != null;

+            if (routeName != null) {
+                routeNames.add(routeName);


since you're only reading the last node in the list, you only need to remember the last string you've seen, not all of them. so you should be able to do away with the list entirely.

ditto on the iOS side

Because of my original lack of retain and release the list version lasted slightly longer before crashing on the iOS side - I'll clean that up.

…lity

…nto pageAccessibility

Hixie · 2018-04-12T05:05:31Z

lib/ui/semantics.dart

+  /// 
+  /// This is used by certain widgets like Drawers and Dialogs, to indicate
+  /// that the node's semantic value can be used for an edge triggered
+  /// semantic update.


maybe mention that changing the value is meaningless

Maybe slightly more specific: ... changing the value of the active edge is meaningless....

goderbauer · 2018-04-12T16:37:13Z

lib/ui/semantics.dart

  static const int _kIsInMutuallyExclusiveGroupIndex = 1 << 8;
  static const int _kIsHeaderIndex = 1 << 9;
  static const int _kIsObscuredIndex = 1 << 10;
+  static const int _kIsEdgeIndex = 1 << 11;


Why not isRoute and isRouteName? Makes it clearer that the two belong together and I have an immediate idea what they mean. I don't think we use the term "edge" anywhere else in the framework with this meaning?

goderbauer · 2018-04-12T16:38:39Z

lib/ui/semantics.dart

+  /// 
+  /// This is used by certain widgets like Drawers and Dialogs, to indicate
+  /// that the node's semantic value can be used for an edge triggered
+  /// semantic update.


Maybe slightly more specific: ... changing the value of the active edge is meaningless....

goderbauer · 2018-04-12T16:42:07Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

    }

+    private void createWindowChangeEvent(SemanticsObject route) {
+        // TYPE_WINDOW_STATE_CHANGED events should always be sent from the root node.


Looks like this comment is out of date? You are using the specific node id now.

goderbauer · 2018-04-12T16:43:09Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

        boolean isFocusable() {
            int scrollableActions = Action.SCROLL_RIGHT.value | Action.SCROLL_LEFT.value
                    | Action.SCROLL_UP.value | Action.SCROLL_DOWN.value;
+            if (hasFlag(Flag.IS_EDGE) && value == null) {


Do we enforce on the framework side that no other relevant information get merged into the IS_EDGE node?

Not yet, but I can make sure the framework created semantics nodes have explicitChildNodes set

goderbauer · 2018-04-12T16:46:29Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

+        String getRouteName() {
+            String name = null;
+            if (children != null) {
+                for (int i = children.size() - 1; i >= 0; --i) {


Remind me why we want the last name and not the first one?

We do want the first one if it is non null & non-empty, cleaned up and added comment

goderbauer · 2018-04-12T16:47:22Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

+            String name = null;
+            if (children != null) {
+                for (int i = children.size() - 1; i >= 0; --i) {
+                    String newName = children.get(i).getRouteName();


Should we stop the recursion if we find another route/edge nested in the current one?

Yes, I've cleaned up these traversal algorithms to not be ... backwards

goderbauer · 2018-04-12T16:48:10Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+      }
+    }
+  }
+  if ([self node].HasFlag(blink::SemanticsFlags::kIsRoute)) {


Check this first before doing the loop above?

…lity

…nto pageAccessibility

…e name

jonahwilliams · 2018-04-12T23:32:44Z

Updated the android accessibility bridge to use label + value + hint instead of just value.

goderbauer · 2018-04-13T16:18:21Z

lib/ui/semantics.dart

+  /// 
+  /// When a semantics node with a node flag is added to the tree, the
+  /// framework will search for the first child in inverse hit-test order with
+  /// a routeName flag and non-empty semantic value and announce it as an edge


[isRouteName] instead of routeName?

goderbauer · 2018-04-13T16:18:59Z

lib/ui/semantics.dart

+  /// Whether the semantics node value is the name of a visually distinct route.
+  /// 
+  /// This is used by certain widgets like Drawers and Dialogs, to indicate
+  /// that the node's semantic value can be used for an edge triggered


.. can be used to announce an edge triggered ...

goderbauer · 2018-04-13T16:20:23Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

+          rootObject.collectRoutes(newRoutes);
+        }
+
+        // Dispatch a TYPE_WINDOW_STATE_CHANGED event if the most recent route id changed from the


This comment ends pretty abruptly :)

goderbauer · 2018-04-13T16:23:51Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

+            // Returns the first non-null and non-empty semantic value of a child
+            // with an isRouteName flag. Otherwise returns null.
+            if (hasFlag(Flag.IS_ROUTE_NAME)) {
+                String routeName = getValueLabelHint();


On iOS we are just using the accessibilityValue, why use all three on Android?

See below - we should probably just use label for both it sounds like

goderbauer · 2018-04-13T16:25:53Z

lib/ui/semantics.dart

+  /// Whether the semantics node value is the name of a visually distinct route.
+  /// 
+  /// This is used by certain widgets like Drawers and Dialogs, to indicate
+  /// that the node's semantic value can be used for an edge triggered


And is it value or value label hint?

Also, should it be label instead of value? I think the app bar title is transfered as a label, right?

The app bar title is a label and not a value - I decided to change it since I noticed that android accessibility bridge sometimes combined the three?

I'm not 100% on the semantic differences between label and value. If label is a better fit then I can do a quick update

I believe label is the more correct one.

Label is meant for static things (like the title of an app bar). Value is for dynamic, often changeable things like the content of a text fields.

Android doesn't have a way to separate between the two, that's why we often munch them together. However, in this case it sounds like only label is the right choice for both platforms.

Got it, that makes more sense given that we don't make additional announcements if the route name changes too

…lity

jonahwilliams · 2018-04-17T02:04:27Z

I've updated the PR to explicitly claim depth-first paint order - this is only correct on Android. There are two PRs pending by @yjbanov to remove the iOS specific geometric order and to change the child order. This could be submitted first and then updated as a part of the future work - or wait til afterwards and update.

I'm not sure how much of a difference it will make, since in most cases there will be only one scopesRoute/namesRoute per level.

goderbauer

LGTM

goderbauer · 2018-04-17T16:17:25Z

lib/ui/semantics.dart

+  /// order for the first node with a [namesRoute] flag and a non-null, 
+  /// non-empty label. The [namesRoute] and [scopesRoute] flags may be on the 
+  /// same node. The label of the found node will be announced as an edge 
+  /// transition. if no non-empty, non-null label is found then:


Capitalize "if"?

goderbauer · 2018-04-17T16:18:14Z

lib/ui/semantics.dart

  /// is a password or contains other sensitive information.
  static const SemanticsFlag isObscured = const SemanticsFlag._(_kIsObscuredIndex);

+  /// Whether the semantics node is the root of a subtree for which values


instead of values say " a route name"? Since we actually announce the label?

goderbauer · 2018-04-17T16:19:02Z

lib/ui/semantics.dart

+  /// semantics update.
+  /// 
+  /// Updating this label within the same active route subtree will not cause 
+  /// additional semantic label updates.


not cause additional announcements? The label itself does get updated, right?

goderbauer · 2018-04-17T16:24:14Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

        boolean isFocusable() {
            int scrollableActions = Action.SCROLL_RIGHT.value | Action.SCROLL_LEFT.value
                    | Action.SCROLL_UP.value | Action.SCROLL_DOWN.value;
+            if (hasFlag(Flag.SCOPES_ROUTE)) {


Maybe add a comment here that we enforce in the framework that no other useful semantics are added to this node?

Also, maybe makes this the first line of the method? The variable scrollableActions above this kinda belongs to the statement below this.

goderbauer · 2018-04-17T16:28:39Z

lib/ui/semantics.dart

+  /// 
+  /// This is used in widgets such as Routes, Drawers, and Dialogs to 
+  /// communicate significant changes in the visible screen.
+  static const SemanticsFlag scopesRoute = const SemanticsFlag._(_kScopesRouteIndex);


Should this document that nodes annotated with this flag generally are not a11y focusable?

goderbauer · 2018-04-17T16:29:06Z

lib/ui/semantics.dart

+  /// 
+  /// Updating this label within the same active route subtree will not cause 
+  /// additional semantic label updates.
+  static const SemanticsFlag namesRoute = const SemanticsFlag._(_kNamesRouteIndex);


Should this document that nodes with this flag are still going to receive a11y focus?

jonahwilliams · 2018-04-17T17:32:37Z

Holding off on submitting until the engine culprit is found, per @tvolkert

chunhtai · 2020-09-08T18:08:44Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

    }
+    NSMutableArray<SemanticsObject*>* newRoutes = [[[NSMutableArray alloc] init] autorelease];
+    [root collectRoutes:newRoutes];
+    for (SemanticsObject* route in newRoutes) {


Hi @jonahwilliams, do you remember why we check any new route in the previous_routes_ here? This will make the it to not announce route change when any new route is in the previous routes

for example [Route1] -> [Route1, Route2] will not announce any route.

Jonah Williams and others added 9 commits March 28, 2018 17:48

java and engine implementation of routeName

6c2c04f

initial ios work

4154b1c

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

156486e

…lity

change android implementation

4e1df1c

remove log

d4e1171

merge

ccee66a

add accessibility call and remove log for ios version

f5c4a04

remove extra import

d13d6c8

remove unecessary change

29f3988

googlebot added the cla: yes label Mar 29, 2018

jonahwilliams mentioned this pull request Mar 29, 2018

Semantics object support for edge triggered semantics flutter/flutter#16081

Merged

yjbanov reviewed Mar 29, 2018

View reviewed changes

explicitly retain and release route name, rename latestRoutes to popu…

dd266b3

…alteRoutes

yjbanov reviewed Mar 29, 2018

View reviewed changes

cleanup release and naming

163d5a0

yjbanov reviewed Mar 29, 2018

View reviewed changes

initialize previous_route in initializer block and explicitly nil

0d4a22b

Hixie reviewed Mar 29, 2018

View reviewed changes

jonahwilliams and others added 8 commits March 29, 2018 15:29

remove list/array allocation and just track most specific route

82fa393

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

e269054

…lity

Merge branch 'pageAccessibility' of github.com:jonahwilliams/engine i…

d179a4f

…nto pageAccessibility

remove extra arg

ca85dde

Merge branch 'pageAccessibility' of github.com:jonahwilliams/engine i…

6d69937

…nto pageAccessibility

explicitly init to null

4f3ed7a

Merge branch 'pageAccessibility' of github.com:jonahwilliams/engine i…

dd03ed5

…nto pageAccessibility

fix typo

25e097d

jonahwilliams changed the title ~~(WIP) AccessibilityBridge support for edge triggered semantics (iOS + Android)~~ AccessibilityBridge support for edge triggered semantics (iOS + Android) Mar 29, 2018

Hixie reviewed Apr 12, 2018

View reviewed changes

goderbauer reviewed Apr 12, 2018

View reviewed changes

Jonah Williams added 7 commits April 12, 2018 11:19

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

b81e720

…lity

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

1577c7e

…lity

Merge branch 'pageAccessibility' of github.com:jonahwilliams/engine i…

fb1002a

…nto pageAccessibility

address some comments

929deb3

update missing flag

3cb9d8a

remove unused variable

e36887a

update android to use label value hint instead of just value for rout…

8625b48

…e name

goderbauer reviewed Apr 13, 2018

View reviewed changes

Jonah Williams added 8 commits April 13, 2018 11:12

address some comments

deb28ba

move both frameworks to semantic label for edge triggered announcements

716c13a

rename route and routename to scopesroute and namesroute

f625040

some cleanup of comments (todo on order) and renaming of ios methods

30b077d

fix missed rename

03ca372

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

b2cd6f7

…lity

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

ee87775

…lity

update doc comment to reflect current child order

a72e8b3

goderbauer approved these changes Apr 17, 2018

View reviewed changes

goderbauer reviewed Apr 17, 2018

View reviewed changes

address comments

53b501a

Merge remote-tracking branch 'upstream/master' into pageAccessibility

b589efb

jonahwilliams merged commit 3405e23 into flutter:master Apr 19, 2018

jonahwilliams deleted the pageAccessibility branch April 19, 2018 18:14

jonahwilliams mentioned this pull request Apr 19, 2018

Roll engine to 3405e23a32af4ea3655e5d1a78955066b2573331 flutter/flutter#16755

Merged

chunhtai reviewed Sep 8, 2020

View reviewed changes

AccessibilityBridge support for edge triggered semantics (iOS + Android) #4901

AccessibilityBridge support for edge triggered semantics (iOS + Android) #4901

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jonahwilliams commented Mar 29, 2018

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Apr 12, 2018

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

jonahwilliams commented Mar 29, 2018 •

edited

Loading