Implement ITextProvider and ITextRangeProvider for UIA #38284

yaakovschectman · 2022-12-14T15:14:41Z

In order for text providing nodes to properly interact with screen reader commands via UIA, we need to implement text and textedit pattern providers. As an example, this allows screen readers to narrate the character after the carat when the user moves the carat in an edit field with the arrow keys.

Unit tests for AXPlatformNodeText(Range)ProviderWin are modeled after those found in the Chromium source. Details for some required modification to account for differences between the two codebases, and a handful of the newly introduced test cases are currently disabled, which is tracked in flutter/flutter#117012.

Functionality for searching for text in a text range does not yet respect the ignore_case option: flutter/flutter#117013.

Part of flutter/flutter#116219

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.

yaakovschectman · 2022-12-14T15:15:15Z

shell/platform/common/accessibility_bridge.h

@@ -176,7 +204,7 @@ class AccessibilityBridge
  std::unordered_map<AccessibilityNodeId,
                     std::shared_ptr<FlutterPlatformNodeDelegate>>
      id_wrapper_map_;
-  ui::AXTree tree_;
+  std::unique_ptr<ui::AXTree> tree_;


Changed this to a pointer so we can return a pointer in the implementation for AX Tree Manager.

yaakovschectman · 2022-12-14T15:16:48Z

shell/platform/common/accessibility_bridge.cc

