Summary: | AX: expose focusable elements even if element or ancestor has aria-hidden=true | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jdiggs, samuel_white, webkit-bug-importer, zalan | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=221646 | ||||||||||||
Attachments: |
|
Description
James Craig
2021-01-11 23:02:03 PST
FYI, the spec change was already approved and merged into ARIA 1.2, but an objection was raised today. Okay to post the patch if done/close, but please hold off on merging. Sorry. https://github.com/w3c/aria/issues/1381 Created attachment 418945 [details]
patch
Created attachment 418964 [details]
patch
Created attachment 418965 [details]
patch
Comment on attachment 418965 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=418965&action=review > LayoutTests/accessibility/focusable-inside-hidden.html:24 > + document.getElementById("button").focus(); It may also be a good test to .blur() the element and then verify it's no longer accessible after focus is lost. --- a/LayoutTests/accessibility/focusable-inside-hidden.html +++ a/LayoutTests/accessibility/focusable-inside-hidden.html + description("This tests that a focusable object inside an aria-hidden is still visible"); I think it is important to notice that is visible after it gains focus. That's what this test is showing. The button element is hidden before it gains focus, even though it is always focusable. + <div role="button">hidden button</div> Not used in the test. Should it be removed? --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp return Accessibility::findAncestor<AccessibilityObject>(*this, true, [] (const AccessibilityObject& object) { - return equalLettersIgnoringASCIICase(object.getAttribute(aria_hiddenAttr), "true"); + return equalLettersIgnoringASCIICase(object.getAttribute(aria_hiddenAttr), "true") && !object.isFocused(); }) != nullptr; For the following case: <A hidden not focused> <B focused> <C /> </B> </A> C would be hidden based on above logic. But C may be focused as part of B. Is this the expected behavior? (In reply to Andres Gonzalez from comment #8) > --- a/Source/WebCore/accessibility/AccessibilityObject.cpp > +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp > > return Accessibility::findAncestor<AccessibilityObject>(*this, true, [] > (const AccessibilityObject& object) { > - return > equalLettersIgnoringASCIICase(object.getAttribute(aria_hiddenAttr), "true"); > + return > equalLettersIgnoringASCIICase(object.getAttribute(aria_hiddenAttr), "true") > && !object.isFocused(); > }) != nullptr; > > For the following case: > > <A hidden not focused> > <B focused> > <C /> > </B> > </A> > > C would be hidden based on above logic. But C may be focused as part of B. > Is this the expected behavior? I am not sure. @JamesCraig? @@ -3567,7 +3570,7 @@ void AccessibilityObject::setIsIgnoredFromParentDataForChild(AXCoreObject* child AccessibilityIsIgnoredFromParentData result = AccessibilityIsIgnoredFromParentData(this); if (!m_isIgnoredFromParentData.isNull()) { - result.isAXHidden = m_isIgnoredFromParentData.isAXHidden || equalLettersIgnoringASCIICase(child->getAttribute(aria_hiddenAttr), "true"); + result.isAXHidden = (m_isIgnoredFromParentData.isAXHidden || equalLettersIgnoringASCIICase(child->getAttribute(aria_hiddenAttr), "true")) && !child->isFocused(); Not strictly related to this change, but it seems strange that we are consulting the child to set a flag that should depends only on its ancestors, as the name of the method indicates. (In reply to Andres Gonzalez from comment #10) > @@ -3567,7 +3570,7 @@ void > AccessibilityObject::setIsIgnoredFromParentDataForChild(AXCoreObject* child > > AccessibilityIsIgnoredFromParentData result = > AccessibilityIsIgnoredFromParentData(this); > if (!m_isIgnoredFromParentData.isNull()) { > - result.isAXHidden = m_isIgnoredFromParentData.isAXHidden || > equalLettersIgnoringASCIICase(child->getAttribute(aria_hiddenAttr), "true"); > + result.isAXHidden = (m_isIgnoredFromParentData.isAXHidden || > equalLettersIgnoringASCIICase(child->getAttribute(aria_hiddenAttr), "true")) > && !child->isFocused(); > > Not strictly related to this change, but it seems strange that we are > consulting the child to set a flag that should depends only on its > ancestors, as the name of the method indicates. Right - looks like it might just have been a convenient place to calculate this Created attachment 419241 [details]
patch
Looks good! We can go ahead with this improvement, and will open a separate bug if needed for the nested case pointed out above when we get more clarification from the standards group. commit-queue failed to commit attachment 419241 [details] to WebKit repository. To retry, please set cq+ flag again.
Committed r272390: <https://trac.webkit.org/changeset/272390> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419241 [details]. |