Fix interaction with Gmail app

1. do not map ignored role when a node has text. Much of Gmail appears to be marked as is important for accessibility == false. Unfortunately, this means most of the content in the mail message pane is actually hidden and skipped by ChromeVox.
Revise our mapping rule so that it only applies the ignored role if there is no text (content description or text).
2. compute a node's name based on its descendants. We currently do this for nodes which are clickable.
This covers Gmail's messages list. Each item in the messages list is exposed to Chrome as a "genericContainer" and has no name from the node itself.

By computing name from contents at this layer, we avoid having each service do it.

Test: manually.
Change-Id: I4f53b022c066cb9a6796ac3a0645c47e7d05bddc
Reviewed-on: https://chromium-review.googlesource.com/c/1318356
Commit-Queue: David Tseng <dtseng@chromium.org>
Reviewed-by: Yuki Awano <yawano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606214}

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

@@ -3,6 +3,8 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/accessibility/accessibility_node_info_data_wrapper.h"
+
+#include "base/strings/string_util.h"
 #include "chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h"
 #include "components/exo/wm_helper.h"
 #include "ui/accessibility/platform/ax_android_constants.h"
@@ -81,7 +83,9 @@ void AccessibilityNodeInfoDataWrapper::PopulateAXRole(
                                  class_name);
   }
 
-  if (!GetProperty(AXBooleanProperty::IMPORTANCE)) {
+  if (!GetProperty(AXBooleanProperty::IMPORTANCE) &&
+      !HasProperty(AXStringProperty::TEXT) &&
+      !HasProperty(AXStringProperty::CONTENT_DESCRIPTION)) {
     out_data->role = ax::mojom::Role::kIgnored;
     return;
   }
@@ -302,6 +306,12 @@ void AccessibilityNodeInfoDataWrapper::Serialize(
       out_data->AddStringAttribute(ax::mojom::StringAttribute::kValue, name);
     else
       out_data->SetName(name);
+  } else if (GetProperty(AXBooleanProperty::CLICKABLE)) {
+    // Compute the name by joining all nodes with names.
+    std::vector<std::string> names;
+    ComputeNameFromContents(this, &names);
+    if (!names.empty())
+      out_data->SetName(base::JoinString(names, " "));
   }
 
   std::string role_description;
@@ -422,14 +432,16 @@ void AccessibilityNodeInfoDataWrapper::Serialize(
   }
 }
 
