Bug 220534 - AX: expose focusable elements even if element or ancestor has aria-hidden=true
Summary: AX: expose focusable elements even if element or ancestor has aria-hidden=true
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-11 23:02 PST by James Craig
Modified: 2021-02-09 19:24 PST (History)
10 users (show)

See Also:


Attachments
patch (6.27 KB, patch)
2021-02-01 17:34 PST, chris fleizach
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (6.31 KB, patch)
2021-02-01 23:14 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (6.23 KB, patch)
2021-02-01 23:15 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (8.76 KB, patch)
2021-02-03 23:19 PST, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2021-01-11 23:02:03 PST
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.
Comment 1 James Craig 2021-01-11 23:02:29 PST
<rdar://problem/71865875>
Comment 2 James Craig 2021-01-14 10:59:31 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
Comment 3 chris fleizach 2021-02-01 17:34:08 PST
Created attachment 418945 [details]
patch
Comment 4 chris fleizach 2021-02-01 23:14:28 PST
Created attachment 418964 [details]
patch
Comment 5 chris fleizach 2021-02-01 23:15:12 PST
Created attachment 418965 [details]
patch
Comment 6 James Craig 2021-02-03 05:40:07 PST
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.
Comment 7 Andres Gonzalez 2021-02-03 11:28:46 PST
--- 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?
Comment 8 Andres Gonzalez 2021-02-03 12:07:32 PST
--- 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?
Comment 9 chris fleizach 2021-02-03 12:08:26 PST
(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?
Comment 10 Andres Gonzalez 2021-02-03 12:20:06 PST
@@ -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.
Comment 11 chris fleizach 2021-02-03 23:16:33 PST
(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
Comment 12 chris fleizach 2021-02-03 23:19:40 PST
Created attachment 419241 [details]
patch
Comment 13 Andres Gonzalez 2021-02-04 13:45:46 PST
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.
Comment 14 EWS 2021-02-04 14:16:45 PST
commit-queue failed to commit attachment 419241 [details] to WebKit repository. To retry, please set cq+ flag again.
Comment 15 EWS 2021-02-04 14:33:16 PST
Committed r272390: <https://trac.webkit.org/changeset/272390>

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