arc-a11y: Move a11y importance computation into DataInfoWrapper

Moving importance computation from AXTreeSourceArc into DataInfoWrapper
makes clear the responsibilities of each classes, where the former is
managing the entire tree and the latter computes properties of each
node.

This will allow us to make unit tests of each DataInfoWrapper classes
easier.

Bug: b:152930082
Test: unit_tests --gtest_filter="AXTreeSourceArcTest.*"
Test: tast tests still passes.
Change-Id: Ia2f03af1788908560fdb92748df7d702b11dada5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2249432Reviewed-by: David Tseng <dtseng@chromium.org>
Reviewed-by: Sara Kato <sarakato@chromium.org>
Commit-Queue: Hiroki Sato <hirokisato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781091}

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

@@ -38,6 +38,7 @@ class AccessibilityInfoDataWrapper {
   virtual bool IsVisibleToUser() const = 0;
   virtual bool IsVirtualNode() const = 0;
   virtual bool IsIgnored() const = 0;
+  virtual bool IsImportantInAndroid() const = 0;
   virtual bool CanBeAccessibilityFocused() const = 0;
   virtual bool IsAccessibilityFocusableContainer() const = 0;
   virtual void PopulateAXRole(ui::AXNodeData* out_data) const = 0;

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

@@ -34,11 +34,8 @@ using AXStringProperty = mojom::AccessibilityStringProperty;
 
 AccessibilityNodeInfoDataWrapper::AccessibilityNodeInfoDataWrapper(
     AXTreeSourceArc* tree_source,
-    AXNodeInfoData* node,
-    bool is_important)
-    : AccessibilityInfoDataWrapper(tree_source),
-      node_ptr_(node),
-      is_important_(is_important) {}
+    AXNodeInfoData* node)
+    : AccessibilityInfoDataWrapper(tree_source), node_ptr_(node) {}
 
 AccessibilityNodeInfoDataWrapper::~AccessibilityNodeInfoDataWrapper() = default;
 
