Too many accessible nodes were marked as clickable on Android

Too many accessible nodes were marked as clickable on Android

Now only nodes that are actually focusable or clickable are marked as
clickable. To make sure TalkBack reaches all interesting nodes, fix the
code that handles ACTION_NEXT_HTML_ELEMENT so that it returns all focusable
nodes in addition to leaf nodes with text.

BUG=645167
Committed: https://crrev.com/33d97d8148948cade0efe3e97c50f8473a225ee9
Cr-Commit-Position: refs/heads/master@{#425626}

[dmazzoni, 2016-10-10 17:34:15]

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

[dmazzoni, 2016-10-12 19:51:29]

Friendly ping

[David Tseng, 2016-10-12 20:28:35]

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

https://codereview.chromium.org/2323043002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_android.cc:161: // because
there is no way to know for sure what might have a click handler.
Is this no longer a concern or do you have a more robust way to detect the
associated dom node has a click handler (e.g. onclick set)?

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

https://codereview.chromium.org/2323043002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_android.cc:210: // Only the
root of the tree, and not child frames, are focusable.
Seems strange to have this restriction; what if a page author sets an aria role
on body like application? This code would allow focus in that case. Also, if an
iframe has an aria label, wouldn't it be interesting?

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

https://codereview.chromium.org/2323043002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:67: //
Otherwise, the interesting nodes are leaf nodes with text.
If I have a <div role="button"> (no text content, not focusable), it's not
interesting?

[dmazzoni, 2016-10-12 22:41:58]

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

https://codereview.chromium.org/2323043002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_android.cc:161: // because
there is no way to know for sure what might have a click handler.
On 2016/10/12 20:28:35, David Tseng wrote:
> Is this no longer a concern or do you have a more robust way to detect the
> associated dom node has a click handler (e.g. onclick set)?

An older version of TalkBack was skipping over anything
that wasn't clickable, so we returned "clickable" for more
than what was actually clickable.

Now TalkBack is fixed, so we can revert to returning
clickable when we think a node is clickable. Same as other
platforms, it's not bad but not 100% perfect. It works
correctly for onclick and for real event handlers, but
doesn't handle event delegation where the listener is on
the ancestor.

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

https://codereview.chromium.org/2323043002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_android.cc:210: // Only the
root of the tree, and not child frames, are focusable.
On 2016/10/12 20:28:35, David Tseng wrote:
> Seems strange to have this restriction; what if a page author sets an aria
role
> on body like application? This code would allow focus in that case. Also, if
an
> iframe has an aria label, wouldn't it be interesting?

Agreed. I changed it so that we skip iframes and child frame
root notes if they don't have a name, but we include them if
they have a name.

I don't think there's an issue with an aria role on the body,
the root of our tree is actually the parent of the body element.

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

https://codereview.chromium.org/2323043002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:67: //
Otherwise, the interesting nodes are leaf nodes with text.
On 2016/10/12 20:28:35, David Tseng wrote:
> If I have a <div role="button"> (no text content, not focusable), it's not
> interesting?

Good idea, I can have it return true if it's a control role.

[dmazzoni, 2016-10-12 22:42:15]

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

[commit-bot: I haz the power, 2016-10-12 22:42:54]

Dry run: 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-13 00:33:45]

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

[commit-bot: I haz the power, 2016-10-13 00:33:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[David Tseng, 2016-10-14 13:10:59]

lgtm

[dmazzoni, 2016-10-14 20:45:45]

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

[commit-bot: I haz the power, 2016-10-14 20:46:02]

Dry run: 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-14 22:19:28]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-10-15 06:31:38]

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

[commit-bot: I haz the power, 2016-10-15 06:31:46]

Dry run: 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-15 07:46:25]

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

[commit-bot: I haz the power, 2016-10-15 07:46:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-10-15 08:05:57]

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

[commit-bot: I haz the power, 2016-10-15 08:06:02]

Dry run: 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-15 09:12:21]

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

[commit-bot: I haz the power, 2016-10-15 09:12:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2016-10-16 03:21:55]

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

[commit-bot: I haz the power, 2016-10-16 03:21:59]

Dry run: 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-16 03:59:29]

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

[commit-bot: I haz the power, 2016-10-16 03:59:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-10-17 04:34:22]

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

[commit-bot: I haz the power, 2016-10-17 04:34:36]

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

[dmazzoni, 2016-10-17 05:14:23]

This required updating all of the Android test expectations.
Ready to land now.

[commit-bot: I haz the power, 2016-10-17 06:20:08]

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

[commit-bot: I haz the power, 2016-10-17 06:20:09]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-10-17 07:01:55]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-10-17 07:01:56]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2323043002/#ps120001
(title: "Fix item markers")

[commit-bot: I haz the power, 2016-10-17 07:02:17]

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-17 07:10:57]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-17 07:12:59]

Description was changed from

==========
Too many accessible nodes were marked as clickable on Android

Now only nodes that are actually focusable or clickable are marked as
clickable. To make sure TalkBack reaches all interesting nodes, fix the
code that handles ACTION_NEXT_HTML_ELEMENT so that it returns all focusable
nodes in addition to leaf nodes with text.

BUG=645167
==========

to

==========
Too many accessible nodes were marked as clickable on Android

Now only nodes that are actually focusable or clickable are marked as
clickable. To make sure TalkBack reaches all interesting nodes, fix the
code that handles ACTION_NEXT_HTML_ELEMENT so that it returns all focusable
nodes in addition to leaf nodes with text.

BUG=645167

Committed: https://crrev.com/33d97d8148948cade0efe3e97c50f8473a225ee9
Cr-Commit-Position: refs/heads/master@{#425626}
==========

[commit-bot: I haz the power, 2016-10-17 07:13:00]

Patchset 7 (id:??) landed as
https://crrev.com/33d97d8148948cade0efe3e97c50f8473a225ee9
Cr-Commit-Position: refs/heads/master@{#425626}