Include platformViewId in semantics tree for iOS by cyanglaz · Pull Request #29304 · flutter/flutter

cyanglaz · 2019-03-13T16:40:21Z

Description

Include the platformViewId of PlatformViews in the semantics tree. The accessibility bridge in the engine can use this id to steal the semantics nodes from the actual platform view and stick them into Flutter's semantics tree.

This PR is the iOS counter part of #28953. It reverts the change in 5b5d6e8 and 03fd797.

Related Issues

#29302

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
My PR includes tests for all changed/updated/fixed behaviors (See Test Coverage).
All existing and new tests are passing.
I updated/added relevant documentation (doc comments with ///).
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes).
No, this is not a breaking change.

…a11y_platformviews

goderbauer · 2019-03-13T17:02:15Z

packages/flutter/lib/src/rendering/platform_view.dart

+  void describeSemanticsConfiguration (SemanticsConfiguration config) {	
+    super.describeSemanticsConfiguration(config);	
+    config.isSemanticBoundary = true;	
+    config.platformViewId = _viewController.id;	


For android, we only add the platform id to the tree if the platform view has actually been created and is ready to go (which may be a few frames after the PlatformView widget has been created). I would assume we want a similar behavior on iOS? (unless on iOS view and widget creation happen in the same frame?)

I think in iOS, the view controller is only initialized after the view is created. https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/services/platform_views.dart#L133 So it should be safe to assign the platformViewId to _viewController.id directly assuming the view has been successfully created. @amirh could you confirm?

goderbauer · 2019-03-13T17:02:42Z

packages/flutter/lib/src/rendering/platform_view.dart

  }

+  @override	
+  void describeSemanticsConfiguration (SemanticsConfiguration config) {	


nit: you'll have to remove the whitespace at the and of these lines to make the analyzer happy on Cirrus.

goderbauer · 2019-03-13T17:03:06Z

packages/flutter/test/widgets/platform_view_test.dart

+      final SemanticsHandle handle = tester.ensureSemantics();
+      final int currentViewId = platformViewsRegistry.getNextPlatformViewId();
+      expect(currentViewId, greaterThanOrEqualTo(0));
+      final FakeIosPlatformViewsController viewsController =


nit: no need to linebreak here, it makes the code harder to read.

goderbauer · 2019-03-13T17:03:15Z

packages/flutter/test/widgets/platform_view_test.dart

+      // is not yet in the tree.
+      await tester.pump();
+
+      final SemanticsNode semantics =


nit: no line break.

packages/flutter/lib/src/rendering/platform_view.dart

amirh

LGTM modulo nit

amirh · 2019-03-13T18:13:52Z

packages/flutter/lib/src/rendering/platform_view.dart

  UiKitViewController get viewController => _viewController;
  UiKitViewController _viewController;
  set viewController(UiKitViewController viewController) {
+    final bool needsSemantics = _viewController.id != viewController.id;


nit: needsSmeanticsUpdate

…nored

cbracken · 2019-03-15T00:48:39Z

packages/flutter/lib/src/rendering/platform_view.dart

      !(event is PointerDownEvent) && !(event is PointerUpEvent);
-}
-
+}


nit: please re-add the trailing newline.

Seems format analyzer fails if we added a trailing newline?

@cbracken I believe this was removing an extra blank line at the end of the file, which shouldn't be there. The trailing newline is still there.

cbracken · 2019-03-15T00:51:54Z

packages/flutter/lib/src/semantics/semantics.dart

  /// If this value is non-null, the SemanticsNode must not have any children
  /// as those would be replaced by the semantics nodes of the referenced
-  /// platform view.
+  /// platform view, and other semantic configurations are ignored.


Here and below: can you clarify what you mean here? (Which other semantic configurations?)

I have updated the comments to include the things are ignored. Please let me know if you think we can do it a different way.

cbracken · 2019-03-15T00:53:22Z

packages/flutter/test/widgets/platform_view_test.dart

+    });
  });
-}
+}


nit: re-add the trailing newline.

cbracken · 2019-03-15T00:55:58Z

