Add accessibility rule and React config #282

khiga8 · 2022-07-19T00:29:26Z

Relates to: github/accessibility#1403 (GitHub staff only)

CC: @github/accessibility-reviewers too for any thoughts

Motivation

We want to start creating and sharing custom accessibility rules for React projects. While there is a jsx-plugin-a11y, GitHub also has some opinionated stances when it comes to accessibility which we want to codify with a linter.

I noticed that GitHub already owns a couple open-source eslint repos so I decided to start adding accessibility rules in this repo rather than starting from scratch. I am thinking that non Primer-specific accessibility rules belong in this repo, while Primer-specific accessibility rules can go in eslint-plugin-primer-react. GitHub staff should check out the following links for more context:

Changes

I am trying to set up a foundation for more accessibility rules to be added and shared between GitHub React projects.

New accessibility rule

I am adding an accessibility rule that discourages use of generic link text in JSX elements. This is the equivalent of an existing ERB lint rule we have (avoid-generic-link-text-counter.md). We want to flag when a link element has an aria-label with generic link text, or when there is no aria-label set but it has a text content of a generic link text.

This is the first time I am writing an eslint rule so please review carefully.

New config

Should I name this to something specific to JSX accessibility linting like jsx-a11y so the config is: github/jsx-a11y?
Or is generic react fine?

Note to reviewer 📣

I'm pretty new to eslint and eslint best practices so if any of this organization seems really funky, please let me know.

Also if you think accessibility rulesets should be organized in a different way or should be in its own repo talk to me! I am in spike phase right now and very open to suggestions/iteration.

khiga8 · 2022-07-19T00:30:34Z

lib/index.js

@@ -9,6 +9,7 @@ module.exports = {
    'no-blur': require('./rules/no-blur'),
    'no-d-none': require('./rules/no-d-none'),
    'no-dataset': require('./rules/no-dataset'),
+    'no-generic-link-text': require('./rules/no-generic-link-text'),


I want this to be prefixed so I can pattern match the code base specifically for accessibility rules. What should I do??? Thoughts on just prefixing all accessibility rules like:

a11y-no-generic-link-text

Related Slack thread on prefixing eslint rules (GitHub Staff only)

I changed the name to a11y-

I like the a11y!

khiga8 · 2022-07-19T00:31:15Z

lib/configs/react.js

+    browser: true
+  },
+  plugins: ['github', 'jsx-a11y'],
+  extends: ['plugin:jsx-a11y/recommended'],


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?

accessibility-bot · 2022-07-19T00:32:46Z

👋 Hello and thanks for pinging us! An accessibility first responder will review this soon.

💻 On PRs for our review: please provide a review environment with steps to validate, screenshots (with alt text), or videos demonstrating functionality we should be checking. This will help speed up our review and feedback cycle.
⚠️ If this is urgent, please visit us in #accessibility on Slack and tag the first responder(s) listed in the channel topic.

…ble label

khiga8 · 2022-07-19T01:00:17Z

docs/rules/no-generic-link-text.md

@@ -0,0 +1,83 @@
+# No Generic Link Text


This is pretty much copied from our erblint-github docs though the examples were changed to .jsx instead of .erb.

The two behave nearly exactly the same so why not 🤷🏻‍♀️

khiga8 · 2022-07-19T01:01:12Z

package.json

    "eslint-plugin-no-only-tests": "^2.6.0",
    "eslint-plugin-prettier": "^4.0.0",
    "eslint-rule-documentation": ">=1.0.0",
+    "jsx-ast-utils": "^3.3.2",


This provides utility methods that make parsing JSX elements much more pleasant. I found it through jsx-a11y-plugin.

Repo for jsx-ast-utils

do u also need to use the typescript ast?

I guess I am asking does this rule work for tsx files?

It seems yes, as long as the .eslintrc.json wherever the linters get run is configured with the typescript parser:

"parser": "@typescript-eslint/parser",

I tested this branch out in dotcom by running:

npm i git://github.com/github/eslint-plugin-github.git#kh-add-a11y --save

(^today I learned how to install a package from a GitHub branch!!!)

I see the lint error show up as expected in my editor!

🕺 ! YAY thanks for checking

