Add a11y support for embedded iOS platform view by cyanglaz · Pull Request #8156 · flutter/engine

cyanglaz · 2019-03-13T19:59:53Z

Follow up the framework change in flutter/flutter#29304.
Inject the accessibility element tree in the semantic node if the node is for platform views.

flutter/flutter#29302

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

goderbauer · 2019-03-13T20:43:52Z

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

+    // We first check if the child is a semantic node for a platform view.
+    // If so, we add the platform view as accessibilityElements of the child.
+    shell::FlutterPlatformViewsController *flutterPlatformViewsController = _bridge.get()->flutter_platform_views_controller();
+    if (child.node.platformViewId > -1 && flutterPlatformViewsController) {


What if child at a later point no longer has a platfromViewId? How do you remove the native a11y elements?

We can clean it up when it happens, but I guess it might require some ugly tracking logic or some ugly type check. Is it even possible that the child object's platformViewId got changed but the child still exists in the tree?

goderbauer · 2019-03-13T20:44:55Z

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

+    if (child.node.platformViewId > -1 && flutterPlatformViewsController) {
+        NSObject<FlutterPlatformView> *platformViewProtocolObject = flutterPlatformViewsController->GetPlatformViewByID(child.node.platformViewId);
+        UIView *platformView = [platformViewProtocolObject view];
+        child.accessibilityElements = @[platformView];


What about accessibilityContainer on platfromView? Doesn't that have to link back?

I don' think we need to set the accessibilityContainer on the platform view since it is already an UIView.
So our a11y tree finds this node for platform views, and we set its accessibilityElements as the platform view. Now the platform view will handle the a11y on however it wants, but in the end, because the child here is still part of our tree, it will return when the accessibilityElements is done traversing.

My understanding is - and this may be incorrect - that in the iOS tree on iOS each node needs to know its parent (the accessibilityContainer) and it's children, if it has any. In this case, the platformView doesn't know what its a11y parent is. This may be a problem if the platform view is contained within a Flutter Scroll view: if you put a11y focus on a node within the platform view, iOS cannot walk up the hierarchy to find the scrolling a11y node and dispatch the scroll event there. At least, that how I think that would work....

I think you are correct if traverse the accessibility dynamically. (Using the a11y protocol methods etc.) My understanding is that If we directly set the accessibilityElements property, iOS would handle the traversal and we don't need to worry about the parents at all. I might be wrong, but when I test with google map view inside a list view it works fine (it goes into the google map then goes out). Is there something else you can think of that might fail if parent is not set? I can try them out.

So I tried to switch the code to make the accessibility dynamically traverse through the platform view. It indeed won't pick up the parent as you mentioned. And we'd have to access the platform view's code to actually do it. Now the platform view is completely controlled by the author of the individual plugin so I think we should not touch it at all since they might want to do something else within their accessibility. I guess the safer way to do it would just be statically assign the accessibilityElements, and let iOS work its magic.

amirh

LGTM

please wait for @goderbauer's approval as well.

goderbauer · 2019-03-14T22:20:26Z

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

  //  We enforce in the framework that no other useful semantics are merged with these nodes.
  if ([self node].HasFlag(blink::SemanticsFlags::kScopesRoute))
    return false;
+  // If the node is the placeholder for a platform view. We know it should be a container.


Are you adding the asserts for this in the framework?

I would probably remove the comment and just add this case to this if above. It's just anther node that doesn't get semantics merged in.

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

… id is set

…iew node

goderbauer · 2019-03-15T00:03:11Z

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

+    child.accessibilityElements = @[ platformView ];
+    return child;
+  } else if (child.accessibilityElements.count > 0 &&
+             [child.accessibilityElements.firstObject isKindOfClass:[UIView class]]) {


Why do we need to do the clearing in two different places? It looks like if you do the clearing in the other place this will always be cleared here already?

oh, forgot to move this testing code out. going to remove it.

chinmaygarde · 2019-03-15T00:31:58Z

cc @cbracken

cbracken · 2019-03-15T00:47:02Z

Thanks -- will take a pass over this tonight!

cbracken

Very quick first pass. Grabbing dinner then I'll finish up!

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

cbracken · 2019-03-15T01:12:57Z

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


  fml::WeakPtr<AccessibilityBridge> GetWeakPtr();

+  FlutterPlatformViewsController* flutter_platform_views_controller() const {


... GetPlatformViewsController() const for consistency with the rest of the engine.

cbracken · 2019-03-15T01:14:18Z

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

+  // We first check if the child is a semantic node for a platform view.
+  // If so, we add the platform view as accessibilityElements of the child.
+  shell::FlutterPlatformViewsController* flutterPlatformViewsController =
+      _bridge.get()->flutter_platform_views_controller();


nit: consider naming it controller for readability. See: Long names are long.

cbracken · 2019-03-15T01:27:31Z

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


  UIView* view_;
  PlatformViewIOS* platform_view_;
+  FlutterPlatformViewsController* flutter_platform_views_controller_;


platform_views_controller for consistency with elsewhere.

cbracken · 2019-03-15T01:29:58Z

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


  //  We enforce in the framework that no other useful semantics are merged with these nodes.
-  if ([self node].HasFlag(blink::SemanticsFlags::kScopesRoute))
+  if ([self node].HasFlag(blink::SemanticsFlags::kScopesRoute) || [self node].platformViewId > -1)


>= kMinPlatformViewId (is there a better name?) and define as constexpr above.

Done, also added a function IsPlatformViewNode() in SemanticsNode to make it more readable.

cbracken · 2019-03-15T01:31:50Z

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

+  // If so, we add the platform view as accessibilityElements of the child.
+  shell::FlutterPlatformViewsController* flutterPlatformViewsController =
+      _bridge.get()->flutter_platform_views_controller();
+  if (child.node.platformViewId > -1 && flutterPlatformViewsController) {


Similar comment here, replace -1 with a named constexpr.

cbracken · 2019-03-15T01:34:48Z

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

+
+  // This 'if' block handles adding accessibility support for the embedded platform view.
+  // We first check if the child is a semantic node for a platform view.
+  // If so, we add the platform view as accessibilityElements of the child.


Avoid documenting the how, and instead focus on the what/why. Maybe something like:
// If the child is a semantic node for a platform view, inject the platform view into the iOS accessibility tree.

cbracken · 2019-03-15T01:36:27Z

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

+      _bridge.get()->flutter_platform_views_controller();
+  if (child.node.platformViewId > -1 && flutterPlatformViewsController) {
+    NSObject<FlutterPlatformView>* platformViewProtocolObject =
+        flutterPlatformViewsController->GetPlatformViewByID(child.node.platformViewId);


Is there a shorter name?

cbracken · 2019-03-15T01:38:25Z

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

+  if (child.node.platformViewId > -1 && flutterPlatformViewsController) {
+    NSObject<FlutterPlatformView>* platformViewProtocolObject =
+        flutterPlatformViewsController->GetPlatformViewByID(child.node.platformViewId);
+    UIView* platformView = [platformViewProtocolObject view];


Is it safe to assume platformViewProtocolObject and [platformViewProtocolObject view] will never be nil? If not, put a guard around the next line since nil is disallowed in NSArray.

Good catch! Done.

cbracken · 2019-03-15T01:39:04Z

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

      }
+
+      BOOL isPlatformViewNode = node.platformViewId > -1;
+      BOOL wasPlatformViewNode = object.node.platformViewId > -1;


Similar comment -- avoid the -1 here.

goderbauer

LGTM for a11y

Please wait for @cbracken's LGTM for ObjC stuff.

goderbauer · 2019-03-15T17:55:42Z

lib/ui/semantics.dart

  /// specified, `childrenInHitTestOrder` and `childrenInTraversalOrder` must be
-  /// empty, and other semantic configurations will be ignored.
+  /// empty, and other semantic configurations will be ignored. The ignored configurations
+  /// include:[flags], [actions], [textSelectionBase], [textSelectionExtent], [scrollChildren],


nit: space missing after :

goderbauer · 2019-03-15T17:56:14Z

lib/ui/semantics.dart

+  /// include:[flags], [actions], [textSelectionBase], [textSelectionExtent], [scrollChildren],
+  /// [scrollIndex], [scrollPosition], [scrollExtentMax], [scrollExtentMin], [elevation],
+  /// [thickness], [label], [hint], [value], [increasedValue], [decreasedValue], [textDirection],
+  /// [transform], [childrenInTraversalOrder], [childrenInHitTestOrder], [additionalActions]


I'm surprised to see that transform is ignored. What if the PlatForm viewed is scaled up or down?

I don't think we current support transforming in platform view. @amirh

goderbauer · 2019-03-15T17:56:40Z

lib/ui/semantics.dart

+  /// empty, and other semantic configurations will be ignored. The ignored configurations
+  /// include:[flags], [actions], [textSelectionBase], [textSelectionExtent], [scrollChildren],
+  /// [scrollIndex], [scrollPosition], [scrollExtentMax], [scrollExtentMin], [elevation],
+  /// [thickness], [label], [hint], [value], [increasedValue], [decreasedValue], [textDirection],


This explicit list seems hard to maintain. Nobody will remember to add a property here if we add a new one.

This is a good point, we also have the same problem in the framework semantics code. I think one problem could be that we have a very long list of properties inside these semantics class, this can lead to maintain hell.
There are ways I can think of.

We can refactor SemanticNode, SemanticConfiguration(in framework), SemanticData(in framework), SemanticProperty(in framework) classes, group this long list of properties to make them shorter. For example, the semanticNode can have scrollingConfig, textConfig, actionConfig, transformConfig, etc and each contains reasonable amount of properties. This requires a lot of refactoring work and regression test, and it is also a breaking change. But I feel like it would be a better long term solution since it can be easily scaled up.

A non breaking way: We can have a different semantic node class to represent a node for platform view.

Or maybe we can do both 1 and 2?

Do you have a different way to do it? Also copying @cbracken on this. Maybe we should open a new issue to track this effort?

would probably be nice, but since it is a breaking change I am not sure if it is currently worth doing. It also doesn't fix the problem. You'd still have to keep a (now shorter) list upto date.

I do not understand what you mean by 2.

Instead of listing all these properties, I'd probably just explain what it means on the platform side to set a paltformViewId.

After offline discussion with @goderbauer, I am going to restructure the platform view subtree.

cyanglaz · 2019-03-18T18:24:59Z

Going to rework the tree structure for platform view node to make it more maintainable. I am going to close this PR for now and reopen when the work is done.

…ne#8156)

flutter/engine@0d83a2e...3a3f707 git log 0d83a2e..3a3f707 --no-merges --oneline 3a3f707 Reland "Allow specification of std::functions as native entrypoints from Dart code." (flutter/engine#8329) d4275d9 Roll src/third_party/skia 576eb87a2d2d..99ccc0ca87e6 (5 commits) (flutter/engine#8328) 3a415c4 Map glfw into third_party, and roll buildroot (flutter/engine#8308) ce9ea58 [fuchsia] Disable FML_TRACE_COUNTER events to unblock roll (flutter/engine#8325) 4849658 Roll src/third_party/dart 8a92d2a8d9..991c9da720 (2 commits) 20c241a Roll src/third_party/skia c476e5da4fef..576eb87a2d2d (7 commits) (flutter/engine#8324) cadcf1c Roll src/third_party/dart f3fd1943fc..8a92d2a8d9 (16 commits) 66141eb Roll src/third_party/skia d1c271bd39f0..c476e5da4fef (2 commits) (flutter/engine#8322) e5b31cd Roll src/third_party/skia cec20280b3fb..d1c271bd39f0 (3 commits) (flutter/engine#8321) 1daff5c Roll src/third_party/skia bf341ae88c83..cec20280b3fb (3 commits) (flutter/engine#8320) 87db585 Roll src/third_party/skia 45d5f702133e..bf341ae88c83 (2 commits) (flutter/engine#8316) fd7d7fa Add a11y support for embedded iOS platform view (flutter/engine#8156) f64ee01 Roll src/third_party/skia d2ca31218bc4..45d5f702133e (11 commits) (flutter/engine#8315) f521df3 Fixed service isolate not being initialized correctly due to bad name (flutter/engine#8251) 80b825c Roll src/third_party/dart 093c2909fe..f3fd1943fc (13 commits) (flutter/engine#8310) 7e77d5c Revert "Allow specification of std::functions as native entrypoints from Dart code. (#8309)" (flutter/engine#8312) 400a86a Allow specification of std::functions as native entrypoints from Dart code. (flutter/engine#8309) 51f23fe [vulkan] Add FUCHSIA external sem/mem extensions 78de8dc Enable lambda like native callbacks in tests and add support for custom entrypoints. (flutter/engine#8299) 2812c7d Roll src/third_party/skia 62fd6c411622..d2ca31218bc4 (9 commits) (flutter/engine#8307) 95f9134 Roll src/third_party/skia d90004516a63..62fd6c411622 (4 commits) (flutter/engine#8306) 358273b Roll src/third_party/skia 33211b2c7a02..d90004516a63 (1 commits) (flutter/engine#8305) 2804057 Roll src/third_party/skia 80dd599e91d0..33211b2c7a02 (1 commits) (flutter/engine#8303) a673bbf Roll src/third_party/skia c5ee499f2c59..80dd599e91d0 (5 commits) (flutter/engine#8301) d88037d Roll src/third_party/dart fa74184b7a..093c2909fe (71 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (bmparr@google.com), and stop the roller if necessary.

Follow up the framework change in flutter/flutter#29304. Inject the accessibility element tree in the semantic node if the node is for platform views. flutter/flutter#29302

This reverts commit 47a9c76.

Chris Yang added 2 commits March 13, 2019 12:56

adding a11y support for ios platform view

3bcb1db

comment fix

8fcf6b0

cyanglaz requested review from amirh and goderbauer March 13, 2019 19:59

googlebot added the cla: yes label Mar 13, 2019

goderbauer reviewed Mar 13, 2019

View reviewed changes

Chris Yang added 2 commits March 13, 2019 16:26

formatting

c036e80

make the node for platform view to be a container

05f77a3

amirh reviewed Mar 14, 2019

View reviewed changes

goderbauer reviewed Mar 14, 2019

View reviewed changes

Chris Yang added 3 commits March 14, 2019 15:42

comments on ignoring other semantic configurations when platform view…

b776a05

… id is set

review fixes

8ed39ee

cleanup a11y elements if platform view node changed to non platform v…

a9a0c15

…iew node

goderbauer reviewed Mar 15, 2019

View reviewed changes

remove unnecessary testing code

0e58ef8

cyanglaz requested a review from chinmaygarde March 15, 2019 00:18

nit fix

4e670db

chinmaygarde requested review from cbracken and removed request for chinmaygarde March 15, 2019 00:32

cbracken suggested changes Mar 15, 2019

View reviewed changes

goderbauer approved these changes Mar 15, 2019

View reviewed changes

Chris Yang added 2 commits March 15, 2019 10:49

review fixes

15c03ce

explain what are ignored

6fb37fe

goderbauer reviewed Mar 15, 2019

View reviewed changes

renaming

3e57265

cyanglaz requested a review from cbracken March 15, 2019 18:13

formatting

4a06b68

cyanglaz deleted the ios_platform_view_a11y branch March 26, 2019 23:04

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 26, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

cd11cb2

…ne#8156)

engine-flutter-autoroll mentioned this pull request Mar 26, 2019

Roll engine 0d83a2ecd1bb..fd7d7fa5a057 (14 commits) flutter/flutter#30009

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

a2abef3

…ne#8156)

engine-flutter-autoroll mentioned this pull request Mar 27, 2019

Roll engine 0d83a2ecd1bb..87db585ade76 (15 commits) flutter/flutter#30013

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

d93003c

…ne#8156)

This was referenced Mar 27, 2019

Roll engine 0d83a2ecd1bb..1daff5ce15b7 (16 commits) flutter/flutter#30021

Closed

Roll engine 0d83a2ecd1bb..e5b31cdd982d (17 commits) flutter/flutter#30029

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

75739ed

…ne#8156)

engine-flutter-autoroll mentioned this pull request Mar 27, 2019

Roll engine 0d83a2ecd1bb..66141eb8e891 (18 commits) flutter/flutter#30034

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

36a6a0b

…ne#8156)

engine-flutter-autoroll mentioned this pull request Mar 27, 2019

Roll engine 0d83a2ecd1bb..cadcf1c5841b (19 commits) flutter/flutter#30039

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

e8db0fe

…ne#8156)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

fc0bb0d

…ne#8156)

engine-flutter-autoroll mentioned this pull request Mar 27, 2019

Roll engine 0d83a2ecd1bb..20c241ac32cd (20 commits) flutter/flutter#30046

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

1f6e2e7

…ne#8156)

engine-flutter-autoroll mentioned this pull request Mar 27, 2019

Roll engine 0d83a2ecd1bb..48496588c6ae (21 commits) flutter/flutter#30052

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

c066105

…ne#8156)

engine-flutter-autoroll mentioned this pull request Mar 27, 2019

Roll engine 0d83a2ecd1bb..ce9ea5869456 (22 commits) flutter/flutter#30057

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

40c60f6

…ne#8156)

engine-flutter-autoroll mentioned this pull request Mar 27, 2019

Roll engine 0d83a2ecd1bb..3a415c4f1e3e (23 commits) flutter/flutter#30060

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

9d8a0ce

…ne#8156)

This was referenced Mar 27, 2019

Roll engine 0d83a2ecd1bb..d4275d9ee190 (24 commits) flutter/flutter#30061

Closed

Roll engine 0d83a2ecd1bb..3a3f707d5a9b (25 commits) flutter/flutter#30063

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2019

fd7d7fa Add a11y support for embedded iOS platform view (flutter/engi…

edc7aaf

…ne#8156)

RBogie added a commit to RBogie/flutter-engine that referenced this pull request Apr 8, 2019

Revert "Add a11y support for embedded iOS platform view (flutter#8156)"

ab8111e

This reverts commit 47a9c76.

RBogie added a commit to RBogie/flutter-engine that referenced this pull request Apr 8, 2019

Revert "Add a11y support for embedded iOS platform view (flutter#8156)"

4bb4f93

This reverts commit 47a9c76.

jmagman mentioned this pull request Nov 5, 2021

[ios_platform_view, a11y] Make FlutterPlatformViewSemanticsContainer a SemanticsObject. #29531

Merged

8 tasks


		fml::WeakPtr<AccessibilityBridge> GetWeakPtr();

		FlutterPlatformViewsController* flutter_platform_views_controller() const {

Conversation

cyanglaz commented Mar 13, 2019 • 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!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Mar 15, 2019

Uh oh!

cbracken commented Mar 15, 2019

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

cyanglaz commented Mar 13, 2019 •

edited

Loading

cyanglaz Mar 15, 2019 •

edited

Loading