Implement aria properties setsize and posinset.

Implement aria properties setsize and posinset.

This CL implements aria properties setsize & posinset and exposes it to
chromium via WebAXObject.

Also, indexInParent has been implemented.

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

[shreeramk, 2015-04-08 09:06:34]

shreeram.k@samsung.com changed reviewers:
+ dmazzoni@chromium.org, nektar@chromium.org

[shreeramk, 2015-04-08 09:07:09]

On 2015/04/08 09:06:34, shreeramk wrote:

Please review. Thanks!!

[shreeramk, 2015-04-08 12:00:07]

shreeram.k@samsung.com changed reviewers:
+ mkwst@chromium.org

[shreeramk, 2015-04-08 12:00:08]

mkwst@chromium.org: Please review changes in web/ and HTMLAttributeNames.in

[Mike West, 2015-04-08 12:27:50]

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

[Mike West, 2015-04-08 12:27:51]

LGTM for web/ and HTMLAttributeNames.in. Those both look fine, but please wait
for Dominic to take a look.

[dmazzoni, 2015-04-09 06:21:41]

posInSet and setSize are only allowed on certain roles. You should restrict it
to only those roles so that we're not exposing those attributes on random
elements where they're not allowed.

What do you think about implementing posInSet() and setSize() instead of
ariaPosInSet and ariaSetSize? For a native list it would return the correct
values too.

I think that'd be better rather than special-casing ARIA, because native APIs
like IAccessible2 want those properties for any list, not just ARIA lists.

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

https://codereview.chromium.org/1071483002/diff/1/Source/modules/accessibilit...
Source/modules/accessibility/AXObject.cpp:710: || hasAttribute(aria_atomicAttr)
Calling hasAttribute so many times in a row couldn't possibly be efficient.
There are now more than a dozen attributes we're checking but *most* elements
will have very few attributes, and no ARIA attributes.

I think it'd be better to iterate over all of the attributes and use a switch()
to see if any of them is an ARIA attribute or the "role" attribute, then we
should cache this computation.

Could you file a bug to do that in another change?

[shreeramk, 2015-04-09 13:50:15]

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:1180: return node()->nodeIndex();
And this to get index?

