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

Consider more nuance to severity of aria-allowed-attr #3756

Closed
scottaohara opened this issue Oct 28, 2022 · 5 comments · Fixed by #4532
Closed

Consider more nuance to severity of aria-allowed-attr #3756

scottaohara opened this issue Oct 28, 2022 · 5 comments · Fixed by #4532
Assignees
Labels
fix Bug fixes rules Issue or false result from an axe-core rule
Milestone

Comments

@scottaohara
Copy link
Contributor

The following is an example of a valid aria-allowed-attr flag with a debatable severity:

<button aria-required=false>foo</button>

when using the web extension, axe 4.4.2, the above mark up is classified as a critical failure.

However, in this case because the attribute is set to false, it is exposed as if the attribute was absent.

Now, if this were aria-required=true then some browser/AT combos would output that this is a "required" button. That's silly. I don't know if I'd even classify that as a 'critical' bug, but I think that still falls into the category of what this rule is trying to prevent.

I just wonder if there's a way to continue to separate out instances like the one I provided where the value of the attribute determines if the property/state is even communicated or not. And in the cases of where the attribute is not communicated, I wonder if severity could better align with the lack of user impact?

@straker
Copy link
Contributor

straker commented Oct 31, 2022

Interesting question. Separating out the severity would be difficult as the severity is tied to the check. Essentially we'd have to separate the concerns of which aria attributes and use cases produce a different severity and move those to a separate check. Each severity level being their own check / logic.

@WilcoFiers
Copy link
Contributor

Pulling @dylanb into the conversation.

Axe-core isn't built to give accurate severity / impact. As Steve says, it's a static property on the check. Axe doesn't have the architecture to adjust impact on the fly. I think that's probably a good thing. Severity is so context sensitive, whatever we do, we're never going to get it right. Now, we could certainly do more than the "nothing at all" we're doing today. On the face of it that's not a bad thing to do. We could make impact dynamic, add methods for adjusting impact based on context. Maybe look at things like the value of an invalid ARIA attribute, look at where in the page the element lives, maybe take the contrast ratio and font thinness into account for color-contrast, etc. There is lots we could do.

What I don't like about doing that is that it reduces predictability axe's results. If that aria-allowed-attr rule used some number of heuristics to adjust impact up or down in certain situations, it immediately becomes difficult to understand why impact is one thing in situation A, and another in situation B. The more heuristics you add, the more "accurate" impact gets, but the less predictable it becomes. The answer to "why does axe-core report this aria-required=false as critical" is because that's the default impact that rule reports. If we made impact variable, I might be able to tell you if you ask me, but you wouldn't be able to tell without digging into the source code.

On the other hand though, Deque's product designs are increasingly putting impact front and center. By making impact as prominent as it is in the extension today, I think it's implying a degree of accuracy that we're not delivering on. I can appreciate the tension devs experience if they're told there are 10 critical issues, when in reality it's all fairly minor stuff. I don't think that's a good experience, especially if that's your manager pointing it out to you.

Thanks for bringing this up @scottaohara. I don't know what the solve is at the moment, but there's something to look into here for sure.

@WilcoFiers
Copy link
Contributor

Revisiting this. In axe-core 4.8 we've decided to go the other direction with impact. Each rule can only report one impact level. This is to improve predictability of this rule.

What I think we should do here isn't to adjust the impact, but to pass this instead. aria-required=false does effectively nothing. It's not that different from using aria-label="" on a div. Axe-core passes that, despite that it's prohibited by ARIA. I think it would be good to go over this rule and identify values that are sort of equivalent to "null".

We can consider whether those should be passed, or if they can be set as incomplete. Failing them doesn't seem right to me. This is a plain old false positive in my opinion. That's a bug. We need to fix that.

@WilcoFiers WilcoFiers added fix Bug fixes rules Issue or false result from an axe-core rule labels Aug 7, 2023
@WilcoFiers WilcoFiers added this to the Axe-core 4.9 milestone Aug 7, 2023
@WilcoFiers
Copy link
Contributor

We should test this out in AT. If the aria-required=false prop is consistently ignored, I think we should add it as an exception to the rule.

@straker
Copy link
Contributor

straker commented Jul 9, 2024

Results of testing the following HTML in various ATs

<button aria-required=false>foo</button>
<button aria-required=true>foo</button>

<div role="button" tabindex="0" aria-required=false>foo</div>
<div role="button" tabindex="0" aria-required=true>foo</div>

<div tabindex="0" aria-required=false>foo</div>
<div tabindex="0" aria-required=true>foo</div>

<div role="grid" tabindex="0" aria-required=false>foo</div>
<div role="grid" tabindex="0" aria-required=true>foo</div>
  • VO / Safari - skipped aria-required=false and didn't announce it, Announced "required" when aria-required=true was used, even on the div without a role
  • JAWS / Chrome - skipped aria-required=false and didn't announce it. Only announced "required" when aria-required=true for the buttons
  • NVDA / Firefox - skipped aria-required=false and didn't announce it. Announced "required" when aria-required=true for the buttons and the table, but not the div without a role

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fixes rules Issue or false result from an axe-core rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants