[Contextual Search] Check for ARIA widget roles.

[Contextual Search] Check for an ARIA widget.

Add support for checking if a given WebNode is an ARIA widget.  This support will be used by Chromium CL 868933002.

This support is implemented as a static helper function.

BUG=451096
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190852

[Donn Denman, 2015-01-29 22:43:27]

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

[Donn Denman, 2015-01-29 22:43:28]

Here's the Blink change to support ARIA widget role-detection.

PTAL when convenient.

[dmazzoni, 2015-01-30 07:30:41]

overall good once you figure out the right way to initialize that array

https://codereview.chromium.org/885163002/diff/1/Source/modules/accessibility...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/885163002/diff/1/Source/modules/accessibility...
Source/modules/accessibility/AXObject.cpp:149: but this generates a "needs
global destructor" error.
Did you try const char* instead of String?

[Donn Denman, 2015-02-02 19:30:14]

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

[Donn Denman, 2015-02-02 19:31:14]

Dimitri, Are you available to review this?  Thanks!

https://codereview.chromium.org/885163002/diff/1/Source/modules/accessibility...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/885163002/diff/1/Source/modules/accessibility...
Source/modules/accessibility/AXObject.cpp:149: but this generates a "needs
global destructor" error.
On 2015/01/30 07:30:41, dmazzoni wrote:
> Did you try const char* instead of String?

That worked, thanks -- don't know why I didn't think of that!

[Donn Denman, 2015-02-04 22:24:25]

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

[Donn Denman, 2015-02-04 23:28:09]

Dominic, Made progress, but I still need some help with a few things.

Writing a good test was a challenge, but I'm very happy with the unit test
included here.  However, it's the first unit test for this module so the
presubmit fails with missing include rules.  Note that I had to comment out the
include in order to upload this patch set.

Added aria-attribute checks, because I noticed many sites use these and they
implicitly indicate that the element is interactive.

Presubmit errors on includes also came up for AXObject.cpp -- Can you help me
figure out how to fix those?

[dmazzoni, 2015-02-05 00:21:33]

On 2015/02/04 23:28:09, Donn Denman wrote:
> Dominic, Made progress, but I still need some help with a few things.
> 
> Writing a good test was a challenge, but I'm very happy with the unit test
> included here.  However, it's the first unit test for this module so the
> presubmit fails with missing include rules.  Note that I had to comment out
the
> include in order to upload this patch set.

Next time use --bypass-hooks to bypass the presubmit hook.

> 
> Added aria-attribute checks, because I noticed many sites use these and they
> implicitly indicate that the element is interactive.
> 
> Presubmit errors on includes also came up for AXObject.cpp -- Can you help me
> figure out how to fix those?

I'll take a look.

[dmazzoni, 2015-02-05 00:32:01]

OK, there's pretty much only one issue, and that's mixing up the blink core
classes (Node, Element, AXObject, etc.) and the public wrappers (WebNode,
WebElement, WebAXObject, etc.).

The core classes aren't allowed to know about the wrappers. AXObject.cpp doesn't
know what a WebNode is, but WebNode is just a trivial wrapper around a Node, so
get its Node and pass that instead. The APIs are almost identical, it should be
an easy fix.

Thanks for adding a unit test file! Lest you think we have no tests, for legacy
reasons all of our tests are at a higher level, because back in the WebKit days
nobody used unit tests - so all of our tests are in
third_party/WebKit/LayoutTests/accessibility.

It'll be nice to have a gtest-style test suite now, so thanks - and sorry you
had to figure some stuff out from scratch to get it to work.

[dmazzoni, 2015-02-05 00:40:23]

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
File Source/modules/accessibility/AXObject.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
Source/modules/accessibility/AXObject.cpp:1031: if (element.isFocusable())
It seems odd to check for focusability in a function just called isARIAWidget.

Maybe it should be called isInsideFocusableElementOrARIAWidget (blink likes long
descriptive names)

