[ios] Link PlatformView back to semantics tree #46471

cyanglaz · 2023-10-02T18:58:50Z

The PlatformView does not have a semantics container when added to semantics tree, this PR gives it a semantics container to ensure accessibility traversal works.

This fixes flutter/flutter#135504, which is a regression of b8332e3

Before b8332e3, the traversal works because the PlatformView is added to the accessibilityElements of the FlutterPlatFormViewSemanticsContainer, which implicitly made the FlutterPlatFormViewSemanticsContainer as the PlatformVIew's AccessibilityContainer.

Now we use the PlatformVIew as the nativeAccessibility of the FlutterPlatFormViewSemanticsContainer, we need to expilicitly set the container.

This needs to be cherry-picked since the commit caused the regression was cherry-picked in b8332e3

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cyanglaz · 2023-10-02T18:59:50Z

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

 @end

-@implementation FlutterScrollableSemanticsObject {
-  fml::scoped_nsobject<SemanticsObjectContainer> _container;


This is unrelated to the PR, but the variable is unused so I removed it

cyanglaz · 2023-10-02T19:00:54Z

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

- (NSArray*)accessibilityElements {
-  return @[ _platformView ];
-}


We implemented nativeAccessibility, this is not necessary.

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

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

stuartmorgan-g · 2023-10-02T19:09:36Z

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

 const float kFloatCompareEpsilon = 0.001;

+@interface FlutterTouchInterceptingView (Test)
+- (id)accessibilityContainer;


Please make a _Test.h file instead of re-declaring methods directly in test files.

This entire redeclaration block should be removed in favor of including the test header.

stuartmorgan-g · 2023-10-02T19:14:43Z

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


+@protocol FlutterSemanticsProtocol <NSObject>
+// Adding functionalities for any object to assign accessibilityContainer.
+// For example, this allows the PlatformView be able to link back to the accessibility tree.


I'm confused by what this property is actually for. I see where it's set, but I don't see anything reading it except in tests. Am I missing some non-test behavior that's influenced by this?

It is read in FlutterTouchInterceptingView: accessibilityContainer, it is a UIAccessibilty property that is read by the UIKit.

I initially added the method to the FlutterTouchInterceptingView so that FlutterTouchInterceptingView can access the container in its accessibiltiyContainer method.

Then I discovered that SemanticsObjectTests is ARC, so we couldn't not include FlutterTouchInterceptingView (which uses MRC features) in the header of SemanticsObjectTests, thus the protocl was added.

This does make the code more confusing. Maybe I should revert back to my initial implementation (no protocol, FlutterTouchInterceptingView has a setFlutterAccessiblityContainer method), then move the related tests to mrc.

format review

cyanglaz · 2023-10-02T23:09:40Z

@stuartmorgan I removed the protocol to reduce confusion, I also refactored the platform view related tests to a separate file + other review comments. PTAL

stuartmorgan-g · 2023-10-03T17:38:13Z

shell/platform/darwin/ios/framework/Source/FlutterTouchInterceptingView+Tests.h

..._Test.h seems to be the established convention in this directory.

stuartmorgan-g · 2023-10-03T17:40:42Z

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

 const float kFloatCompareEpsilon = 0.001;

+@interface FlutterTouchInterceptingView (Test)
+- (id)accessibilityContainer;


This entire redeclaration block should be removed in favor of including the test header.

stuartmorgan-g · 2023-10-03T17:42:24Z

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

 // Get embedded view
 - (UIView*)embeddedView;
+
+// The FlutterTouchInterceptingView assigns `flutterAccessibilityContainer` as its


Instance methods don't need to say that they are about the class they are on; this can just say "Sets flutterAccessibilityContainer as this view's accessibilityContainer."

stuartmorgan-g · 2023-10-03T17:46:12Z

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

+  // Check if there's no more strong references to `platformView` after container and platformView
+  // are released.
+  [container release];
+  XCTAssertEqual(platformView.retainCount, 1u);


This is fragile to implementation details of the code using the platform view. It would help to at least wrap all the code between the declaration and here in an autorelease pool.

cyanglaz · 2023-10-03T22:33:31Z

@stuartmorgan Updated, PTAL!

stuartmorgan-g

LGTM with one last nit.

stuartmorgan-g · 2023-10-04T11:54:19Z

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

+  // If the `FlutterPlatformViewsController` does not contain any `FlutterPlatformView` object or
+  // a `FlutterPlatformView` object associated with the view_id cannot be found, the method
+  // returns nil.
+  FlutterTouchInterceptingView* GetFlutterTouchInterceptingViewByID(int64_t);


Nit: I missed that this parameter doesn't have a name; it should be int64_t view_id.

auto-submit · 2023-10-04T16:18:28Z

auto label is removed for flutter/engine/46471, due to - The status or check suite Linux Web Framework tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ine#46471)

…136007) flutter/engine@8e6dd23...8e3a05c 2023-10-04 chinmaygarde@google.com Update buildroot to 5d60bd2. (flutter/engine#46564) 2023-10-04 ychris@google.com [ios] Link PlatformView back to semantics tree (flutter/engine#46471) 2023-10-04 skia-flutter-autoroll@skia.org Roll Skia from d061d21eed0d to 073737b7f4ab (2 revisions) (flutter/engine#46563) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

The PlatformView does not have a semantics container when added to semantics tree, this PR gives it a semantics container to ensure accessibility traversal works. This fixes flutter/flutter#135504, which is a regression of flutter@b8332e3 Before flutter@b8332e3, the traversal works because the PlatformView is added to the accessibilityElements of the FlutterPlatFormViewSemanticsContainer, which implicitly made the FlutterPlatFormViewSemanticsContainer as the PlatformVIew's AccessibilityContainer. Now we use the PlatformVIew as the nativeAccessibility of the FlutterPlatFormViewSemanticsContainer, we need to expilicitly set the container. This needs to be cherry-picked since the commit caused the regression was cherry-picked in flutter@b8332e3 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…lutter#136007) flutter/engine@8e6dd23...8e3a05c 2023-10-04 chinmaygarde@google.com Update buildroot to 5d60bd2. (flutter/engine#46564) 2023-10-04 ychris@google.com [ios] Link PlatformView back to semantics tree (flutter/engine#46471) 2023-10-04 skia-flutter-autoroll@skia.org Roll Skia from d061d21eed0d to 073737b7f4ab (2 revisions) (flutter/engine#46563) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Cherry pick 250daad Cherry pick issue flutter/flutter#136266 Description: The PlatformView does not have a semantics container when added to semantics tree, this PR gives it a semantics container to ensure accessibility traversal works. This fixes flutter/flutter#135504, which is a regression of b8332e3 Before b8332e3, the traversal works because the PlatformView is added to the accessibilityElements of the FlutterPlatFormViewSemanticsContainer, which implicitly made the FlutterPlatFormViewSemanticsContainer as the PlatformVIew's AccessibilityContainer. Now we use the PlatformVIew as the nativeAccessibility of the FlutterPlatFormViewSemanticsContainer, we need to expilicitly set the container. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

The PlatformView does not have a semantics container when added to semantics tree, this PR gives it a semantics container to ensure accessibility traversal works. This fixes flutter/flutter#135504, which is a regression of flutter@b8332e3 Before flutter@b8332e3, the traversal works because the PlatformView is added to the accessibilityElements of the FlutterPlatFormViewSemanticsContainer, which implicitly made the FlutterPlatFormViewSemanticsContainer as the PlatformVIew's AccessibilityContainer. Now we use the PlatformVIew as the nativeAccessibility of the FlutterPlatFormViewSemanticsContainer, we need to expilicitly set the container. This needs to be cherry-picked since the commit caused the regression was cherry-picked in flutter@b8332e3 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Cherry pick 250daad Cherry pick issue flutter/flutter#136552 Description: The PlatformView does not have a semantics container when added to semantics tree, this PR gives it a semantics container to ensure accessibility traversal works. This fixes flutter/flutter#135504, which is a regression of b8332e3 Before b8332e3, the traversal works because the PlatformView is added to the accessibilityElements of the FlutterPlatFormViewSemanticsContainer, which implicitly made the FlutterPlatFormViewSemanticsContainer as the PlatformVIew's AccessibilityContainer. Now we use the PlatformVIew as the nativeAccessibility of the FlutterPlatFormViewSemanticsContainer, we need to expilicitly set the container. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

The PlatformView does not have a semantics container when added to semantics tree, this PR gives it a semantics container to ensure accessibility traversal works. This fixes flutter/flutter#135504, which is a regression of b8332e3 Before b8332e3, the traversal works because the PlatformView is added to the accessibilityElements of the FlutterPlatFormViewSemanticsContainer, which implicitly made the FlutterPlatFormViewSemanticsContainer as the PlatformVIew's AccessibilityContainer. Now we use the PlatformVIew as the nativeAccessibility of the FlutterPlatFormViewSemanticsContainer, we need to expilicitly set the container. This needs to be cherry-picked since the commit caused the regression was cherry-picked in b8332e3 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Link PlatformView back to semantics tree

9b4f4da

github-actions bot added the platform-ios label Oct 2, 2023

cyanglaz commented Oct 2, 2023

View reviewed changes

cyanglaz requested a review from stuartmorgan-g October 2, 2023 19:01

stuartmorgan-g suggested changes Oct 2, 2023

View reviewed changes

review

f049d57

format review

forma

63a75a3

cyanglaz force-pushed the a11y_regression_platformview branch from 073536f to 63a75a3 Compare October 3, 2023 16:21

stuartmorgan-g suggested changes Oct 3, 2023

View reviewed changes

Chris Yang added 2 commits October 3, 2023 13:32

review

bd64896

fix license golden

3895bc7

stuartmorgan-g approved these changes Oct 4, 2023

View reviewed changes

review

c5779e3

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2023

Merge branch 'main' into a11y_regression_platformview

8691533

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2023

auto-submit bot merged commit 250daad into flutter:main Oct 4, 2023

engine-flutter-autoroll mentioned this pull request Oct 4, 2023

Roll Flutter Engine from 8e6dd239fb1a to 8e3a05c9878e (3 revisions) flutter/flutter#136007

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 4, 2023

250daadbc [ios] Link PlatformView back to semantics tree (flutter/eng…

60bdb3b

…ine#46471)

cyanglaz deleted the a11y_regression_platformview branch October 4, 2023 23:29

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Oct 11, 2023

[cp][ios] Link PlatformView back to semantics tree (flutter#46471)

c996597

ryanaltair mentioned this pull request Apr 2, 2024

land hostname fix xx-infinity/engine#1

Open

[ios] Link PlatformView back to semantics tree #46471

[ios] Link PlatformView back to semantics tree #46471

Uh oh!

Conversation

cyanglaz commented Oct 2, 2023

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

cyanglaz Oct 2, 2023 • 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!

cyanglaz commented Oct 2, 2023

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!

cyanglaz commented Oct 3, 2023

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Oct 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cyanglaz Oct 2, 2023 •

edited

Loading