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

Removing invalid attribute #2654

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Removing invalid attribute #2654

wants to merge 2 commits into from

Conversation

drakej
Copy link

@drakej drakej commented Nov 23, 2022

Signed-off-by: Jonathan Drake drakej@drakej.com

  • Bug fix
  • New feature
  • Chore
  • Breaking change
  • There is an open issue which this change addresses
  • I have read the CONTRIBUTING document.
  • My commits follow the Git Commit Guidelines
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • My change requires a change to Typescript typings.
    • I have updated the typings accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Fixes issue #2646 which I filed before making this PR to fix it.

drakej and others added 2 commits November 23, 2022 14:09
@drakej
Copy link
Author

drakej commented Feb 1, 2023

This handles an issue where the aria-hidden attribute is considered invalid by automated tools that check for WCAG compliance.

@TheSharpieOne
Copy link
Member

TheSharpieOne commented Feb 3, 2023

This handles an issue where the aria-hidden attribute is considered invalid by automated tools that check for WCAG compliance.

Is there a specific WCAG test number it is failing?

MDN defines it's cases as:

The aria-hidden attribute can be used to hide non-interactive content from the accessibility API.

Adding aria-hidden="true" to an element removes that element and all of its children from the accessibility tree. This can improve the experience for assistive technology users by hiding:

Purely decorative content, such as icons or images
Duplicated content, such as repeated text
Offscreen or collapsed content, such as menus
The presence of the aria-hidden attribute hides content from assistive technology but doesn't visually hide anything.

aria-hidden="true" should not be used on elements that can receive focus. Additionally, since this attribute is inherited by an element's children, it should not be added onto the parent or ancestor of a focusable element.

This falls under the "Offscreen or collapsed content, such as menus".

As for the "aria-hidden="true" should not be used on elements that can receive focus". The children (the dropdown menu items) are focusable, but only when it is open. The value of aria-hidden here is tied to the isOpen value so that when it is open and the children are focusable, they aria-hidden value is false and when it is closed and the children are not focusable (read: not to be given focus), the aria-hidden value is true.

So it seems like the tool you are using might have a false positive. It doesn't seem like it is checking if the content is offscreen or collapsed.

That said, MDN also mentions this:

On the surface, the aria-hidden="true" and the role="presentation" and its synonym role="none" seem similar, but the intent behind each is different.

aria-hidden="true" will remove the entire element from the accessibility API.
role="presentation" and role="none" will remove the semantic meaning of an element while still exposing it and its content to assistive technology.
aria-hidden="true" should not be added when:

The HTML hidden attribute is present
The element or the element's ancestor is hidden with display: none
The element or the element's ancestor is hidden with visibility: hidden
In all three scenarios, the attribute is unnecessary to add because the element has already been removed from the accessibility tree. Visually hiding elements with display or visibility hides content from the screen and from assistive technologies.

In this case, the menu does receive display: none, when isOpen is false so aira-hidden doesn't really provide any benefit and is probably superfluous.

@drakej
Copy link
Author

drakej commented Feb 3, 2023

https://www.w3.org/WAI/standards-guidelines/act/rules/6cfa84/proposed/

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-hidden

DropDownMenu has children navigation links which are focus-able which is an issue because of this attribute being set above them. I didn't have an WCAG rule specifically because I ran into the issue from not being able to focus on the links in the screen reader as expected. I tracked this back to reactstrap because Bootstrap UI's markup, CSS, and javascript do not have these issues. I feel like that is more than enough research into why this is a problem.

In this case, the menu does receive display: none, when isOpen is false so aira-hidden doesn't really provide any benefit and is probably superfluous.

It isn't probably, it is because it was added and doesn't add any value. If someone could make an argument for why it should be there then great, but right now this thing is breaking our compliance on real sites and our only fix is to stop using reactstrap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants