Prevent ARC window title announced twice

Previous CL (http://crrev/c/2024635) wired ARC window title to its root
node. However, this made ChromeVox announce window title twice. One is
from the window title and the other is from a name of the root node.

This CL partially reverts the previous change and makes default fallback
focus to the window itself.

This change also makes only the root window of each ARC task be modal
instead of assigning to every root node.
Android framework sends only actionable windows to AccessibilityService.

Bug: b:150342403
Test: unit_tests --gtest_filter="AXTreeSourceArcTest.*"
Test: manual. Open any ARC app and title is not announced twice.
Change-Id: Ie6527af9b01c03a175517dcfe7d88ca29b4b034a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2099671
Commit-Queue: Hiroki Sato <hirokisato@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Reviewed-by: Sara Kato <sarakato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750893}

chrome/browser/chromeos/arc/accessibility/accessibility_node_info_data_wrapper.cc

@@ -274,9 +274,6 @@ void AccessibilityNodeInfoDataWrapper::PopulateAXState(
 
   if (!is_important_)
     out_data->AddState(ax::mojom::State::kIgnored);
-
-  if (tree_source_->IsRootOfNodeTree(GetId()))
-    out_data->AddState(ax::mojom::State::kFocusable);
 }
 
 void AccessibilityNodeInfoDataWrapper::Serialize(
@@ -359,19 +356,6 @@ void AccessibilityNodeInfoDataWrapper::Serialize(
     ComputeNameFromContents(this, &names);
     if (!names.empty())
       out_data->SetName(base::JoinString(names, " "));
-  } else if (is_node_tree_root) {
-    AccessibilityInfoDataWrapper* parent =
-        tree_source_->GetParent(tree_source_->GetFromId(node_ptr_->id));
-    if (parent && parent->GetWindow()) {
-      std::string title;
-      if (arc::GetProperty(parent->GetWindow()->string_properties,
-                           mojom::AccessibilityWindowStringProperty::TITLE,
-                           &title) &&
-          !title.empty()) {
-        out_data->SetName(title);
-        out_data->SetNameFrom(ax::mojom::NameFrom::kTitle);
-      }
-    }
   }
 
   std::string role_description;
@@ -422,9 +406,6 @@ void AccessibilityNodeInfoDataWrapper::Serialize(
     out_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSupportsTextLocation,
                                true);
   }
