[Contextual Search] Check ARIA Roles before triggering.

[Contextual Search] Add more node checks before triggering.

Now showUnhandledTapUIIfNeeded makes an additional check to see if it is appropriate to trigger: if the tapped node is an ARIA widget or not, according to the accessibility code in Blink.

This CL is dependent on a Blink CL: 885163002

BUG=451096
Committed: https://crrev.com/57e54f55650add38970fe4b4e48cf3010a4a12fe
Cr-Commit-Position: refs/heads/master@{#318275}

[Donn Denman, 2015-01-22 23:19:18]

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

[Donn Denman, 2015-01-22 23:19:18]

Dominic, would you mind doing a first-pass review of this code?  I have done
very few accessibility-related CLs so I'm not sure what the best practices might
be.  Also, I think you might be the best reviewer for the semantics of what I'm
trying to do, e.g. check cursor or tabindex to see if the node is interactive.

It seems strange to check the cursor for a mobile-only feature, so I'm thinking
I'll take that out, but leaving in case you think it could be useful.

This lacks unit tests, will do once the overall approach gets an OK.

I think the list of wiget / composite roles belongs somewhere else, but not sure
what the best place is.  Also I'm surprised that several of the standard widget
roles are not listed, and I presume they should be added.

Thanks, in advance!

[dmazzoni, 2015-01-23 17:39:56]

How about splitting this into two changes? I think that properly checking for a
focusable ancestor and for the pointer cursor will probably help at least as
much as checking ARIA roles. I think it's probably more important to get this
working and write proper tests for it.

For the second part of the change, I made some comments on the ARIA role check.
The cleanest and most maintainable way to do it would be to get a WebAXObject
from the WebNode and add a new helper function that checks if the role is a
"widget" role. That keeps all ARIA parsing and categorization in Blink.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2142:
current_cursor_.GetCursorInfo(&cursor_info);
I haven't used current_cursor_ before - are we sure that this is always the
computed cursor of |tapped_node|?

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2147: !has_wai_aria_role &&
!tapped_node.isFocusable() &&
I think you need to search up the ancestor chain to see if an ancestor is
focusable, because the tap could be on an element inside a focusable element.

Also, note that text nodes are not elements and so they can't be focusable.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2155: bool RenderWidget::hasAriaRole(const
WebNode& node) {
This should probably be named hasWidgetAriaRole or something like that.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2158: ui::AX_ROLE_ALERT,
ui::AXRole is not a list of ARIA roles, it's a list of all accessible roles,
which is a superset of the ARIA roles. As an example, the reason you didn't find
ui::AX_ROLE_GRID_CELL is because both role="gridcell" and the <td> element map
to a single role internally, ui::AX_ROLE_CELL. So I think you should just store
a list of strings.

All of the code that parses ARIA lives in Blink currently. Perhaps this should
be a helper function in WebAXObject.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2198: std::string role =
element.getAttribute("role").utf8().data();
The role attribute is actually allowed to be a space-delimited list of role
names; you have to parse them one at a time and take the first one that's known.
That's a good argument in favor of using WebAXObject instead of duplicating
logic here.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2201: for (unsigned i = 0;
I'd use a hash_set for this - even if we don't think performance matters because
it only happens once per tap, I think it's cleaner.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2203: if
(ui::ToString(widget_and_composite_roles[i]) == role)
This should be case-insensitive. I'd convert the attribute value to lowercase
and then do the hash lookup.

[Donn Denman, 2015-01-29 22:42:40]

Dominic, PTAL when convenient, and let me know if you think this is approach
seems reasonable.  Then I'll add unit tests and send it to someone for a real
review.

Note that this is now dependent on Blink CL 885163002.

I decided to keep the ARIA role-checking even though the focusability check
works fine because making a node focusable changes the UI (putting a yellow box
around it on Android) which some authors may not want.

Thanks for all the help on this!

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2142:
current_cursor_.GetCursorInfo(&cursor_info);
On 2015/01/23 17:39:56, dmazzoni wrote:
> I haven't used current_cursor_ before - are we sure that this is always the
> computed cursor of |tapped_node|?

I couldn't figure out the cursor stuff, so decided to drop it from this CL.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2147: !has_wai_aria_role &&
!tapped_node.isFocusable() &&
On 2015/01/23 17:39:56, dmazzoni wrote:
> I think you need to search up the ancestor chain to see if an ancestor is
> focusable, because the tap could be on an element inside a focusable element.
> 
> Also, note that text nodes are not elements and so they can't be focusable.

Done -- Now checking all ancestors for focusability.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2155: bool RenderWidget::hasAriaRole(const
WebNode& node) {
On 2015/01/23 17:39:56, dmazzoni wrote:
> This should probably be named hasWidgetAriaRole or something like that.

Moved this logic into Blink.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2158: ui::AX_ROLE_ALERT,
On 2015/01/23 17:39:56, dmazzoni wrote:
> ui::AXRole is not a list of ARIA roles, it's a list of all accessible roles,
> which is a superset of the ARIA roles. As an example, the reason you didn't
find
> ui::AX_ROLE_GRID_CELL is because both role="gridcell" and the <td> element map
> to a single role internally, ui::AX_ROLE_CELL. So I think you should just
store
> a list of strings.
> 
> All of the code that parses ARIA lives in Blink currently. Perhaps this should
> be a helper function in WebAXObject.

Thanks for explaining this.  Moved to WebAXObject.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2198: std::string role =
element.getAttribute("role").utf8().data();
On 2015/01/23 17:39:56, dmazzoni wrote:
> The role attribute is actually allowed to be a space-delimited list of role
> names; you have to parse them one at a time and take the first one that's
known.
> That's a good argument in favor of using WebAXObject instead of duplicating
> logic here.

Done in the Blink CL.  Thanks for pointing this out.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2201: for (unsigned i = 0;
On 2015/01/23 17:39:56, dmazzoni wrote:
> I'd use a hash_set for this - even if we don't think performance matters
because
> it only happens once per tap, I think it's cleaner.

Done.

https://codereview.chromium.org/868933002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2203: if
(ui::ToString(widget_and_composite_roles[i]) == role)
On 2015/01/23 17:39:56, dmazzoni wrote:
> This should be case-insensitive. I'd convert the attribute value to lowercase
> and then do the hash lookup.

Done in the Blink CL.  Thanks for pointing this out too!

[dmazzoni, 2015-01-30 07:34:20]

This is looking a lot better! Would like to see a test because as you can see
it's easy to get the logic subtly wrong

https://codereview.chromium.org/868933002/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/868933002/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:2208: if (curNode.isFocusable())
This isn't going to work, because <body> is focusable, so if you keep walking up
you'll eventually reach the body and it will always return true.

Perhaps you could stop when you reach the body element?

All the more reason this needs tests

https://codereview.chromium.org/868933002/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:2213: if (role.length() > 0) {
nit: !role.isEmpty()

[Donn Denman, 2015-02-02 21:14:15]

Thanks for the review Dominic!

https://codereview.chromium.org/868933002/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/868933002/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:2208: if (curNode.isFocusable())
On 2015/01/30 07:34:20, dmazzoni wrote:
> This isn't going to work, because <body> is focusable, so if you keep walking
up
> you'll eventually reach the body and it will always return true.
> 
> Perhaps you could stop when you reach the body element?
> 
> All the more reason this needs tests

I'm surprised I didn't see this when testing it out using hands-on interaction. 
I did a few attempts at stopping at the body element, but didn't see any better
way than just checking if the curNode.nodeName() == "BODY", so let me know if
you have any suggestions or further thoughts on this.

https://codereview.chromium.org/868933002/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:2213: if (role.length() > 0) {
On 2015/01/30 07:34:20, dmazzoni wrote:
> nit: !role.isEmpty()

Done.

[dmazzoni, 2015-02-02 21:19:52]

https://codereview.chromium.org/868933002/diff/20001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/868933002/diff/20001/content/renderer/render_...
content/renderer/render_widget.cc:2208: if (curNode.isFocusable())
On 2015/02/02 21:14:15, Donn Denman wrote:
> On 2015/01/30 07:34:20, dmazzoni wrote:
> > This isn't going to work, because <body> is focusable, so if you keep
walking
> up
> > you'll eventually reach the body and it will always return true.
> > 
> > Perhaps you could stop when you reach the body element?
> > 
> > All the more reason this needs tests
> 
> I'm surprised I didn't see this when testing it out using hands-on
interaction. 
> I did a few attempts at stopping at the body element, but didn't see any
better
> way than just checking if the curNode.nodeName() == "BODY", so let me know if
> you have any suggestions or further thoughts on this.

I'm not sure I know a better way from the blink public interface. I'll let a
Blink owner decide what the best solution is.

[Donn Denman, 2015-02-02 21:20:01]

donnd@chromium.org changed reviewers:
+ sievers@chromium.org

[Donn Denman, 2015-02-02 21:20:02]

Does anyone have a suggestion for a good way to test this change to
render_widget.cc?  Ideally there would be an html test file with a set of simple
test cases and a way to detect if showUnhandledTapUIIfNeeded was called as
expected somewhere in the call chain for showUnhandledTapUIIfNeeded (which
extends from render_widget up through render_widget_host_view_android and
ContentViewCore.  See CL 816953004 for that piping).

[dmazzoni, 2015-02-02 21:24:07]

I think you could
use content/renderer/accessibility/renderer_accessibility_browsertest.cc as
a skeleton for loading HTML and then doing some unit tests against
RenderWidget. There are others, try searching for browser tests in
content/renderer

On Mon, Feb 2, 2015 at 1:20 PM, <donnd@chromium.org> wrote:

> Does anyone have a suggestion for a good way to test this change to
> render_widget.cc?  Ideally there would be an html test file with a set of
> simple
> test cases and a way to detect if showUnhandledTapUIIfNeeded was called as
> expected somewhere in the call chain for showUnhandledTapUIIfNeeded (which
> extends from render_widget up through render_widget_host_view_android and
> ContentViewCore.  See CL 816953004 for that piping).
>
>
> https://codereview.chromium.org/868933002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Donn Denman, 2015-02-25 21:06:14]

donnd@chromium.org changed reviewers:
+ nasko@chromium.org
- dmazzoni@chromium.org

[Donn Denman, 2015-02-25 21:06:15]

sievers or nasko, PTAL at this simple change.  Thanks!

[nasko, 2015-02-25 21:12:01]

On 2015/02/25 21:06:15, Donn Denman wrote:
> sievers or nasko, PTAL at this simple change.  Thanks!

Not my area of expertise, so I'd defer to sievers. It will be good though to
have a test case with this CL.

[Donn Denman, 2015-02-25 21:21:29]

donnd@chromium.org changed reviewers:
- nasko@chromium.org

[Donn Denman, 2015-02-25 21:21:39]

I can easily promise to add a test case downstream (for Android, in a separate
CL) but we don't have an existing test suite upstream yet.

[no sievers, 2015-02-25 21:26:14]

lgtm

[Donn Denman, 2015-02-26 17:28:30]

The CQ bit was checked by donnd@chromium.org

[commit-bot: I haz the power, 2015-02-26 17:29:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/868933002/100001

[commit-bot: I haz the power, 2015-02-26 19:03:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-26 19:04:26]

Patchset 6 (id:??) landed as
https://crrev.com/57e54f55650add38970fe4b4e48cf3010a4a12fe
Cr-Commit-Position: refs/heads/master@{#318275}