@@ -74,11 +71,9 @@ bool AccessibilityNodeInfoDataWrapper::IsVirtualNode() const {
 
 bool AccessibilityNodeInfoDataWrapper::IsIgnored() const {
   if (!tree_source_->IsScreenReaderMode())
-    return !is_important_;
+    return !IsImportantInAndroid();
 
-  // Most conditions are precomputed in AXTreeSourceArc::BuildImportanceTable().
-  // TODO(hirokisato): migrate importance computation here.
-  if (!is_important_)
+  if (!IsImportantInAndroid() || !HasImportantProperty())
     return true;
 
   if (IsAccessibilityFocusableContainer())
@@ -90,6 +85,10 @@ bool AccessibilityNodeInfoDataWrapper::IsIgnored() const {
   return !HasAccessibilityFocusableText();
 }
 
+bool AccessibilityNodeInfoDataWrapper::IsImportantInAndroid() const {
+  return IsVirtualNode() || GetProperty(AXBooleanProperty::IMPORTANCE);
+}
+
 bool AccessibilityNodeInfoDataWrapper::CanBeAccessibilityFocused() const {
   // Using HasText() here is incomplete because it doesn't match the
   // populated ax name. However, this method is used only from AXTreeSourceArc
@@ -102,7 +101,7 @@ bool AccessibilityNodeInfoDataWrapper::CanBeAccessibilityFocused() const {
 
 bool AccessibilityNodeInfoDataWrapper::IsAccessibilityFocusableContainer()
     const {
-  if (!is_important_)
+  if (!IsImportantInAndroid())
     return false;
 
   return GetProperty(AXBooleanProperty::SCREEN_READER_FOCUSABLE) ||
@@ -584,7 +583,19 @@ bool AccessibilityNodeInfoDataWrapper::GetProperty(
 
 bool AccessibilityNodeInfoDataWrapper::HasStandardAction(
     AXActionType action) const {
-  return arc::HasStandardAction(node_ptr_, action);
+  if (!node_ptr_->int_list_properties)
+    return false;
+
+  auto itr = node_ptr_->int_list_properties->find(
+      AXIntListProperty::STANDARD_ACTION_IDS);
+  if (itr == node_ptr_->int_list_properties->end())
+    return false;
+
+  for (const auto supported_action : itr->second) {
+    if (static_cast<AXActionType>(supported_action) == action)
+      return true;
+  }
+  return false;
 }
 
 bool AccessibilityNodeInfoDataWrapper::HasCoveringSpan(
@@ -622,7 +633,7 @@ bool AccessibilityNodeInfoDataWrapper::HasText() const {
 }
 
 bool AccessibilityNodeInfoDataWrapper::HasAccessibilityFocusableText() const {
-  if (!is_important_ || !HasText())
+  if (!IsImportantInAndroid() || !HasText())
     return false;
 
   // If any ancestor has a focusable property, the text is used by that node.
@@ -700,4 +711,54 @@ bool AccessibilityNodeInfoDataWrapper::IsToplevelScrollItem() const {
       ->IsScrollableContainer();
 }
 
+bool AccessibilityNodeInfoDataWrapper::HasImportantProperty() const {
+  if (!has_important_property_cache_.has_value())
+    has_important_property_cache_ = HasImportantPropertyInternal();
+
+  return *has_important_property_cache_;
+}
+
+bool AccessibilityNodeInfoDataWrapper::HasImportantPropertyInternal() const {
+  if (HasNonEmptyStringProperty(node_ptr_,
+                                AXStringProperty::CONTENT_DESCRIPTION) ||
+      HasNonEmptyStringProperty(node_ptr_, AXStringProperty::TEXT) ||
+      HasNonEmptyStringProperty(node_ptr_, AXStringProperty::PANE_TITLE) ||
+      HasNonEmptyStringProperty(node_ptr_, AXStringProperty::HINT_TEXT) ||
+      cached_name_.has_value()) {
+    return true;
+  }
+
+  // These properties are sorted in the same order of mojom file.
+  if (GetProperty(AXBooleanProperty::CHECKABLE) ||
+      GetProperty(AXBooleanProperty::FOCUSABLE) ||
+      GetProperty(AXBooleanProperty::SELECTED) ||
+      GetProperty(AXBooleanProperty::CLICKABLE) ||
+      GetProperty(AXBooleanProperty::EDITABLE)) {
+    return true;
+  }
+
+  if (HasStandardAction(AXActionType::FOCUS) ||
+      HasStandardAction(AXActionType::CLEAR_FOCUS) ||
+      HasStandardAction(AXActionType::CLICK)) {
+    return true;
+  }
+
+  ui::AXNodeData data;
+  PopulateAXRole(&data);
+  if (ui::IsControl(data.role))
+    return true;
+
+  // Check if any ancestor has an important property.
+  std::vector<AccessibilityInfoDataWrapper*> children;
+  GetChildren(&children);
+  for (AccessibilityInfoDataWrapper* child : children) {
+    if (static_cast<AccessibilityNodeInfoDataWrapper*>(child)
+            ->HasImportantProperty()) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 }  // namespace arc

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

@@ -20,8 +20,7 @@ class AXTreeSourceArc;
 class AccessibilityNodeInfoDataWrapper : public AccessibilityInfoDataWrapper {
  public:
   AccessibilityNodeInfoDataWrapper(AXTreeSourceArc* tree_source,
-                                   mojom::AccessibilityNodeInfoData* node,
-                                   bool is_important);
+                                   mojom::AccessibilityNodeInfoData* node);
 
   ~AccessibilityNodeInfoDataWrapper() override;
 
@@ -34,6 +33,7 @@ class AccessibilityNodeInfoDataWrapper : public AccessibilityInfoDataWrapper {
   bool IsVisibleToUser() const override;
   bool IsVirtualNode() const override;
   bool IsIgnored() const override;
+  bool IsImportantInAndroid() const override;
   bool CanBeAccessibilityFocused() const override;
   bool IsAccessibilityFocusableContainer() const override;
   void PopulateAXRole(ui::AXNodeData* out_data) const override;
@@ -76,15 +76,20 @@ class AccessibilityNodeInfoDataWrapper : public AccessibilityInfoDataWrapper {
   bool IsScrollableContainer() const;
   bool IsToplevelScrollItem() const;
 
-  mojom::AccessibilityNodeInfoData* node_ptr_ = nullptr;
+  bool HasImportantProperty() const;
+  bool HasImportantPropertyInternal() const;
 
-  bool is_important_;
+  mojom::AccessibilityNodeInfoData* node_ptr_ = nullptr;
 
   base::Optional<ax::mojom::Role> role_;
   base::Optional<std::string> cached_name_;
   mojom::AccessibilityLiveRegionType container_live_status_ =
       mojom::AccessibilityLiveRegionType::NONE;
 
+  // This property is a cached value so that we can avoid same computation.
+  // mutable because once the value is computed it won't change.
+  mutable base::Optional<bool> has_important_property_cache_;
+
   DISALLOW_COPY_AND_ASSIGN(AccessibilityNodeInfoDataWrapper);
 };
 

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

@@ -52,6 +52,10 @@ bool AccessibilityWindowInfoDataWrapper::IsIgnored() const {
   return false;
 }
 
+bool AccessibilityWindowInfoDataWrapper::IsImportantInAndroid() const {
+  return true;
+}
+
 bool AccessibilityWindowInfoDataWrapper::CanBeAccessibilityFocused() const {
   // Windows are too generic to be Accessibility focused in Chrome, although
   // they can be Accessibility focused in Android by virtue of having

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

@@ -31,6 +31,7 @@ class AccessibilityWindowInfoDataWrapper : public AccessibilityInfoDataWrapper {
   bool IsVisibleToUser() const override;
   bool IsVirtualNode() const override;
   bool IsIgnored() const override;
+  bool IsImportantInAndroid() const override;
   bool CanBeAccessibilityFocused() const override;
   bool IsAccessibilityFocusableContainer() const override;
   void PopulateAXRole(ui::AXNodeData* out_data) const override;

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

@@ -10,12 +10,6 @@
 
 namespace arc {
 
-using AXActionType = mojom::AccessibilityActionType;
-using AXBooleanProperty = mojom::AccessibilityBooleanProperty;
-using AXIntListProperty = mojom::AccessibilityIntListProperty;
-using AXNodeInfoData = mojom::AccessibilityNodeInfoData;
-using AXStringProperty = mojom::AccessibilityStringProperty;
-
 ax::mojom::Event ToAXEvent(mojom::AccessibilityEventType arc_event_type,
                            AccessibilityInfoDataWrapper* source_node,
                            AccessibilityInfoDataWrapper* focused_node) {
@@ -134,60 +128,4 @@ std::string ToLiveStatusString(mojom::AccessibilityLiveRegionType type) {
   return std::string();  // Dummy.
 }
 
-bool IsImportantInAndroid(AXNodeInfoData* node) {
-  if (!node)
-    return false;
-
-  return node->is_virtual_node ||
-         GetBooleanProperty(node, AXBooleanProperty::IMPORTANCE);
-}
-
-bool HasImportantProperty(AXNodeInfoData* node) {
-  if (!node)
-    return false;
-
-  // These properties are used to compute accessibility name in
-  // AccessibilityNodeInfoDataWrapper.
-  // TODO(hirokisato): Also check LABELED_BY.
-  if (HasNonEmptyStringProperty(node, AXStringProperty::CONTENT_DESCRIPTION) ||
-      HasNonEmptyStringProperty(node, AXStringProperty::TEXT) ||
-      HasNonEmptyStringProperty(node, AXStringProperty::PANE_TITLE) ||
-      HasNonEmptyStringProperty(node, AXStringProperty::HINT_TEXT)) {
-    return true;
-  }
-
-  // These properties are sorted in the same order of mojom file.
-  if (GetBooleanProperty(node, AXBooleanProperty::CHECKABLE) ||
-      GetBooleanProperty(node, AXBooleanProperty::FOCUSABLE) ||
-      GetBooleanProperty(node, AXBooleanProperty::SELECTED) ||
-      GetBooleanProperty(node, AXBooleanProperty::EDITABLE)) {
-    return true;
-  }
-
-  if (HasStandardAction(node, AXActionType::FOCUS) ||
-      HasStandardAction(node, AXActionType::CLEAR_FOCUS) ||
-      HasStandardAction(node, AXActionType::CLICK)) {
-    return true;
-  }
-
-  // TODO(hirokisato): Consider to check ui::IsControl(role).
-  return false;
-}
-
-bool HasStandardAction(AXNodeInfoData* node, AXActionType action) {
-  if (!node || !node->int_list_properties)
-    return false;
-
-  auto itr =
-      node->int_list_properties->find(AXIntListProperty::STANDARD_ACTION_IDS);
-  if (itr == node->int_list_properties->end())
-    return false;
-
-  for (const auto supported_action : itr->second) {
-    if (static_cast<AXActionType>(supported_action) == action)
-      return true;
-  }
-  return false;
-}
-
 }  // namespace arc

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

@@ -25,13 +25,6 @@ base::Optional<mojom::AccessibilityActionType> ConvertToAndroidAction(
 
 std::string ToLiveStatusString(mojom::AccessibilityLiveRegionType type);
 
-bool IsImportantInAndroid(mojom::AccessibilityNodeInfoData* node);
-
-bool HasImportantProperty(mojom::AccessibilityNodeInfoData* node);
-
-bool HasStandardAction(mojom::AccessibilityNodeInfoData* node,
-                       mojom::AccessibilityActionType action);
-
 template <class DataType, class PropType>
 bool GetBooleanProperty(DataType* node, PropType prop) {
   if (!node->boolean_properties)

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

@@ -14,8 +14,6 @@
 #include "chrome/browser/chromeos/arc/accessibility/accessibility_window_info_data_wrapper.h"
 #include "chrome/browser/chromeos/arc/accessibility/arc_accessibility_util.h"
 #include "chrome/browser/chromeos/arc/accessibility/geometry_util.h"
-#include "chrome/browser/ui/aura/accessibility/automation_manager_aura.h"
-#include "components/exo/wm_helper.h"
 #include "extensions/browser/api/automation_internal/automation_event_router.h"
 #include "extensions/common/extension_messages.h"
 #include "ui/accessibility/ax_enums.mojom.h"
@@ -30,10 +28,8 @@ using AXEventType = mojom::AccessibilityEventType;
 using AXIntProperty = mojom::AccessibilityIntProperty;
 using AXIntListProperty = mojom::AccessibilityIntListProperty;
 using AXNodeInfoData = mojom::AccessibilityNodeInfoData;
-using AXNodeInfoDataPtr = mojom::AccessibilityNodeInfoDataPtr;
 using AXStringProperty = mojom::AccessibilityStringProperty;
 using AXWindowInfoData = mojom::AccessibilityWindowInfoData;
-using AXWindowInfoDataPtr = mojom::AccessibilityWindowInfoDataPtr;
 using AXWindowIntListProperty = mojom::AccessibilityWindowIntListProperty;
 
 namespace {
@@ -96,11 +92,12 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
     }
   }
 
-  // Mapping from node id 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) {
     int32_t node_id = event_data->node_data[i]->id;
-    node_data_index_map[node_id] = i;
+    AXNodeInfoData* node = event_data->node_data[i].get();
+    tree_map_[node_id] =
+        std::make_unique<AccessibilityNodeInfoDataWrapper>(this, node);
+
     std::vector<int32_t> children;
     if (GetProperty(event_data->node_data[i].get()->int_list_properties,
                     AXIntListProperty::CHILD_NODE_IDS, &children)) {
@@ -108,14 +105,6 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
         parent_map_[child] = node_id;
     }
   }
-  std::vector<bool> is_important(event_data->node_data.size());
-  BuildImportanceTable(event_data, node_data_index_map, &is_important);
-  for (int i = event_data->node_data.size() - 1; i >= 0; --i) {
-    int32_t id = event_data->node_data[i]->id;
-    AXNodeInfoData* node = event_data->node_data[i].get();
-    tree_map_[id] = std::make_unique<AccessibilityNodeInfoDataWrapper>(
-        this, node, is_important[i]);
-  }
 
   // Assuming |nodeData| is in pre-order, compute cached bounds in post-order to
   // avoid an O(n^2) amount of work as the computed bounds uses descendant
@@ -224,8 +213,7 @@ bool AXTreeSourceArc::IsRootOfNodeTree(int32_t id) const {
 AccessibilityInfoDataWrapper* AXTreeSourceArc::GetFirstImportantAncestor(
     AccessibilityInfoDataWrapper* info_data) const {
   AccessibilityInfoDataWrapper* parent = GetParent(info_data);
-  while (parent && parent->IsNode() &&
-         !IsImportantInAndroid(parent->GetNode())) {
+  while (parent && parent->IsNode() && !parent->IsImportantInAndroid()) {
     parent = GetParent(parent);
   }
   return parent;
@@ -310,58 +298,6 @@ void AXTreeSourceArc::ComputeEnclosingBoundsInternal(
   return;
 }
 
-void AXTreeSourceArc::BuildImportanceTable(
-    AXEventData* event_data,
-    const std::map<int32_t, int32_t>& node_id_to_nodes_index,
-    std::vector<bool>* out_values) const {
-  // TODO(hirokisato): move this logic into AccessibilityNodeInfoDataWrapper.
-  DCHECK(out_values);
-  DCHECK(out_values->size() == event_data->node_data.size());
-
-  // First, compute whether each node has important properties in its subtree.
-  for (size_t i = 0; i < event_data->window_data->size(); ++i) {
-    auto itr = node_id_to_nodes_index.find(
-        event_data->window_data->at(i)->root_node_id);
-    if (itr == node_id_to_nodes_index.end())
-      continue;
-
-    int32_t root_node_index = itr->second;
-    BuildHasImportantProperty(root_node_index, event_data->node_data,
-                              node_id_to_nodes_index, out_values);
-  }
-
-  // Second, node is important in Chrome if it's important in Android and has
-  // any important property.
-  for (size_t i = 0; i < event_data->node_data.size(); ++i)
-    out_values->at(i) = out_values->at(i) &
-                        IsImportantInAndroid(event_data->node_data[i].get());
-}
-
-bool AXTreeSourceArc::BuildHasImportantProperty(
-    int32_t nodes_index,
-    const std::vector<AXNodeInfoDataPtr>& nodes,
-    const std::map<int32_t, int32_t>& node_id_to_nodes_index,
-    std::vector<bool>* has_important_prop_cache) const {
-  DCHECK(has_important_prop_cache);
-  AXNodeInfoData* node = nodes[nodes_index].get();
-
-  bool has_important_prop = HasImportantProperty(node) ||
-                            cached_names_.find(node->id) != cached_names_.end();
-
-  std::vector<int32_t> children;
-  if (GetProperty(node->int_list_properties, AXIntListProperty::CHILD_NODE_IDS,
-                  &children)) {
-    for (const int32_t child : children) {
-      has_important_prop |= BuildHasImportantProperty(
-          node_id_to_nodes_index.at(child), nodes, node_id_to_nodes_index,
-          has_important_prop_cache);
-    }
-  }
-
-  has_important_prop_cache->at(nodes_index) = has_important_prop;
-  return has_important_prop;
-}
-
 AccessibilityInfoDataWrapper* AXTreeSourceArc::FindFirstFocusableNode(
     AccessibilityInfoDataWrapper* info_data) const {
   if (!IsValid(info_data))
@@ -485,12 +421,7 @@ bool AXTreeSourceArc::UpdateAndroidFocusedId(const AXEventData& event_data) {
                                       : nullptr;
 
   // Ensure that the focused node correctly gets focus.
-  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;
+  while (focused_node && !focused_node->IsImportantInAndroid()) {
     AccessibilityInfoDataWrapper* parent = GetParent(focused_node);
     if (parent) {
       android_focused_id_ = parent->GetId();

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

@@ -109,19 +109,6 @@ class AXTreeSourceArc : public ui::AXTreeSource<AccessibilityInfoDataWrapper*,
   void ComputeEnclosingBoundsInternal(AccessibilityInfoDataWrapper* info_data,
                                       gfx::Rect* computed_bounds) const;
 
-  // Builds a mapping from index in |nodes| to whether ignored state should be
-  // applied to the node in chrome accessibility.
-  void BuildImportanceTable(
-      mojom::AccessibilityEventData* event_data,
-      const std::map<int32_t, int32_t>& node_id_to_nodes_index,
-      std::vector<bool>* out_node) const;
-
-  bool BuildHasImportantProperty(
-      int32_t nodes_index,
-      const std::vector<mojom::AccessibilityNodeInfoDataPtr>& nodes,
-      const std::map<int32_t, int32_t>& node_id_to_nodes_index,
-      std::vector<bool>* has_important_prop_cache) const;
-
   // Find the most top-left focusable node under the given node.
   AccessibilityInfoDataWrapper* FindFirstFocusableNode(
       AccessibilityInfoDataWrapper* info_data) const;

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

@@ -1401,6 +1401,8 @@ TEST_F(AXTreeSourceArcTest, SerializeAndUnserialize) {
   // |node2| is ignored by default because
   // AXBooleanProperty::IMPORTANCE has a default false value.
 
+  SetScreenReaderMode(true);
+
   CallNotifyAccessibilityEvent(event.get());
   EXPECT_EQ(1, GetDispatchedEventCount(ax::mojom::Event::kFocus));
   ExpectTree(