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

feat: role-supports-aria-props rule (no aria-label-misuse) #362

Merged
merged 8 commits into from
Jan 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 21 additions & 23 deletions .devcontainer/devcontainer.json
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"
}
}
2 changes: 1 addition & 1 deletion .eslintrc.js
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,
Expand Down
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ For each component, you may specify a `default` and/or `props`. `default` may ma
"settings": {
"github": {
"components": {
"Box": { "default": "p" },
"Link": { "props": {"as": { "undefined": "a", "a": "a", "button": "button"}}},
"Box": {"default": "p"},
Copy link
Contributor Author

@smockle smockle Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was Prettier.

"Link": {"props": {"as": {"undefined": "a", "a": "a", "button": "button"}}}
}
}
}
Expand Down Expand Up @@ -94,3 +94,4 @@ This config will be interpreted in the following way:
#### Accessibility-focused rules (prefixed with a11y)

- [No Generic Link Text](./docs/rules/a11y-no-generic-link-text.md)
- [Role Supports ARIA Props](./docs/rules/role-supports-aria-props.md)
10 changes: 7 additions & 3 deletions docs/rules/a11y-no-generic-link-text.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,18 @@ Please perform browser tests and spot checks:
```

```jsx
<a href="github.com/about" aria-label="Why dogs are awesome">Read more</a>
<a href="github.com/about" aria-label="Why dogs are awesome">
Read more
</a>
```

```jsx
<a href="github.com/about" aria-describedby="element123">Read more</a>
<a href="github.com/about" aria-describedby="element123">
Read more
</a>
```

### **Correct** code for this rule 👍
### **Correct** code for this rule 👍

```jsx
<a href="github.com/about">Learn more about GitHub</a>
Expand Down
1 change: 1 addition & 0 deletions docs/rules/array-foreach.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ for (const el of els) {
```

Use [`entries()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/entries) to get access to the index:

```js
for (const [i, el] of els.entries()) {
el.name = `Element ${i}`
Expand Down
4 changes: 2 additions & 2 deletions docs/rules/authenticity-token.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ on('click', '.js-my-button', function (e) {

fetch(form.action, {
method: form.method,
body: new FormData(form)
body: new FormData(form),
}).then(function () {
alert('Success!')
})
Expand All @@ -52,7 +52,7 @@ An alternate, but less preferred approach is to include the a signed CSRF url in
on('click', '.js-my-button', function (e) {
csrfRequest(this.getAttribute('data-url'), {
method: 'PUT',
body: data
body: data,
}).then(function () {
alert('Success!')
})
Expand Down
2 changes: 1 addition & 1 deletion docs/rules/no-useless-passive.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ window.addEventListener(
() => {
console.log('Scroll event fired!')
},
{passive: true}
{passive: true},
)
```

Expand Down
2 changes: 1 addition & 1 deletion docs/rules/prefer-observers.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ window.addEventListener('scroll', () => {
const isIntersecting = checkIfElementIntersects(
element.getBoundingClientRect(),
window.innerHeight,
document.clientHeight
document.clientHeight,
)
element.classList.toggle('intersecting', isIntersecting)
})
Expand Down
2 changes: 1 addition & 1 deletion docs/rules/require-passive-events.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ window.addEventListener(
() => {
/* ... */
},
{passive: true}
{passive: true},
)
```

Expand Down
74 changes: 74 additions & 0 deletions docs/rules/role-supports-aria-props.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# Role Supports ARIA Props
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
```
2 changes: 2 additions & 0 deletions lib/configs/react.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,7 @@ module.exports = {
extends: ['plugin:jsx-a11y/recommended'],
rules: {
'github/a11y-no-generic-link-text': 'error',
'github/role-supports-aria-props': 'error',
'jsx-a11y/role-supports-aria-props': 'off',
},
}
1 change: 1 addition & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module.exports = {
'no-then': require('./rules/no-then'),
'no-useless-passive': require('./rules/no-useless-passive'),
'prefer-observers': require('./rules/prefer-observers'),
'role-supports-aria-props': require('./rules/role-supports-aria-props'),
'require-passive-events': require('./rules/require-passive-events'),
'unescaped-html-literal': require('./rules/unescaped-html-literal'),
},
Expand Down
101 changes: 101 additions & 0 deletions lib/rules/role-supports-aria-props.js
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`
Copy link
Contributor Author

@smockle smockle Dec 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint-plugin-jsx-a11y (the upstream project) holds that, “[W]e don't want to store knowledge of WAI-ARIA in this plugin. That knowledge should be stored in aria-query.”

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']) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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. <input type="radio"> vs <input type="checkbox">). But, given my nonchalance towards separation of concerns, the readability and performance gains, and the fact that this can’t lead to false positives, I opted for this simpler hardcoded list.

// - 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}.`,
})
}
}
},
}
},
}
Loading