-  if (is_node_tree_root) {
-    out_data->AddBoolAttribute(ax::mojom::BoolAttribute::kModal, true);
-  }
 
   // Range info.
   if (node_ptr_->range_info) {

chrome/browser/chromeos/arc/accessibility/accessibility_window_info_data_wrapper.cc

@@ -78,14 +78,12 @@ void AccessibilityWindowInfoDataWrapper::PopulateAXState(
     ui::AXNodeData* out_data) const {
   // ARC++ window states are not reflected in ax::mojom::State, and for the
   // most part aren't needed.
-  // Focusable in Android simply means a node within the window is focusable.
-  // Since the window itself is not focusable in Android, it doesn't make sense
-  // to include Focusable as an AXState.
 }
 
 void AccessibilityWindowInfoDataWrapper::Serialize(
     ui::AXNodeData* out_data) const {
-  if (!tree_source_->GetRoot())
+  AccessibilityInfoDataWrapper* root = tree_source_->GetRoot();
+  if (!root)
     return;
 
   AccessibilityInfoDataWrapper::Serialize(out_data);
@@ -97,6 +95,16 @@ void AccessibilityWindowInfoDataWrapper::Serialize(
     out_data->SetNameFrom(ax::mojom::NameFrom::kTitle);
   }
 
+  if (root->GetId() == GetId()) {
+    // Make the root window of each ARC task modal
+    out_data->AddBoolAttribute(ax::mojom::BoolAttribute::kModal, true);
+
+    // Focusable in Android simply means a node within the window is focusable.
+    // The window itself is not focusable in Android, but ChromeVox sets the
+    // focus to the entire window, explicitly specify this.
+    out_data->AddState(ax::mojom::State::kFocusable);
+  }
+
   // Not all properties are currently used in Chrome Accessibility.
 
   // Boolean properties:

chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc

@@ -164,35 +164,26 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
     // TODO (sarakato): Add proper fix once cause of invalid node is known.
     if (!IsValid(root)) {
       return;
-    } else if (root->IsNode()) {
-      android_focused_id_ = root_id_;
     } else {
-      std::vector<AccessibilityInfoDataWrapper*> children;
-      root->GetChildren(&children);
-      for (const AccessibilityInfoDataWrapper* child : children) {
-        if (child->IsNode()) {
-          int32_t child_id = child->GetId();
-          DCHECK(IsRootOfNodeTree(child_id));
-          android_focused_id_ = child_id;
-          break;
-        }
-      }
+      android_focused_id_ = root_id_;
     }
   }
 
-  AXNodeInfoData* focused_node =
-      android_focused_id_.has_value()
-          ? GetFromId(*android_focused_id_)->GetNode()
-          : nullptr;
+  AccessibilityInfoDataWrapper* focused_node =
+      android_focused_id_.has_value() ? GetFromId(*android_focused_id_)
+                                      : nullptr;
 
   // Ensure that the focused node correctly gets focus.
-  while (android_focused_id_.has_value() && android_focused_id_ != root_id_ &&
-         !IsImportantInAndroid(focused_node)) {
-    AccessibilityInfoDataWrapper* parent =
-        GetFromId(parent_map_[*android_focused_id_]);
-    if (parent && parent->IsNode()) {
+  while (focused_node) {
+    bool focusable_node = focused_node->IsNode() &&
+                          !IsImportantInAndroid(focused_node->GetNode());
+    bool root_window = focused_node->GetId() == root_id_;
+    if (!focusable_node && !root_window)
+      break;
+    AccessibilityInfoDataWrapper* parent = GetParent(focused_node);
+    if (parent) {
       android_focused_id_ = parent->GetId();
-      focused_node = parent->GetNode();
+      focused_node = parent;
     } else {
       break;
     }
@@ -205,7 +196,8 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
 
   event_bundle.events.emplace_back();
   ui::AXEvent& event = event_bundle.events.back();
-  event.event_type = ToAXEvent(event_data->event_type, focused_node);
+  event.event_type = ToAXEvent(
+      event_data->event_type, focused_node ? focused_node->GetNode() : nullptr);
   event.id = event_data->source_id;
 
   if ((event_data->event_type == AXEventType::WINDOW_CONTENT_CHANGED ||

chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc_unittest.cc

@@ -368,9 +368,9 @@ TEST_F(AXTreeSourceArcTest, ReorderChildrenByLayout) {
 
   // Sanity check tree output.
   ExpectTree(
-      "id=100 window (0, 0)-(0, 0) child_ids=10\n"
-      "  id=10 genericContainer FOCUSABLE INVISIBLE (0, 0)-(0, 0) "
-      "restriction=disabled modal=true child_ids=1,2\n"
+      "id=100 window FOCUSABLE (0, 0)-(0, 0) modal=true child_ids=10\n"
+      "  id=10 genericContainer INVISIBLE (0, 0)-(0, 0) restriction=disabled "
+      "child_ids=1,2\n"
       "    id=1 button FOCUSABLE (100, 100)-(100, 100) restriction=disabled "
       "class_name=android.widget.Button name=button1\n"
       "    id=2 button FOCUSABLE (100, 100)-(10, 100) restriction=disabled "
@@ -513,15 +513,6 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   CallSerializeNode(root, &data);
   ASSERT_FALSE(
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
-
-  // Root window title propagates to the name of the root node of the window if
-  // it isn't specified.
-  SetProperty(root_window, AXWindowStringProperty::TITLE, "Window Title");
-  CallNotifyAccessibilityEvent(event.get());
-  CallSerializeNode(root, &data);
-  ASSERT_TRUE(
-      data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
-  ASSERT_EQ("Window Title", name);
 }
 
 TEST_F(AXTreeSourceArcTest, AccessibleNameComputationTextField) {
@@ -699,6 +690,7 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputationWindowWithChildren) {
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
   EXPECT_EQ("window title", name);
   EXPECT_NE(ax::mojom::Role::kRootWebArea, data->role);
+  EXPECT_TRUE(data->GetBoolAttribute(ax::mojom::BoolAttribute::kModal));
 
   CallSerializeWindow(child, &data);
   ASSERT_TRUE(
@@ -711,7 +703,6 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputationWindowWithChildren) {
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
   EXPECT_EQ("node text", name);
   EXPECT_EQ(ax::mojom::Role::kStaticText, data->role);
-  EXPECT_TRUE(data->GetBoolAttribute(ax::mojom::BoolAttribute::kModal));
 
   CallSerializeNode(child_node, &data);
   ASSERT_TRUE(
@@ -907,13 +898,6 @@ TEST_F(AXTreeSourceArcTest, GetTreeDataAppliesFocus) {
   AXWindowInfoData* child = event->window_data->back().get();
   child->window_id = 1;
 
-  CallNotifyAccessibilityEvent(event.get());
-  ui::AXTreeData data;
-
-  // Nothing should be focused when there are no nodes.
-  EXPECT_TRUE(CallGetTreeData(&data));
-  EXPECT_EQ(ui::AXNode::kInvalidAXID, data.focus_id);
-
   // Add a child node.
   root->root_node_id = 2;
   event->node_data.push_back(AXNodeInfoData::New());
@@ -923,10 +907,11 @@ TEST_F(AXTreeSourceArcTest, GetTreeDataAppliesFocus) {
 
   CallNotifyAccessibilityEvent(event.get());
 
+  ui::AXTreeData data;
   EXPECT_TRUE(CallGetTreeData(&data));
-  EXPECT_EQ(2, data.focus_id);
+  EXPECT_EQ(root->window_id, data.focus_id);
 
-  EXPECT_EQ(2, GetDispatchedEventCount(ax::mojom::Event::kLayoutComplete));
+  EXPECT_EQ(1, GetDispatchedEventCount(ax::mojom::Event::kLayoutComplete));
 }
 
 TEST_F(AXTreeSourceArcTest, OnViewSelectedEvent) {
@@ -1169,9 +1154,9 @@ TEST_F(AXTreeSourceArcTest, SerializeAndUnserialize) {
   CallNotifyAccessibilityEvent(event.get());
   EXPECT_EQ(1, GetDispatchedEventCount(ax::mojom::Event::kFocus));
   ExpectTree(
-      "id=100 window (0, 0)-(0, 0) child_ids=10\n"
-      "  id=10 genericContainer FOCUSABLE IGNORED INVISIBLE (0, 0)-(0, 0) "
-      "restriction=disabled modal=true child_ids=1\n"
+      "id=100 window FOCUSABLE (0, 0)-(0, 0) modal=true child_ids=10\n"
+      "  id=10 genericContainer IGNORED INVISIBLE (0, 0)-(0, 0) "
+      "restriction=disabled child_ids=1\n"
       "    id=1 genericContainer IGNORED INVISIBLE (0, 0)-(0, 0) "
       "restriction=disabled child_ids=2\n"
       "      id=2 genericContainer IGNORED INVISIBLE (0, 0)-(0, 0) "
@@ -1191,9 +1176,9 @@ TEST_F(AXTreeSourceArcTest, SerializeAndUnserialize) {
 
   CallNotifyAccessibilityEvent(event.get());
   ExpectTree(
-      "id=100 window (0, 0)-(0, 0) child_ids=10\n"
-      "  id=10 genericContainer FOCUSABLE INVISIBLE (0, 0)-(0, 0) "
-      "restriction=disabled modal=true child_ids=1\n"
+      "id=100 window FOCUSABLE (0, 0)-(0, 0) modal=true child_ids=10\n"
+      "  id=10 genericContainer INVISIBLE (0, 0)-(0, 0) "
+      "restriction=disabled child_ids=1\n"
       "    id=1 genericContainer IGNORED INVISIBLE (0, 0)-(0, 0) "
       "restriction=disabled child_ids=2\n"
       "      id=2 genericContainer IGNORED INVISIBLE (0, 0)-(0, 0) "
@@ -1341,7 +1326,7 @@ TEST_F(AXTreeSourceArcTest, SyncFocus) {
   EXPECT_EQ(1, GetDispatchedEventCount(ax::mojom::Event::kLocationChanged));
 
   // When the focused node disappeared from the tree, move the focus to the
-  // root of the node tree.
+  // root.
   root->int_list_properties->clear();
   event->node_data.resize(1);
 
@@ -1349,7 +1334,7 @@ TEST_F(AXTreeSourceArcTest, SyncFocus) {
   CallNotifyAccessibilityEvent(event.get());
 
   EXPECT_TRUE(CallGetTreeData(&data));
-  EXPECT_EQ(root->id, data.focus_id);
+  EXPECT_EQ(root_window->window_id, data.focus_id);
 }
 
 TEST_F(AXTreeSourceArcTest, LiveRegion) {