@@ -524,7 +533,7 @@ void AccessibilityBridge::SetTreeData(const SemanticsNode& node,
  // 1. this text field has a valid selection
  // 2. this text field doesn't have a valid selection but had selection stored
  //    in the tree.
-  if (flags & FlutterSemanticsFlag::kFlutterSemanticsFlagIsTextField) {
+  if (flags & FlutterSemanticsFlag::kFlutterSemanticsFlagIsTextField && flags & FlutterSemanticsFlag::kFlutterSemanticsFlagIsFocused) {


We want to only update these values for the focused node so that the text selection data points to the focused node, not to whichever text node happens to be latest in the update.

yaakovschectman · 2022-12-14T15:17:28Z

shell/platform/windows/accessibility_bridge_windows.cc

+    case ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED: {
+        ui::AXNode::AXID focus_id = GetAXTreeData().sel_focus_object_id;
+        auto focus_delegate = GetFlutterPlatformNodeDelegateFromID(focus_id).lock();
+        DispatchWinAccessibilityEvent(
+            std::static_pointer_cast<FlutterPlatformNodeDelegateWindows>(focus_delegate), ax::mojom::Event::kDocumentSelectionChanged);
+        break;
+      }


Document selection change event should be fired on the node containing the changed selection, not the root.

yaakovschectman · 2022-12-14T15:18:45Z

third_party/accessibility/ax/ax_node_position_unittest.cc

@@ -1078,7 +1079,7 @@ TEST_F(AXPositionTest, GetMaxTextOffsetAndGetTextWithGeneratedContent) {
  root_1.role = ax::mojom::Role::kRootWebArea;
  root_1.child_ids = {text_field_2.id};

-  text_field_2.role = ax::mojom::Role::kTextField;
+  text_field_2.role = ax::mojom::Role::kGroup;


Chromium uses kTextField for parent nodes containing further text-containing children. We use it to represent an editable text field; we use kGroup for general grouping nodes.

yaakovschectman · 2022-12-14T15:19:58Z

third_party/accessibility/ax/platform/ax_platform_node_win.cc

-      // https://github.com/flutter/flutter/issues/109804
+    case UIA_TextEditPatternId:
+    case UIA_TextPatternId:
+      if (IsText() || IsTextField() || data.role == ax::mojom::Role::kRootWebArea) {


We do not use kRootWebArea, but it is expected by unittests.

shell/platform/common/accessibility_bridge.h

loic-sharma · 2022-12-16T22:21:20Z

third_party/accessibility/ax/platform/ax_platform_node_textprovider_win.cc

@@ -0,0 +1,354 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.


This is Chromium code, right? Should these have Chromium license headers?

loic-sharma · 2022-12-16T22:29:33Z

shell/platform/common/accessibility_bridge.cc

+  // TODO(schectman): figure out when we actually want this attribute set or
+  // not.
+  node_data.AddBoolAttribute(ax::mojom::BoolAttribute::kIsLineBreakingObject,
+                             true);


Just to double-check, should this be kept?

Yeah, I probably will need to set up a new issue for this if there isn't one yet. I don't think we currently have a good way to keep track of this attribute that is used by the AX code, so this is a placeholder as I think commonly used elements usually behave as their own line wrt a11y.

shell/platform/common/accessibility_bridge.h

shell/platform/common/flutter_platform_node_delegate.cc

loic-sharma · 2022-12-16T23:09:29Z

third_party/accessibility/ax/ax_position.h

@@ -1316,7 +1331,8 @@ class AXPosition {
          child_position->affinity_ = ax::mojom::TextAffinity::kUpstream;
          break;
        }
-        child_position = text_position->CreateChildPositionAt(i);
+        AXPositionInstance child = text_position->CreateChildPositionAt(i);
+        child_position = std::move(child);


Is this needed?

third_party/accessibility/ax/ax_position.h

loic-sharma · 2022-12-16T23:29:51Z

third_party/accessibility/ax/platform/ax_platform_node_textrangeprovider_win.cc

+  if (!common_anchor || !start_anchor || !end_anchor)
+    return UIA_E_ELEMENTNOTAVAILABLE;
+
+  SAFEARRAY* safe_array = SafeArrayCreateVector(VT_UNKNOWN, 0, 0);


Why is this different from the Chromium implementation? Is this something we should add in the future? If so, should we leave a TODO here?

I'll add a TODO. This would require us to implement a method to get all children nodes that are between two particular nodes on the tree in traversal order, which I am not aware of currently having.

flutter-dashboard · 2022-12-19T16:16:52Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #38284 at sha ab4d15a

loic-sharma · 2022-12-27T22:24:03Z

third_party/accessibility/ax/ax_position.h

+        max_text_offset_in_parent = dummy_position->MaxTextOffset();
+        if (parent_offset > max_text_offset_in_parent) {
+          parent_offset = max_text_offset_in_parent;
+        }


Could you add a comment as to why this is necessary?

loic-sharma · 2022-12-27T22:57:00Z

third_party/accessibility/ax/platform/ax_platform_node_textprovider_win.cc

+  auto current_line_start = start->Clone();
+  while (!current_line_start->IsNullPosition() && *current_line_start < *end) {
+    auto current_line_end = current_line_start->CreateNextLineEndPosition(
+        AXBoundaryBehavior::CrossBoundary);


Could you explain why AXBoundaryDetection::kDontCheckInitialPosition was removed here?

The method signature for our version of CreateNextLineEndPosition takes a parameter of a different type than Chrome's current version of the same. We take a AXBoundaryBehavior, while Chromium takes a struct consisting of AXBoundaryBehavior, AXBoundaryDetection

loic-sharma · 2022-12-27T23:01:42Z

third_party/accessibility/ax/platform/ax_platform_node_textprovider_win_unittest.cc

+  for (auto data : nodes) {
+    if (!data.node->IsText() && !data.node->data().IsTextField()) {
+      continue;
+    }


Is this necessary? If looks like the original test passed without this

I think it is because we treat the kRootWebArea role differently than in Chrome.

Copy ITextProvider methods into AXPlatformNodeWin Apply selection change event to focus node instead of root The rumor come out: Why does is reads violate in map find No more memory errors, expansion still wobbly

yaakovschectman · 2022-12-28T18:04:26Z

I will attempt to split this PR into several smaller PRs with more organized commits.

yaakovschectman added accessibility affects: desktop platform-windows labels Dec 14, 2022

yaakovschectman requested review from cbracken, loic-sharma, dkwingsmt and a-wallen December 14, 2022 15:14

yaakovschectman self-assigned this Dec 14, 2022

yaakovschectman commented Dec 14, 2022

View reviewed changes

yaakovschectman marked this pull request as ready for review December 14, 2022 19:28