Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive in aria-required-children "unknown" path for child not in accessibility tree with aria-hidden="true" #3850

Closed
1 task done
dbjorge opened this issue Jan 7, 2023 · 21 comments · Fixed by #3949
Labels
fix Bug fixes good first issue For first-time contributors pr A pr has been created for the issue rules Issue or false result from an axe-core rule
Milestone

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Jan 7, 2023

Product

axe-core

Product Version

4.6.2

Latest Version

  • I have tested the issue with the latest version of the product

Issue Description

Expectation

The following pattern should not result in an aria-required-children failure:

<div role="menu">
  <div role="menuitem">menu item</div>
  <div aria-hidden="true">shouldn't be flagged but is</div>
</div>

Actual

This results in a failure with the following failureSummary:

Fix any of the following:
  Element has children which are not allowed (see related nodes)
  Element has no aria-busy="true" attribute

How to Reproduce

Minimal repro (+ an example where the same case without aria-hidden="true" does not trigger the failure)

Additional context

Per #3462, the "unknown" failure path introduced in aria-required-children in #3597 attempts to detect cases similar to this ACT rule's "failed example 3", where a child in the accessibility tree (emphasis mine) does not have a role that matches one of the requiredOwned roles for the parent.

However, we think the logic introduced in #3597 to determine whether a child is "in the accessibility tree" for the purposes of this rule is a bit off - it does so by asking "does the child have any global ARIA attribute OR is it focusable". However, aria-hidden is a global ARIA attribute which currently triggers the first clause of this check, which is inconsistent with the rule. Per the glossary of the ACT rule in question, an element which is programmatically hidden should be treated as "not included in the accessibility tree" for the purposes of this check:

Included in the accessibility tree

Programmatically hidden elements are removed from the accessibility tree. However, some browsers will leave focusable elements with an aria-hidden attribute set to true in the accessibility tree. Because they are hidden, these elements are considered not included in the accessibility tree. This may cause confusion for users of assistive technologies because they may still be able to interact with these focusable elements using sequential keyboard navigation, even though the element should not be included in the accessibility tree.

Right now, even a non-focusable child whose only global ARIA attribute is aria-hidden="true" causes a failure, which I think is pretty firmly a false positive. The wording of the ACT rule as written suggests that a programmatically hidden child should not trigger a failure even if it is focusable - I think it would be reasonable to flag that as a best-practice level warning anyway, but I think the non-focusable case clearly shouldn't fail.

@dbjorge dbjorge added the ungroomed Ticket needs a maintainer to prioritize and label label Jan 7, 2023
@dylanb
Copy link
Contributor

dylanb commented Jan 7, 2023

@dbjorge I believe these should also have role="presentation" on them

Have you tested to ensure that something like

<div role="menu">
  <div role="menuitem">menu item Dan</div>
  <div aria-hidden="true">shouldn't be flagged but is</div>
  <div role="menuitem">menu item Dylan</div>
  <div aria-hidden="true">shouldn't be flagged but is</div>
  <div role="menuitem">menu item John</div>
</div>

Still reads all the ordinals and counts for the items correctly in all ATs? Also add the role="presentation" and test that.

We have seen interspersed items like this without a presentational role can throw off counts and reading of positional ordinals

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 9, 2023

Thanks @dylanb , that's a good point. I've added a few test scenarios to the original repro codepen and tested them in the following AT/OS/Browser combinations:

- E3 (menu, no role=pres) E4 (list, no role=pres) E5 (menu, with role=pres) E6 (list, with role=pres)
NVDA 2022.4, Windows 11, Edge 108 ✓ does not announce cardinality ✓ "list of 3 items" ✓ does not announce cardinality ✓ "list of 3 items"
NVDA 2022.4, Windows 11, Firefox 108 ✓ does not announce cardinality ✓ "list of 3 items" ✓ does not announce cardinality ✓ "list of 3 items"
JAWS 2023.2212.23, Windows 11, Edge 108 ✓ does not announce cardinality ✓ "list of 3 items" ✓ does not announce cardinality ✓ "list of 3 items"
VoiceOver, MacOS 13, Safari 16.1 ✓ "menu 3 items" ✓ "list 3 items" ✓ "menu 3 items" ✓ "list 3 items"
Talkback, Android 13, Chrome 108 ✓ does not announce cardinality ✓ "3 items" ✓ does not announce cardinality ✓ "3 items"

