Skip to content

Add accessibility rule and React config #282

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

Merged
merged 13 commits into from
Jul 21, 2022
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
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ The available configs are:
- Rules useful for github applications.
- `browser`
- Useful rules when shipping your app to the browser.
- `react`
- Recommended rules for React applications.
- `recommended`
- Recommended rules for every application.
- `typescript`
Expand All @@ -59,3 +61,7 @@ The available configs are:
- [Prefer Observers](./docs/rules/prefer-observers.md)
- [Require Passive Events](./docs/rules/require-passive-events.md)
- [Unescaped HTML Literal](./docs/rules/unescaped-html-literal.md)

#### Accessibility-focused rules (prefixed with a11y)

- [No Generic Link Text](./docs/rules/a11y-no-generic-link-text.md)
83 changes: 83 additions & 0 deletions docs/rules/a11y-no-generic-link-text.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
# No Generic Link Text

## Rule Details

Avoid setting generic link text like, "Click here", "Read more", and "Learn more" which do not make sense when read out of context.

Screen reader users often tab through links on a page to quickly find content without needing to listen to the full page. When link text is too generic, it becomes difficult to quickly identify the destination of the link. While it is possible to provide a more specific link text by setting the `aria-label`, this results in divergence between the label and the text and is not an ideal, future-proof solution.

Additionally, generic link text can also problematic for heavy zoom users where the link context is out of view.

Ensure that your link text is descriptive and the purpose of the link is clear even when read out of context of surrounding text.
Learn more about how to write descriptive link text at [Access Guide: Write descriptive link text](https://www.accessguide.io/guide/descriptive-link-text)

### Use of ARIA attributes

If you _must_ use ARIA to replace the visible link text, include the visible text at the beginning.

For example, on a pricing plans page, the following are good:

- Visible text: `Learn more`
- Accessible label: `Learn more about GitHub pricing plans`

Accessible ✅

```html
<a href="..." aria-label="Learn more about GitHub pricing plans">Learn more</a>
```

Inaccessible 🚫

```html
<a href="..." aria-label="GitHub pricing plans">Learn more</a>
```

Including the visible text in the ARIA label satisfies [SC 2.5.3: Label in Name](https://www.w3.org/WAI/WCAG21/Understanding/label-in-name.html).

#### False negatives

Caution: because of the restrictions of static code analysis, we may not catch all violations.

Please perform browser tests and spot checks:

- when `aria-label` is set dynamically
- when using `aria-labelledby`

## Resources

- [Primer: Links](https://primer.style/design/accessibility/links)
- [Understanding Success Criterion 2.4.4: Link Purpose (In Context)](https://www.w3.org/WAI/WCAG21/Understanding/link-purpose-in-context.html)
- [WebAim: Links and Hypertext](https://webaim.org/techniques/hypertext/)
- [Deque: Use link text that make sense when read out of context](https://dequeuniversity.com/tips/link-text)

## Examples

### **Incorrect** code for this rule 👎

```jsx
<a href="github.com/about">Learn more</a>
```

```jsx
<a href="github.com/about">Read more</a>
```

```jsx
<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>
```

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

```jsx
<a href="github.com/about">Learn more about GitHub</a>
```

```jsx
<a href="github.com/new">Create a new repository</a>
```

## Version
10 changes: 10 additions & 0 deletions lib/configs/react.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module.exports = {
Copy link
Contributor Author

@khiga8 khiga8 Jul 19, 2022

Choose a reason for hiding this comment

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

Maybe I should distinguish this config as being accessibility rules for react....like github/jsx-a11y. (We don't currently have any other React rules so it's currently just github/react .)

thoughts? @github/accessibility-reviewers . also open to other suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for future rules github/react is kinda nice?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like having a11y-specific rules! But if it's too early to make that distinction until there are more react rules & accessibility rules, I am fine with this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm not sure either... I'm down to keep this for now and revisit later!

env: {
browser: true
},
plugins: ['github', 'jsx-a11y'],
extends: ['plugin:jsx-a11y/recommended'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming that any config we want to recommend as part of "react" belongs here, so I added the jsx-a11y plugin. does this look right?

rules: {
'github/a11y-no-generic-link-text': 'error'
}
}
4 changes: 3 additions & 1 deletion lib/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module.exports = {
rules: {
'a11y-no-generic-link-text': require('./rules/a11y-no-generic-link-text'),
'array-foreach': require('./rules/array-foreach'),
'async-currenttarget': require('./rules/async-currenttarget'),
'async-preventdefault': require('./rules/async-preventdefault'),
Expand All @@ -23,6 +24,7 @@ module.exports = {
browser: require('./configs/browser'),
internal: require('./configs/internal'),
recommended: require('./configs/recommended'),
typescript: require('./configs/typescript')
typescript: require('./configs/typescript'),
react: require('./configs/react')
}
}
68 changes: 68 additions & 0 deletions lib/rules/a11y-no-generic-link-text.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
const {elementType, getProp, getPropValue} = require('jsx-ast-utils')

const bannedLinkText = ['read more', 'here', 'click here', 'learn more', 'more']

/* Downcase and strip extra whitespaces and punctuation */
const stripAndDowncaseText = text => {
return text
.toLowerCase()
.replace(/[.,/#!$%^&*;:{}=\-_`~()]/g, '')
.replace(/\s{2,}/g, ' ')
.trim()
}

module.exports = {
meta: {
docs: {
description: 'disallow generic link text',
url: require('../url')(module)
},
schema: []
},

create(context) {
return {
JSXOpeningElement: node => {
if (elementType(node) !== 'a') return

Choose a reason for hiding this comment

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

In general we will probably use the Link component from @primer/react more often than raw anchor tags, and this check would miss those.

I don't think we can catch all of the possible cases just using a linter, but it might be more thorough to apply this check to any element with an href prop. That will at least catch the cases where we have components that are thin wrappers around anchor tags.

Copy link
Contributor Author

@khiga8 khiga8 Jul 20, 2022

Choose a reason for hiding this comment

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

I have a follow up PR to address this lack of support for linting custom components. I'd love your feedback there -- #283.

if (getProp(node.attributes, 'aria-labelledby')) return

let cleanTextContent // text content we can reliably fetch

const parent = node.parent
let jsxTextNode
if (parent.children && parent.children.length > 0 && parent.children[0].type === 'JSXText') {
jsxTextNode = parent.children[0]
cleanTextContent = stripAndDowncaseText(parent.children[0].value)
}

const ariaLabel = getPropValue(getProp(node.attributes, 'aria-label'))
const cleanAriaLabelValue = ariaLabel && stripAndDowncaseText(ariaLabel)

if (ariaLabel) {
if (bannedLinkText.includes(cleanAriaLabelValue)) {
context.report({
node,
message:
'Avoid setting generic link text like `Here`, `Click here`, `Read more`. Make sure that your link text is both descriptive and concise.'
})
}
if (cleanTextContent && !cleanAriaLabelValue.includes(cleanTextContent)) {
context.report({
node,
message: 'When using ARIA to set a more descriptive text, it must fully contain the visible label.'
})
}
} else {
if (cleanTextContent) {
if (!bannedLinkText.includes(cleanTextContent)) return
context.report({
node: jsxTextNode,
message:
'Avoid setting generic link text like `Here`, `Click here`, `Read more`. Make sure that your link text is both descriptive and concise.'
})
}
}
}
}
}
}
Loading