Tighten the applicable condition of clickable property for ARC++ nodes

When android clickable nodes are nested, currently only the outer nodes
are clickable. In this case, even if there are multiple clickable inner
nodes, only the outer node was clickable.

This CL changes to set clickable property only when the node that is
clickable in Android and there is no clickable descendant node of it.

menu is accessible from ChromeVox.)

Bug: b:141969848
Test: unit_tests --gtest_filter="AXTreeSourceArcTest.*"
Test: manually (open PlayStore and confirm that search box and hamburger
Change-Id: I6f62658f8d620d650678162d3547d86af66efa75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849548
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@{#707744}

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

@@ -28,8 +28,11 @@ using AXStringProperty = mojom::AccessibilityStringProperty;
 
 AccessibilityNodeInfoDataWrapper::AccessibilityNodeInfoDataWrapper(
     AXTreeSourceArc* tree_source,
-    AXNodeInfoData* node)
-    : AccessibilityInfoDataWrapper(tree_source), node_ptr_(node) {}
+    AXNodeInfoData* node,
+    bool is_clickable_leaf)
+    : AccessibilityInfoDataWrapper(tree_source),
+      node_ptr_(node),
+      is_clickable_leaf_(is_clickable_leaf) {}
 
 bool AccessibilityNodeInfoDataWrapper::IsNode() const {
   return true;
@@ -323,7 +326,7 @@ void AccessibilityNodeInfoDataWrapper::Serialize(
       out_data->SetName(names[0]);
     else if (names.size() > 1)
       out_data->SetName(base::JoinString(names, " "));
-  } else if (GetProperty(AXBooleanProperty::CLICKABLE)) {
+  } else if (is_clickable_leaf_) {
     // Compute the name by joining all nodes with names.
     std::vector<std::string> names;
     ComputeNameFromContents(this, &names);
@@ -375,7 +378,7 @@ void AccessibilityNodeInfoDataWrapper::Serialize(
   if (GetProperty(AXBooleanProperty::SCROLLABLE)) {
     out_data->AddBoolAttribute(ax::mojom::BoolAttribute::kScrollable, true);
   }
-  if (GetProperty(AXBooleanProperty::CLICKABLE)) {
+  if (is_clickable_leaf_) {
     out_data->AddBoolAttribute(ax::mojom::BoolAttribute::kClickable, true);
   }
   if (GetProperty(AXBooleanProperty::SELECTED)) {

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

@@ -17,7 +17,8 @@ class AXTreeSourceArc;
 class AccessibilityNodeInfoDataWrapper : public AccessibilityInfoDataWrapper {
  public:
   AccessibilityNodeInfoDataWrapper(AXTreeSourceArc* tree_source,
-                                   mojom::AccessibilityNodeInfoData* node);
+                                   mojom::AccessibilityNodeInfoData* node,
+                                   bool is_clickable_leaf);
 
   // AccessibilityInfoDataWrapper overrides.
   bool IsNode() const override;
@@ -56,6 +57,8 @@ class AccessibilityNodeInfoDataWrapper : public AccessibilityInfoDataWrapper {
 
   mojom::AccessibilityNodeInfoData* node_ptr_ = nullptr;
 
+  bool is_clickable_leaf_;
+
   DISALLOW_COPY_AND_ASSIGN(AccessibilityNodeInfoDataWrapper);
 };
 

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

@@ -4,7 +4,6 @@
 
 #include "chrome/browser/chromeos/arc/accessibility/arc_accessibility_util.h"
 
-#include "components/arc/mojom/accessibility_helper.mojom.h"
 #include "ui/accessibility/ax_enums.mojom.h"
 
 namespace arc {
@@ -64,4 +63,16 @@ ax::mojom::Event ToAXEvent(
   return ax::mojom::Event::kChildrenChanged;
 }
 
+bool GetBooleanProperty(mojom::AccessibilityNodeInfoData* node,
+                        mojom::AccessibilityBooleanProperty prop) {
+  if (!node || !node->boolean_properties)
+    return false;
+
+  auto it = node->boolean_properties->find(prop);
+  if (it == node->boolean_properties->end())
+    return false;
+
+  return it->second;
+}
+
 }  // namespace arc

chrome/browser/chromeos/arc/accessibility/arc_accessibility_util.h

@@ -6,18 +6,36 @@
 #define CHROME_BROWSER_CHROMEOS_ARC_ACCESSIBILITY_ARC_ACCESSIBILITY_UTIL_H_
 
 #include <stdint.h>
+#include <vector>
 
+#include "components/arc/mojom/accessibility_helper.mojom.h"
 #include "ui/accessibility/ax_enum_util.h"
 
 namespace arc {
-namespace mojom {
-enum class AccessibilityEventType;
-class AccessibilityNodeInfoData;
-}  // namespace mojom
 
 ax::mojom::Event ToAXEvent(mojom::AccessibilityEventType arc_event_type,
                            mojom::AccessibilityNodeInfoData* node_info_data);
 
+// TODO(hirokisato) clean up GetProperty methods in AccessibilityNodeInfoData
+// and AccessibilityWindowInfoData.
+bool GetBooleanProperty(mojom::AccessibilityNodeInfoData* node,
+                        mojom::AccessibilityBooleanProperty prop);
+
+template <class InfoDataType, class PropType>
+bool GetIntListProperty(InfoDataType* node,
+                        PropType prop,
+                        std::vector<int32_t>* out_value) {
+  if (!node || !node->int_list_properties)
+    return false;
+
+  auto it = node->int_list_properties->find(prop);
+  if (it == node->int_list_properties->end())
+    return false;
+
+  *out_value = it->second;
+  return true;
+}
+
 }  // namespace arc
 
 #endif  // CHROME_BROWSER_CHROMEOS_ARC_ACCESSIBILITY_ARC_ACCESSIBILITY_UTIL_H_

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

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h"
 
+#include <stack>
 #include <string>
 
 #include "base/strings/stringprintf.h"
@@ -21,11 +22,14 @@
 
 namespace arc {
 
+using AXBooleanProperty = mojom::AccessibilityBooleanProperty;
 using AXEventData = mojom::AccessibilityEventData;
 using AXEventType = mojom::AccessibilityEventType;
 using AXIntListProperty = mojom::AccessibilityIntListProperty;
 using AXNodeInfoData = mojom::AccessibilityNodeInfoData;
+using AXNodeInfoDataPtr = mojom::AccessibilityNodeInfoDataPtr;
 using AXWindowInfoData = mojom::AccessibilityWindowInfoData;
+using AXWindowIntListProperty = mojom::AccessibilityWindowIntListProperty;
 
 AXTreeSourceArc::AXTreeSourceArc(Delegate* delegate)
     : current_tree_serializer_(new AXTreeArcSerializer(this)),
@@ -55,17 +59,21 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
   // Finally, we cache each node's computed bounds, based on its descendants.
   std::map<int32_t, int32_t> all_parent_map;
   std::map<int32_t, std::vector<int32_t>> all_children_map;
+  // Mapping nodeId to index in event_data->node_data.
+  std::map<int32_t, int32_t> node_data_index_map;
+
   for (size_t i = 0; i < event_data->node_data.size(); ++i) {
-    if (!event_data->node_data[i]->int_list_properties)
-      continue;
-    auto it = event_data->node_data[i]->int_list_properties->find(
-        AXIntListProperty::CHILD_NODE_IDS);
-    if (it == event_data->node_data[i]->int_list_properties->end())
-      continue;
-    all_children_map[event_data->node_data[i]->id] = it->second;
-    for (size_t j = 0; j < it->second.size(); ++j)
-      all_parent_map[it->second[j]] = event_data->node_data[i]->id;
+    int32_t node_id = event_data->node_data[i]->id;
+    node_data_index_map[node_id] = i;
+    std::vector<int32_t> children;
+    if (GetIntListProperty(event_data->node_data[i].get(),
+                           AXIntListProperty::CHILD_NODE_IDS, &children)) {
+      for (const int32_t child : children)
+        all_parent_map[child] = node_id;
+      all_children_map.emplace(node_id, children);
     }
+  }
+
   if (event_data->window_data) {
     for (size_t i = 0; i < event_data->window_data->size(); ++i) {
       int32_t window_id = event_data->window_data->at(i)->window_id;
@@ -74,16 +82,15 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
         all_parent_map[root_node_id] = window_id;
         all_children_map[window_id] = {root_node_id};
       }
-      if (!event_data->window_data->at(i)->int_list_properties)
-        continue;
-      auto it = event_data->window_data->at(i)->int_list_properties->find(
-          mojom::AccessibilityWindowIntListProperty::CHILD_WINDOW_IDS);
-      if (it == event_data->window_data->at(i)->int_list_properties->end())
-        continue;
+      std::vector<int32_t> children;
+      if (GetIntListProperty(event_data->window_data->at(i).get(),
+                             AXWindowIntListProperty::CHILD_WINDOW_IDS,
+                             &children)) {
+        for (const int32_t child : children)
+          all_parent_map[child] = window_id;
         all_children_map[window_id].insert(all_children_map[window_id].begin(),
-                                         it->second.begin(), it->second.end());
-      for (size_t j = 0; j < it->second.size(); ++j)
-        all_parent_map[it->second[j]] = window_id;
+                                           children.begin(), children.end());
+      }
     }
   }
 
@@ -106,9 +113,9 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
     int32_t parent = stack.top();
     stack.pop();
     const std::vector<int32_t>& children = all_children_map[parent];
-    for (auto it = children.begin(); it != children.end(); ++it) {
-      parent_map_[*it] = parent;
-      stack.push(*it);
+    for (const int32_t child : children) {
+      parent_map_[child] = parent;
+      stack.push(child);
     }
   }
 
@@ -119,8 +126,10 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
     if (parent_map_.find(id) == parent_map_.end() && id != root_id_)
       continue;
     AXNodeInfoData* node = event_data->node_data[i].get();
-    tree_map_[id] =
-        std::make_unique<AccessibilityNodeInfoDataWrapper>(this, node);
+    bool is_clickable_leaf =
+        ComputeIsClickableLeaf(i, event_data->node_data, node_data_index_map);
+    tree_map_[id] = std::make_unique<AccessibilityNodeInfoDataWrapper>(
+        this, node, is_clickable_leaf);
 
     if (tree_map_[id]->IsFocused())
       focused_id_ = id;
@@ -366,6 +375,35 @@ void AXTreeSourceArc::ComputeEnclosingBoundsInternal(
   return;
 }
 
+bool AXTreeSourceArc::ComputeIsClickableLeaf(
+    int32_t root_index,
+    const std::vector<AXNodeInfoDataPtr>& nodes,
+    const std::map<int32_t, int32_t>& node_id_to_array_index) const {
+  AXNodeInfoData* node = nodes[root_index].get();
+  if (!GetBooleanProperty(node, AXBooleanProperty::CLICKABLE))
+    return false;
+
+  std::vector<int32_t> children;
+  if (!GetIntListProperty(node, AXIntListProperty::CHILD_NODE_IDS, &children))
+    return true;
+
+  std::stack<int32_t, std::vector<int32_t>> stack(children);
+  while (!stack.empty()) {
+    int32_t parent_id = stack.top();
+    stack.pop();
+    AXNodeInfoData* parent_node =
+        nodes[node_id_to_array_index.at(parent_id)].get();
+    if (GetBooleanProperty(parent_node, AXBooleanProperty::CLICKABLE))
+      return false;
+    if (GetIntListProperty(parent_node, AXIntListProperty::CHILD_NODE_IDS,
+                           &children)) {
+      for (const int32_t child : children)
+        stack.push(child);
+    }
+  }
+  return true;
+}
+
 void AXTreeSourceArc::Reset() {
   tree_map_.clear();
   parent_map_.clear();

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

@@ -105,6 +105,12 @@ class AXTreeSourceArc : public ui::AXTreeSource<AccessibilityInfoDataWrapper*,
   void ComputeEnclosingBoundsInternal(AccessibilityInfoDataWrapper* info_data,
                                       gfx::Rect& computed_bounds) const;
 
+  // Computes the node is clickable and there is no clickable descendant of it.
+  bool ComputeIsClickableLeaf(
+      int32_t root_index,
+      const std::vector<mojom::AccessibilityNodeInfoDataPtr>& nodes,
+      const std::map<int32_t, int32_t>& node_id_to_array_index) const;
+
   // Resets tree state.
   void Reset();
 

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

@@ -164,24 +164,25 @@ class AXTreeSourceArcTest : public testing::Test,
   void CallGetChildren(
       AXNodeInfoData* node,
       std::vector<AccessibilityInfoDataWrapper*>* out_children) const {
-    AccessibilityNodeInfoDataWrapper node_data(tree_source_.get(), node);
-    tree_source_->GetChildren(&node_data, out_children);
+    AccessibilityInfoDataWrapper* node_data = tree_source_->GetFromId(node->id);
+    tree_source_->GetChildren(node_data, out_children);
   }
 
   void CallSerializeNode(AXNodeInfoData* node,
                          std::unique_ptr<ui::AXNodeData>* out_data) const {
     ASSERT_TRUE(out_data);
-    AccessibilityNodeInfoDataWrapper node_data(tree_source_.get(), node);
+    AccessibilityInfoDataWrapper* node_data = tree_source_->GetFromId(node->id);
     *out_data = std::make_unique<ui::AXNodeData>();
-    tree_source_->SerializeNode(&node_data, out_data->get());
+    tree_source_->SerializeNode(node_data, out_data->get());
   }
 
   void CallSerializeWindow(AXWindowInfoData* window,
                            std::unique_ptr<ui::AXNodeData>* out_data) const {
     ASSERT_TRUE(out_data);
-    AccessibilityWindowInfoDataWrapper window_data(tree_source_.get(), window);
+    AccessibilityInfoDataWrapper* window_data =
+        tree_source_->GetFromId(window->window_id);
     *out_data = std::make_unique<ui::AXNodeData>();
-    tree_source_->SerializeNode(&window_data, out_data->get());
+    tree_source_->SerializeNode(window_data, out_data->get());
   }
 
   AccessibilityInfoDataWrapper* CallGetFromId(int32_t id) const {
@@ -484,6 +485,7 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   // Text (empty).
   SetProperty(root, AXStringProperty::TEXT, "");
 
+  CallNotifyAccessibilityEvent(event.get());
   CallSerializeNode(root, &data);
   // With crrev/1786363, empty text on node will not set the name.
   ASSERT_FALSE(
@@ -493,6 +495,7 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   root->string_properties->clear();
   SetProperty(root, AXStringProperty::TEXT, "label text");
 
+  CallNotifyAccessibilityEvent(event.get());
   CallSerializeNode(root, &data);
   ASSERT_TRUE(
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
@@ -501,6 +504,7 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   // Content description (empty), text (non-empty).
   SetProperty(root, AXStringProperty::CONTENT_DESCRIPTION, "");
 
+  CallNotifyAccessibilityEvent(event.get());
   CallSerializeNode(root, &data);
   ASSERT_TRUE(
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
@@ -510,6 +514,7 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   root->string_properties.value()[AXStringProperty::CONTENT_DESCRIPTION] =
       "label content description";
 
+  CallNotifyAccessibilityEvent(event.get());
   CallSerializeNode(root, &data);
   ASSERT_TRUE(
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
@@ -524,14 +529,27 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   SetProperty(child1, AXStringProperty::TEXT, "child1 label text");
   SetProperty(child2, AXStringProperty::TEXT, "child2 label text");
 
+  CallNotifyAccessibilityEvent(event.get());
   CallSerializeNode(root, &data);
   ASSERT_TRUE(
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
   ASSERT_EQ("child1 label text child2 label text", name);
 
+  // If a child is also clickable, do not use child property.
+  SetProperty(child1, AXBooleanProperty::CLICKABLE, true);
+  SetProperty(child2, AXBooleanProperty::CLICKABLE, true);
+
+  CallNotifyAccessibilityEvent(event.get());
+  CallSerializeNode(root, &data);
+  ASSERT_FALSE(
+      data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
+
   // If the node has a name, it should override the contents.
+  child1->boolean_properties->clear();
+  child2->boolean_properties->clear();
   SetProperty(root, AXStringProperty::TEXT, "root label text");
 
+  CallNotifyAccessibilityEvent(event.get());
   CallSerializeNode(root, &data);
   ASSERT_TRUE(
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
@@ -548,11 +566,10 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   // populated.
   root->boolean_properties->clear();
   root->string_properties->clear();
+  CallNotifyAccessibilityEvent(event.get());
   CallSerializeNode(root, &data);
   ASSERT_FALSE(
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
-
-  EXPECT_EQ(1, GetDispatchedEventCount(ax::mojom::Event::kFocus));
 }
 
 TEST_F(AXTreeSourceArcTest, AccessibleNameComputationWindow) {