Make AXVirtualView bounds relative to the owner view bounds

This changelist includes the following fixes for virtual views:

- Ensures that virtual view accessible bounds are provided for
  |TreeView| nodes when requested. PopulateAccessibilityData() is
  responsible for providing the values, but it was being bypassed in
  GetBoundsRect() and HitTestSync(), which were accessing
  |custom_data_| instead of calling GetData().

- Converts the virtual view accessible bounds to device pixels by
  scaling it by the device scale factor.

- Makes virtual view accessible bounds robust to changes in the owner
  view's bounds in screen. This can happen if the host window was moved
  or resized.

Bug: 811277
Change-Id: Icb9379e635e8c09d299a9022f24714f32ffed150
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2129987Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Kevin Babbitt <kbabbitt@microsoft.com>
Commit-Queue: Brian Liu Xu <brx@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#757448}

ui/views/accessibility/ax_virtual_view.cc

@@ -11,6 +11,7 @@
 #include <utility>
 
 #include "base/callback.h"
+#include "base/containers/adapters.h"
 #include "base/no_destructor.h"
 #include "build/build_config.h"
 #include "ui/accessibility/ax_action_data.h"
@@ -236,6 +237,15 @@ const ui::AXNodeData& AXVirtualView::GetData() const {
 
   if (populate_data_callback_ && GetOwnerView())
     populate_data_callback_.Run(&node_data);
+
+  // According to the ARIA spec, the node should not be ignored if it is
+  // focusable. This is to ensure that the focusable node is both understandable
+  // and operable.
+  if (node_data.HasState(ax::mojom::State::kIgnored) &&
+      node_data.HasState(ax::mojom::State::kFocusable)) {
+    node_data.RemoveState(ax::mojom::State::kIgnored);
+  }
+
   return node_data;
 }
 
