Aria disabled does not apply to descendant elements

Aria disabled does not apply to descendant elements

According to w3c ARIA spec, parent's disabled state should be propagated to all it's children.
One example needs clarification:
<div role="group" aria-disabled="true">
  <button type="submit" aria-disabled="false">submit</button>
</div>
In this case descendant overrides parent's disabled state.

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

[k.czech, 2015-03-16 12:33:37]

k.czech@samsung.com changed reviewers:
+ dmazzoni@chromium.org

[k.czech, 2015-03-16 12:44:44]

On 2015/03/16 12:33:38, k.czech wrote:

Dominic: I'm not really sure whether child's explicit defined
aria-disabled=false will overcome its parent's aria-disabled=true.
w3c spec does not really say it. What do you think about that ?.

<div aria-disabled="true">
  <a id="link1" href="#">link</a>
  <button id="button1" type="submit" aria-disabled="false">submit</button>
</div>

in this configuration button should be disabled or not ?

[dmazzoni, 2015-03-16 15:45:04]

I posted a question on the bug. You could be right but I didn't interpret the
spec that way and I want to ask for clarification.

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

https://codereview.chromium.org/1012663002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:730: for (AXObject* axObject =
const_cast<AXNodeObject*>(this); axObject; axObject = axObject->parentObject())
{
I'm trying to avoid code that iterates over all ancestors. Please make this a
cached attribute like  m_cachedIsInertOrAriaHidden and
m_cachedIsDescendantOfBarrenParent.

[k.czech, 2015-03-23 16:03:52]

On 2015/03/16 15:45:04, dmazzoni wrote:
> I posted a question on the bug. You could be right but I didn't interpret the
> spec that way and I want to ask for clarification.
> 
>
https://codereview.chromium.org/1012663002/diff/20001/Source/modules/accessib...
> File Source/modules/accessibility/AXNodeObject.cpp (right):
> 
>
https://codereview.chromium.org/1012663002/diff/20001/Source/modules/accessib...
> Source/modules/accessibility/AXNodeObject.cpp:730: for (AXObject* axObject =
> const_cast<AXNodeObject*>(this); axObject; axObject =
axObject->parentObject())
> {
> I'm trying to avoid code that iterates over all ancestors. Please make this a
> cached attribute like  m_cachedIsInertOrAriaHidden and
> m_cachedIsDescendantOfBarrenParent.

Dominic: any chance to look at this  ?

[dmazzoni, 2015-03-23 16:09:37]

On 2015/03/23 16:03:52, k.czech wrote:
> On 2015/03/16 15:45:04, dmazzoni wrote:
> > I posted a question on the bug. You could be right but I didn't interpret
the
> > spec that way and I want to ask for clarification.
> > 
> >
>
https://codereview.chromium.org/1012663002/diff/20001/Source/modules/accessib...
> > File Source/modules/accessibility/AXNodeObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1012663002/diff/20001/Source/modules/accessib...
> > Source/modules/accessibility/AXNodeObject.cpp:730: for (AXObject* axObject =
> > const_cast<AXNodeObject*>(this); axObject; axObject =
> axObject->parentObject())
> > {
> > I'm trying to avoid code that iterates over all ancestors. Please make this
a
> > cached attribute like  m_cachedIsInertOrAriaHidden and
> > m_cachedIsDescendantOfBarrenParent.
> 
> Dominic: any chance to look at this  ?

Sure. I didn't realize you had updated it, no email is sent when you upload a
new revision, so please always send email when you've made a change and want me
to look at it again.

[dmazzoni, 2015-03-23 16:16:51]

https://codereview.chromium.org/1012663002/diff/40001/LayoutTests/accessibili...
File LayoutTests/accessibility/aria-disabled.html (right):

https://codereview.chromium.org/1012663002/diff/40001/LayoutTests/accessibili...
LayoutTests/accessibility/aria-disabled.html:34: shouldBe("succeeded", "false");
I know this existed before, but as long as you're improving this test, could you
change all of these to look like this instead, rather than using "succeeded"?

shouldBe("group.isEnabled", "false")

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

https://codereview.chromium.org/1012663002/diff/40001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:444: bool
AXObject::computeIsDescendantOfDisabledParent() const
Please rewrite this to look more like computeIsInertOrAriaHidden instead.

Right now for every element on the page you're walking up all the parents to see
if it's disabled - but a faster way to do this is to check the current element,
then check the parent by calling isDescendantOfDisabledParent - then if the
parent was already computed you don't have to repeat that work.

computeIsDescendantOfBarrenParent could probably be improved too, but we don't
have to do it in this change.

https://codereview.chromium.org/1012663002/diff/40001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:446: for (AXObject* parent =
const_cast<AXObject*>(this); parent; parent = parent->parentObject()) {
Why do you need the const_cast? Can you write it as:

    for (const AXObject* parent = this; ...

?

If not, that's okay, but try it that way first.

[k.czech, 2015-03-26 16:03:36]

On 2015/03/23 16:16:51, dmazzoni wrote:
>
https://codereview.chromium.org/1012663002/diff/40001/LayoutTests/accessibili...
> File LayoutTests/accessibility/aria-disabled.html (right):
> 
>
https://codereview.chromium.org/1012663002/diff/40001/LayoutTests/accessibili...
> LayoutTests/accessibility/aria-disabled.html:34: shouldBe("succeeded",
"false");
> I know this existed before, but as long as you're improving this test, could
you
> change all of these to look like this instead, rather than using "succeeded"?
> 
> shouldBe("group.isEnabled", "false")
Done.
> 
>
https://codereview.chromium.org/1012663002/diff/40001/Source/modules/accessib...
> File Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://codereview.chromium.org/1012663002/diff/40001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:444: bool
> AXObject::computeIsDescendantOfDisabledParent() const
> Please rewrite this to look more like computeIsInertOrAriaHidden instead.
> 
> Right now for every element on the page you're walking up all the parents to
see
> if it's disabled - but a faster way to do this is to check the current
element,
> then check the parent by calling isDescendantOfDisabledParent - then if the
> parent was already computed you don't have to repeat that work.
Sounds great. Done.
> 
> computeIsDescendantOfBarrenParent could probably be improved too, but we don't
> have to do it in this change.
I will do this.
> 
>
https://codereview.chromium.org/1012663002/diff/40001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:446: for (AXObject* parent =
> const_cast<AXObject*>(this); parent; parent = parent->parentObject()) {
> Why do you need the const_cast? Can you write it as:
> 
>     for (const AXObject* parent = this; ...
> 
> ?
> 
> If not, that's okay, but try it that way first.
I guess we do not need this loop, when checking parent whether is cached.

[dmazzoni, 2015-03-27 19:21:17]

lgtm

OK to commit with the name change below, thanks!

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

https://codereview.chromium.org/1012663002/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:432: bool isDescendantOfDisabledParent()
const;
One small nitpick, sorry - let's call it isDescendantOfDisabledNode or
isInDisabledSubtree.

The current name - isDescendantOfDisabledParent - implies that it's checking if
the *parent* is disabled but not necessarily if the node itself is disabled -
but it's actually checking the node itself.

This is different than isDescendantOfBarrenParent, which checks if the *parent*
(or higher ancestor) can have children or not, but doesn't check that on the
current node.

[k.czech, 2015-03-30 07:59:27]

On 2015/03/27 19:21:17, dmazzoni wrote:
> lgtm
> 
> OK to commit with the name change below, thanks!
> 
>
https://codereview.chromium.org/1012663002/diff/60001/Source/modules/accessib...
> File Source/modules/accessibility/AXObject.h (right):
> 
>
https://codereview.chromium.org/1012663002/diff/60001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.h:432: bool
isDescendantOfDisabledParent()
> const;
> One small nitpick, sorry - let's call it isDescendantOfDisabledNode or
> isInDisabledSubtree.
> 
You are right, that sounds more descriptive, thanks.

[k.czech, 2015-03-30 08:00:23]

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

[k.czech, 2015-03-30 08:00:24]

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/1012663002/#ps80001
(title: "Changed methods names to ..DisabledNode()")

[commit-bot: I haz the power, 2015-03-30 08:00:31]

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

[commit-bot: I haz the power, 2015-03-30 09:25:10]

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