[macOS] Remove a single accessibility root assumption #40316

loic-sharma · 2023-03-15T18:07:48Z

Today, the root accessibility semantics node is guaranteed to have ID 0. In a multi-window world, each view will have its own semantics tree, but semantic node IDs will be globally unique. In other words, the semantics tree's root will no longer be guaranteed to have ID 0. As a result, AccessibilityBridge::kRootNodeId is deprecated and all uses should be removed.

Part of flutter/flutter#119391

Manual test

Manual test...

Apply the framework patch below, run an app using your local engine, and verify the accessibility works as expected:

diff --git a/packages/flutter/lib/src/semantics/semantics.dart b/packages/flutter/lib/src/semantics/semantics.dart
index 969f822f5f..6431b3ed28 100644
--- a/packages/flutter/lib/src/semantics/semantics.dart
+++ b/packages/flutter/lib/src/semantics/semantics.dart
@@ -1650,7 +1650,7 @@ class SemanticsNode extends AbstractNode with DiagnosticableTreeMixin {
     this.key,
     VoidCallback? showOnScreen,
     required SemanticsOwner owner,
-  }) : _id = 0,
+  }) : _id = 123,
        _showOnScreen = showOnScreen {
     attach(owner);
   }
@@ -1663,7 +1663,7 @@ class SemanticsNode extends AbstractNode with DiagnosticableTreeMixin {
   // bits are reserved for engine generated IDs.
   static const int _maxFrameworkAccessibilityIdentifier = (1<<16) - 1;

-  static int _lastIdentifier = 0;
+  static int _lastIdentifier = 123;
   static int _generateNewId() {
     _lastIdentifier = (_lastIdentifier + 1) % _maxFrameworkAccessibilityIdentifier;
     return _lastIdentifier;

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 Hixie said 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.

loic-sharma · 2023-03-15T18:21:28Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMac.mm

@@ -187,9 +187,7 @@
      if (ax_node.data().HasState(ax::mojom::State::kEditable)) {


@chunhtai Do you know a widget that exercises this scenario? This scenario can only be reached by nodes with the kEditable state that don't have the kTextField role.

The logic to create AXNodes only gives the kEditable state to semantic nodes with the kFlutterSemanticsFlagIsTextField flag but not the kFlutterSemanticsFlagIsReadOnly flag. However, such a semantics node would have the kTextField role. In other words, it seems this code is unreachable. Please let me know if I misunderstood something.

Is there documentation in the ax::mojo::State enum for this role?

The ax::mojo::State enum is defined as strictly powers of two? i.e. there's no state kFoo that happens to be kBar | kEditable I assume? Agreed that at first glance, it certainly looks like this is probably dead code.

Looks like these are defined as enum class so the answer appears to be that at least today, this is unused code.

Oh just to make sure it's clear: semantic nodes have both state bit flags and a role enum. These are separate :)

This is copied code from chromium, I don't think flutter have such widget that would run this code. We may in the future though, and we would need to update the common a11y_bridge to support that, but I am ok if we delete this code path.

Gotcha. I'll leave it in for now given we may have such a widget in the future.

dkwingsmt

LGTM

dkwingsmt · 2023-03-15T22:24:18Z

Probably need opinions from people who are more familiar. Also, shouldn't you mark it Ready For Review?

cbracken · 2023-03-15T23:59:17Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacTest.mm

+  // can query semantics information from.
+  engine.semanticsEnabled = YES;
+  auto bridge = std::reinterpret_pointer_cast<AccessibilityBridgeMacSpy>(
+      viewController.accessibilityBridge.lock());


#40334 ... you saw nothing 🤫

loic-sharma · 2023-03-16T01:39:00Z

Also, shouldn't you mark it Ready For Review?

I had left this as a draft due to this comment above that might affect this pull request substantially. I'll mark it as ready for now though :)

cbracken

Looks good!

chunhtai

LGTM

chunhtai · 2023-03-17T18:39:59Z

shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMac.mm

@@ -187,9 +187,7 @@
      if (ax_node.data().HasState(ax::mojom::State::kEditable)) {


This is copied code from chromium, I don't think flutter have such widget that would run this code. We may in the future though, and we would need to update the common a11y_bridge to support that, but I am ok if we delete this code path.

…ter/engine#40316)

…122942) * e6334f166 [web] Remove image codecs from Canvaskit Chromium (flutter/engine#40309) * 56727d62c Revert "[Impeller] mark decoded images as optimized for GPU access (#40356)" (flutter/engine#40387) * 2cd19e3d1 Wrap the iOS platform message handler in an autorelease pool block (flutter/engine#40373) * bab7853ad Update analyzer for api_conform_test (flutter/engine#40386) * 87b2e82d1 Roll Fuchsia Mac SDK from z32cF6YFs6CvZbY3g... to 4ZrEK2uzGdp_Gz3DU... (flutter/engine#40385) * fc57995fe Ignore some MTLCompiler failures in impeller unit tests (flutter/engine#40391) * 2398c5222 Add doc comment to Pipeline (flutter/engine#40388) * f585d4bc5 [macOS] Remove a single accessibility root assumption (flutter/engine#40316) * 940cf3c98 remove temporary flag and make FlutterTest the default font for real (flutter/engine#40352) * a1bf9fd2a drawTextBlob should not be compatible with opacity inheritance (flutter/engine#40396) * 55bf0d85e Use bundled analyzer everywhere (flutter/engine#40398) * 8e580414a Roll Skia from 9bfb45d3e065 to 49b902e5fb91 (11 revisions) (flutter/engine#40397) * 77c53d25e Default the CanvasKit base URL to local artifacts. (flutter/engine#40293) * 625ea5395 Roll Skia from 49b902e5fb91 to aa983f5486f0 (7 revisions) (flutter/engine#40404) * 867679fac [Impeller] Add playground flag to render for a specific amount of time. (flutter/engine#40377) * d74169608 [Impeller] Remove unused bounds method from typographer interface (flutter/engine#40406) * 941323d77 Provisional iOS impeller flag flip (flutter/engine#40405) * bb971ab55 Revert "Default the CanvasKit base URL to local artifacts. (#40293)" (flutter/engine#40415)

…ter/engine#40316)

flutter-dashboard bot added the platform-macos label Mar 15, 2023

loic-sharma commented Mar 15, 2023

View reviewed changes

loic-sharma requested review from dkwingsmt, cbracken and chunhtai March 15, 2023 21:24

dkwingsmt approved these changes Mar 15, 2023

View reviewed changes

cbracken reviewed Mar 15, 2023

View reviewed changes

loic-sharma marked this pull request as ready for review March 16, 2023 01:38

cbracken approved these changes Mar 16, 2023

View reviewed changes

[macOS] Remove single accessibility root assumptions

5484f89

loic-sharma force-pushed the macos_a11y_root branch from 248e83c to 5484f89 Compare March 16, 2023 17:54

loic-sharma mentioned this pull request Mar 16, 2023

☂️ Remove accessibility root ID assumptions from the engine flutter/flutter#119391

Closed

8 tasks

Remove deprecated root node ID constant

57de4d6

chunhtai approved these changes Mar 17, 2023

View reviewed changes

loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 17, 2023

auto-submit bot merged commit f585d4b into flutter:main Mar 17, 2023

engine-flutter-autoroll mentioned this pull request Mar 17, 2023

Roll Flutter Engine from 991cf41eb850 to a1bf9fd2adb5 (10 revisions) flutter/flutter#122931

Closed

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

f585d4bc5 [macOS] Remove a single accessibility root assumption (flut…

00b9c1c

…ter/engine#40316)

engine-flutter-autoroll mentioned this pull request Mar 17, 2023

Roll Flutter Engine from 991cf41eb850 to 77c53d25eeeb (13 revisions) flutter/flutter#122933

Closed

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

f585d4bc5 [macOS] Remove a single accessibility root assumption (flut…

0d44cc5

…ter/engine#40316)

engine-flutter-autoroll mentioned this pull request Mar 18, 2023

Roll Flutter Engine from 991cf41eb850 to 625ea5395042 (14 revisions) flutter/flutter#122934

Closed

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

f585d4bc5 [macOS] Remove a single accessibility root assumption (flut…

c7810d6

…ter/engine#40316)

engine-flutter-autoroll mentioned this pull request Mar 18, 2023

Roll Flutter Engine from 991cf41eb850 to 867679facd71 (15 revisions) flutter/flutter#122935

Closed

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

f585d4bc5 [macOS] Remove a single accessibility root assumption (flut…

a2a671c

…ter/engine#40316)

engine-flutter-autoroll mentioned this pull request Mar 18, 2023

Roll Flutter Engine from 991cf41eb850 to d741696082ad (16 revisions) flutter/flutter#122938

Closed

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

f585d4bc5 [macOS] Remove a single accessibility root assumption (flut…

68da371

…ter/engine#40316)

engine-flutter-autoroll mentioned this pull request Mar 18, 2023

Roll Flutter Engine from 991cf41eb850 to 941323d77c31 (17 revisions) flutter/flutter#122939

Closed

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

f585d4bc5 [macOS] Remove a single accessibility root assumption (flut…

9f4cbf0

…ter/engine#40316)

engine-flutter-autoroll mentioned this pull request Mar 18, 2023

Roll Flutter Engine from 991cf41eb850 to bb971ab55f41 (18 revisions) flutter/flutter#122942

Merged

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

f585d4bc5 [macOS] Remove a single accessibility root assumption (flut…

7627c17

…ter/engine#40316)

engine-flutter-autoroll mentioned this pull request Mar 18, 2023

Manual roll Flutter Engine from 991cf41eb850 to bb971ab55f41 (18 revisions) flutter/flutter#122955

Merged

zanderso pushed a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 18, 2023

f585d4bc5 [macOS] Remove a single accessibility root assumption (flut…

ee69a83

…ter/engine#40316)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[macOS] Remove a single accessibility root assumption #40316

[macOS] Remove a single accessibility root assumption #40316

Uh oh!

loic-sharma commented Mar 15, 2023 •

edited

Loading

Uh oh!

loic-sharma Mar 15, 2023 •

edited

Loading

Uh oh!

cbracken Mar 16, 2023 •

edited

Loading

Uh oh!

cbracken Mar 16, 2023

Uh oh!

loic-sharma Mar 16, 2023

Uh oh!

chunhtai Mar 17, 2023

Uh oh!

loic-sharma Mar 17, 2023

Uh oh!

dkwingsmt left a comment

Uh oh!

dkwingsmt commented Mar 15, 2023

Uh oh!

cbracken Mar 15, 2023 •

edited

Loading

Uh oh!

loic-sharma commented Mar 16, 2023 •

edited

Loading

Uh oh!

cbracken left a comment

Uh oh!

chunhtai left a comment

Uh oh!

chunhtai Mar 17, 2023

Uh oh!

Uh oh!

		@@ -187,9 +187,7 @@
		if (ax_node.data().HasState(ax::mojom::State::kEditable)) {

[macOS] Remove a single accessibility root assumption #40316

[macOS] Remove a single accessibility root assumption #40316

Uh oh!

Conversation

loic-sharma commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manual test

Pre-launch Checklist

Uh oh!

loic-sharma Mar 15, 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!

cbracken Mar 16, 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!

cbracken Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

loic-sharma Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Mar 15, 2023

Uh oh!

cbracken Mar 15, 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!

loic-sharma commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loic-sharma commented Mar 15, 2023 •

edited

Loading

loic-sharma Mar 15, 2023 •

edited

Loading

cbracken Mar 16, 2023 •

edited

Loading

cbracken Mar 15, 2023 •

edited

Loading

loic-sharma commented Mar 16, 2023 •

edited

Loading