Improve accessible hit testing heuristic.

content/browser/accessibility/browser_accessibility.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/accessibility/browser_accessibility.h"
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 296 matching lines @@
 
   // Adjust the bounds by the top left corner of the containing view's bounds
   // in screen coordinates.
   bounds.Offset(manager_->GetViewBounds().OffsetFromOrigin());
 
   return bounds;
 }
 
 BrowserAccessibility* BrowserAccessibility::BrowserAccessibilityForPoint(
     const gfx::Point& point) {
   // Walk the children recursively looking for the BrowserAccessibility that

[aboxhall, 2014/03/27 23:28:27]

Suggest moving this comment above the for loop.

[dmazzoni, 2014/03/28 05:34:15]

Done.

   // most tightly encloses the specified point.
+
+  // The best result found that's a child of this object.
+  BrowserAccessibility* child_result = NULL;
+  // The best result that's an indirect descendant like grandchild, etc.
+  BrowserAccessibility* descendant_result = NULL;
+
   for (int i = static_cast<int>(PlatformChildCount()) - 1; i >= 0; --i) {
     BrowserAccessibility* child = PlatformGetChild(i);
 
     // Skip table columns because cells are only contained in rows,
     // not columns.
     if (child->role() == ui::AX_ROLE_COLUMN)
       continue;
 
-    if (child->GetGlobalBoundsRect().Contains(point))
-      return child->BrowserAccessibilityForPoint(point);
+    if (child->GetGlobalBoundsRect().Contains(point)) {
+      BrowserAccessibility* result = child->BrowserAccessibilityForPoint(point);
+      if (result == child && !child_result)
+        child_result = result;
+      if (result != child && !descendant_result)

[aboxhall, 2014/03/27 23:28:27]

I thought you mentioned the descendant being a control like a button, input etc?
Did I misunderstand?

[dmazzoni, 2014/03/28 05:34:15]

I used that as an example, but I don't think that should be part of the
heuristic - when doing touch exploration, you're just as likely to want to touch
static text as you are a button or link.

Here's another explanation of why the heuristic helps: suppose there are two
rows of buttons, none of which overlap. However, the containers for each row of
buttons overlap a fair amount just because of the way the page was laid out.

Without this heuristic, the algorithm would greedily pick the container that
encloses the point, even if the button in question was in the other container.
That's what we want to prevent.

[aboxhall, 2014/03/28 16:13:32]

Ok. We should probably add a comment here explaining this preference, then. It
sounds to me like we basically want to pick the deepest descendant because
actual content is always going to be in leaf nodes.

+        descendant_result = result;
+    }
   }
+
+  if (descendant_result)
+    return descendant_result;
+  if (child_result)
+    return child_result;
+
   return this;
 }
 
 void BrowserAccessibility::Destroy() {
   for (std::vector<BrowserAccessibility*>::iterator iter = children_.begin();
        iter != children_.end();
        ++iter) {
     (*iter)->Destroy();
   }
   children_.clear();
@@ skipping 289 matching lines @@
   if (role_ == ui::AX_ROLE_STATIC_TEXT)
     return static_cast<int>(GetStringAttribute(ui::AX_ATTR_VALUE).size());
 
   int len = 0;
   for (size_t i = 0; i < children_.size(); ++i)
     len += children_[i]->GetStaticTextLenRecursive();
   return len;
 }
 
 }  // namespace content