Bug 112373 – AX: aria-hidden on container does not hide descendant popup buttons

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

There are a number of elements that do not hide themselves when a parent uses aria-hidden=true

These includes

image-map links
media controls
popup menus
list boxes
range elements
date/time elements
progress

They are all subclasses and were only doing accessibilityIsIgnored() == NO, but they need to inherit from a base reference that handles aria-hidden.

[chris fleizach]

Created attachment 193166 [details]
patch

[chris fleizach]

Adding Tim to help with review

[WebKit Review Bot]

Comment on attachment 193166 [details]
patch

Attachment 193166 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17008991

New failing tests:
accessibility/aria-hidden-hides-all-elements.html
fast/js/dfg-inline-resolve.html

[chris fleizach]

any idea why this would fail cr-linux but not the other cr bots?
any idea how to get what the diff is for the failing test?

[Tim Horton]

(In reply to comment #4)
> any idea why this would fail cr-linux but not the other cr bots?

Of the chromium bots, only cr-linux runs tests.

> any idea how to get what the diff is for the failing test?

Politely ask a chromium engineer, or build the chromium port? I don't think there's a good way right now.

[Dominic Mazzoni]

The reason for the Chromium failure is because accessibilityPlatformIncludesObject returns IncludeObject for a MenuListPopup or MenuListOpen, and AccessibilityMenuList::addChildren only calls accessibilityPlatformIncludesObject to decide whether or not to add the child, rather than calling accessibilityIsIgnored

Have AccessibilityMenuList::addChildren call accessibilityIsIgnored instead, that fixes it. To make that work you'll have to move the setParent call up before that line, otherwise accessibilityIsIgnored can't walk up the parent hierarchy.

Otherwise this patch looks great, it's a good bug fix and a good code cleanup, too.

[chris fleizach]

Created attachment 193345 [details]
patch

Thanks a lot for helping with this one Dom

[WebKit Review Bot]

Comment on attachment 193345 [details]
patch

Attachment 193345 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17122574

New failing tests:
accessibility/insert-selected-option-into-select-causes-crash.html
accessibility/canvas-accessibilitynodeobject.html
http/tests/navigation/post-goback2.html
http/tests/cache/subresource-failover-to-network.html
fast/css/font-family-pictograph.html
fast/dom/option-properties.html
http/tests/navigation/success200-loadsame.html
http/tests/navigation/relativeanchor-goback.html
http/tests/navigation/success200-reload.html
http/tests/navigation/timerredirect-goback.html
http/tests/navigation/restore-form-state-https.html
accessibility/duplicate-axrenderobject-crash.html
accessibility/aria-hidden-hides-all-elements.html
http/tests/navigation/metaredirect-goback.html
accessibility/menu-list-sends-change-notification.html
http/tests/navigation/postredirect-frames.html
http/tests/navigation/success200-goback.html
http/tests/navigation/postredirect-goback1.html
http/tests/inspector/console-cd-completions.html
http/tests/navigation/javascriptlink-goback.html
http/tests/navigation/success200-frames-loadsame.html
http/tests/navigation/postredirect-goback2.html
http/tests/navigation/redirect302-goback.html
fast/dom/HTMLSelectElement/selected-false.html
http/tests/inspector/console-cd.html
http/tests/navigation/postredirect-basic.html
fast/dom/HTMLSelectElement/select-selectedIndex-bug-12942.html
http/tests/navigation/anchor-goback.html
fast/dom/HTMLSelectElement/selected-index-preserved-when-option-text-changes.html

[Build Bot]

Comment on attachment 193345 [details]
patch

Attachment 193345 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17215271

New failing tests:
accessibility/insert-selected-option-into-select-causes-crash.html
platform/mac/accessibility/popup-button-exposes-axvalue.html
accessibility/aria-hidden-hides-all-elements.html
accessibility/canvas-accessibilitynodeobject.html
platform/mac/accessibility/role-subrole-roledescription.html
accessibility/duplicate-axrenderobject-crash.html

[Ryosuke Niwa]

It seems like a whole bunch of accessibility tests are failing?

[chris fleizach]

Created attachment 193404 [details]
patch

forgot to set the parent early like Dominic suggested

[Ryosuke Niwa]

Comment on attachment 193404 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1870
> +bool AccessibilityObject::accessibilityIsIgnoredDefaultForObject() const

I would call this accessibilityIsIgnoredByDefault(). In particular, ForObject doesn't seem to add any value.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1878
> +    AccessibilityObjectInclusion decision = accessibilityIsIgnoredBase();
> +    if (decision == IncludeObject)
> +        return false;
> +    if (decision == IgnoreObject)
> +        return true;
> +    
> +    return false;

It seems like we can replace all of this by:
return accessibilityIsIgnoredBase() == IgnoreObject.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1886
> +    // aria-hidden hides this object and any children.

I think this comment is more confusing than helpful. Remove?

> Source/WebCore/accessibility/AccessibilityObject.cpp:1895
> +AccessibilityObjectInclusion AccessibilityObject::accessibilityIsIgnoredBase() const

I don't think Base is a descriptive adjective here. It's also misleading to call this function accessibilityIsIgnoredBase given it returns AccessibilityObjectInclusion.
How about something like defaultObjectInclusion() ?

> Source/WebCore/accessibility/AccessibilityObject.cpp:1897
> +    // Anything marked as aria-hidden or a child of something aria-hidden must be hidden.

This comment repeats the code. Please remove it.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1901
> +    // Anything that is a presentational role must be hidden.

Ditto.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1912
> +    // Allow the platform to make a decision.
> +    AccessibilityObjectInclusion decision = accessibilityPlatformIncludesObject();
> +    if (decision == IncludeObject)
> +        return IncludeObject;
> +    if (decision == IgnoreObject)
> +        return IgnoreObject;
> +    
> +    return DefaultBehavior;   

Why don't we just do return accessibilityPlatformIncludesObject() instead?

> LayoutTests/accessibility/aria-hidden-hides-all-elements.html:15
> +
> +<div id="content" role="group">
> +
> +<main id="main" aria-hidden="true">
> +
> +<video controls="controls"></video>
> +
> +<select id="select"><option>a</select>
> +

So many blank lines!

[chris fleizach]

Created attachment 193425 [details]
patch

patch to address ryosuke's comments

Thanks for the feedback. All good ideas.

[Ryosuke Niwa]

Comment on attachment 193425 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1898
> +    

OMG, all three functions look so much simpler now.

[WebKit Review Bot]

Comment on attachment 193425 [details]
patch

Clearing flags on attachment: 193425

Committed r145988: <http://trac.webkit.org/changeset/145988>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.