-const std::vector<int32_t>* AccessibilityNodeInfoDataWrapper::GetChildren() {
+void AccessibilityNodeInfoDataWrapper::GetChildren(
+    std::vector<ArcAccessibilityInfoData*>* children) const {
   if (!node_ptr_->int_list_properties)
-    return nullptr;
+    return;
   auto it =
       node_ptr_->int_list_properties->find(AXIntListProperty::CHILD_NODE_IDS);
   if (it == node_ptr_->int_list_properties->end())
-    return nullptr;
-  return &(it->second);
+    return;
+  for (int32_t id : it->second)
+    children->push_back(tree_source_->GetFromId(id));
 }
 
 bool AccessibilityNodeInfoDataWrapper::GetProperty(
@@ -532,4 +544,29 @@ bool AccessibilityNodeInfoDataWrapper::HasCoveringSpan(
   return false;
 }
 
+void AccessibilityNodeInfoDataWrapper::ComputeNameFromContents(
+    const AccessibilityNodeInfoDataWrapper* data,
+    std::vector<std::string>* names) const {
+  // Take the name from either content description or text. It's not clear
+  // whether labelled by should be taken into account here.
+  std::string name;
+  if (!data->GetProperty(AXStringProperty::CONTENT_DESCRIPTION, &name) ||
+      name.empty())
+    data->GetProperty(AXStringProperty::TEXT, &name);
+
+  // Stop when we get a name for this subtree.
+  if (!name.empty()) {
+    names->push_back(name);
+    return;
+  }
+
+  // Otherwise, continue looking for a name in this subtree.
+  std::vector<ArcAccessibilityInfoData*> children;
+  data->GetChildren(&children);
+  for (ArcAccessibilityInfoData* child : children) {
+    ComputeNameFromContents(
+        static_cast<AccessibilityNodeInfoDataWrapper*>(child), names);
+  }
+}
+
 }  // namespace arc

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

@@ -32,7 +32,8 @@ class AccessibilityNodeInfoDataWrapper : public ArcAccessibilityInfoData {
   void PopulateAXRole(ui::AXNodeData* out_data) const override;
   void PopulateAXState(ui::AXNodeData* out_data) const override;
   void Serialize(ui::AXNodeData* out_data) const override;
-  const std::vector<int32_t>* GetChildren() override;
+  void GetChildren(
+      std::vector<ArcAccessibilityInfoData*>* children) const override;
 
   mojom::AccessibilityNodeInfoData* node() { return node_ptr_; }
 
@@ -51,6 +52,9 @@ class AccessibilityNodeInfoDataWrapper : public ArcAccessibilityInfoData {
   bool HasCoveringSpan(mojom::AccessibilityStringProperty prop,
                        mojom::SpanType span_type) const;
 
+  void ComputeNameFromContents(const AccessibilityNodeInfoDataWrapper* data,
+                               std::vector<std::string>* names) const;
+
   AXTreeSourceArc* tree_source_ = nullptr;
   mojom::AccessibilityNodeInfoData* node_ptr_ = nullptr;
 

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

@@ -125,22 +125,21 @@ void AccessibilityWindowInfoDataWrapper::Serialize(
   // and LAYER_ORDER in ax::mojom::IntAttributes.
 }
 
-const std::vector<int32_t>* AccessibilityWindowInfoDataWrapper::GetChildren() {
-  if (children_.size() != 0)
-    return &children_;
-
+void AccessibilityWindowInfoDataWrapper::GetChildren(
+    std::vector<ArcAccessibilityInfoData*>* children) const {
   // Populate the children vector by combining the child window IDs with the
   // root node ID.
   if (window_ptr_->int_list_properties) {
     auto it = window_ptr_->int_list_properties->find(
         mojom::AccessibilityWindowIntListProperty::CHILD_WINDOW_IDS);
     if (it != window_ptr_->int_list_properties->end()) {
-      children_.insert(children_.begin(), it->second.begin(), it->second.end());
+      for (int32_t id : it->second)
+        children->push_back(tree_source_->GetFromId(id));
     }
   }
+
   if (window_ptr_->root_node_id)
-    children_.push_back(window_ptr_->root_node_id);
-  return &children_;
+    children->push_back(tree_source_->GetFromId(window_ptr_->root_node_id));
 }
 
 bool AccessibilityWindowInfoDataWrapper::GetProperty(

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

@@ -34,7 +34,8 @@ class AccessibilityWindowInfoDataWrapper : public ArcAccessibilityInfoData {
   void PopulateAXRole(ui::AXNodeData* out_data) const override;
   void PopulateAXState(ui::AXNodeData* out_data) const override;
   void Serialize(ui::AXNodeData* out_data) const override;
-  const std::vector<int32_t>* GetChildren() override;
+  void GetChildren(
+      std::vector<ArcAccessibilityInfoData*>* children) const override;
 
  private:
   bool GetProperty(mojom::AccessibilityWindowBooleanProperty prop) const;
@@ -48,7 +49,6 @@ class AccessibilityWindowInfoDataWrapper : public ArcAccessibilityInfoData {
 
   AXTreeSourceArc* tree_source_ = nullptr;
   mojom::AccessibilityWindowInfoData* window_ptr_ = nullptr;
-  std::vector<int32_t> children_;
 
   DISALLOW_COPY_AND_ASSIGN(AccessibilityWindowInfoDataWrapper);
 };

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

@@ -37,7 +37,8 @@ class ArcAccessibilityInfoData {
   virtual void PopulateAXRole(ui::AXNodeData* out_data) const = 0;
   virtual void PopulateAXState(ui::AXNodeData* out_data) const = 0;
   virtual void Serialize(ui::AXNodeData* out_data) const = 0;
-  virtual const std::vector<int32_t>* GetChildren() = 0;
+  virtual void GetChildren(
+      std::vector<ArcAccessibilityInfoData*>* children) const = 0;
 };
 
 }  // namespace arc

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

@@ -177,16 +177,14 @@ void AXTreeSourceArc::GetChildren(
     std::vector<ArcAccessibilityInfoData*>* out_children) const {
   if (!info_data)
     return;
-  const std::vector<int32_t>* children = info_data->GetChildren();
-  if (!children)
+
+  info_data->GetChildren(out_children);
+  if (out_children->empty())
     return;
 
   std::map<int32_t, size_t> id_to_index;
-  for (size_t i = 0; i < children->size(); ++i) {
-    ArcAccessibilityInfoData* child = GetFromId((*children)[i]);
-    out_children->push_back(child);
-    id_to_index[child->GetId()] = i;
-  }
+  for (size_t i = 0; i < out_children->size(); i++)
+    id_to_index[out_children->at(i)->GetId()] = i;
 
   // Sort children based on their enclosing bounding rectangles, based on their
   // descendants.
@@ -346,13 +344,12 @@ void AXTreeSourceArc::ComputeEnclosingBoundsInternal(
     computed_bounds.Union(info_data->GetBounds());
     return;
   }
-  const std::vector<int32_t>* children = info_data->GetChildren();
-  if (!children)
+  std::vector<ArcAccessibilityInfoData*> children;
+  info_data->GetChildren(&children);
+  if (children.empty())
     return;
-  for (size_t i = 0; i < children->size(); ++i) {
-    ComputeEnclosingBoundsInternal(GetFromId((*children)[i]), computed_bounds);
-  }
-
+  for (ArcAccessibilityInfoData* child : children)
+    ComputeEnclosingBoundsInternal(child, computed_bounds);
   return;
 }
 

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

@@ -231,6 +231,18 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   AXNodeInfoData* root = event->node_data.back().get();
   root->id = 0;
   SetProperty(root, AXStringProperty::CLASS_NAME, "");
+  SetProperty(root, AXIntListProperty::CHILD_NODE_IDS,
+              std::vector<int>({1, 2}));
+
+  // Child.
+  event->node_data.push_back(AXNodeInfoData::New());
+  AXNodeInfoData* child1 = event->node_data.back().get();
+  child1->id = 1;
+
+  // Another child.
+  event->node_data.push_back(AXNodeInfoData::New());
+  AXNodeInfoData* child2 = event->node_data.back().get();
+  child2->id = 2;
 
   // Populate the tree source with the data.
   CallNotifyAccessibilityEvent(event.get());
@@ -277,6 +289,36 @@ TEST_F(AXTreeSourceArcTest, AccessibleNameComputation) {
   ASSERT_TRUE(
       data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
   ASSERT_EQ("label content description", name);
+
+  // Name from contents.
+
+  // Root node has no name, but has descendants with name.
+  root->string_properties->clear();
+  // Name from contents only happens if a node is clickable.
+  SetProperty(root, AXBooleanProperty::CLICKABLE, true);
+  SetProperty(child1, AXStringProperty::TEXT, "child1 label text");
+  SetProperty(child2, AXStringProperty::TEXT, "child2 label text");
+
+  CallSerializeNode(root, &data);
+  ASSERT_TRUE(
+      data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
+  ASSERT_EQ("child1 label text child2 label text", name);
+
+  // If the node has a name, it should override the contents.
+  SetProperty(root, AXStringProperty::TEXT, "root label text");
+
+  CallSerializeNode(root, &data);
+  ASSERT_TRUE(
+      data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
+  ASSERT_EQ("root label text", name);
+
+  // Clearing both clickable and name from root, the name should not be
+  // populated.
+  root->boolean_properties->clear();
+  root->string_properties->clear();
+  CallSerializeNode(root, &data);
+  ASSERT_FALSE(
+      data->GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
 }
 
 // TODO(katie): Maybe remove this test when adding AccessibilityWindowInfoData