Fix AXTree::RelativeToTreeBounds for nodes with position but zero size.

AXTree::RelativeToTreeBounds has some logic that tries to avoid
returning an empty bounding rect. If the current node has zero size,
it tries to compute the size either from the union of that node's
children, or if that fails, from the bounds of that node's nearest
ancestor with a size.

There was a subtle bug in this logic if a node has a position but
zero size. In a case like this:

root (0,0) size (800x600)
  window (100,100) size (400,300)
    button (120, 120) size (0,0)

The algorithm was returning a position of (220,220) incorrectly.

The fundamental problem was trying to fix a bounding box with zero size
at the same time as it was trying to walk up the ancestry to turn relative
positions to absolute positions.

The new algorithm just does it in two passes: first convert relative to
absolute, then if the bounds has no size, find the nearest ancestor with
a size and expand the current node as much as possible within that
ancestor's bounds.

Bug: 1005977
Change-Id: Ib4963fa98dabf958be49d3e13d6d932280b01b1b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1815257
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Anastasia Helfinstein <anastasi@google.com>
Cr-Commit-Position: refs/heads/master@{#700398}

content/test/data/accessibility/html/absolute-offscreen-expected-blink.txt

@@ -4,11 +4,11 @@ rootWebArea clipsChildren=true
 ++++++genericContainer pageSize=(50, 20) unclippedSize=(50, 20)
 ++++++++staticText name='Visible'
 ++++++++++inlineTextBox name='Visible'
-++++genericContainer size=(0, 0) pageSize=(1, 1) clipsChildren=true
+++++genericContainer size=(0, 0) pageSize=(1, 1) unclippedSize=(50, 20) clipsChildren=true
 ++++++genericContainer offscreen pageSize=(1, 1) unclippedSize=(50, 20)
 ++++++++staticText offscreen pageSize=(1, 1) name='Invisible'
 ++++++++++inlineTextBox offscreen pageSize=(1, 1) name='Invisible'
 ++++genericContainer clipsChildren=true
 ++++++genericContainer offscreen pageSize=(50, 1) unclippedSize=(50, 20)
 ++++++++staticText offscreen name='Invisible'
-++++++++++inlineTextBox offscreen name='Invisible'
+++++++++++inlineTextBox offscreen name='Invisible'
\ No newline at end of file

content/test/data/accessibility/html/bounds-inherits-expected-blink.txt

 rootWebArea
 ++genericContainer ignored
 ++++genericContainer size=(200, 200)
-++++++genericContainer size=(0, 0)
-++++++++genericContainer offscreen size=(0, 0)
+++++++genericContainer offscreen size=(0, 0)
+++++++++genericContainer offscreen size=(0, 0)
\ No newline at end of file

ui/accessibility/ax_tree.cc

@@ -597,10 +597,11 @@ void AXTree::UpdateData(const AXTreeData& new_data) {
     observer.OnTreeDataChanged(this, old_data, new_data);
 }
 
-gfx::RectF AXTree::RelativeToTreeBounds(const AXNode* node,
-                                        gfx::RectF bounds,
-                                        bool* offscreen,
-                                        bool clip_bounds) const {
+gfx::RectF AXTree::RelativeToTreeBoundsInternal(const AXNode* node,
+                                                gfx::RectF bounds,
+                                                bool* offscreen,
+                                                bool clip_bounds,
+                                                bool allow_recursion) const {
   // If |bounds| is uninitialized, which is not the same as empty,
   // start with the node bounds.
   if (bounds.width() == 0 && bounds.height() == 0) {
@@ -610,10 +611,16 @@ gfx::RectF AXTree::RelativeToTreeBounds(const AXNode* node,
     // try to compute good bounds from the children.
     // If a tree update is in progress, skip this step as children may be in a
     // bad state.
-    if (bounds.IsEmpty() && !GetTreeUpdateInProgressState()) {
+    if (bounds.IsEmpty() && !GetTreeUpdateInProgressState() &&
+        allow_recursion) {
       for (size_t i = 0; i < node->children().size(); i++) {
         ui::AXNode* child = node->children()[i];
-        bounds.Union(GetTreeBounds(child));
+
+        bool ignore_offscreen;
+        gfx::RectF child_bounds = RelativeToTreeBoundsInternal(
+            child, gfx::RectF(), &ignore_offscreen, clip_bounds,
+            /* allow_recursion = */ false);
+        bounds.Union(child_bounds);
       }
       if (bounds.width() > 0 && bounds.height() > 0) {
         return bounds;
@@ -624,19 +631,15 @@ gfx::RectF AXTree::RelativeToTreeBounds(const AXNode* node,
                   node->data().relative_bounds.bounds.y());
   }
 
+  const AXNode* original_node = node;
   while (node != nullptr) {
     if (node->data().relative_bounds.transform)
       node->data().relative_bounds.transform->TransformRect(&bounds);
-    const AXNode* container;
-
-    // Normally we apply any transforms and offsets for each node and
-    // then walk up to its offset container - however, if the node has
-    // no width or height, walk up to its nearest ancestor until we find
-    // one that has bounds.
-    if (bounds.width() == 0 && bounds.height() == 0)
-      container = node->parent();
-    else
-      container = GetFromId(node->data().relative_bounds.offset_container_id);
+    // Apply any transforms and offsets for each node and then walk up to
+    // its offset container. If no offset container is specified, coordinates
+    // are relative to the root node.
+    const AXNode* container =
+        GetFromId(node->data().relative_bounds.offset_container_id);
     if (!container && container != root())
       container = root();
     if (!container || container == node)
@@ -645,18 +648,6 @@ gfx::RectF AXTree::RelativeToTreeBounds(const AXNode* node,
     gfx::RectF container_bounds = container->data().relative_bounds.bounds;
     bounds.Offset(container_bounds.x(), container_bounds.y());
 
-    // If we don't have any size yet, take the size from this ancestor.
-    // The rationale is that it's not useful to the user for an object to
-    // have no width or height and it's probably a bug; it's better to
-    // reflect the bounds of the nearest ancestor rather than a 0x0 box.
-    // Tag this node as 'offscreen' because it has no true size, just a
-    // size inherited from the ancestor.
-    if (bounds.width() == 0 && bounds.height() == 0) {
-      bounds.set_size(container_bounds.size());
-      if (offscreen != nullptr)
-        *offscreen |= true;
-    }
-
     int scroll_x = 0;
     int scroll_y = 0;
     if (container->data().GetIntAttribute(ax::mojom::IntAttribute::kScrollX,
@@ -672,7 +663,7 @@ gfx::RectF AXTree::RelativeToTreeBounds(const AXNode* node,
 
     // Calculate the clipped bounds to determine offscreen state.
     gfx::RectF clipped = bounds;
-    // If this is the root web area, make sure we clip the node to fit.
+    // If this node has the kClipsChildren attribute set, clip the rect to fit.
     if (container->data().GetBoolAttribute(
             ax::mojom::BoolAttribute::kClipsChildren)) {
       if (!intersection.IsEmpty()) {
@@ -718,9 +709,56 @@ gfx::RectF AXTree::RelativeToTreeBounds(const AXNode* node,
     node = container;
   }
 
+  // If we don't have any size yet, try to adjust the bounds to fill the
+  // nearest ancestor that does have bounds.
+  //
+  // The rationale is that it's not useful to the user for an object to
+  // have no width or height and it's probably a bug; it's better to
+  // reflect the bounds of the nearest ancestor rather than a 0x0 box.
+  // Tag this node as 'offscreen' because it has no true size, just a
+  // size inherited from the ancestor.
+  if (bounds.width() == 0 && bounds.height() == 0) {
+    const AXNode* ancestor = original_node->parent();
+    gfx::RectF ancestor_bounds;
+    while (ancestor) {
+      ancestor_bounds = ancestor->data().relative_bounds.bounds;
+      if (ancestor_bounds.width() > 0 || ancestor_bounds.height() > 0)
+        break;
+      ancestor = ancestor->parent();
+    }
+
+    if (ancestor && allow_recursion) {
+      bool ignore_offscreen;
+      bool allow_recursion = false;
+      ancestor_bounds = RelativeToTreeBoundsInternal(
+          ancestor, gfx::RectF(), &ignore_offscreen, clip_bounds,
+          allow_recursion);
+
+      gfx::RectF original_bounds = original_node->data().relative_bounds.bounds;
+      if (original_bounds.x() == 0 && original_bounds.y() == 0) {
+        bounds = ancestor_bounds;
+      } else {
+        bounds.set_width(std::max(0.0f, ancestor_bounds.right() - bounds.x()));
+        bounds.set_height(
+            std::max(0.0f, ancestor_bounds.bottom() - bounds.y()));
+      }
+      if (offscreen != nullptr)
+        *offscreen |= true;
+    }
+  }
+
   return bounds;
 }
 
+gfx::RectF AXTree::RelativeToTreeBounds(const AXNode* node,
+                                        gfx::RectF bounds,
+                                        bool* offscreen,
+                                        bool clip_bounds) const {
+  bool allow_recursion = true;
+  return RelativeToTreeBoundsInternal(node, bounds, offscreen, clip_bounds,
+                                      allow_recursion);
+}
+
 gfx::RectF AXTree::GetTreeBounds(const AXNode* node,
                                  bool* offscreen,
                                  bool clip_bounds) const {

ui/accessibility/ax_tree.h

@@ -275,6 +275,14 @@ class AX_EXPORT AXTree : public AXNode::OwnerTree {
                             std::vector<AXNode*>* new_children,
                             AXTreeUpdateState* update_state);
 
+  // Internal implementation of RelativeToTreeBounds. It calls itself
+  // recursively but ensures that it can only do so exactly once!
+  gfx::RectF RelativeToTreeBoundsInternal(const AXNode* node,
+                                          gfx::RectF node_bounds,
+                                          bool* offscreen,
+                                          bool clip_bounds,
+                                          bool allow_recursion) const;
+
   base::ObserverList<AXTreeObserver> observers_;
   AXNode* root_ = nullptr;
   std::unordered_map<int32_t, AXNode*> id_map_;

ui/accessibility/ax_tree_unittest.cc

@@ -1372,6 +1372,38 @@ TEST(AXTreeTest, GetBoundsWithScrolling) {
   EXPECT_EQ("(115, 70) size (50 x 5)", GetBoundsAsString(tree, 3));
 }
 
+// When a node has zero size, we try to get the bounds from an ancestor.
+TEST(AXTreeTest, GetBoundsOfNodeWithZeroSize) {
+  AXTreeUpdate tree_update;
+  tree_update.root_id = 1;
+  tree_update.nodes.resize(5);
+  tree_update.nodes[0].id = 1;
+  tree_update.nodes[0].relative_bounds.bounds = gfx::RectF(0, 0, 800, 600);
+  tree_update.nodes[0].child_ids = {2};
+  tree_update.nodes[1].id = 2;
+  tree_update.nodes[1].relative_bounds.bounds = gfx::RectF(100, 100, 300, 200);
+  tree_update.nodes[1].child_ids = {3, 4, 5};
+
+  // This child has relative coordinates and no offset and no size.
+  tree_update.nodes[2].id = 3;
+  tree_update.nodes[2].relative_bounds.offset_container_id = 2;
+  tree_update.nodes[2].relative_bounds.bounds = gfx::RectF(0, 0, 0, 0);
+
+  // This child has relative coordinates and an offset, but no size.
+  tree_update.nodes[3].id = 4;
+  tree_update.nodes[3].relative_bounds.offset_container_id = 2;
+  tree_update.nodes[3].relative_bounds.bounds = gfx::RectF(20, 20, 0, 0);
+
+  // This child has absolute coordinates, an offset, and no size.
+  tree_update.nodes[4].id = 5;
+  tree_update.nodes[4].relative_bounds.bounds = gfx::RectF(120, 120, 0, 0);
+
+  AXTree tree(tree_update);
+  EXPECT_EQ("(100, 100) size (300 x 200)", GetBoundsAsString(tree, 3));
+  EXPECT_EQ("(120, 120) size (280 x 180)", GetBoundsAsString(tree, 4));
+  EXPECT_EQ("(120, 120) size (280 x 180)", GetBoundsAsString(tree, 5));
+}
+
 TEST(AXTreeTest, GetBoundsEmptyBoundsInheritsFromParent) {
   AXTreeUpdate tree_update;
   tree_update.root_id = 1;