Improve Android accessible hit testing.

Improve Android accessible hit testing.

Take advantage of a new function we defined already for what nodes are
"interesting" on Android.

When we get a hit test result back and it's not interesting, first see if
there's a single child that's interesting. If so, return that. If not,
walk up to the first interesting ancestor.

That will avoid returning something like a wrapper <div>.

BUG=656146
Committed: https://crrev.com/46fca134595100df7461475cc05516e94b000064
Cr-Commit-Position: refs/heads/master@{#426938}

[dmazzoni, 2016-10-17 04:35:53]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org

[aboxhall, 2016-10-18 21:03:25]

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_android.cc:296:
BrowserAccessibilityAndroid::GetInterestingChild() const {
Trying to think of a less confusing name for this - it's not necessarily a
child, not necessarily a direct child, and doesn't necessarily return an
interesting child even if one exists.

Maybe something like "SoleInterestingNodeForSubtree"?

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_android.cc:305: if (result
&& interesting_child) {
Maybe s/result/interesting_child/ and
s/interesting_child/sole_interesting_child/ or similar?

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:710:
const BrowserAccessibilityAndroid* interesting_child =
Do we need to make sure the child matches the hit test?

[dmazzoni, 2016-10-18 22:09:36]

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_android.cc:296:
BrowserAccessibilityAndroid::GetInterestingChild() const {
On 2016/10/18 21:03:24, aboxhall wrote:
> Trying to think of a less confusing name for this - it's not necessarily a
> child, not necessarily a direct child, and doesn't necessarily return an
> interesting child even if one exists.
> 
> Maybe something like "SoleInterestingNodeForSubtree"?

Good idea, I changed it to GetSoleInterestingDescendant()

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_android.cc:305: if (result
&& interesting_child) {
On 2016/10/18 21:03:24, aboxhall wrote:
> Maybe s/result/interesting_child/ and
> s/interesting_child/sole_interesting_child/ or similar?

Done.

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2422773002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:710:
const BrowserAccessibilityAndroid* interesting_child =
On 2016/10/18 21:03:24, aboxhall wrote:
> Do we need to make sure the child matches the hit test?

Good thought, but I don't think so - the only place we'd use
this is when we've already decided that the node that does
match the hit test is uninteresting (like a wrapper) so it
seems okay to land on the child even if the child is not hit.

An example of where this works well is if you have a <div>
containing text but with a lot of padding - previously,
clicking on the text would read the text, but clicking on
the padding would land on the div and TalkBack wouldn't read
anything. Now it would focus the text when you click on the
padding around it, but if there are multiple text boxes
inside it would instead focus an ancestor that wraps all
of them that actually has a role or label

[dmazzoni, 2016-10-18 22:09:49]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 22:10:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2016-10-18 22:17:28]

https://codereview.chromium.org/2422773002/diff/20001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_android.h (right):

https://codereview.chromium.org/2422773002/diff/20001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.h:56: // If this
node isn't interesting, recursively checks all of the
This comment is missing a critical point, especially given the name gives the
same misleading idea: it's not strictly a descendant, it may be |this|.

[commit-bot: I haz the power, 2016-10-18 23:16:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 23:16:33]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-10-19 23:00:45]

https://codereview.chromium.org/2422773002/diff/20001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_android.h (right):

https://codereview.chromium.org/2422773002/diff/20001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.h:56: // If this
node isn't interesting, recursively checks all of the
On 2016/10/18 22:17:28, aboxhall wrote:
> This comment is missing a critical point, especially given the name gives the
> same misleading idea: it's not strictly a descendant, it may be |this|.

Ah, good point.

Renamed and reworded, take a look.

[dmazzoni, 2016-10-19 23:00:48]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 23:02:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2016-10-19 23:21:29]

lgtm

https://codereview.chromium.org/2422773002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/2422773002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.cc:300: const
BrowserAccessibilityAndroid* sole_interesting_node = nullptr;
This is fine, of course, but I can't help pointing out that at this point it
actually is strictly a descendant :)

[commit-bot: I haz the power, 2016-10-20 00:13:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 00:13:25]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-10-21 22:39:22]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-10-21 22:39:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-22 01:01:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-22 01:03:41]

Description was changed from

==========
Improve Android accessible hit testing.

Take advantage of a new function we defined already for what nodes are
"interesting" on Android.

When we get a hit test result back and it's not interesting, first see if
there's a single child that's interesting. If so, return that. If not,
walk up to the first interesting ancestor.

That will avoid returning something like a wrapper <div>.

BUG=656146
==========

to

==========
Improve Android accessible hit testing.

Take advantage of a new function we defined already for what nodes are
"interesting" on Android.

When we get a hit test result back and it's not interesting, first see if
there's a single child that's interesting. If so, return that. If not,
walk up to the first interesting ancestor.

That will avoid returning something like a wrapper <div>.

BUG=656146

Committed: https://crrev.com/46fca134595100df7461475cc05516e94b000064
Cr-Commit-Position: refs/heads/master@{#426938}
==========

[commit-bot: I haz the power, 2016-10-22 01:03:42]

Patchset 3 (id:??) landed as
https://crrev.com/46fca134595100df7461475cc05516e94b000064
Cr-Commit-Position: refs/heads/master@{#426938}