Bug 98787

Summary: AX: aria-hidden=false does not work as expected
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dmazzoni, ossy, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
webkit.review.bot: commit-queue-
patch
none
patch
webkit-ews: commit-queue-
patch
none
patch
none
patch
none
patch
buildbot: commit-queue-
patch bdakin: review+

Description chris fleizach 2012-10-09 09:23:06 PDT
* 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
Comment 1 chris fleizach 2012-10-09 09:23:21 PDT
rdar://12359052
Comment 2 Dominic Mazzoni 2012-10-09 09:28:22 PDT
Hopefully AccessibilityNodeObject should make it possible for us to support this case now.
Comment 3 chris fleizach 2012-10-09 09:28:59 PDT
(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
Comment 4 chris fleizach 2012-10-10 16:47:53 PDT
Created attachment 168093 [details]
patch
Comment 5 WebKit Review Bot 2012-10-10 16:49:52 PDT
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.
Comment 6 chris fleizach 2012-10-10 16:54:45 PDT
Created attachment 168096 [details]
patch
Comment 7 WebKit Review Bot 2012-10-10 21:40:41 PDT
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
Comment 8 Dominic Mazzoni 2012-10-10 22:40:41 PDT
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.
Comment 9 chris fleizach 2012-10-10 22:49:12 PDT
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
Comment 10 Dominic Mazzoni 2012-10-10 23:18:03 PDT
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.
Comment 11 chris fleizach 2012-10-10 23:27:56 PDT
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
Comment 12 Dominic Mazzoni 2012-10-10 23:36:38 PDT
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.
Comment 13 chris fleizach 2012-10-12 15:01:32 PDT
Created attachment 168490 [details]
patch
Comment 14 chris fleizach 2012-10-12 15:01:58 PDT
Created attachment 168491 [details]
patch
Comment 15 WebKit Review Bot 2012-10-12 15:05:05 PDT
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.
Comment 16 Early Warning System Bot 2012-10-12 15:12:01 PDT
Comment on attachment 168491 [details]
patch 

Attachment 168491 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14294027
Comment 17 Early Warning System Bot 2012-10-12 15:15:04 PDT
Comment on attachment 168491 [details]
patch 

Attachment 168491 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14288032
Comment 18 chris fleizach 2012-10-12 15:15:20 PDT
Created attachment 168492 [details]
patch
Comment 19 chris fleizach 2012-10-12 15:16:39 PDT
Created attachment 168494 [details]
patch
Comment 20 chris fleizach 2012-10-12 15:18:51 PDT
Created attachment 168495 [details]
patch
Comment 21 Dominic Mazzoni 2012-10-15 23:14:37 PDT
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  <eae@chromium.org>

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.)
Comment 22 chris fleizach 2012-10-16 10:48:36 PDT
(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  <eae@chromium.org>
> 
> 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.)
Comment 23 chris fleizach 2012-10-16 10:55:21 PDT
Created attachment 168975 [details]
patch
Comment 24 Build Bot 2012-10-16 11:32:39 PDT
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
Comment 25 chris fleizach 2012-10-16 16:13:40 PDT
Created attachment 169049 [details]
patch
Comment 26 Beth Dakin 2012-10-18 16:58:25 PDT
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.
Comment 27 chris fleizach 2012-10-19 09:57:19 PDT
(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
Comment 28 chris fleizach 2012-10-19 10:16:43 PDT
http://trac.webkit.org/changeset/131915
Comment 29 Csaba Osztrogon√°c 2012-10-19 14:59:34 PDT
Reverted r131915 for reason:

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

Committed r131947: <http://trac.webkit.org/changeset/131947>
Comment 30 Csaba Osztrogon√°c 2012-10-19 15:02:22 PDT
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.
Comment 31 chris fleizach 2012-10-19 15:45:53 PDT
(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
Comment 32 chris fleizach 2012-10-19 16:25:16 PDT
http://trac.webkit.org/changeset/131957

Committed again with fix for !HAVE(Accessibility)
Comment 33 chris fleizach 2012-10-23 17:39:19 PDT
http://trac.webkit.org/changeset/131957
Comment 34 chris fleizach 2013-12-03 09:38:03 PST
Re-added layout tests
https://trac.webkit.org/changeset/160011