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.
Created attachment 193166 [details] patch
Adding Tim to help with review
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
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?
(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.
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.
Created attachment 193345 [details] patch Thanks a lot for helping with this one Dom
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
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
It seems like a whole bunch of accessibility tests are failing?
Created attachment 193404 [details] patch forgot to set the parent early like Dominic suggested
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!
Created attachment 193425 [details] patch patch to address ryosuke's comments Thanks for the feedback. All good ideas.
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.
Comment on attachment 193425 [details] patch Clearing flags on attachment: 193425 Committed r145988: <http://trac.webkit.org/changeset/145988>
All reviewed patches have been landed. Closing bug.