AX: aria-hidden=false does not work as expected

[chris fleizach]

* SUMMARY
aria-hidden=false does not work as expected

* NOTES
For example, a heading only perceivable to AT. HTML @hidden attribute means unrendered, but aria-hidden="false" should reverse this for the APIs. It currently does not.

<h1 hidden aria-hidden="false">Foo</h1>

http://www.w3.org/WAI/PF/aria/complete#aria-hidden

[chris fleizach]

rdar://12359052

[Dominic Mazzoni]

Hopefully AccessibilityNodeObject should make it possible for us to support this case now.

[chris fleizach]

(In reply to comment #2)
> Hopefully AccessibilityNodeObject should make it possible for us to support this case now.

Yes it does. I have it just about working correctly now using Node object

[chris fleizach]

Created attachment 168093 [details]
patch

[WebKit Review Bot]

Attachment 168093 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1
Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/AccessibilityNodeObject.cpp:321:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 168096 [details]
patch

[WebKit Review Bot]

Comment on attachment 168096 [details]
patch 

Attachment 168096 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14262009

New failing tests:
accessibility/aria-hidden-negates-no-visibility.html

[Dominic Mazzoni]

Comment on attachment 168096 [details]
patch 

View in context: https://bugs.webkit.org/attachment.cgi?id=168096&action=review

Unofficial review.

I'm concerned about a few details, but once those are fixed I think this is a great change!

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2726
> +        if (child->renderer()) {

It looks like this adds an extra O(n^2) calculation even when none of the children have aria-hidden=false. You should avoid searching for the last render sibling is unless one of the children actually has aria-hidden=false, otherwise it's just wasted work.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2727
> +            

Nit: extra blank line

> Source/WebCore/accessibility/AXObjectCache.cpp:326
> +    if (!inCanvasSubtree && !isHidden)

Won't this start creating accessibility objects for things that are hidden but NOT aria-hidden=false? I don't think we want to do that.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:228
> +    // So for now, let's return the position of the ancestor that does have a position,

I like this idea, but I think it'd be better to give it the size of the ancestor, too. I think that's more likely to be correct for a screen magnifier.

For example, if an element within a canvas has focus, then in the absence of any other information I think the magnifier should zoom to the whole canvas, not the upper-left corner.

[chris fleizach]

Comment on attachment 168096 [details]
patch 

View in context: https://bugs.webkit.org/attachment.cgi?id=168096&action=review

thanks for the review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2726
>> +        if (child->renderer()) {
> 
> It looks like this adds an extra O(n^2) calculation even when none of the children have aria-hidden=false. You should avoid searching for the last render sibling is unless one of the children actually has aria-hidden=false, otherwise it's just wasted work.

Why do you think it's an extra O(n^2). It should be an extra O(n).

I could scan through the node list first and check if there are hidden nodes, in which case, i could run this algorithm

or I could scan through the node list and then if i found a hidden node, i could try to scan backwards in the sibling list to find the last renderer node that was inserted, but it gets tricky because I also have to account for other hidden nodes that were inserted.

>> Source/WebCore/accessibility/AXObjectCache.cpp:326
>> +    if (!inCanvasSubtree && !isHidden)
> 
> Won't this start creating accessibility objects for things that are hidden but NOT aria-hidden=false? I don't think we want to do that.

This allows you to create hidden nodes. The call sites are already checking for aria-hidden, but I could put in another aria-hidden check here as well just to make sure

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:228
>> +    // So for now, let's return the position of the ancestor that does have a position,
> 
> I like this idea, but I think it'd be better to give it the size of the ancestor, too. I think that's more likely to be correct for a screen magnifier.
> 
> For example, if an element within a canvas has focus, then in the absence of any other information I think the magnifier should zoom to the whole canvas, not the upper-left corner.

So if the parent was the <body> element the size would be the same size of the whole webpage with that case.

We could conditionalize so that if the parent wasn't the body, we could take the parent's size. what do you think

[Dominic Mazzoni]

Comment on attachment 168096 [details]
patch 

View in context: https://bugs.webkit.org/attachment.cgi?id=168096&action=review

>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2726
>>> +        if (child->renderer()) {
>> 
>> It looks like this adds an extra O(n^2) calculation even when none of the children have aria-hidden=false. You should avoid searching for the last render sibling is unless one of the children actually has aria-hidden=false, otherwise it's just wasted work.
> 
> Why do you think it's an extra O(n^2). It should be an extra O(n).
> 
> I could scan through the node list first and check if there are hidden nodes, in which case, i could run this algorithm
> 
> or I could scan through the node list and then if i found a hidden node, i could try to scan backwards in the sibling list to find the last renderer node that was inserted, but it gets tricky because I also have to account for other hidden nodes that were inserted.

I think m_children.find(childRenderer) is O(n), so the whole loop is O(n^2). Let me know if that's wrong.

This will be rare. I don't think it matters much if it's slow for the few cases where it does apply. So I think that your first idea is fine - do a quick scan for hidden nodes and quit early if there aren't any.

>>> Source/WebCore/accessibility/AXObjectCache.cpp:326
>>> +    if (!inCanvasSubtree && !isHidden)
>> 
>> Won't this start creating accessibility objects for things that are hidden but NOT aria-hidden=false? I don't think we want to do that.
> 
> This allows you to create hidden nodes. The call sites are already checking for aria-hidden, but I could put in another aria-hidden check here as well just to make sure

I couldn't recall. If it's up to the callers, let's make the check more explicit (check for aria-hidden), but put both checks (canvas and aria-hidden) inside a #define that only runs in debug mode

>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:228
>>> +    // So for now, let's return the position of the ancestor that does have a position,
>> 
>> I like this idea, but I think it'd be better to give it the size of the ancestor, too. I think that's more likely to be correct for a screen magnifier.
>> 
>> For example, if an element within a canvas has focus, then in the absence of any other information I think the magnifier should zoom to the whole canvas, not the upper-left corner.
> 
> So if the parent was the <body> element the size would be the same size of the whole webpage with that case.
> 
> We could conditionalize so that if the parent wasn't the body, we could take the parent's size. what do you think

Would it be so bad if the size was the whole webpage? In VoiceOver it'd be the same as if you stop interacting with the html content. The visual representation of that "hidden" element could be anywhere in the document, so I think the whole document makes sense.

Alternative idea: what about making it the width of the parent, but only up to some maximum height? That'd look visually like it's something at the top of the container.

[chris fleizach]

Comment on attachment 168096 [details]
patch 

View in context: https://bugs.webkit.org/attachment.cgi?id=168096&action=review

>>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2726
>>>> +        if (child->renderer()) {
>>> 
>>> It looks like this adds an extra O(n^2) calculation even when none of the children have aria-hidden=false. You should avoid searching for the last render sibling is unless one of the children actually has aria-hidden=false, otherwise it's just wasted work.
>> 
>> Why do you think it's an extra O(n^2). It should be an extra O(n).
>> 
>> I could scan through the node list first and check if there are hidden nodes, in which case, i could run this algorithm
>> 
>> or I could scan through the node list and then if i found a hidden node, i could try to scan backwards in the sibling list to find the last renderer node that was inserted, but it gets tricky because I also have to account for other hidden nodes that were inserted.
> 
> I think m_children.find(childRenderer) is O(n), so the whole loop is O(n^2). Let me know if that's wrong.
> 
> This will be rare. I don't think it matters much if it's slow for the few cases where it does apply. So I think that your first idea is fine - do a quick scan for hidden nodes and quit early if there aren't any.

ah good call for the find method.

>>>> Source/WebCore/accessibility/AXObjectCache.cpp:326
>>>> +    if (!inCanvasSubtree && !isHidden)
>>> 
>>> Won't this start creating accessibility objects for things that are hidden but NOT aria-hidden=false? I don't think we want to do that.
>> 
>> This allows you to create hidden nodes. The call sites are already checking for aria-hidden, but I could put in another aria-hidden check here as well just to make sure
> 
> I couldn't recall. If it's up to the callers, let's make the check more explicit (check for aria-hidden), but put both checks (canvas and aria-hidden) inside a #define that only runs in debug mode

we can add an ASSERT and put in both checks

>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:228
>>>> +    // So for now, let's return the position of the ancestor that does have a position,
>>> 
>>> I like this idea, but I think it'd be better to give it the size of the ancestor, too. I think that's more likely to be correct for a screen magnifier.
>>> 
>>> For example, if an element within a canvas has focus, then in the absence of any other information I think the magnifier should zoom to the whole canvas, not the upper-left corner.
>> 
>> So if the parent was the <body> element the size would be the same size of the whole webpage with that case.
>> 
>> We could conditionalize so that if the parent wasn't the body, we could take the parent's size. what do you think
> 
> Would it be so bad if the size was the whole webpage? In VoiceOver it'd be the same as if you stop interacting with the html content. The visual representation of that "hidden" element could be anywhere in the document, so I think the whole document makes sense.
> 
> Alternative idea: what about making it the width of the parent, but only up to some maximum height? That'd look visually like it's something at the top of the container.

I like the idea of showing that they are a portion of the parent in size (rarely do children encompass the whole bounds of the parent). Were you thinking the height would change based on the size, or would it be more static

[Dominic Mazzoni]

Comment on attachment 168096 [details]
patch 

View in context: https://bugs.webkit.org/attachment.cgi?id=168096&action=review

>>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:228
>>>>> +    // So for now, let's return the position of the ancestor that does have a position,
>>>> 
>>>> I like this idea, but I think it'd be better to give it the size of the ancestor, too. I think that's more likely to be correct for a screen magnifier.
>>>> 
>>>> For example, if an element within a canvas has focus, then in the absence of any other information I think the magnifier should zoom to the whole canvas, not the upper-left corner.
>>> 
>>> So if the parent was the <body> element the size would be the same size of the whole webpage with that case.
>>> 
>>> We could conditionalize so that if the parent wasn't the body, we could take the parent's size. what do you think
>> 
>> Would it be so bad if the size was the whole webpage? In VoiceOver it'd be the same as if you stop interacting with the html content. The visual representation of that "hidden" element could be anywhere in the document, so I think the whole document makes sense.
>> 
>> Alternative idea: what about making it the width of the parent, but only up to some maximum height? That'd look visually like it's something at the top of the container.
> 
> I like the idea of showing that they are a portion of the parent in size (rarely do children encompass the whole bounds of the parent). Were you thinking the height would change based on the size, or would it be more static

I don't think the height should change with size. I think it'd make sense if the height was roughly the height of a line of text - but no more than the height of the parent.

Let's try something like that and see how it looks.

[chris fleizach]

Created attachment 168490 [details]
patch

[chris fleizach]

Created attachment 168491 [details]
patch

[WebKit Review Bot]

Attachment 168491 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1
Source/WebCore/accessibility/AccessibilityNodeObject.cpp:236:  Use std::min() or std::min<type>() instead of the MIN() macro.  [runtime/max_min_macros] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 168491 [details]
patch 

Attachment 168491 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14294027

[Early Warning System Bot]

Comment on attachment 168491 [details]
patch 

Attachment 168491 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14288032

[chris fleizach]

Created attachment 168492 [details]
patch

[chris fleizach]

Created attachment 168494 [details]
patch

[chris fleizach]

Created attachment 168495 [details]
patch

[Dominic Mazzoni]

Comment on attachment 168495 [details]
patch 

Unofficial review - looks good, just one more possible technical issue and a few more stylistic suggestions.

View in context: https://bugs.webkit.org/attachment.cgi?id=168495&action=review

> LayoutTests/accessibility/aria-hidden-negates-no-visibility.html:24
> +    description("This tests ensures that aria-hidden=false will allow a node to be in the AX hierarchy even if its not rendered or visible");

Nit: its -> it's

> LayoutTests/ChangeLog:-849
> -2012-10-11  Emil A Eklund  <[e-mail]>

Accidental diff/merge error?

> Source/WebCore/accessibility/AXObjectCache.h:211
> +bool isNodeARIAHiddenFalse(Node*);

Maybe ARIA -> Aria? It's not 100% consistent, but it looks like Aria uses title case in WebKit method names for the most part.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1090
>      // Ignore invisible elements.

Maybe delete this comment now - it doesn't make sense anymore, and the comment below covers the non-obvious case.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2722
> +    // First do a quick run through to determine if we have any hidden nodes (most often we will not), so that.

Finish comment

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2734
> +    // Determine the index at which this item should be inserted. The goal is to place it as close to

Comment is confusing now after the refactoring. How about: iterate through all of the children, including those that may have already been added, and try to insert hidden nodes in the correct place in the DOM order.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2740
> +            AccessibilityObject* childRenderer = axObjectCache()->get(child->renderer());

childRenderer is a confusing variable name because it's an axobj, not a renderobj. Maybe childObj, or childAXObj or axChildObj, etc.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2744
> +                    childRenderer = children.last().get();

Isn't it possible that all of the elements of childRenderer->children() are ignored too? Maybe this should be a recursive helper function: getlastUnignoredAccessibilityObjectForRenderer. That name would make it more clear what you're trying to do here and you wouldn't need a comment.

(If I'm wrong and there's no way you could have multiple unignored levels, I still think a helper along those lines would be more clear.)

[chris fleizach]

(In reply to comment #21)
> (From update of attachment 168495 [details])
> Unofficial review - looks good, just one more possible technical issue and a few more stylistic suggestions.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=168495&action=review
> 
> > LayoutTests/accessibility/aria-hidden-negates-no-visibility.html:24
> > +    description("This tests ensures that aria-hidden=false will allow a node to be in the AX hierarchy even if its not rendered or visible");
> 
> Nit: its -> it's
> 
> > LayoutTests/ChangeLog:-849
> > -2012-10-11  Emil A Eklund  <[e-mail]>
> 
> Accidental diff/merge error?
> 
> > Source/WebCore/accessibility/AXObjectCache.h:211
> > +bool isNodeARIAHiddenFalse(Node*);
> 
> Maybe ARIA -> Aria? It's not 100% consistent, but it looks like Aria uses title case in WebKit method names for the most part.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1090
> >      // Ignore invisible elements.
> 
> Maybe delete this comment now - it doesn't make sense anymore, and the comment below covers the non-obvious case.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2722
> > +    // First do a quick run through to determine if we have any hidden nodes (most often we will not), so that.
> 
> Finish comment
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2734
> > +    // Determine the index at which this item should be inserted. The goal is to place it as close to
> 
> Comment is confusing now after the refactoring. How about: iterate through all of the children, including those that may have already been added, and try to insert hidden nodes in the correct place in the DOM order.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2740
> > +            AccessibilityObject* childRenderer = axObjectCache()->get(child->renderer());
> 
> childRenderer is a confusing variable name because it's an axobj, not a renderobj. Maybe childObj, or childAXObj or axChildObj, etc.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2744
> > +                    childRenderer = children.last().get();
> 
> Isn't it possible that all of the elements of childRenderer->children() are ignored too? Maybe this should be a recursive helper function: getlastUnignoredAccessibilityObjectForRenderer. That name would make it more clear what you're trying to do here and you wouldn't need a comment.
> 

In this case we're able to rely on the fact that children() contains only unignored descendants, so asking for the last one should always give you what you want. 

> (If I'm wrong and there's no way you could have multiple unignored levels, I still think a helper along those lines would be more clear.)

[chris fleizach]

Created attachment 168975 [details]
patch

[Build Bot]

Comment on attachment 168975 [details]
patch

Attachment 168975 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14396218

New failing tests:
accessibility/aria-hidden-negates-no-visibility.html

[chris fleizach]

Created attachment 169049 [details]
patch

[Beth Dakin]

Comment on attachment 169049 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169049&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:793
> +bool isNodeAriaHiddenFalse(Node* node)

It would be a bit nicer to name this isNodeAriaHidden() and then just test !isNodeAriaHidden() when you want to know if it is false.

[chris fleizach]

(In reply to comment #26)
> (From update of attachment 169049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169049&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:793
> > +bool isNodeAriaHiddenFalse(Node* node)
> 
> It would be a bit nicer to name this isNodeAriaHidden() and then just test !isNodeAriaHidden() when you want to know if it is false.

Will do. Thanks for the review

[chris fleizach]

http://trac.webkit.org/changeset/131915

[Csaba Osztrogonác]

Reverted r131915 for reason:

It broke the build on platforms with \!HAVE(ACCESSIBILITY)

Committed r131947: <http://trac.webkit.org/changeset/131947>

[Csaba Osztrogonác]

It broke the Qt build ( !HAVE(ACCESSIBILITY) platform), because
isNodeAriaVisible() is declared and used outside HAVE(ACCESSIBILITY)
ifdef guard, but its implementation is inside HAVE(ACCESSIBILITY) guard.

[chris fleizach]

(In reply to comment #30)
> It broke the Qt build ( !HAVE(ACCESSIBILITY) platform), because
> isNodeAriaVisible() is declared and used outside HAVE(ACCESSIBILITY)
> ifdef guard, but its implementation is inside HAVE(ACCESSIBILITY) guard.

Seems like you could have fixed that rather than rolling out.
I also suggest putting your builder in the EWS system if you want to avoid this kind of problem

[chris fleizach]

http://trac.webkit.org/changeset/131957

Committed again with fix for !HAVE(Accessibility)

[chris fleizach]

http://trac.webkit.org/changeset/131957

[chris fleizach]

Re-added layout tests
https://trac.webkit.org/changeset/160011