Use relative bounding boxes throughout Chrome accessibility

chrome/renderer/extensions/automation_internal_custom_bindings.cc

Index: chrome/renderer/extensions/automation_internal_custom_bindings.cc
diff --git a/chrome/renderer/extensions/automation_internal_custom_bindings.cc b/chrome/renderer/extensions/automation_internal_custom_bindings.cc
index ad12627f32dc6cf6cfbd84b8659ea17ea46bcb6c..7aa41502567002aeff5fa437ccaaf7592b4dfab6 100644
--- a/chrome/renderer/extensions/automation_internal_custom_bindings.cc
+++ b/chrome/renderer/extensions/automation_internal_custom_bindings.cc
@@ -85,15 +85,15 @@ v8::Local<v8::Object> RectToV8Object(v8::Isolate* isolate,
 
 // Compute the bounding box of a node, fixing nodes with empty bounds by
 // unioning the bounds of their children.
-static gfx::Rect ComputeLocalNodeBounds(TreeCache* cache, ui::AXNode* node) {
-  gfx::Rect bounds = gfx::ToEnclosingRect(node->data().location);
+static gfx::RectF ComputeLocalNodeBounds(TreeCache* cache, ui::AXNode* node) {
+  gfx::RectF bounds = node->data().location;
   if (bounds.width() > 0 && bounds.height() > 0)
     return bounds;
 
   // Compute the bounds of each child.
   for (size_t i = 0; i < node->children().size(); i++) {
     ui::AXNode* child = node->children()[i];
-    gfx::Rect child_bounds = ComputeLocalNodeBounds(cache, child);
+    gfx::RectF child_bounds = ComputeLocalNodeBounds(cache, child);
 
     // Ignore children that don't have valid bounds themselves.
     if (child_bounds.width() == 0 || child_bounds.height() == 0)
@@ -115,35 +115,39 @@ static gfx::Rect ComputeLocalNodeBounds(TreeCache* cache, ui::AXNode* node) {
 // Compute the bounding box of a node in global coordinates, walking up the
 // parent hierarchy to offset by frame offsets and scroll offsets.
 static gfx::Rect ComputeGlobalNodeBounds(TreeCache* cache, ui::AXNode* node) {

[aboxhall, 2016/08/12 16:05:10]

Is there a reason this needs to return Rect rather than RectF?

Also, can you refresh my memory on why we need this code both here and in
BrowserAccessibility?

[dmazzoni, 2016/08/15 05:31:11]

We need to use floats for the "intermediate" computation so we
don't get way off due to rounding errors, now that we're possibly
doing matrix transformations to convert from local to global
coordinates.

But once we have global coordinates, integer pixel coordinates are
fine. I didn't see any need to also rewrite the automation API and
ChromeVox code that gets the coordinates of objects on the screen
and make them deal with floats. Once you have global coordinates,
pixels are close enough.
Because BrowserAccessibility is in */browser/ and Automation
Internals are in */renderer/

Ideally we should share this code, but to do that would require
some more refactoring. Basically the code in ui/accessibility
handles anything relating to a single AXTree, but not a bunch of
frames. Right now BrowserAccessibility and Automation both have
their own code to deal with multiple trees (iframes and things like
that) and join them together. It'd be nice to move more of that
to ui/accessibility, but it's nontrivial because ui/accessibility
shouldn't know anything about iframes, and content/ shouldn't know
about the "desktop" tree, and so on.

-  gfx::Rect bounds = ComputeLocalNodeBounds(cache, node);
-
-  ui::AXNode* root = cache->tree.root();
-  while (root) {
-    // Apply scroll offsets.
-    if (root != node) {
-      int sx = 0;
-      int sy = 0;
-      if (root->data().GetIntAttribute(ui::AX_ATTR_SCROLL_X, &sx) &&
-          root->data().GetIntAttribute(ui::AX_ATTR_SCROLL_Y, &sy)) {
-        bounds.Offset(-sx, -sy);
+  gfx::RectF bounds = ComputeLocalNodeBounds(cache, node);
+
+  while (node) {
+    if (node->data().transform)
+      node->data().transform->TransformRect(&bounds);
+
+    ui::AXNode* container =
+        cache->tree.GetFromId(node->data().offset_container_id);
+    if (!container) {
+      if (node == cache->tree.root()) {
+        container = cache->owner->GetParent(node, &cache);
+      } else {
+        container = cache->tree.root();
       }
     }
 
-    if (root->data().transform) {
-      gfx::RectF boundsf(bounds);
-      root->data().transform->TransformRect(&boundsf);
-      bounds = gfx::Rect(boundsf.x(), boundsf.y(), boundsf.width(),
-                         boundsf.height());
-    }
-
-    ui::AXNode* parent = cache->owner->GetParent(root, &cache);
-    if (!parent)
+    if (!container || container == node)
       break;
 
-    root = cache->tree.root();
+    gfx::RectF container_bounds = ComputeLocalNodeBounds(cache, container);
+    bounds.Offset(container_bounds.x(), container_bounds.y());
+
+    int sx = 0;

[aboxhall, 2016/08/12 16:05:10]

Maybe s/sx/scroll_x/ and similarly for sy, while we're here?

[dmazzoni, 2016/08/15 05:31:11]

Done

+    int sy = 0;
+    if (container->data().GetIntAttribute(ui::AX_ATTR_SCROLL_X, &sx) &&
+        container->data().GetIntAttribute(ui::AX_ATTR_SCROLL_Y, &sy)) {
+      bounds.Offset(-sx, -sy);
+    }
+
+    node = container;
   }
 
-  return bounds;
+  return gfx::ToEnclosingRect(bounds);
 }
 
 ui::AXNode* FindNodeWithChildTreeId(ui::AXNode* node, int child_tree_id) {