The AX tree inclusion section of ARIA now includes this line: The accessibility tree inclusion section includes this line: > [include in the accessibility tree…] "Elements that are focusable even if the element or one of its ancestor elements has its aria-hidden attribute set to true." https://w3c.github.io/aria/#tree_inclusion The ARIA WG discussed the "focusable" vs "expose only when focused" difference and came to the consensus that there were too many instances where authors did hidden focusable elements accidentally (including one recent report on the Benevity site). So as I understand it, the way to hide focusable elements would be to make not rendered, or explicitly not focusable. Visible, but explicitly not focusable <button tabindex=“-1" aria-hidden=“true"> Not rendered, and therefore implicitly not focusable. <button style=“display:none;"> Also, once WebKit implements @inert, that would also be sufficient: <div inert> <button aria-hidden=“true">this is inert, so not focusable among other things.. The aria-hidden is likely redundant here.
<rdar://problem/71865875>
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].