or alternatively you could remove the focusability check and do that in a
separate function.

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
Source/modules/accessibility/AXObject.cpp:1040: } while (!curNode.isNull() &&
curNode.nodeName() != "BODY");
Now that you're inside Blink you can call isHTMLBodyElement

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
File Source/modules/accessibility/AXObject.h (right):

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
Source/modules/accessibility/AXObject.h:613: static bool isARIAWidget(const
WebNode&);
This should take a Node& instead

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
File Source/modules/accessibility/AXObjectTest.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
Source/modules/accessibility/AXObjectTest.cpp:50:
EXPECT_FALSE(AXObject::isARIAWidget(static_cast<WebNode>(root->getElementById("plain"))));
No need to cast to WebNode, just pass the Node

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/web/WebAXO...
File Source/web/WebAXObject.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/web/WebAXO...
Source/web/WebAXObject.cpp:1338: return AXObject::isARIAWidget(webNode);
call webNode.unwrap() to get a Node from a WebNode

[Donn Denman, 2015-02-06 22:53:36]

Thanks Dominic, that resolved all my issues!

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
File Source/modules/accessibility/AXObject.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
Source/modules/accessibility/AXObject.cpp:1031: if (element.isFocusable())
On 2015/02/05 00:40:23, dmazzoni wrote:
> It seems odd to check for focusability in a function just called isARIAWidget.
> 
> Maybe it should be called isInsideFocusableElementOrARIAWidget (blink likes
long
> descriptive names)
> 
> or alternatively you could remove the focusability check and do that in a
> separate function.
I changed the name, thanks for the suggestion!

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
Source/modules/accessibility/AXObject.cpp:1040: } while (!curNode.isNull() &&
curNode.nodeName() != "BODY");
On 2015/02/05 00:40:23, dmazzoni wrote:
> Now that you're inside Blink you can call isHTMLBodyElement

Done.

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
File Source/modules/accessibility/AXObject.h (right):

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
Source/modules/accessibility/AXObject.h:613: static bool isARIAWidget(const
WebNode&);
On 2015/02/05 00:40:23, dmazzoni wrote:
> This should take a Node& instead

Done.

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
File Source/modules/accessibility/AXObjectTest.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/modules/ac...
Source/modules/accessibility/AXObjectTest.cpp:50:
EXPECT_FALSE(AXObject::isARIAWidget(static_cast<WebNode>(root->getElementById("plain"))));
On 2015/02/05 00:40:23, dmazzoni wrote:
> No need to cast to WebNode, just pass the Node

Done -- looks like I don't even need the cast, just a dereference.

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/web/WebAXO...
File Source/web/WebAXObject.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/40001/Source/web/WebAXO...
Source/web/WebAXObject.cpp:1338: return AXObject::isARIAWidget(webNode);
On 2015/02/05 00:40:23, dmazzoni wrote:
> call webNode.unwrap() to get a Node from a WebNode
> 

Done.

[Donn Denman, 2015-02-06 22:55:25]

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

[Donn Denman, 2015-02-06 22:55:26]

Dimitri, This is ready for your review of changes to WebNode and WebAXObject. 
PTAL. TIA!

[Donn Denman, 2015-02-10 02:39:36]

On 2015/02/06 22:55:26, Donn Denman wrote:
> Dimitri, This is ready for your review of changes to WebNode and WebAXObject. 
> PTAL. TIA!

Dominic, Please take another look to verify that this looks good to you. 
Thanks!

[dmazzoni, 2015-02-10 07:40:55]

lgtm

https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
File Source/modules/accessibility/AXObject.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
Source/modules/accessibility/AXObject.cpp:1060: if (!strcmp("aria-haspopup",
attribute))
Why are you special-casing aria-haspopup? Since you're not specializing on the
value of the other attributes you may as well just assume that any value of
aria-haspopup means it's interactive too.

https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
File Source/modules/accessibility/AXObjectTest.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
Source/modules/accessibility/AXObjectTest.cpp:43: "<div id=\"focusable-parent\"
tabindex=\"2\"><div>focusable-parent</div></div><br>"
I don't see a test for this line. You should probably have the id on the child.

https://chromiumcodereview.appspot.com/885163002/diff/60001/public/web/WebNode.h
File public/web/WebNode.h (right):

https://chromiumcodereview.appspot.com/885163002/diff/60001/public/web/WebNod...
public/web/WebNode.h:129: BLINK_EXPORT bool
isInsideFocusableElementOrARIAWidget() const;
Do you need to define this interface on both WebNode and WebAXObject? It seems
like either one would suffice.

[Donn Denman, 2015-02-24 23:37:52]

Thanks for the review Dominic.