Is this correct API to get child's index in parent?

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:1191: return
node()->parentNode()->countChildren();
Is this correct API to get the child count of parent?

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:724: if (parentObject() &&
((roleValue() == ListBoxOptionRole && parentObject()->roleValue() ==
ListBoxRole)
I'd like your feedback on this check condition.

[dmazzoni, 2015-04-09 15:15:38]

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:1177: if
(supportssetSizePosInSet()) {
nit: how about supportsSetSizeAndPosInSet

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:1180: return node()->nodeIndex();
On 2015/04/09 13:50:15, shreeramk wrote:
> And this to get index?
> 
> Is this correct API to get child's index in parent?

Yes

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:1191: return
node()->parentNode()->countChildren();
On 2015/04/09 13:50:15, shreeramk wrote:
> Is this correct API to get the child count of parent?

Yes, but please check that parentNode() is not null just in case

[dmazzoni, 2015-04-09 15:19:20]

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1071483002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:724: if (parentObject() &&
((roleValue() == ListBoxOptionRole && parentObject()->roleValue() ==
ListBoxRole)
On 2015/04/09 13:50:15, shreeramk wrote:
> I'd like your feedback on this check condition.

This looks right, but it should also apply to ListItemRole with a parent of
ListRole, and TreeItemRole with a parent of TreeRole or TreeItemRole (tree items
can be nested).

Oh, and also MenuListOptionRole with a a parent of MenuListRole.

I think that's all, but you can check the ARIA spec for aria-posinset and you
can look over the list of roles to see if any others look like list-like roles.

I'd write it using a few more lines of code to make it more readable, something
like:

AXObject* parent = parentObject();
if (!parent)
    return false;

int role = roleValue();
int parentRole = parent->roleValue();

if ((role == ListItemRole && parentRole == ListRole)
    || (role == ListBoxOptionRole && parentRole == ListBoxRole)
...

[shreeramk, 2015-04-10 08:35:04]

Addressed comments.

PTAL. Thanks!!

[dmazzoni, 2015-04-10 17:35:21]

Please write layout tests for this. I know it will require three patches (one to
land this change, one to expose setSize and posInSet from WebAXObjectProxy, and
one to enable the tests) but I think it'd be good if you could develop those
changes all at once and then just land them in order.

https://codereview.chromium.org/1071483002/diff/40001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1071483002/diff/40001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:1180: return node()->nodeIndex();
Do a null check here.

(The reason this can be null is that AXObjects are reference-counted, so when an
HTML element is destroyed, we get a callback and "detach" the AXObject from its
node (and similarly from layoutObject). The AXObject gets deleted pretty soon
usually, but it's possible that it still exists for a short period of time when
detached so we always need to check if node() and layoutObject() are null.)

https://codereview.chromium.org/1071483002/diff/40001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:1192: return
node()->parentNode()->countChildren();
I just realized that this isn't going to do the right thing for:

<select>
  <optgroup>
    <option>1</option>
    <option>2</option>
  </optgroup>
  <optgroup>
    <option>3</option>
    <option>4</option>
  </optgroup>
</select>

In this case the DOM tree is more complicated, but the AX tree will be simpler,
it will look more like:

AXMenuListPopup
  AXMenuListOption
  AXMenuListOption
  AXMenuListOption
  AXMenuListOption

or:

AXListBox
  AXListBoxOption
  AXListBoxOption
  AXListBoxOption
  AXListBoxOption

So I think it'd be best to count the children of the AXObject's parent object.

Similarly for posInSet, above - that may mean implementing our own indexInParent
function if it doesn't exist yet.

[shreeramk, 2015-04-13 03:49:59]

https://codereview.chromium.org/1071483002/diff/40001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1071483002/diff/40001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:1192: return
node()->parentNode()->countChildren();
> So I think it'd be best to count the children of the AXObject's parent object.
> 
> Similarly for posInSet, above - that may mean implementing our own
indexInParent
> function if it doesn't exist yet.

Yes it doesn't exist currently.

From blink we can return the attr values of aria_posinset & aria_setsize if
those are present else in chromium side we can call indexinparent and
platformchildcount api to get the required values. Will that not be a good
option?

[dmazzoni, 2015-04-13 06:39:39]

On 2015/04/13 03:49:59, shreeramk wrote:
>
https://codereview.chromium.org/1071483002/diff/40001/Source/modules/accessib...
> File Source/modules/accessibility/AXNodeObject.cpp (right):
> 
>
https://codereview.chromium.org/1071483002/diff/40001/Source/modules/accessib...
> Source/modules/accessibility/AXNodeObject.cpp:1192: return
> node()->parentNode()->countChildren();
> > So I think it'd be best to count the children of the AXObject's parent
object.
> > 
> > Similarly for posInSet, above - that may mean implementing our own
> indexInParent
> > function if it doesn't exist yet.
> 
> Yes it doesn't exist currently.
> 
> From blink we can return the attr values of aria_posinset & aria_setsize if
> those are present else in chromium side we can call indexinparent and
> platformchildcount api to get the required values. Will that not be a good
> option?

No, wherever possible I'd like to have Blink take care of ARIA and have Chromium
not have to worry about it. I know the code isn't entirely like that now, but
we're slowly moving in that direction.

What you should have in mind is that Blink should do the work of interpreting
the web specs, including ARIA, and present Chromium with the information it
needs, and then Chromium adapts that to each platform.

[shreeramk, 2015-04-16 09:58:45]

Comments addressed.
PTAL.

Thanks!!

[dmazzoni, 2015-04-16 17:19:48]

lgtm

Great!

I'm assuming the test will pass when you add posInSet and setSize to
web_ax_object_proxy?

https://codereview.chromium.org/1071483002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1071483002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:744: const auto& siblings =
parentObject()->children();
You need to handle the case where parentObject() is null, just in case.

https://codereview.chromium.org/1071483002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:749: return index + 1;
It's very unusual to have 1-based indexes in code.

I think indexInParent should be 0-based in case we end up reusing this code for
something else later. You should convert it to a 1-based index for posInSet
only.

https://codereview.chromium.org/1071483002/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1071483002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:560: int indexInParent() const;
Add a comment saying this is 0-based.

https://codereview.chromium.org/1071483002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:561: virtual int posInSet() const {
return 0; }
Add a comment saying that this is the 1-based position.

https://codereview.chromium.org/1071483002/diff/60001/public/web/WebAXObject.h
File public/web/WebAXObject.h (right):

https://codereview.chromium.org/1071483002/diff/60001/public/web/WebAXObject....
public/web/WebAXObject.h:195: // Position in set & Size of set.
Say "1-based position in set" so that it's clear that the first index isn't 0.

[shreeramk, 2015-04-17 08:06:04]

The CQ bit was checked by shreeram.k@samsung.com

[shreeramk, 2015-04-17 08:06:05]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1071483002/#ps80001
(title: "Fixing nits")

[commit-bot: I haz the power, 2015-04-17 08:06:16]

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

[commit-bot: I haz the power, 2015-04-17 08:26:59]

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