feat: flag unsupported roles #1064
Conversation
Adds `unsupported` property to the lookupTable.role dictionary. New option for aria.isValidRole to flagUnsupported roles. Defaults to false since this commons function is reused for multiple checks. TODO: Add `unsupported` to lookupTable.attributes and figure out what to do with globalAttributes Part of #918
| out.push([`${key}`, 'No']); | ||
| } | ||
| break; | ||
| case 'all': | ||
| default: | ||
| out.push([`${key}`, subject.includes(key) ? 'Yes' : 'No']); | ||
| out.push([ | ||
| `${key}`, |
There was a problem hiding this comment.
What's this template string for? This could just be out.push(key, ...).
There was a problem hiding this comment.
I'm not sure, that's a question for @JKODU
There was a problem hiding this comment.
@stephenmathieson @dylanb
out is what eventually becomes the array to pass to mdTable like so - https://github.com/dequelabs/axe-core/blob/develop/build/tasks/aria-supported.js#L78
And the generated output is here - https://github.com/dequelabs/axe-core/blob/develop/doc/aria-supported.md
There was a problem hiding this comment.
Right, but why are we wrapping a variable in backticks here? Consider the following:
var a = `${key}`
var b = key| @@ -0,0 +1,11 @@ | |||
| { | |||
| "id": "unsupportedrole", | |||
There was a problem hiding this comment.
Should this be hyphenated? unsupported-role is more ascetically pleasing and easier to read (IMO).
There was a problem hiding this comment.
I was following abstractrole and invalidrole, but I could change it.
There was a problem hiding this comment.
Ah, makes sense. I'm +1 for consistency, so no need to change
There was a problem hiding this comment.
Now that I'm working on attributes, I wish we would have added a hyphen.... the role thing is inconsistent with the rest of axe-core checks. oh well...
| @@ -75,14 +87,11 @@ module.exports = function(grunt) { | |||
| } in axe-core.`, | |||
| mdTable([ | |||
| ['aria-role', 'axe-core support'], | |||
| ...getDiff(roles, Object.keys(axe.commons.aria.lookupTable.role)) | |||
| ...getDiff(roles, axe.commons.aria.lookupTable.role) | |||
There was a problem hiding this comment.
Object.keys creates an array of just the roles, I needed the whole object.
| "impact": "critical", | ||
| "messages": { | ||
| "pass": "ARIA role is supported", | ||
| "fail": "The role used is not widely supported in assistive technologies" |
There was a problem hiding this comment.
Should this say "in screen readers", or "in screen readers and other assistive technologies"? Also, should we include the actual role?
| context: null, | ||
| unsupported: false | ||
| }, | ||
| figure: { |
There was a problem hiding this comment.
I think we should add the other properties here too.
There was a problem hiding this comment.
What other properties would apply?
Adds
unsupportedproperty to thelookupTable.roledictionary. New option foraria.isValidRoletoflagUnsupportedroles. Defaults tofalsesince this commons function is reused for multiple checks.Part one of #918.
TODO:
unsupportedto lookupTable.attributesaxe.commons.supportAPI and enable control of it throughaxe.configureReviewer checks
Required fields, to be filled out by PR reviewer(s)