https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
File Source/modules/accessibility/AXObject.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
Source/modules/accessibility/AXObject.cpp:1060: if (!strcmp("aria-haspopup",
attribute))
On 2015/02/10 07:40:54, dmazzoni wrote:
> Why are you special-casing aria-haspopup? Since you're not specializing on the
> value of the other attributes you may as well just assume that any value of
> aria-haspopup means it's interactive too.

Makes sense -- done.  It looked to me like this was the only attribute that had
a value meaning it is *not* interactive, and thought that might be
auto-generated in some cases.   But it's probably true that the widget is
*sometimes* interactive and even though it's currently not, it's probably fine
to not trigger.   Simplifies the code, which is always good!  Done.

https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
File Source/modules/accessibility/AXObjectTest.cpp (right):

https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
Source/modules/accessibility/AXObjectTest.cpp:43: "<div id=\"focusable-parent\"
tabindex=\"2\"><div>focusable-parent</div></div><br>"
On 2015/02/10 07:40:54, dmazzoni wrote:
> I don't see a test for this line. You should probably have the id on the
child.

Nice catch!  Done.

https://chromiumcodereview.appspot.com/885163002/diff/60001/public/web/WebNode.h
File public/web/WebNode.h (right):

https://chromiumcodereview.appspot.com/885163002/diff/60001/public/web/WebNod...
public/web/WebNode.h:129: BLINK_EXPORT bool
isInsideFocusableElementOrARIAWidget() const;
On 2015/02/10 07:40:54, dmazzoni wrote:
> Do you need to define this interface on both WebNode and WebAXObject? It seems
> like either one would suffice.

Done -- removed the WebAXObject API.

[Donn Denman, 2015-02-24 23:38:58]

On 2015/02/24 23:37:52, Donn Denman wrote:
> Thanks for the review Dominic.
> 
>
https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
> File Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
> Source/modules/accessibility/AXObject.cpp:1060: if (!strcmp("aria-haspopup",
> attribute))
> On 2015/02/10 07:40:54, dmazzoni wrote:
> > Why are you special-casing aria-haspopup? Since you're not specializing on
the
> > value of the other attributes you may as well just assume that any value of
> > aria-haspopup means it's interactive too.
> 
> Makes sense -- done.  It looked to me like this was the only attribute that
had
> a value meaning it is *not* interactive, and thought that might be
> auto-generated in some cases.   But it's probably true that the widget is
> *sometimes* interactive and even though it's currently not, it's probably fine
> to not trigger.   Simplifies the code, which is always good!  Done.
> 
>
https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
> File Source/modules/accessibility/AXObjectTest.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/885163002/diff/60001/Source/modules/ac...
> Source/modules/accessibility/AXObjectTest.cpp:43: "<div
id=\"focusable-parent\"
> tabindex=\"2\"><div>focusable-parent</div></div><br>"
> On 2015/02/10 07:40:54, dmazzoni wrote:
> > I don't see a test for this line. You should probably have the id on the
> child.
> 
> Nice catch!  Done.
> 
>
https://chromiumcodereview.appspot.com/885163002/diff/60001/public/web/WebNode.h
> File public/web/WebNode.h (right):
> 
>
https://chromiumcodereview.appspot.com/885163002/diff/60001/public/web/WebNod...
> public/web/WebNode.h:129: BLINK_EXPORT bool
> isInsideFocusableElementOrARIAWidget() const;
> On 2015/02/10 07:40:54, dmazzoni wrote:
> > Do you need to define this interface on both WebNode and WebAXObject? It
seems
> > like either one would suffice.
> 
> Done -- removed the WebAXObject API.

Dimitri, Can you look at this now, or recommend a reviewer for the WebNode
addition I'm making?  Thanks!

[dmazzoni, 2015-02-24 23:57:19]

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

[dmazzoni, 2015-02-24 23:57:20]

+mkwst as an alternate reviewer for Source/web and public/web

[Mike West, 2015-02-25 06:32:49]

LGTM.

[Donn Denman, 2015-02-25 17:41:56]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2015-02-25 17:41:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/885163002/#ps80001 (title:
"Removed changes to WebAXObject.")

[commit-bot: I haz the power, 2015-02-25 17:43:04]

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

[commit-bot: I haz the power, 2015-02-25 19:51:15]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190852