khiga8 · 2022-07-19T16:28:54Z

lib/configs/react.js

@@ -0,0 +1,10 @@
+module.exports = {


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.

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

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.

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

khiga8 · 2022-07-20T15:54:59Z

Hi 👋

This is ready for review. @github/web-systems-reviewers. I plan to follow up on this with a separate PR where we introduce a config that allows mapping Primer components to a tag, allowing them to be linted. So ultimately we want this lint to also catch <Link>

jbrown1618 · 2022-07-20T20:15:17Z

lib/rules/a11y-no-generic-link-text.js

@@ -0,0 +1,68 @@
+const {elementType, getProp, getPropValue} = require('jsx-ast-utils')
+
+const bannedLinkText = ['read more', 'here', 'click here', 'learn more', 'more', 'here']


nit: 'here' is in this list twice.

jbrown1618 · 2022-07-20T20:21:24Z

lib/rules/a11y-no-generic-link-text.js

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


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.

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

khiga8 added 5 commits July 18, 2022 17:07

add new accessibility rule

ff98741

basically copy over docs from erblint-github

fa7dd7e

fix lints

b69e44f

set up react config

ac1edd7

update test

de77ce7

khiga8 commented Jul 19, 2022

View reviewed changes

khiga8 added 3 commits July 18, 2022 17:54

Add additional check to make sure accessible name fully contains visi…

57dab5d

…ble label

Update test

19eca61

Run prettier

e39e9c6

khiga8 commented Jul 19, 2022

View reviewed changes

khiga8 marked this pull request as ready for review July 19, 2022 01:03

khiga8 requested a review from a team as a code owner July 19, 2022 01:03

khiga8 requested a review from jfuchs July 19, 2022 01:03

add react rules

78b7eeb

khiga8 commented Jul 19, 2022

View reviewed changes

khiga8 added 3 commits July 19, 2022 10:04

update README

c1733dc

update README

af98a18

update location of report

cd9d7eb

khiga8 mentioned this pull request Jul 20, 2022

[spike] Allow custom component to be mapped to element type #283

Merged

jbrown1618 reviewed Jul 20, 2022

View reviewed changes

Remove redundant word in array

eb5d423

jfuchs approved these changes Jul 20, 2022

View reviewed changes

khiga8 merged commit 70a4fb0 into main Jul 21, 2022

khiga8 deleted the kh-add-a11y branch July 21, 2022 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add accessibility rule and React config #282

Add accessibility rule and React config #282

khiga8 commented Jul 19, 2022 •

edited

khiga8 Jul 19, 2022 •

edited

khiga8 Jul 19, 2022

kendallgassner Jul 19, 2022

khiga8 Jul 19, 2022

accessibility-bot commented Jul 19, 2022

khiga8 Jul 19, 2022

khiga8 Jul 19, 2022

kendallgassner Jul 19, 2022

kendallgassner Jul 19, 2022 •

edited

khiga8 Jul 19, 2022

kendallgassner Jul 19, 2022

khiga8 Jul 19, 2022 •

edited

kendallgassner Jul 19, 2022

lindseywild Jul 19, 2022

khiga8 Jul 19, 2022

khiga8 commented Jul 20, 2022 •

edited

jbrown1618 Jul 20, 2022

jbrown1618 Jul 20, 2022

khiga8 Jul 20, 2022 •

edited

		@@ -0,0 +1,68 @@
		const {elementType, getProp, getPropValue} = require('jsx-ast-utils')

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

Add accessibility rule and React config #282

Add accessibility rule and React config #282

Conversation

khiga8 commented Jul 19, 2022 • edited

Motivation

Changes

New accessibility rule

New config

Note to reviewer 📣

khiga8 Jul 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

accessibility-bot commented Jul 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kendallgassner Jul 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 Jul 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 commented Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 Jul 20, 2022 • edited

Choose a reason for hiding this comment

khiga8 commented Jul 19, 2022 •

edited

khiga8 Jul 19, 2022 •

edited

kendallgassner Jul 19, 2022 •

edited

khiga8 Jul 19, 2022 •

edited

khiga8 commented Jul 20, 2022 •

edited

khiga8 Jul 20, 2022 •

edited