Bug 125592 – AX: make aria-hidden=false work with subtrees

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

All the examples from

http://www.html5accessibility.com/tests/hidden2013.html

should work.

Right now, cases 5 and 6 fail
<p id="e" hidden aria-hidden="false">test html5 hidden</p>
<p id="f" style="display:none" aria-hidden="false">test display:none</p>

Because aria-hidden=false is only applying to the element itself, not the sub-tree below itself

[chris fleizach]

<rdar://problem/15574925>

[chris fleizach]

Created attachment 218984 [details]
patch

[Ian 'Hixie' Hickson]

*** Bug 125591 has been marked as a duplicate of this bug. ***

[Ian 'Hixie' Hickson]

Note that making aria-hidden override CSS display:none is controversial over in Mozilla land:
   https://bugzilla.mozilla.org/show_bug.cgi?id=945194

[Mario Sanchez Prada]

Comment on attachment 218984 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:980
> +    for (Node* testNode = node; testNode; testNode = testNode->parentNode()) {

Maybe a small comment here indicating that you are doing this to be able to work with subtrees would be helpful

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:417
> +        if (m_node->parentNode() && m_node->parentNode()->hasTagName(iframeTag) && m_node->parentNode()->renderer())
> +            return true;

Perhaps it would be useful to add a check in the layout test for this situation with iframes, given that you are adding this explicit check here

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:420
> +        // Whitespace only text elements should be ignored when they have no renderer.
> +        String string = stringValue().stripWhiteSpace().simplifyWhiteSpace();

Shouldn't you be checking that the object has no renderer, as you mention in the comment?

> LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:22
> +

Extra line

> LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:23
> +    description("This tests that setting a role on an img still allows the alt attribute to be used for the description..");

It looks like this description is not the right one

> LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:39
> +

Extra line

[chris fleizach]

Created attachment 219219 [details]
patch

[Build Bot]

Comment on attachment 219219 [details]
patch

Attachment 219219 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/49108065

New failing tests:
accessibility/aria-hidden-false-works-in-subtrees.html

[Build Bot]

Created attachment 219244 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[chris fleizach]

Created attachment 219430 [details]
patch

[Mario Sanchez Prada]

Comment on attachment 219430 [details]
patch

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

Lgtm. I've just have a suggestion to replace shouldBe() with debug(), see it below...

> LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:35
> +        shouldBe("iframeChild.role", "'AXRole: AXGroup'");

Could you replace this shouldBe() with a debug() line before landing, to make it more cross-platform friendly?

[chris fleizach]

(In reply to comment #10)
> (From update of attachment 219430 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219430&action=review
> 
> Lgtm. I've just have a suggestion to replace shouldBe() with debug(), see it below...
> 
> > LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html:35
> > +        shouldBe("iframeChild.role", "'AXRole: AXGroup'");
> 
> Could you replace this shouldBe() with a debug() line before landing, to make it more cross-platform friendly?

yep no problem

[chris fleizach]

https://trac.webkit.org/changeset/160789