This looks consistent with reporting this case as a false positive to me, though it would be good if someone could test against VO+iOS+Safari (I don't have an iPhone to test that case with).

@dylanb
Copy link
Contributor

dylanb commented Jan 9, 2023

@dbjorge when you remove the item in question and just have the menu items, what are the announcements?

Also, when you cycle through the items does it announce one of three, two of three and three of three correctly?

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 9, 2023

@dbjorge when you remove the item in question and just have the menu items, what are the announcements?

I added new cases E7 (menu) and E8 (list) for this. In all 5 AT combinations I tested, the announcements were identical for these cases vs E3/E5 and E4/E6 respectively (with the exception that VoiceOver will read aria-hidden things in some specific navigation modes (#161740), which isn't specific to these scenarios).

Also, when you cycle through the items does it announce one of three, two of three and three of three correctly?

  • NVDA and JAWS do not make X of Y announcements for any of these scenarios (E3-E8), regardless of the presence of hidden elements.
  • VoiceOver announces one of three/etc correctly in all scenarios (E3-E8).
  • Talkback announces one of three/etc correctly in all list scenarios (E4, E6, E8) and does not make X of Y announcements for any menu case (E3, E5, E7) regardless of the presence of hidden elements.

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 9, 2023

And, again, I haven't tested with VoiceOver on iOS (I don't have a test device for it). It would be good to verify that the behavior is consistent there before making a decision about the rule logic.

@WilcoFiers WilcoFiers added fix Bug fixes rules Issue or false result from an axe-core rule and removed ungroomed Ticket needs a maintainer to prioritize and label labels Jan 10, 2023
@WilcoFiers WilcoFiers added this to the Axe-core 4.7 milestone Jan 10, 2023
@WilcoFiers WilcoFiers added the good first issue For first-time contributors label Jan 11, 2023
@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 11, 2023

I've also gone ahead and added E9 and E10, which use siblings which are both aria-hidden="true" and also tabindex="0". These results were also announced the same as the other menu/list cases, and the cases that normally announce cardinalities still did so as if there were 3 items, ignoring the hidden+focusable items.

Assuming iOS Safari works similarly, this makes me think that the correct fix would be to follow the ACT rule as written and ignore all programmatically hidden owned/child items from being considered as causing failures of this "unallowed" path, even if they are focusable. A hidden+focusable item will separately cause an aria-hidden-focus failure, but I don't think it appears to cause any additional problems with the list/menu readouts beyond what aria-hidden-focus already covers.

@WilcoFiers / @dylanb , can you confirm that this matches your expectation of what a fix should look like?

@WilcoFiers
Copy link
Contributor

@dbjorge Yeah, that sounds right. Let aria-hidden-focus do its thing. The one little thing to also keep in mind is that we should make this work with inert too. Instead of looking for aria-hidden on the children, best to use isVisibleToScreenReaders to determine what to ignore.

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 18, 2023

@dbjorge Yeah, that sounds right. Let aria-hidden-focus do its thing. The one little thing to also keep in mind is that we should make this work with inert too. Instead of looking for aria-hidden on the children, best to use isVisibleToScreenReaders to determine what to ignore.

Perfect, thanks! Definitely agree with using isVisibleToScreenReaders, just yesterday I helped diagnose an instance of this false positive that involved display: none instead of aria-hidden.

@WilcoFiers WilcoFiers assigned WilcoFiers and unassigned WilcoFiers Jan 24, 2023
@smhigley
Copy link
Contributor

Just ran into this false positive in the wild as well, +1 to ignoring nodes not matching isVisibleToScreenReaders.

@dbjorge I do not get the false positive with display: none, so I think that might be an unrelated error.

@dbjorge
Copy link
Contributor Author

dbjorge commented Jan 26, 2023

@smhigley display: none isn't enough to trigger the rule on its own like aria-hidden does, but a child with both display: none and either a global ARIA attribute or a non-required-owned role attribute will still trigger the issue even though such a child should be ignored as "not visible to screen readers". I've added E11 to the codepen repro demonstrating this.

@molily
Copy link

molily commented Mar 9, 2023

Hi, I get the same error message for this code:

<svg role="presentation" >
  <g role="table">
    <text x="10" y="10" aria-hidden="true" role="presentation">
      visual text with aria-hidden="true"
    </text>
    <g transform="translate(0, 50)">
      <g role="row">
        <text role="columnheader" x="0">columnheader</text>
        <text role="columnheader" x="200">columnheader</text>
        <text role="columnheader" x="400">columnheader</text>
      </g>
    </g>
    <g transform="translate(0, 100)">
      <g role="row">
        <text role="cell" x="0">cell</text>
        <text role="cell" x="200">cell</text>
        <text role="cell" x="400">cell</text>
      </g>
    </g>
  </g>
</svg>

axe violation: aria-required-children on <g role="table">: Element has children which are not allowed (see related nodes), the text with aria-hidden being the related node.

The code works as expected in VoiceOver + Safari, JAWS + Chrome and NVDA + Edge. The text with aria-hidden is neither in the accessibility tree, nor focussable. Background: Accessible SVG line graphs

Can somebody please confirm that this the same bug? Thank you very much.

@veselints
Copy link

One more, a bit odd, scenario with listbox and options:

<div role="listbox" aria-label="Hour">
  <div role="presentation" tabindex="-1">
    <ul role="presentation">
      <li role="option" aria-selected="true">
        <span>12</span>
      </li>
      <li role="option" aria-selected="false">
        <span>1</span>
      </li>
    </ul>
  </div>
</div>

aria-required-children would not fail if the tabindex=-1 is not present on the <div role="presentation" element.

@straker
Copy link
Contributor

straker commented Mar 22, 2023

@veselints I believe the tabindex=-1 is triggering the violation. This is because a focusable element cannot be presentational, so we count the element as a child for this rule.

@straker
Copy link
Contributor

straker commented Mar 22, 2023

@molily Apologies, I seemed to have missed your post. I believe your example is the same issue and the fix to exclude aria-hidden elements should fix your example as well.

@veselints
Copy link

@straker but tabindex=-1 means the element is not focusable. I know it is redundant in this case, as the <div> is not focusable anyways, but it does not make the element focusable. The check for focusable elements with role=presentation should look for elements with attribute tabindex that has value equal to or greater than zero. It should ignore negative tabindex values, as that does not make the element focusable.

@straker
Copy link
Contributor

straker commented Mar 22, 2023

@veselints elements with tabindex=-1 are not focusable with the tab key, but they are programmatically focusable though javascript with el.focus(). Because of this they are still considered focusable for screen reader purposes.

@veselints
Copy link

@straker thank you for that clarification! In that case, the tabindex must be completely removed from the element.

@padmavemulapati
Copy link

Validated with the latest axe-core develop branch code base, aria-required-children not failing for children with aria-hidden=true
The below code snippet:

<ul>
  <div aria-hidden="true">foo</div>
  <li>bar</li>
</ul>

passes the list rule, and

<dl>
  <span aria-hidden="true">foo</span>
  <dt>bar</dt>
  <dd>baz</dd>
</dl>

passes the definition-list rule.

Image

codeofdusk added a commit to codeofdusk/accessibility-insights-web that referenced this issue Jul 29, 2023
codeofdusk added a commit to codeofdusk/accessibility-insights-web that referenced this issue Jul 29, 2023
codeofdusk added a commit to codeofdusk/accessibility-insights-web that referenced this issue Jul 30, 2023
codeofdusk added a commit to codeofdusk/accessibility-insights-web that referenced this issue Jul 31, 2023
@clamli
Copy link

clamli commented Aug 1, 2023

Hi @straker, noticed that the aria-required-children violation is still being triggered for the code snippet below, even after the fix was applied.

<div role="listbox">
  <div>
    <div aria-hidden="true"></div>
  </div>
</div>

However, the rule will pass if the aria-hidden="true" is changed to role="presentation".

  1. Why is it a aria-required-children violation if the inner element is already hidden?
  2. Since role="presentation" hides the semantics of the element but still exposes it to assistive technology whereas aria-hidden="true" hides the element entirely, should we silence the rule for this aria-hidden scenario too?

@WilcoFiers
Copy link
Contributor

@clamli While the issue is close, the fix hasn't been released. You can test this out by installing axe-core@next from NPM (or waiting until axe-core 4.8 is released, targeted next week).

If I run your listbox code above I get it back as "incomplete" ("needs review" in the extension). That's because the listbox has no options. If you add an option it passes:

<div role="listbox" aria-label="Hello">
  <div>
    <div aria-hidden="true"></div>
  </div>
  <div role="option" aria-selected="true">World</div>
</div>

@clamli
Copy link

clamli commented Aug 1, 2023

@WilcoFiers, thanks for answering, that makes sense! One more question, why does it pass if we change aria-hidden="true" to role="presentation" even if there is no option in the listbox? Should we make them consistent?

codeofdusk added a commit to microsoft/accessibility-insights-web that referenced this issue Aug 1, 2023
…description, and promote scrollable-region-focusable (#6849)

This commit updates axe-core to its latest version, 4.7.2, from
4.6.3. It also
bumps the accessibility-insights-report package version. As part of the
axe-core update:

* The `aria-roledescription` rule has been deprecated
(dequelabs/axe-core#3649).
* `scrollable-region-focusable` axe-core rule failures have been
promoted from "needs review" to "serious" (manually removed from
explicit "needs review" but included automatically by axe-core), see
dequelabs/axe-core#3862 and dequelabs/axe-core#3939.
* `falsePositiveRemoval` introduced in
ed75fda has been removed as the
corresponding dequelabs/axe-core#3850 has been resolved.

Part of Feature 2086120 (internal access required to view).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fixes good first issue For first-time contributors pr A pr has been created for the issue rules Issue or false result from an axe-core rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants