Rewire ios accessibility bridge receive semantics enabled signal #56691

chunhtai · 2024-11-18T23:15:28Z

previously the only correct way to enable semantics is that ios embedding receive signal from native OS, it call SetSemanticsEnabled to shell and then to dart to enable semantics tree generation.

If for some reason framework decide to enable semantics first, e.g. through SemanticsBinding.instance.ensureSemantics(typically due to integration test or ci that wants to test semantics), the update will be dropped in shell. Even if it later on receive signal from native OS to turn on semantics, it can't construct the complete accessibility tree in embedding because the updatesemantics sends diff update and previous updates are gone forever. It will end up in a broken state.

This pr changes so that the only source of truth will be in the framework side. When framework starts generating the the semantics tree, it will call SetSemanticsTreeEnabled through dart:ui, and the embedding needs to prepare itself to accept semantics update after receiving the message.

This however require some refactoring on iOS embedding because it will only create a11y bridge when receiving OS notification when assitive technologies turns on.

This requires three phase transition

add an empty dart:ui API setSemanticsTreeEnabled
makes framework calls the empty API.
merge this pr with actual implementation of setSemanticsTreeEnabled

I also need to do the Android part

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.

chunhtai · 2024-11-18T23:54:25Z

shell/platform/darwin/ios/platform_view_ios.h

+  /**
+   * Gets the accessibility bridge created in this platform view.
+   */
+  AccessibilityBridge* GetAccessibilityBridge() { return accessibility_bridge_.get(); }


These are public for testing only, i am not sure how to best test the change without exposing these

This is fine for now; refactoring this class is on my to do list for a rainy day, but please add a note pointing out this is only for testing so we can clean it up in the refactor.

chunhtai · 2024-11-18T23:56:13Z

shell/platform/darwin/ios/platform_view_ios.h

@@ -113,35 +128,18 @@ class PlatformViewIOS final : public PlatformView {
    id<NSObject> observer_ = nil;
  };

-  /// Wrapper that guarantees we communicate clearing Accessibility
-  /// information to Dart.
-  class AccessibilityBridgeManager {


The sole purpose for this class is to call setSemanticsEnabled when a11y bridge is created. This is not needed even before the change because the a11y bridge will only be created when setSemanticsEnabled is called before the change.

I think this class is obsolete

Agreed, yeah this is ICantBelieveItsNotUniquePtr with one additional feature.

chunhtai · 2024-11-18T23:57:38Z

shell/platform/darwin/ios/platform_view_ios.mm

+    if (generating) {
+      if (!accessibility_bridge_) {
+        accessibility_bridge_ = std::make_unique<AccessibilityBridge>(owner_controller_, this,
+                                                                      platform_views_controller_);


Can I assume platform_views_controller_ contains the same owner_controller_.platformViewsController. From the code I think yes, but not too sure.

cbracken · 2024-11-19T19:46:58Z

shell/platform/darwin/ios/platform_view_ios.h

@@ -36,7 +36,7 @@ namespace flutter {
 * lifecycle. It's a long lived bridge owned by the `FlutterEngine` and can be attached and
 * detached sequentially to multiple `FlutterViewController`s and `FlutterView`s.
 */
-class PlatformViewIOS final : public PlatformView {
+class PlatformViewIOS : public PlatformView {


Why was it necessary to remove the final?

cbracken · 2024-11-19T19:56:39Z

shell/platform/darwin/ios/platform_view_ios.mm

@@ -171,14 +190,7 @@ new PlatformMessageHandlerIos(task_runners.GetPlatformTaskRunner())) {}
                        "PlatformViewIOS has no ViewController.";
    return;
  }
-  if (enabled && !accessibility_bridge_) {


GitHub won't let me comment on the line above, but I think you're safe to delete the if (!owner_controller_) { ... } block now, since it's no longer used here.

cbracken · 2024-11-19T19:57:34Z

shell/platform/darwin/ios/platform_view_ios.h

+  /**
+   * Handles accessibility message from accessibility channel.
+   */
+  void handleAccessibilityMessage(__weak id message, FlutterReply reply);


This is a C++ class, so C++ naming applies:

Suggested change

void handleAccessibilityMessage(__weak id message, FlutterReply reply);

void HandleAccessibilityMessage(__weak id message, FlutterReply reply);

Need to fix up the call sites though.

cbracken · 2024-11-19T19:57:52Z

shell/platform/darwin/ios/platform_view_ios.h

+  /**
+   * Send accessibility message to accessibility channel.
+   */
+  void sendAccessibilityMessage(__weak id message);


This is a C++ class, so C++ naming applies:

Suggested change

void sendAccessibilityMessage(__weak id message);

void SendAccessibilityMessage(__weak id message);

Need to fix up the call sites.

cbracken · 2024-11-19T20:00:17Z

shell/platform/darwin/ios/platform_view_ios.mm

+        binaryMessenger:owner_controller.engine.binaryMessenger
+                  codec:[FlutterStandardMessageCodec sharedInstance]];
+    [accessibility_channel_ setMessageHandler:^(id message, FlutterReply reply) {
+      handleAccessibilityMessage(message, reply);


Suggested change

handleAccessibilityMessage(message, reply);

HandleAccessibilityMessage(message, reply);

cbracken · 2024-11-19T20:00:43Z

shell/platform/darwin/ios/platform_view_ios.mm

+  }
+}
+
+void PlatformViewIOS::sendAccessibilityMessage(__weak id message) {


Suggested change

void PlatformViewIOS::sendAccessibilityMessage(__weak id message) {

void PlatformViewIOS::SendAccessibilityMessage(__weak id message) {

cbracken · 2024-11-19T20:00:54Z

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

@@ -74,7 +65,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,

 void AccessibilityBridge::AccessibilityObjectDidBecomeFocused(int32_t id) {
  last_focused_semantics_object_id_ = id;
-  [accessibility_channel_ sendMessage:@{@"type" : @"didGainFocus", @"nodeId" : @(id)}];
+  platform_view_->sendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)});


Suggested change

platform_view_->sendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)});

platform_view_->SendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)});

cbracken · 2024-11-19T20:03:29Z

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

  if ([type isEqualToString:@"announce"]) {
-    NSString* message = annotatedEvent[@"data"][@"message"];
-    ios_delegate_->PostAccessibilityNotification(UIAccessibilityAnnouncementNotification, message);
+    NSString* messageToAnnounce = message[@"data"][@"message"];


This should be message_to_announce since we're in a C++ method. I'm not in love with it either.

https://google.github.io/styleguide/objcguide.html#objective-c

cbracken · 2024-11-19T20:04:21Z