Bug 220534 – AX: expose focusable elements even if element or ancestor has aria-hidden=true

• Component: Accessibility
• Status: RESOLVED FIXED

[James Craig]

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.

[James Craig]

<rdar://problem/71865875>

[James Craig]

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

[chris fleizach]

Created attachment 418945 [details]
patch

[chris fleizach]

Created attachment 418964 [details]
patch

[chris fleizach]

Created attachment 418965 [details]
patch

[James Craig]

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.

[Andres Gonzalez]

--- 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?

[Andres Gonzalez]

--- 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?

[chris fleizach]

(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?

[Andres Gonzalez]

@@ -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.

[chris fleizach]

(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

[chris fleizach]

Created attachment 419241 [details]
patch

[Andres Gonzalez]

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.

[EWS]

commit-queue failed to commit attachment 419241 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

Committed r272390: <https://trac.webkit.org/changeset/272390>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419241 [details].