Clean up ARC++ a11y bounds computation

This CL cleans up the a11y bounds computations in ARC++;
- Aggregate the bounds computation logic inside
  AccessibilityInfoDataWrapper.
- Explicitly use |offset_container_id| of AXRelativeBounds.
_ Keep the device scale factor in AXTreeSourceArc. Set and update it
  from ArcAccessibilityHelperBridge.

This CL also fixes the bounds computation of notification window and
input method window, which were currently not adjusted by device
scaling.

Bug: b:147707554
Test: manual. Open PlayStore, change device scale factor by Ctrl +/-, bounds are correct.
Test: manual. Open TalkBack-test app, create notification windows, bounds are correct.
Change-Id: Ie5f6904fcf8c18fd86d9fc1e99a1db7970a14be9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2066411
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@{#746603}

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

@@ -5,7 +5,11 @@
 #include "chrome/browser/chromeos/arc/accessibility/accessibility_info_data_wrapper.h"
 
 #include "chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h"
+#include "chrome/browser/chromeos/arc/accessibility/geometry_util.h"
 #include "components/exo/wm_helper.h"
+#include "ui/gfx/geometry/rect_f.h"
+#include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_delegate.h"
 
 namespace arc {
 
@@ -16,33 +20,47 @@ AccessibilityInfoDataWrapper::AccessibilityInfoDataWrapper(
 void AccessibilityInfoDataWrapper::Serialize(ui::AXNodeData* out_data) const {
   out_data->id = GetId();
   PopulateAXRole(out_data);
+  PopulateBounds(out_data);
+}
+
+void AccessibilityInfoDataWrapper::PopulateBounds(
+    ui::AXNodeData* out_data) const {
+  AccessibilityInfoDataWrapper* root = tree_source_->GetRoot();
+  gfx::Rect info_data_bounds = GetBounds();
+  gfx::RectF& out_bounds = out_data->relative_bounds.bounds;
+
+  if (root && exo::WMHelper::HasInstance()) {
+    if (tree_source_->is_notification() ||
+        tree_source_->is_input_method_window() || root->GetId() != GetId()) {
+      // By default, populate the bounds relative to the tree root.
+      const gfx::Rect& root_bounds = root->GetBounds();
 
-  exo::WMHelper* wm_helper =
-      exo::WMHelper::HasInstance() ? exo::WMHelper::GetInstance() : nullptr;
-
-  if (tree_source_->GetRoot() && wm_helper) {
-    // This is the computed bounds which relies upon the existence of an
-    // associated focused window and a root node.
-    aura::Window* active_window = (tree_source_->is_notification() ||
-                                   tree_source_->is_input_method_window())
-                                      ? nullptr
-                                      : wm_helper->GetActiveWindow();
-    const gfx::Rect& local_bounds = tree_source_->GetBounds(
-        tree_source_->GetFromId(GetId()), active_window);
-    out_data->relative_bounds.bounds.SetRect(local_bounds.x(), local_bounds.y(),
-                                             local_bounds.width(),
-                                             local_bounds.height());
+      info_data_bounds.Offset(-1 * root_bounds.x(), -1 * root_bounds.y());
+      out_bounds = ToChromeScale(info_data_bounds);
+      out_data->relative_bounds.offset_container_id = root->GetId();
+    } else {
+      // For the root node of application tree, populate the bounds to be
+      // relative to its container View.
+      views::Widget* widget = views::Widget::GetWidgetForNativeView(
+          exo::WMHelper::GetInstance()->GetActiveWindow());
+      DCHECK(widget);
+      DCHECK(widget->widget_delegate());
+      DCHECK(widget->widget_delegate()->GetContentsView());
+      const gfx::Rect& root_bounds =
+          widget->widget_delegate()->GetContentsView()->GetBoundsInScreen();
+
+      out_bounds = ToChromeBounds(info_data_bounds, widget);
+      out_bounds.Offset(-1 * root_bounds.x(), -1 * root_bounds.y());
+    }
+
+    // |out_bounds| is in Chrome DPI here. As ARC is considered the same as web
+    // in Chrome automation, scale the bounds by device scale factor.
+    out_bounds.Scale(tree_source_->device_scale_factor());
   } else {
     // We cannot compute global bounds, so use the raw bounds.
-    const auto& bounds = GetBounds();
-    out_data->relative_bounds.bounds.SetRect(bounds.x(), bounds.y(),
-                                             bounds.width(), bounds.height());
+    out_bounds.SetRect(info_data_bounds.x(), info_data_bounds.y(),
+                       info_data_bounds.width(), info_data_bounds.height());
   }
-
-  // TODO(hirokisato): Try using offset_container_id to make bounds calculations
-  // more efficient. If this is the child of the root, set the
-  // offset_container_id to be the root. Otherwise, set it to the first node
-  // child of the root.
 }
 
 }  // namespace arc

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

@@ -43,6 +43,14 @@ class AccessibilityInfoDataWrapper {
 
  protected:
   AXTreeSourceArc* tree_source_;
+
+ private:
+  // Populate bounds of a node which can be passed to AXNodeData.location.
+  // Bounds are returned in the following coordinates depending on whether it's
+  // root or not.
+  // - Root node is relative to its container, i.e. focused window.
+  // - Non-root node is relative to the root node of this tree.
+  void PopulateBounds(ui::AXNodeData* out_data) const;
 };
 
 }  // namespace arc

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

@@ -46,6 +46,12 @@ using ash::ArcNotificationSurfaceManager;
 
 namespace {
 
+float DeviceScaleFactorFromWindow(aura::Window* window) {
+  if (!window || !window->GetToplevelWindow())
+    return 1.0;
+  return window->GetToplevelWindow()->layer()->device_scale_factor();
+}
+
 void DispatchFocusChange(arc::mojom::AccessibilityNodeInfoData* node_data,
                          Profile* profile) {
   chromeos::AccessibilityManager* accessibility_manager =
@@ -388,16 +394,25 @@ void ArcAccessibilityHelperBridge::OnNotificationStateChanged(
   auto key = KeyForNotification(notification_key);
   switch (state) {
     case arc::mojom::AccessibilityNotificationStateType::SURFACE_CREATED: {
-      if (GetFromKey(key))
-        return;
+      aura::Window* window = nullptr;
+      auto* surface_manager = ArcNotificationSurfaceManager::Get();
+      if (surface_manager) {
+        ArcNotificationSurface* surface =
+            surface_manager->GetArcSurface(notification_key);
+        if (surface)
+          window = surface->GetWindow();
+      }
 
-      AXTreeSourceArc* tree_source = CreateFromKey(std::move(key));
-      ui::AXTreeData tree_data;
-      if (!tree_source->GetTreeData(&tree_data)) {
-        NOTREACHED();
+      AXTreeSourceArc* tree_source = GetFromKey(key);
+      if (tree_source) {
+        tree_source->set_device_scale_factor(
+            DeviceScaleFactorFromWindow(window));
         return;
       }
-      UpdateTreeIdOfNotificationSurface(notification_key, tree_data.tree_id);
+
+      tree_source = CreateFromKey(std::move(key), window);
+      UpdateTreeIdOfNotificationSurface(notification_key,
+                                        tree_source->ax_tree_id());
       break;
     }
     case arc::mojom::AccessibilityNotificationStateType::SURFACE_REMOVED:
@@ -499,11 +514,9 @@ void ArcAccessibilityHelperBridge::OnNotificationSurfaceAdded(
   if (!tree)
     return;
 
-  ui::AXTreeData tree_data;
-  if (!tree->GetTreeData(&tree_data))
-    return;
-
-  surface->SetAXTreeId(tree_data.tree_id);
+  surface->SetAXTreeId(tree->ax_tree_id());
+  tree->set_device_scale_factor(
+      DeviceScaleFactorFromWindow(surface->GetWindow()));
 
   // Dispatch ax::mojom::Event::kChildrenChanged to force AXNodeData of the
   // notification updated. As order of OnNotificationSurfaceAdded call is not
@@ -624,7 +637,7 @@ ArcAccessibilityHelperBridge::OnGetTextLocationDataResultInternal(
     return base::nullopt;
 
   gfx::RectF rect_f = arc::ToChromeScale(*result_rect);
-  arc::ScaleDeviceFactor(rect_f, active_window->GetToplevelWindow());
+  rect_f.Scale(DeviceScaleFactorFromWindow(active_window));
   return gfx::ToEnclosingRect(rect_f);
 }
 
@@ -716,7 +729,7 @@ void ArcAccessibilityHelperBridge::UpdateWindowProperties(
     TreeKey key = KeyForTaskId(task_id);
     AXTreeSourceArc* tree = GetFromKey(key);
     if (!tree)
-      tree = CreateFromKey(std::move(key));
+      tree = CreateFromKey(std::move(key), window);
 
     // Just after the creation of window, widget has not been set yet and this
     // is not dispatched to ShellSurfaceBase. Thus, call this every time.
@@ -804,10 +817,9 @@ void ArcAccessibilityHelperBridge::HandleFilterTypeAllEvent(
       return;
 
     if (!trees_.count(KeyForInputMethod())) {
-      auto* tree = CreateFromKey(KeyForInputMethod());
-      ui::AXTreeData tree_data;
-      tree->GetTreeData(&tree_data);
-      input_method_surface->SetChildAxTreeId(tree_data.tree_id);
+      auto* tree = CreateFromKey(KeyForInputMethod(),
+                                 input_method_surface->host_window());
+      input_method_surface->SetChildAxTreeId(tree->ax_tree_id());
     }
 
     tree_source = GetFromKey(KeyForInputMethod());
@@ -832,8 +844,11 @@ void ArcAccessibilityHelperBridge::HandleFilterTypeAllEvent(
     tree_source = GetFromKey(key);
 
     if (!tree_source) {
-      tree_source = CreateFromKey(key);
+      tree_source = CreateFromKey(key, active_window);
       SetChildAxTreeIDForWindow(active_window, tree_source->ax_tree_id());
+    } else {
+      tree_source->set_device_scale_factor(
+          DeviceScaleFactorFromWindow(active_window));
     }
   }
 
@@ -884,8 +899,11 @@ void ArcAccessibilityHelperBridge::DispatchCustomSpokenFeedbackToggled(
   GetEventRouter()->BroadcastEvent(std::move(event));
 }
 
-AXTreeSourceArc* ArcAccessibilityHelperBridge::CreateFromKey(TreeKey key) {
-  auto tree = std::make_unique<AXTreeSourceArc>(this);
+AXTreeSourceArc* ArcAccessibilityHelperBridge::CreateFromKey(
+    TreeKey key,
+    aura::Window* window) {
+  auto tree = std::make_unique<AXTreeSourceArc>(
+      this, DeviceScaleFactorFromWindow(window));
   auto* tree_ptr = tree.get();
   trees_.insert(std::make_pair(std::move(key), std::move(tree)));
   return tree_ptr;

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

@@ -158,7 +158,7 @@ class ArcAccessibilityHelperBridge
 
   void DispatchCustomSpokenFeedbackToggled(bool enabled) const;
 
-  AXTreeSourceArc* CreateFromKey(TreeKey);
+  AXTreeSourceArc* CreateFromKey(TreeKey, aura::Window* window);
   AXTreeSourceArc* GetFromKey(const TreeKey&);
   AXTreeSourceArc* GetFromTreeId(ui::AXTreeID tree_id) const;
 

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

@@ -18,13 +18,7 @@
 #include "extensions/browser/api/automation_internal/automation_event_router.h"
 #include "extensions/common/extension_messages.h"
 #include "ui/accessibility/ax_enums.mojom.h"
-#include "ui/accessibility/platform/ax_android_constants.h"
-#include "ui/aura/window.h"
-#include "ui/gfx/geometry/rect_conversions.h"
-#include "ui/gfx/geometry/rect_f.h"
-#include "ui/views/view.h"
-#include "ui/views/widget/widget.h"
-#include "ui/views/widget/widget_delegate.h"
+#include "ui/gfx/geometry/rect.h"
 
 namespace arc {
 
@@ -53,8 +47,9 @@ bool IsDrawerLayout(AXNodeInfoData* node) {
 }
 }  // namespace
 
-AXTreeSourceArc::AXTreeSourceArc(Delegate* delegate)
-    : current_tree_serializer_(new AXTreeArcSerializer(this)),
+AXTreeSourceArc::AXTreeSourceArc(Delegate* delegate, float device_scale_factor)
+    : device_scale_factor_(device_scale_factor),
+      current_tree_serializer_(new AXTreeArcSerializer(this)),
       is_notification_(false),
       is_input_method_window_(false),
       delegate_(delegate) {}
@@ -221,12 +216,17 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
 
   event_bundle.updates.emplace_back();
 
-  // Force the tree, starting at the target of the event, to update, so
-  // unignored fields get updated.
-  event_bundle.updates[0].node_id_to_clear = event_data->source_id;
-  current_tree_serializer_->InvalidateSubtree(GetFromId(event_data->source_id));
-
-  current_tree_serializer_->SerializeChanges(GetFromId(event_data->source_id),
+  // Force the tree, to update, so unignored fields get updated.
+  // On event type of WINDOW_STATE_CHANGED, update the entire tree so that
+  // window location is correctly calculated.
+  int32_t node_id_to_clear =
+      (event_data->event_type == AXEventType::WINDOW_STATE_CHANGED)
+          ? *root_id_
+          : event_data->source_id;
+  event_bundle.updates[0].node_id_to_clear = node_id_to_clear;
+  current_tree_serializer_->InvalidateSubtree(GetFromId(node_id_to_clear));
+
+  current_tree_serializer_->SerializeChanges(GetFromId(node_id_to_clear),
                                              &event_bundle.updates.back());
 
   GetAutomationEventRouter()->DispatchAccessibilityEvents(event_bundle);
@@ -251,48 +251,6 @@ void AXTreeSourceArc::UpdateAccessibilityFocusLocation(int32_t id) {
   chrome_focused_bounds_ = node->GetBounds();
 }
 
-const gfx::Rect AXTreeSourceArc::GetBounds(
-    AccessibilityInfoDataWrapper* info_data,
-    aura::Window* active_window) const {
-  DCHECK(root_id_.has_value());
-
-  gfx::Rect info_data_bounds = info_data->GetBounds();
-
-  if (!active_window) {
-    const gfx::Rect root_bounds = GetRoot()->GetBounds();
-    info_data_bounds.Offset(-1 * root_bounds.x(), -1 * root_bounds.y());
-    return info_data_bounds;
-  }
-
-  // By default, the node bounds is relative to the tree root.
-  if (info_data->GetId() != root_id_) {
-    const gfx::Rect root_bounds = GetRoot()->GetBounds();
-    info_data_bounds.Offset(-1 * root_bounds.x(), -1 * root_bounds.y());
-
-    gfx::RectF info_data_bounds_f = ToChromeScale(info_data_bounds);
-    arc::ScaleDeviceFactor(info_data_bounds_f,
-                           active_window->GetToplevelWindow());
-    return gfx::ToEnclosingRect(info_data_bounds_f);
-  }
-
-  // For the root node, the node bounds is relative to its container View.
-  views::Widget* widget = views::Widget::GetWidgetForNativeView(active_window);
-  DCHECK(widget);
-
-  gfx::RectF info_data_bounds_f = arc::ToChromeBounds(info_data_bounds, widget);
-
-  DCHECK(widget->widget_delegate());
-  DCHECK(widget->widget_delegate()->GetContentsView());
-  const gfx::Rect root_bounds =
-      widget->widget_delegate()->GetContentsView()->GetBoundsInScreen();
-
-  info_data_bounds_f.Offset(-1 * root_bounds.x(), -1 * root_bounds.y());
-
-  arc::ScaleDeviceFactor(info_data_bounds_f,
-                         active_window->GetToplevelWindow());
-  return gfx::ToEnclosingRect(info_data_bounds_f);
-}
-
 void AXTreeSourceArc::InvalidateTree() {
   current_tree_serializer_->Reset();
 }

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

@@ -21,10 +21,6 @@
 #include "ui/accessibility/ax_tree_source.h"
 #include "ui/views/view.h"
 
-namespace aura {
-class Window;
-}
-
 namespace ui {
 struct AXEvent;
 }
@@ -47,7 +43,7 @@ class AXTreeSourceArc : public ui::AXTreeSource<AccessibilityInfoDataWrapper*,
     virtual void OnAction(const ui::AXActionData& data) const = 0;
   };
 
-  explicit AXTreeSourceArc(Delegate* delegate);
+  AXTreeSourceArc(Delegate* delegate, float device_scale_factor);
   ~AXTreeSourceArc() override;
 
   // Notify automation of an accessibility event.
@@ -63,16 +59,6 @@ class AXTreeSourceArc : public ui::AXTreeSource<AccessibilityInfoDataWrapper*,
   // Update Chrome's accessibility focused node by id.
   void UpdateAccessibilityFocusLocation(int32_t id);
 
-  // Returns bounds of a node which can be passed to AXNodeData.location. Bounds
-  // are returned in the following coordinates depending on whether it's root or
-  // not.
-  // - Root node is relative to its container, i.e. focused window.
-  // - Non-root node is relative to the root node of this tree.
-  //
-  // focused_window is nullptr for notification.
-  const gfx::Rect GetBounds(AccessibilityInfoDataWrapper* info_data,
-                            aura::Window* focused_window) const;
-
   // Invalidates the tree serializer.
   void InvalidateTree();
 
@@ -89,6 +75,9 @@ class AXTreeSourceArc : public ui::AXTreeSource<AccessibilityInfoDataWrapper*,
   void SerializeNode(AccessibilityInfoDataWrapper* info_data,
                      ui::AXNodeData* out_data) const override;
 
+  float device_scale_factor() const { return device_scale_factor_; }
+  void set_device_scale_factor(float dsf) { device_scale_factor_ = dsf; }
+
   bool is_notification() { return is_notification_; }
 
   bool is_input_method_window() { return is_input_method_window_; }
@@ -166,6 +155,9 @@ class AXTreeSourceArc : public ui::AXTreeSource<AccessibilityInfoDataWrapper*,
   // Maps an AccessibilityInfoDataWrapper ID to its tree data.
   std::map<int32_t, std::unique_ptr<AccessibilityInfoDataWrapper>> tree_map_;
 
+  // The device scale factor of the display which the window is on.
+  float device_scale_factor_;
+
   // Maps an AccessibilityInfoDataWrapper ID to its parent.
   std::map<int32_t, int32_t> parent_map_;
 

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

@@ -142,7 +142,7 @@ class AXTreeSourceArcTest : public testing::Test,
    public:
     TestAXTreeSourceArc(AXTreeSourceArc::Delegate* delegate,
                         MockAutomationEventRouter* router)
-        : AXTreeSourceArc(delegate), router_(router) {}
+        : AXTreeSourceArc(delegate, 1.0), router_(router) {}
 
    private:
     extensions::AutomationEventRouterInterface* GetAutomationEventRouter()

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

@@ -35,11 +35,6 @@ gfx::RectF ToChromeBounds(const gfx::Rect& bounds, views::Widget* widget) {
 
   return chrome_bounds;
 }
-
-void ScaleDeviceFactor(gfx::RectF& bounds, aura::Window* toplevel_window) {
-  DCHECK(toplevel_window);
-  bounds.Scale(toplevel_window->layer()->device_scale_factor());
-}
 }  // namespace arc
 
 #endif  // CHROME_BROWSER_CHROMEOS_ARC_ACCESSIBILITY_GEOMETRY_UTIL_H_

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

@@ -7,10 +7,6 @@
 
 // TODO(hirokisato) support multiple display.
 
-namespace aura {
-class Window;
-}
-
 namespace gfx {
 class RectF;
 }
@@ -27,9 +23,6 @@ gfx::RectF ToChromeScale(const gfx::Rect& rect);
 // Given ARC pixels in screen coordinate, returns DIPs in Chrome OS main
 // display. This function adjusts differences between ARC and Chrome.
 gfx::RectF ToChromeBounds(const gfx::Rect& rect, views::Widget* widget);
-
-// Given DIPs in Chrome OS main display, scales it into pixels.
-void ScaleDeviceFactor(gfx::RectF& rect, aura::Window* toplevel_window);
 }  // namespace arc
 
 #endif  // CHROME_BROWSER_CHROMEOS_ARC_ACCESSIBILITY_GEOMETRY_UTIL_H_