axe-core icon indicating copy to clipboard operation
axe-core copied to clipboard

Consider more nuance to severity of aria-allowed-attr

Open scottaohara opened this issue 3 years ago • 3 comments

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?

scottaohara avatar Oct 28 '22 18:10 scottaohara

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.

straker avatar Oct 31 '22 14:10 straker

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 avatar Nov 04 '22 11:11 WilcoFiers

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 avatar Aug 07 '23 11:08 WilcoFiers

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.

WilcoFiers avatar Jul 08 '24 09:07 WilcoFiers

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

straker avatar Jul 09 '24 21:07 straker