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

aria-allowed-attr possible regression? #3241

Closed
thepassle opened this issue Oct 28, 2021 · 16 comments
Closed

aria-allowed-attr possible regression? #3241

thepassle opened this issue Oct 28, 2021 · 16 comments
Labels
core Issues in the core code (lib/core) fix Bug fixes pr A pr has been created for the issue
Milestone

Comments

@thepassle
Copy link

Product: axe-core

Expectation: element with both aria-controls and aria-expanded passes

Actual: element with both aria-controls and aria-expanded fails

Motivation: I believe it might have been an accidental regression via https://github.com/dequelabs/axe-core/blob/develop/CHANGELOG.md#:~:text=aria-allowed-attr%3A%20check%20for%20invalid


axe-core version: 4.3.4

We recently found a test started failing that uses axe-core. We previously had [email protected] installed, where the test passes, the test has started failing since [email protected], which leads me to believe it may have been an accidental regression via this change: https://github.com/dequelabs/axe-core/blob/develop/CHANGELOG.md#:~:text=aria-allowed-attr%3A%20check%20for%20invalid

For context, we have an element with both an aria-controls and aria-expanded attribute, e.g.:

<my-el aria-controls="menu" aria-expanded="false"></my-el>

As far as my accessibility knowledge goes, this should be valid (but I'm happy to be corrected if I'm wrong). I also checked with a couple of colleagues, who also expected this to be valid.

@straker
Copy link
Contributor

straker commented Oct 28, 2021

It appears one of our changes didn't make it into the Changelog for some reason, sorry about that. We added a change in 4.3.4 to aria-allowed-attr to flag non-global aria attributes when there is no role applied to the element.

@straker
Copy link
Contributor

straker commented Oct 28, 2021

I take that back, I don't think that should have been released quite yet, but it seems we released something from develop that shouldn't have been there yet. Will confirm.

@WilcoFiers
Copy link
Contributor

@thepassle Do you have a real-world example of this? I'm interested to see what those custom elements you're using non-global ARIA attributes look like. My understanding of this is that sticking attributes like that on custom elements will create problems. I'm curious to see if/how you may have managed to avoid the issues of that.

@thepassle
Copy link
Author

The example in the OP pretty much is the real world example, ive only omitted non-relevant attributes (class, data attrs, etc), and the custom element name obviously isnt my-el.

Im not sure why you think using aria attributes on custom elements would cause any issues.

@straker
Copy link
Contributor

straker commented Oct 29, 2021

I think mostly what we're after is how the custom element's internal HTML uses the attributes. I'm guessing you reflect those attributes onto some internal element? So for example, a custom button would reflect their attributes onto the <button> element:

<custom-button aria-pressed="true"></custom-button>

Would turn into:

<custom-button aria-pressed="true">
  #shadow-root
    <button aria-pressed="true"></button>
</custom-button>

@thepassle
Copy link
Author

thepassle commented Oct 29, 2021

It doesnt do anything like that, the custom element itself is a valid host to put the aria attributes on, since its just a HTMLElement like any other element.

The fact that im using a custom element at all isnt really relevant to the issue, the same problem would occur for any other element

@straker
Copy link
Contributor

straker commented Oct 29, 2021

Gotcha. So the issue is that without a semantic role, the element isn't allowed to use a non-global ARIA attribute like aria-expanded. I would love to see a minimal working example of the element to play around with how screen readers treat the aria-expanded.

Regardless, we just released a hot fix for this as v4.3.5, so updating to the latest should no longer report the issue. As an FYI, we still plan to release this, but properly under v4.4.0.

@thepassle
Copy link
Author

So the issue is that without a semantic role, the element isn't allowed to use a non-global ARIA attribute like aria-expanded

Interesting, do you have a source for that? Ive not heard of that requirement before, Id love to learn more

@straker
Copy link
Contributor

straker commented Oct 29, 2021

ARIA in HTML lists which ARIA roles and attributes are allowed on what elements. For custom elements, the allowed ARIA attributes are any global ARIA attribute and any allowed attributes defined by the role. Since the element has no role, only global ARIA attributes are allowed.

The ARIA spec will list which attributes are allowed on which roles, and also lists the global ARIA attributes.

@thepassle
Copy link
Author

TIL, thanks!

@WilcoFiers
Copy link
Contributor

We've released 4.3.5, which has disabled this test. It wasn't meant to go out in a patch, we had it planned for the 4.4.0 release. We're going to take a closer look at how this can affect custom elements. There may be ways in which this can be alright, but Steve's right that these attributes don't work just by themselves. Either you reflect them onto another element, or you make that particular component fully accessible (providing role and focus).

@svanherk
Copy link

svanherk commented Nov 1, 2021

If it helps, we were also hit by this problem and use these attributes on custom elements. We do what you described above - our custom button that opens a dropdown is written like this:

<custom-button aria-expanded="true"></custom-button>

And turns into this:

<custom-button aria-expanded="true">
  #shadow-root
    <button aria-expanded="true"></button>
</custom-button>

As you said, we really shouldn't have been doing this but it was interpreted correctly by the screenreader. For our newer components we've been using data-aria-expanded or just expanded on the custom element instead. We have a number of older components that will need to be updated and fixed throughout our application, so we'll be blocked from going to 4.4.0 until we do that.

@straker straker added this to the Axe-core 4.4 milestone Nov 4, 2021
@straker
Copy link
Contributor

straker commented Nov 4, 2021

@svanherk Thanks for letting us know. We'll try to come up with a better solution for 4.4.0 taking that into account.

@straker straker added core Issues in the core code (lib/core) fix Bug fixes labels Nov 4, 2021
@WilcoFiers
Copy link
Contributor

I'm leaning towards flagging unknown elements in this rule for review, except if they have a valid role or a tabindex attribute. Those kinds of elements we just don't know enough about what's going on. This seems like the safest way to test them.

@straker
Copy link
Contributor

straker commented Jan 10, 2022

A followup on this issue. Axe-core 4.4 (releasing at the end of this month) will add back the violation for non-global ARIA attributes on elements with a role. We have also introduced a fix for custom elements to report as Needs Review rather than a failure.

@padmavemulapati
Copy link

Validated with the latest dev branch code base,
If we pass non-global-attribute , <button aria-controls="menu" aria-expanded="false">Search</button> it is passing,
image

if we pass global-attr , <div aria-controls="menu" aria-expanded="false"></div> failing
image

and with custom element, <my-el aria-controls="menu" aria-expanded="false"></my-el> its incomplete
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues in the core code (lib/core) fix Bug fixes pr A pr has been created for the issue
Projects
None yet
Development

No branches or pull requests

5 participants