Add ATSupported checks to ARIA rules

[WilcoFiers]

One of the difficulties with the ARIA rules is that it is unclear if an ARIA attribute / role wasn't supported, or if they made had a typo. To solve this we should include all available ARIA attributes and add checks that flag unsupported properties as violations. This has the added benefit of making it easy to ignore unsupported properties by simply disabling these checks.

[WilcoFiers]

On second thought, perhaps a separate rule instead... Thoughts?

[marcysutton]

More of an explanation about ARIA roles and attributes could also help to differentiate between these two common scenarios, which are pretty cryptic:

• attribute refers to an invalid or non-existing ID attribute
• the attribute is completely invalid

[marcysutton]

include all available ARIA attributes

Like adding everything from the ARIA spec to the lookupTable? We can't rely on aria-query completely due to its reliance on Sets, IIRC?

add checks that flag unsupported properties as violations

It seems like the actual marking of support should live in the lookupTable, and the checks simply look up those flags and report them to the user. Disabling the new checks would put axe back to how it operates now.

Is there anything else you want to add @WilcoFiers?

[WilcoFiers]

I don't have it all worked out... I do think we should use aria-query. We can't use it directly, but there's probably a way that we can use it to build an object structure that we can safely include into axe.

As for where the support data should be. Probably somewhere as a data structure in commons.aria. I think it should probably be its own structure, rather than weave it into lookuptable, so that it's easier to configure.

If you're not sure how to proceed, maybe a quick design doc would help.

[WilcoFiers]

Some more thoughts. I think we should turn whatever we do into one, or possibly multiple JSON files, rather than have them live in JS code. All this is static data, lets store it that way.

[marcysutton]

Wilco and I paired on this for a while today, here's what we decided was the least complicated to implement and iterate on in the future:

Mapping checks to unsupported data

• Add unsupported property to roles in lookupTable with default falsey value
  • Add a new check for aria-roles called unsupported-roles that flags rules with unsupported: true as issues
• Add unsupported property to attributes in lookupTable
  • Add a new check for aria-allowed-attr called unsupported-attr that flags aria attributes with unsupported: true as issues

Making it configurable through the API

• axe.commons.support() => change unsupported values in the lookupTable
  • i.e. axe.commons.support().fallbackRoles = true
  • Support axe.configure({ support }) to call axe.commons.support()

Future improvements

• Once the lookupTable aligns more with aria-query, generate lookupTable from aria-query at build time instead of keeping our own.
• Build support data from a support.json file instead of directly into the lookup table (hopefully we'll have the ability to import or require by that time)

--
I'm going to start with aria-roles, as it is the most straight-forward part to implement.

[WilcoFiers]

I'm liking this plan a lot :-). One tweak to make to it. for axe.commons.support(), use it as a setter. It could work something like this:

axe.commons.support({
  roles: { figure: false },
  ariaProps: { roledescription: false }
})

[marcysutton]

@WilcoFiers I have a note to do something about global attributes, which are a flat array. But those attributes are repeated in the more granular ARIA attribute object, so I don't think there is anything specific we need to do for that. Does it really matter if an unsupported attribute is global?

[marcysutton]

Here's something I found that could be related: the aria-allowed-attr-matches file requires some kind of role to find out whether an attribute is allowed, which is probably too granular for the new unsupported-attr check. I think it would be good to look at all ARIA attributes present on the node for support regardless of role, because people might put them wherever.

The matches file also duplicates some of the same work as the aria-allowed-attr check evaluate function, so that code already exists elsewhere to filter nodes out. Do you agree we should change the matches file? It would be good to confirm before I start messing with the tests, since it changes the rule design a bit. https://github.com/dequelabs/axe-core/blob/develop/test/rule-matches/aria-allowed-attr-matches.js

I've written the new check to look at any ARIA attribute on the node and consult the lookupTable on whether it is supported. Here's what I have so far: #1254 Feel free to discuss here or on the PR!

Thanks :)

[dylanb]

closing after consultation