Improve accessible hit testing heuristic.

Improve accessible hit testing heuristic.

When doing recursive hit testing and the point falls within two children
of the same element, prefer the one that has an indirect descendant
(i.e. grandchild) match.

A better fix would be to send an IPC to the renderer and let Blink do
the hit testing, I filed another bug (357371) for that. This is a stopgap.

Note, though, that we still need this hit testing algorithm for
synchronous APIs (like on Windows), so it's worth improving this.

BUG=357247
R=aboxhall@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260161

[aboxhall, 2014-03-27 23:28:26]

https://codereview.chromium.org/215633003/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/215633003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:317: // Walk the children
recursively looking for the BrowserAccessibility that
Suggest moving this comment above the for loop.

https://codereview.chromium.org/215633003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:337: if (result != child
&& !descendant_result)
I thought you mentioned the descendant being a control like a button, input etc?
Did I misunderstand?

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

https://codereview.chromium.org/215633003/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/215633003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:317: // Walk the children
recursively looking for the BrowserAccessibility that
On 2014/03/27 23:28:27, aboxhall wrote:
> Suggest moving this comment above the for loop.

Done.

https://codereview.chromium.org/215633003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:337: if (result != child
&& !descendant_result)
On 2014/03/27 23:28:27, aboxhall wrote:
> I thought you mentioned the descendant being a control like a button, input
etc?
> Did I misunderstand?

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]

lgtm

LGTM once explanatory comment added.

https://codereview.chromium.org/215633003/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/215633003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility.cc:337: if (result != child
&& !descendant_result)
On 2014/03/28 05:34:15, dmazzoni wrote:
> On 2014/03/27 23:28:27, aboxhall wrote:
> > I thought you mentioned the descendant being a control like a button, input
> etc?
> > Did I misunderstand?
> 
> 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.

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.

[dmazzoni, 2014-03-28 16:37:53]

Committed patchset #4 manually as r260161 (presubmit successful).