@@ -302,12 +312,24 @@ gfx::Rect AXVirtualView::GetBoundsRect(
     const ui::AXCoordinateSystem coordinate_system,
     const ui::AXClippingBehavior clipping_behavior,
     ui::AXOffscreenResult* offscreen_result) const {
+  // We could optionally add clipping here if ever needed.
+  // TODO(nektar): Implement bounds that are relative to the parent.
+  gfx::Rect bounds = gfx::ToEnclosingRect(GetData().relative_bounds.bounds);
+  View* owner_view = GetOwnerView();
+  if (owner_view && owner_view->GetWidget())
+    View::ConvertRectToScreen(owner_view, &bounds);
   switch (coordinate_system) {
     case ui::AXCoordinateSystem::kScreenDIPs:
-      // We could optionally add clipping here if ever needed.
-      // TODO(nektar): Implement bounds that are relative to the parent.
-      return gfx::ToEnclosingRect(custom_data_.relative_bounds.bounds);
-    case ui::AXCoordinateSystem::kScreenPhysicalPixels:
+      return bounds;
+    case ui::AXCoordinateSystem::kScreenPhysicalPixels: {
+      float scale_factor = 1.0;
+      if (owner_view && owner_view->GetWidget()) {
+        gfx::NativeView native_view = owner_view->GetWidget()->GetNativeView();
+        if (native_view)
+          scale_factor = ui::GetScaleFactorForNativeView(native_view);
+      }
+      return gfx::ScaleToEnclosingRect(bounds, scale_factor);
+    }
     case ui::AXCoordinateSystem::kRootFrame:
     case ui::AXCoordinateSystem::kFrame:
       NOTIMPLEMENTED();
@@ -318,27 +340,39 @@ gfx::Rect AXVirtualView::GetBoundsRect(
 gfx::NativeViewAccessible AXVirtualView::HitTestSync(
     int screen_physical_pixel_x,
     int screen_physical_pixel_y) const {
-  if (custom_data_.relative_bounds.bounds.Contains(
-          static_cast<float>(screen_physical_pixel_x),
-          static_cast<float>(screen_physical_pixel_y))) {
-    if (!IsIgnored())
-      return GetNativeObject();
-  }
+  const ui::AXNodeData& node_data = GetData();
+  if (node_data.HasState(ax::mojom::State::kInvisible))
+    return nullptr;
 
   // Check if the point is within any of the virtual children of this view.
   // AXVirtualView's HitTestSync is a recursive function that will return the
   // deepest child, since it does not support relative bounds.
-  for (const std::unique_ptr<AXVirtualView>& child : children_) {
+  // Search the greater indices first, since they're on top in the z-order.
+  for (const std::unique_ptr<AXVirtualView>& child :
+       base::Reversed(children_)) {
     gfx::NativeViewAccessible result =
         child->HitTestSync(screen_physical_pixel_x, screen_physical_pixel_y);
     if (result)
       return result;
   }
+
+  // If it's not inside any of our virtual children, and it's inside the bounds
+  // of this virtual view, then it's inside this virtual view.
+  gfx::Rect bounds_in_screen_physical_pixels =
+      GetBoundsRect(ui::AXCoordinateSystem::kScreenPhysicalPixels,
+                    ui::AXClippingBehavior::kUnclipped);
+  if (bounds_in_screen_physical_pixels.Contains(
+          static_cast<float>(screen_physical_pixel_x),
+          static_cast<float>(screen_physical_pixel_y)) &&
+      !node_data.IsIgnored()) {
+    return GetNativeObject();
+  }
+
   return nullptr;
 }
 
 gfx::NativeViewAccessible AXVirtualView::GetFocus() {
-  auto* owner_view = GetOwnerView();
+  View* owner_view = GetOwnerView();
   if (owner_view) {
     if (!(owner_view->HasFocus())) {
       return nullptr;
@@ -389,15 +423,7 @@ gfx::AcceleratedWidget AXVirtualView::GetTargetForNativeAccessibilityEvent() {
 }
 
 bool AXVirtualView::IsIgnored() const {
-  const ui::AXNodeData& node_data = GetData();
-
-  // According to the ARIA spec, the node should not be ignored if it is
-  // focusable. This is to ensure that the focusable node is both understandable
-  // and operable.
-  if (node_data.HasState(ax::mojom::State::kFocusable))
-    return false;
-
-  return node_data.IsIgnored();
+  return GetData().IsIgnored();
 }
 
 bool AXVirtualView::HandleAccessibleAction(

ui/views/accessibility/ax_virtual_view.h

@@ -142,7 +142,7 @@ class VIEWS_EXPORT AXVirtualView : public ui::AXPlatformNodeDelegateBase {
   gfx::Rect GetBoundsRect(
       const ui::AXCoordinateSystem coordinate_system,
       const ui::AXClippingBehavior clipping_behavior,
-      ui::AXOffscreenResult* offscreen_result) const override;
+      ui::AXOffscreenResult* offscreen_result = nullptr) const override;
   gfx::NativeViewAccessible HitTestSync(
       int screen_physical_pixel_x,
       int screen_physical_pixel_y) const override;

ui/views/accessibility/ax_virtual_view_unittest.cc

@@ -626,6 +626,51 @@ TEST_F(AXVirtualViewTest, Navigation) {
   EXPECT_EQ(0, virtual_child_4->GetIndexInParent());
 }
 
+TEST_F(AXVirtualViewTest, HitTesting) {
+  ASSERT_EQ(0, virtual_label_->GetChildCount());
+
+  const gfx::Vector2d offset_from_origin =
+      button_->GetBoundsInScreen().OffsetFromOrigin();
+
+  // Test that hit testing is recursive.
+  AXVirtualView* virtual_child_1 = new AXVirtualView;
+  virtual_child_1->GetCustomData().relative_bounds.bounds =
+      gfx::RectF(0, 0, 10, 10);
+  virtual_label_->AddChildView(base::WrapUnique(virtual_child_1));
+  AXVirtualView* virtual_child_2 = new AXVirtualView;
+  virtual_child_2->GetCustomData().relative_bounds.bounds =
+      gfx::RectF(5, 5, 5, 5);
+  virtual_child_1->AddChildView(base::WrapUnique(virtual_child_2));
+  gfx::Point point_1 = gfx::Point(2, 2) + offset_from_origin;
+  EXPECT_EQ(virtual_child_1->GetNativeObject(),
+            virtual_child_1->HitTestSync(point_1.x(), point_1.y()));
+  gfx::Point point_2 = gfx::Point(7, 7) + offset_from_origin;
+  EXPECT_EQ(virtual_child_2->GetNativeObject(),
+            virtual_label_->HitTestSync(point_2.x(), point_2.y()));
+
+  // Test that hit testing follows the z-order.
+  AXVirtualView* virtual_child_3 = new AXVirtualView;
+  virtual_child_3->GetCustomData().relative_bounds.bounds =
+      gfx::RectF(5, 5, 10, 10);
+  virtual_label_->AddChildView(base::WrapUnique(virtual_child_3));
+  AXVirtualView* virtual_child_4 = new AXVirtualView;
+  virtual_child_4->GetCustomData().relative_bounds.bounds =
+      gfx::RectF(10, 10, 10, 10);
+  virtual_child_3->AddChildView(base::WrapUnique(virtual_child_4));
+  EXPECT_EQ(virtual_child_3->GetNativeObject(),
+            virtual_label_->HitTestSync(point_2.x(), point_2.y()));
+  gfx::Point point_3 = gfx::Point(12, 12) + offset_from_origin;
+  EXPECT_EQ(virtual_child_4->GetNativeObject(),
+            virtual_label_->HitTestSync(point_3.x(), point_3.y()));
+
+  // Test that hit testing skips ignored nodes but not their descendants.
+  virtual_child_3->GetCustomData().AddState(ax::mojom::State::kIgnored);
+  EXPECT_EQ(virtual_child_2->GetNativeObject(),
+            virtual_label_->HitTestSync(point_2.x(), point_2.y()));
+  EXPECT_EQ(virtual_child_4->GetNativeObject(),
+            virtual_label_->HitTestSync(point_3.x(), point_3.y()));
+}
+
 // Test for GetTargetForNativeAccessibilityEvent().
 #if defined(OS_WIN)
 TEST_F(AXVirtualViewTest, GetTargetForEvents) {

ui/views/controls/table/table_view.cc

@@ -1422,28 +1422,33 @@ void TableView::UpdateVirtualAccessibilityChildrenBounds() {
 }
 
 gfx::Rect TableView::CalculateHeaderRowAccessibilityBounds() const {
-  return AdjustRectForAXRelativeBounds(header_->GetVisibleBounds());
+  gfx::Rect header_bounds = header_->GetVisibleBounds();
+  gfx::Point header_origin = header_bounds.origin();
+  ConvertPointToTarget(header_, this, &header_origin);
+  header_bounds.set_origin(header_origin);
+  return header_bounds;
 }
 
 gfx::Rect TableView::CalculateHeaderCellAccessibilityBounds(
     const int visible_column_index) const {
+  const gfx::Rect& header_bounds = CalculateHeaderRowAccessibilityBounds();
   const VisibleColumn& visible_column = visible_columns_[visible_column_index];
-  gfx::Rect header_cell_bounds(visible_column.x, header_->y(),
-                               visible_column.width, header_->height());
-  return AdjustRectForAXRelativeBounds(header_cell_bounds);
+  gfx::Rect header_cell_bounds(visible_column.x, header_bounds.y(),
+                               visible_column.width, header_bounds.height());
+  return header_cell_bounds;
 }
 
 gfx::Rect TableView::CalculateTableRowAccessibilityBounds(
     const int row_index) const {
   gfx::Rect row_bounds = GetRowBounds(row_index);
-  return AdjustRectForAXRelativeBounds(row_bounds);
+  return row_bounds;
 }
 
 gfx::Rect TableView::CalculateTableCellAccessibilityBounds(
     const int row_index,
     const int visible_column_index) const {
   gfx::Rect cell_bounds = GetCellBounds(row_index, visible_column_index);
-  return AdjustRectForAXRelativeBounds(cell_bounds);
+  return cell_bounds;
 }
 
 void TableView::UpdateAccessibilityFocus() {
@@ -1510,13 +1515,6 @@ AXVirtualView* TableView::GetVirtualAccessibilityCell(
   return i->get();
 }
 
-gfx::Rect TableView::AdjustRectForAXRelativeBounds(
-    const gfx::Rect& rect) const {
-  gfx::Rect converted_rect = rect;
-  View::ConvertRectToScreen(this, &converted_rect);
-  return converted_rect;
-}
-
 DEFINE_ENUM_CONVERTERS(TableTypes,
                        {TableTypes::TEXT_ONLY, base::ASCIIToUTF16("TEXT_ONLY")},
                        {TableTypes::ICON_AND_TEXT,

ui/views/controls/table/table_view.h

@@ -383,11 +383,6 @@ class VIEWS_EXPORT TableView : public views::View,
   // |visible_column_index| indexes into |visible_columns_|.
   AXVirtualView* GetVirtualAccessibilityCell(int row, int visible_column_index);
 
-  // Returns |rect|, adjusted for use in AXRelativeBounds by translating it into
-  // screen coordinates. The result must be converted to gfx::RectF when setting
-  // into AXRelativeBounds.
-  gfx::Rect AdjustRectForAXRelativeBounds(const gfx::Rect& rect) const;
-
   ui::TableModel* model_ = nullptr;
 
   std::vector<ui::TableColumn> columns_;

ui/views/controls/table/table_view_unittest.cc

@@ -80,23 +80,33 @@ class TableViewTestHelper {
 
     // Generate the bounds for the header row and cells.
     auto header_row = std::vector<gfx::Rect>();
-    header_row.push_back(table_->CalculateHeaderRowAccessibilityBounds());
+    gfx::Rect header_row_bounds =
+        table_->CalculateHeaderRowAccessibilityBounds();
+    View::ConvertRectToScreen(table_, &header_row_bounds);
+    header_row.push_back(header_row_bounds);
     for (size_t column_index = 0; column_index < visible_col_count();
          column_index++) {
-      header_row.push_back(
-          table_->CalculateHeaderCellAccessibilityBounds(column_index));
+      gfx::Rect header_cell_bounds =
+          table_->CalculateHeaderCellAccessibilityBounds(column_index);
+      View::ConvertRectToScreen(table_, &header_cell_bounds);
+      header_row.push_back(header_cell_bounds);
     }
     expected_bounds.push_back(header_row);
 
     // Generate the bounds for the table rows and cells.
     for (int row_index = 0; row_index < table_->GetRowCount(); row_index++) {
       auto table_row = std::vector<gfx::Rect>();
-      table_row.push_back(
-          table_->CalculateTableRowAccessibilityBounds(row_index));
+      gfx::Rect table_row_bounds =
+          table_->CalculateTableRowAccessibilityBounds(row_index);
+      View::ConvertRectToScreen(table_, &table_row_bounds);
+      table_row.push_back(table_row_bounds);
       for (size_t column_index = 0; column_index < visible_col_count();
            column_index++) {
-        table_row.push_back(table_->CalculateTableCellAccessibilityBounds(
-            row_index, column_index));
+        gfx::Rect table_cell_bounds =
+            table_->CalculateTableCellAccessibilityBounds(row_index,
+                                                          column_index);
+        View::ConvertRectToScreen(table_, &table_cell_bounds);
+        table_row.push_back(table_cell_bounds);
       }
       expected_bounds.push_back(table_row);
     }

ui/views/controls/tree/tree_view.cc

@@ -922,7 +922,6 @@ void TreeView::PopulateAccessibilityData(InternalNode* node,
     data->AddAction(ax::mojom::Action::kFocus);
     data->AddAction(ax::mojom::Action::kScrollToMakeVisible);
     gfx::Rect node_bounds = GetBackgroundBoundsForNode(node);
-    View::ConvertRectToScreen(this, &node_bounds);
     data->relative_bounds.bounds = gfx::RectF(node_bounds);
   } else {
     data->AddState(ax::mojom::State::kInvisible);