packages/flutter/lib/src/rendering/platform_view.dart

  UiKitViewController get viewController => _viewController;
  UiKitViewController _viewController;
  set viewController(UiKitViewController viewController) {
+    final bool needsSemanticsUpdate = _viewController.id != viewController.id;


Move this below the assert to avoid the potential for a null-pointer exception. Granted either way you get an exception, just the other way is clearer for someone hitting it.

goderbauer · 2019-03-15T17:20:30Z

packages/flutter/lib/src/semantics/semantics.dart

+  /// platform view, and other semantic configurations are ignored. The ignored
+  /// configurations include: [flags], [actions], [label], [value], [increasedValue],
+  /// [decreasedValue], [hint], [textDirection], [textSelection], [scrollChildCount],
+  /// [scrollIndex], [scrollPosition], [scrollExtentMax], [scrollExtentMin], [transform], 


Ignoring transform is odd. Is that actually true? What if the PlatformView is scaled to a smaller/larger size?

goderbauer · 2019-03-15T17:21:17Z

packages/flutter/lib/src/semantics/semantics.dart

-  /// platform view.
-  ///
+  /// platform view, and other semantic configurations are ignored. The ignored
+  /// configurations include: [flags], [actions], [label], [value], [increasedValue],


This explicit list seems hard to maintain. When we add a new property nobody will remember to add it here.

goderbauer · 2019-03-19T17:57:13Z

Looks like some debug prints (and commented out code) made it into this PR?

cyanglaz · 2019-03-19T18:01:17Z

Looks like some debug prints (and commented out code) made it into this PR?

oops. Fixed it.

goderbauer

LGTM

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

Chris Yang added 4 commits March 12, 2019 10:56

add ios implementation

1d9f7de

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

89c43b6

…a11y_platformviews

Merge branch 'master' into ios_a11y_platformviews

3b96c36

revert formatting changes to the test file

7c2554a

cyanglaz requested review from amirh and goderbauer March 13, 2019 16:40

googlebot added the cla: yes label Mar 13, 2019

goderbauer reviewed Mar 13, 2019

View reviewed changes

goderbauer added platform-ios iOS applications specifically framework flutter/packages/flutter repository. See also f: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: platform-views Embedding Android/iOS views in Flutter apps labels Mar 13, 2019

Chris Yang added 2 commits March 13, 2019 10:28

review nit fixes

d33f702

make the analyzer happy

7a11524

amirh approved these changes Mar 13, 2019

View reviewed changes

cyanglaz mentioned this pull request Mar 13, 2019

Add a11y support for embedded iOS platform view flutter/engine#8156

Merged

Chris Yang added 6 commits March 13, 2019 13:02

rename needsSemantics -> needsSemanticsUpdate

af2c889

typo

7f4b34f

dart doc on platformviewid will enforce other configurations to be ig…

f28f971

…nored

more comments on platform view id

ae53ffb

remove whitespaces to make the bot happy

ad9baf3

remove more whitespaces

c56b817

cbracken reviewed Mar 15, 2019

View reviewed changes

review fixes

0b92712

goderbauer reviewed Mar 15, 2019

View reviewed changes

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

remove untrue document

751cfbc

Chris Yang added 2 commits March 19, 2019 11:03

remove testing code

fd59629

remove testing code

1f306c5

goderbauer approved these changes Mar 20, 2019

View reviewed changes

Merge branch 'master' into ios_a11y_platformviews

59bf079

cyanglaz merged commit 3b3f6c7 into flutter:master Mar 27, 2019

cyanglaz deleted the ios_a11y_platformviews branch March 27, 2019 20:56

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

+                  });
                 });
-              }
+              }

                
                    No newline at end of file

Conversation

cyanglaz commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

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!

amirh 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!

cbracken Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cbracken Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cbracken Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 commented Mar 19, 2019

Uh oh!

cyanglaz commented Mar 19, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cyanglaz commented Mar 13, 2019 •

edited

Loading

cbracken Mar 15, 2019 •

edited

Loading

cbracken Mar 15, 2019 •

edited

Loading

cbracken Mar 15, 2019 •

edited

Loading