-
Notifications
You must be signed in to change notification settings - Fork 56
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
feat: role-supports-aria-props
rule (no aria-label
-misuse)
#362
Changes from all commits
8c9445e
4c71d04
ca32268
e2d8376
84c6fe5
5497b3a
50a07db
cd0a526
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,30 @@ | ||
// For format details, see https://aka.ms/devcontainer.json. For config options, see the README at: | ||
// https://github.com/microsoft/vscode-dev-containers/tree/v0.222.0/containers/javascript-node | ||
{ | ||
"name": "Node.js", | ||
"build": { | ||
"dockerfile": "Dockerfile", | ||
// Update 'VARIANT' to pick a Node version: 16, 14, 12. | ||
// Append -bullseye or -buster to pin to an OS version. | ||
// Use -bullseye variants on local arm64/Apple Silicon. | ||
"args": { "VARIANT": "16" } | ||
}, | ||
"name": "Node.js", | ||
"build": { | ||
"dockerfile": "Dockerfile", | ||
// Update 'VARIANT' to pick a Node version: 16, 14, 12. | ||
// Append -bullseye or -buster to pin to an OS version. | ||
// Use -bullseye variants on local arm64/Apple Silicon. | ||
"args": {"VARIANT": "16"} | ||
}, | ||
|
||
// Set *default* container specific settings.json values on container create. | ||
"settings": {}, | ||
// Set *default* container specific settings.json values on container create. | ||
"settings": {}, | ||
|
||
// Add the IDs of extensions you want installed when the container is created. | ||
"extensions": [ | ||
"dbaeumer.vscode-eslint" | ||
], | ||
// Add the IDs of extensions you want installed when the container is created. | ||
"extensions": ["dbaeumer.vscode-eslint"], | ||
|
||
// Use 'forwardPorts' to make a list of ports inside the container available locally. | ||
// "forwardPorts": [], | ||
// Use 'forwardPorts' to make a list of ports inside the container available locally. | ||
// "forwardPorts": [], | ||
|
||
// Use 'postCreateCommand' to run commands after the container is created. | ||
// "postCreateCommand": "yarn install", | ||
// Use 'postCreateCommand' to run commands after the container is created. | ||
// "postCreateCommand": "yarn install", | ||
|
||
// Comment out to connect as root instead. More info: https://aka.ms/vscode-remote/containers/non-root. | ||
"remoteUser": "node", | ||
"features": { | ||
"git": "latest" | ||
} | ||
// Comment out to connect as root instead. More info: https://aka.ms/vscode-remote/containers/non-root. | ||
"remoteUser": "node", | ||
"features": { | ||
"git": "latest" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
module.exports = { | ||
root: true, | ||
parserOptions: { | ||
ecmaVersion: 2020, | ||
ecmaVersion: 13, | ||
}, | ||
env: { | ||
es6: true, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,7 @@ window.addEventListener( | |
() => { | ||
/* ... */ | ||
}, | ||
{passive: true} | ||
{passive: true}, | ||
) | ||
``` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
# Role Supports ARIA Props | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
## Rule Details | ||
|
||
This rule enforces that elements with explicit or implicit roles defined contain only `aria-*` properties supported by that `role`. | ||
|
||
For example, this rule aims to discourage common misuse of the `aria-label` and `aria-labelledby` attribute. `aria-label` and `aria-labelledby` support is only guaranteed on interactive elements like `button` or `a`, or on static elements like `div` and `span` with a permitted `role`. This rule will allow `aria-label` and `aria-labelledby` usage on `div` and `span` elements if it set to a role other than the ones listed in [WSC: a list of ARIA roles which cannot be named](https://w3c.github.io/aria/#namefromprohibited). This rule will never permit usage of `aria-label` and `aria-labelledby` on `h1`, `h2`, `h3`, `h4`, `h5`, `h6`, `strong`, `i`, `p`, `b`, or `code`. | ||
|
||
### "Help! I'm trying to set a tooltip on a static element and this rule flagged it!" | ||
|
||
Please do not use tooltips on static elements. It is a highly discouraged, inaccessible pattern. | ||
See [Primer: Tooltip alternatives](https://primer.style/design/accessibility/tooltip-alternatives) for what to do instead. | ||
|
||
### Resources | ||
|
||
- [w3c/aria Consider prohibiting author naming certain roles #833](https://github.com/w3c/aria/issues/833) | ||
- [Not so short note on aria-label usage - Big Table Edition](https://html5accessibility.com/stuff/2020/11/07/not-so-short-note-on-aria-label-usage-big-table-edition/) | ||
- [Your tooltips are bogus](https://heydonworks.com/article/your-tooltips-are-bogus/) | ||
- [Primer: Tooltip alternatives](https://primer.style/design/accessibility/tooltip-alternatives) | ||
|
||
### Disclaimer | ||
|
||
There are conflicting resources and opinions on what elements should support these naming attributes. For now, this rule will operate under a relatively simple heuristic aimed to minimize false positives. This may have room for future improvements. Learn more at [W3C Name Calcluation](https://w3c.github.io/aria/#namecalculation). | ||
|
||
### **Incorrect** code for this rule 👎 | ||
|
||
```erb | ||
<span class="tooltipped" aria-label="This is a tooltip">I am some text.</span> | ||
``` | ||
|
||
```erb | ||
<span aria-label="This is some content that will completely override the button content">Please be careful of the following.</span> | ||
``` | ||
|
||
```erb | ||
<div aria-labelledby="heading1">Goodbye</div> | ||
``` | ||
|
||
```erb | ||
<h1 aria-label="This will override the page title completely">Page title</h1> | ||
``` | ||
|
||
### **Correct** code for this rule 👍 | ||
|
||
```erb | ||
<button aria-label="Close"> | ||
<svg src="closeIcon"></svg> | ||
</button> | ||
``` | ||
|
||
```erb | ||
<button aria-label="Bold" aria-describedby="tooltip1"> | ||
<svg src="boldIcon"></svg> | ||
</button> | ||
<p id="tooltip1" class="tooltip">Add bold text or turn selection into bold text</p> | ||
``` | ||
|
||
```erb | ||
<span>Hello</span> | ||
``` | ||
|
||
```erb | ||
<div>Goodbye</div> | ||
``` | ||
|
||
```erb | ||
<h1>Page title</h1> | ||
``` | ||
|
||
```erb | ||
<div role="dialog" aria-labelledby="dialogHeading"> | ||
<h1 id="dialogHeading">Heading</h1> | ||
</div> | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
// @ts-check | ||
const {aria, elementRoles, roles} = require('aria-query') | ||
const {getProp, getPropValue, propName} = require('jsx-ast-utils') | ||
const {getElementType} = require('../utils/get-element-type') | ||
const ObjectMap = require('../utils/object-map') | ||
|
||
// Clean-up `elementRoles` from `aria-query` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think that’s a good architectural principle in general, but unnecessary for temporary code like this file. Here, I’m okay mixing WAI-ARIA knowledge with rule code. It’ll get cleaned up once this custom rule is removed (replaced by the updated upstream rule). |
||
const elementRolesMap = new ObjectMap() | ||
for (const [key, value] of elementRoles.entries()) { | ||
// - Remove unused `constraints` key | ||
delete key.constraints | ||
key.attributes = key.attributes?.filter(attribute => !('constraints' in attribute)) | ||
// - Remove empty `attributes` key | ||
if (!key.attributes || key.attributes?.length === 0) { | ||
delete key.attributes | ||
} | ||
elementRolesMap.set(key, value) | ||
} | ||
// - Remove insufficiently-disambiguated `menuitem` entry | ||
elementRolesMap.delete({name: 'menuitem'}) | ||
// - Disambiguate `menuitem` and `menu` roles by `type` | ||
elementRolesMap.set({name: 'menuitem', attributes: [{name: 'type', value: 'command'}]}, ['menuitem']) | ||
elementRolesMap.set({name: 'menuitem', attributes: [{name: 'type', value: 'radio'}]}, ['menuitemradio']) | ||
elementRolesMap.set({name: 'menuitem', attributes: [{name: 'type', value: 'toolbar'}]}, ['toolbar']) | ||
elementRolesMap.set({name: 'menu', attributes: [{name: 'type', value: 'toolbar'}]}, ['toolbar']) | ||
|
||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: | ||
'Enforce that elements with explicit or implicit roles defined contain only `aria-*` properties supported by that `role`.', | ||
url: require('../url')(module), | ||
}, | ||
schema: [], | ||
}, | ||
|
||
create(context) { | ||
return { | ||
JSXOpeningElement(node) { | ||
// Assemble a key for looking-up the element’s role in the `elementRolesMap` | ||
// - Get the element’s name | ||
const key = {name: getElementType(context, node)} | ||
// - Get the element’s disambiguating attributes | ||
for (const prop of ['aria-expanded', 'type', 'size', 'role', 'href', 'multiple', 'scope']) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally, I’d dynamically generated this list of the attributes that are necessary for disambiguating elements with multiple roles (e.g. |
||
// - Only provide `aria-expanded` when it’s required for disambiguation | ||
if (prop === 'aria-expanded' && key.name !== 'summary') continue | ||
const value = getPropValue(getProp(node.attributes, prop)) | ||
if (value) { | ||
if (!('attributes' in key)) { | ||
key.attributes = [] | ||
} | ||
if (prop === 'href') { | ||
key.attributes.push({name: prop}) | ||
} else { | ||
key.attributes.push({name: prop, value}) | ||
} | ||
} | ||
} | ||
// Get the element’s explicit or implicit role | ||
const role = getPropValue(getProp(node.attributes, 'role')) ?? elementRolesMap.get(key)?.[0] | ||
|
||
// Return early if role could not be determined | ||
if (!role) return | ||
|
||
// Get allowed ARIA attributes: | ||
// - From the role itself | ||
let allowedProps = Object.keys(roles.get(role)?.props || {}) | ||
// - From parent roles | ||
for (const parentRole of roles.get(role)?.superClass.flat() ?? []) { | ||
allowedProps = allowedProps.concat(Object.keys(roles.get(parentRole)?.props || {})) | ||
} | ||
// Dedupe, for performance | ||
allowedProps = Array.from(new Set(allowedProps)) | ||
|
||
// Get prohibited ARIA attributes: | ||
// - From the role itself | ||
let prohibitedProps = roles.get(role)?.prohibitedProps || [] | ||
// - From parent roles | ||
for (const parentRole of roles.get(role)?.superClass.flat() ?? []) { | ||
prohibitedProps = prohibitedProps.concat(roles.get(parentRole)?.prohibitedProps || []) | ||
} | ||
// - From comparing allowed vs all ARIA attributes | ||
prohibitedProps = prohibitedProps.concat(aria.keys().filter(x => !allowedProps.includes(x))) | ||
// Dedupe, for performance | ||
prohibitedProps = Array.from(new Set(prohibitedProps)) | ||
|
||
for (const prop of node.attributes) { | ||
// Return early if prohibited ARIA attribute is set to an ignorable value | ||
if (getPropValue(prop) == null || prop.type === 'JSXSpreadAttribute') return | ||
|
||
if (prohibitedProps?.includes(propName(prop))) { | ||
context.report({ | ||
node, | ||
message: `The attribute ${propName(prop)} is not supported by the role ${role}.`, | ||
}) | ||
} | ||
} | ||
}, | ||
} | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was Prettier.