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

smockle · 2022-12-17T06:21:18Z

Fixes https://github.com/github/accessibility/issues/2427

From that issue:

If you use <span aria-label="I’m a tooltip!">Info</span> in Rails, ERBLint’s NoAriaLabelMisue rule will flag it, since aria-label is not supported on non-interactive elements like span.

If you use <span aria-label="I’m a tooltip!">Info</span> in React, ESLint doesn’t flag it.

This PR adapts the work I did in jsx-eslint/eslint-plugin-jsx-a11y#911 into a custom rule we can use until the upstream role-suports-aria-props is updated to flag <span aria-label="I’m a tooltip!">Info</span>.

smockle · 2022-12-17T06:25:24Z

@jfuchs Tests pass locally, but it looks like CI checks are failing with Node.js 12.x, but Node.js 12.x has reached end-of-life: https://nodejs.dev/en/about/releases/. Can the Node.js 12.x CI checks be removed?

smockle · 2022-12-17T06:25:56Z

.eslintrc.js

@@ -1,7 +1,7 @@
 module.exports = {
  root: true,
  parserOptions: {
-    ecmaVersion: 2020
+    ecmaVersion: 13


This was added to support private fields (#data on ObjectMap)

smockle · 2022-12-17T06:26:31Z

docs/rules/role-supports-aria-props.md

@@ -0,0 +1,74 @@
+# Role Supports ARIA Props


This was adapted from https://github.com/github/erblint-github/blob/main/docs/rules/accessibility/no-aria-label-misuse-counter.md

smockle · 2022-12-17T06:28:40Z

tests/utils/object-map.js

@@ -0,0 +1,180 @@
+// @ts-check


Every method of ObjectMap has at least one test in this file.

smockle · 2022-12-17T06:36:55Z

lib/rules/role-supports-aria-props.js

+const {getElementType} = require('../utils/get-element-type')
+const ObjectMap = require('../utils/object-map')
+
+// Clean-up `elementRoles` from `aria-query`


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).

smockle · 2022-12-17T06:40:03Z

tests/role-supports-aria-props.js

@@ -0,0 +1,512 @@
+// @ts-check
+
+// Tests in this file were adapted from https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/__tests__/src/rules/role-supports-aria-props-test.js, which was authored by Ethan Cohen and is distributed under the MIT license as follows:


Do we have a convention for this kind of attribution?

smockle · 2022-12-19T15:19:09Z

lib/utils/object-map.js

@@ -0,0 +1,58 @@
+// @ts-check
+const {isDeepStrictEqual} = require('util')


Shoutout to util.isDeepStrictEqual, which I (re?)discovered while working on this.

It’s better than JSON.stringify-based object comparison because it:

Supports objects which can’t be serialized (e.g. due to circular references), and

Ignores key order when determining equality

smockle · 2022-12-19T15:19:43Z

README.md

@@ -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"},


This was Prettier.

smockle · 2022-12-19T15:25:29Z

lib/rules/role-supports-aria-props.js

+        // - 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']) {


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.

README.md

…d rules” section

khiga8

Could we additionally configure this rule in: configs/react.js? This will ensure that the rule is enabled by default for those using this plugin's react preset.

This otherwise looks good to me! Deferring to @github/web-systems-reviewers for final approval.

khiga8 · 2022-12-19T16:26:49Z

tests/role-supports-aria-props.js

+      code: '<div aria-label />',
+      errors: [getErrorMessage('aria-label', 'generic')]
+    },
+    {
+      code: '<div aria-labelledby />',
+      errors: [getErrorMessage('aria-labelledby', 'generic')]


…upports-aria-props' in the React config

smockle · 2022-12-19T19:00:50Z

From @khiga8 in #362 (review):

Could we additionally configure this rule in: configs/react.js? This will ensure that the rule is enabled by default for those using this plugin's react preset.

Good point! Done in e2d8376.

I chose 'error', since that’s what we used previously, via jsx-a11y’s recommended set: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/4abc751d87a8491219a9a3d2dacd80ea8adcb79b/src/index.js#L196

theinterned · 2022-12-29T22:39:15Z

@jfuchs Tests pass locally, but it looks like CI checks are failing with Node.js 12.x, but Node.js 12.x has reached end-of-life: https://nodejs.dev/en/about/releases/. Can the Node.js 12.x CI checks be removed?

@smockle I just saw this comment! Lo and behold, I also just opened this PR #369

smockle · 2023-01-03T14:28:37Z

@jfuchs Tests pass locally, but it looks like CI checks are failing with Node.js 12.x, but Node.js 12.x has reached end-of-life: https://nodejs.dev/en/about/releases/. Can the Node.js 12.x CI checks be removed?

@smockle I just saw this comment! Lo and behold, I also just opened this PR #369

Awesome, thanks @theinterned! I’ll wait for #369 to be merged, then rebase here.

smockle · 2023-01-10T18:03:01Z

…I’ll wait for #369 to be merged, then rebase here.

#380 (which supersedes #369) was merged; rebasing this PR now!

jfuchs · 2023-01-10T18:29:14Z

The prettier config update serial commas, sorry. eslint . --fix should get your branch right!

smockle · 2023-01-10T19:55:16Z

@jfuchs Thanks! I added the missing serial commas in cd0a526. Now CI is green and there are 2 approvals; is anything else required before I merge this?

jfuchs

@jfuchs Thanks! I added the missing serial commas in cd0a526. Now CI is green and there are 2 approvals; is anything else required before I merge this?

Nope, but here's an approval for good measure!

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [eslint-plugin-github](https://togithub.com/github/eslint-plugin-github) | [`4.6.0` -> `4.6.1`](https://renovatebot.com/diffs/npm/eslint-plugin-github/4.6.0/4.6.1) | [![age](https://badges.renovateapi.com/packages/npm/eslint-plugin-github/4.6.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/eslint-plugin-github/4.6.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/eslint-plugin-github/4.6.1/compatibility-slim/4.6.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/eslint-plugin-github/4.6.1/confidence-slim/4.6.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>github/eslint-plugin-github</summary> ### [`v4.6.1`](https://togithub.com/github/eslint-plugin-github/releases/tag/v4.6.1) [Compare Source](https://togithub.com/github/eslint-plugin-github/compare/v4.6.0...v4.6.1) #### What's Changed - Update events handled by `no-useless-passive` and `require-passive-events` by [@boris-petrov](https://togithub.com/boris-petrov) in [github/eslint-plugin-github#354 - feat: `role-supports-aria-props` rule (no `aria-label`-misuse) by [@smockle](https://togithub.com/smockle) in [github/eslint-plugin-github#362 - Updated dependencies #### New Contributors - [@boris-petrov](https://togithub.com/boris-petrov) made their first contribution in [github/eslint-plugin-github#354 - [@smockle](https://togithub.com/smockle) made their first contribution in [github/eslint-plugin-github#362 **Full Changelog**: github/eslint-plugin-github@v4.6.0...v4.6.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "monthly" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/WtfJoke/setup-tectonic).

smockle added 2 commits December 17, 2022 05:30

wip: role-supports-aria-props

8c9445e

fix: Tests passing

4c71d04

smockle requested a review from a team as a code owner December 17, 2022 06:21

smockle requested a review from jfuchs December 17, 2022 06:21

smockle changed the title ~~Feat: role-supports-aria-props rule (no aria-label-misuse)~~ feat: role-supports-aria-props rule (no aria-label-misuse) Dec 17, 2022

smockle requested a review from khiga8 December 17, 2022 06:22

smockle commented Dec 17, 2022

View reviewed changes

tests/utils/object-map.js

@@ -0,0 +1,180 @@

// @ts-check

Copy link

Contributor Author

smockle Dec 17, 2022

Choose a reason for hiding this comment

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

Every method of ObjectMap has at least one test in this file.

smockle commented Dec 17, 2022

View reviewed changes

smockle commented Dec 19, 2022

View reviewed changes

khiga8 reviewed Dec 19, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

docs: Move 'role-supports-aria-props' rule into “Accessibility-focsue…

ca32268

…d rules” section

smockle requested a review from khiga8 December 19, 2022 16:44

khiga8 approved these changes Dec 19, 2022

View reviewed changes

feat: Replace 'jsx-a11y/role-supports-aria-props' with 'github/role-s…

e2d8376

…upports-aria-props' in the React config

theinterned approved these changes Dec 28, 2022

View reviewed changes

Merge branch 'main' into smockle/wip

84c6fe5

smockle added 3 commits January 10, 2023 19:43

chore: Run Prettier

5497b3a

Merge branch 'main' into smockle/wip

50a07db

chore: Run Prettier

cd0a526

jfuchs approved these changes Jan 10, 2023

View reviewed changes

smockle merged commit cc51b62 into main Jan 10, 2023

smockle deleted the smockle/wip branch January 10, 2023 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

smockle commented Dec 17, 2022

smockle commented Dec 17, 2022 •

edited

smockle Dec 17, 2022

smockle Dec 17, 2022

smockle Dec 17, 2022

smockle Dec 17, 2022 •

edited

smockle Dec 17, 2022

smockle Dec 19, 2022

smockle Dec 19, 2022 •

edited

smockle Dec 19, 2022

khiga8 left a comment

khiga8 Dec 19, 2022

smockle commented Dec 19, 2022

theinterned commented Dec 29, 2022

smockle commented Jan 3, 2023

smockle commented Jan 10, 2023

jfuchs commented Jan 10, 2023

smockle commented Jan 10, 2023

jfuchs left a comment

		@@ -0,0 +1,512 @@
		// @ts-check

		// Tests in this file were adapted from https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/__tests__/src/rules/role-supports-aria-props-test.js, which was authored by Ethan Cohen and is distributed under the MIT license as follows:

		@@ -0,0 +1,58 @@
		// @ts-check
		const {isDeepStrictEqual} = require('util')

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

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

Conversation

smockle commented Dec 17, 2022

smockle commented Dec 17, 2022 • edited

smockle Dec 17, 2022

Choose a reason for hiding this comment

smockle Dec 17, 2022

Choose a reason for hiding this comment

smockle Dec 17, 2022

Choose a reason for hiding this comment

smockle Dec 17, 2022 • edited

Choose a reason for hiding this comment

smockle Dec 17, 2022

Choose a reason for hiding this comment

smockle Dec 19, 2022

Choose a reason for hiding this comment

smockle Dec 19, 2022 • edited

Choose a reason for hiding this comment

smockle Dec 19, 2022

Choose a reason for hiding this comment

khiga8 left a comment

Choose a reason for hiding this comment

khiga8 Dec 19, 2022

Choose a reason for hiding this comment

smockle commented Dec 19, 2022

theinterned commented Dec 29, 2022

smockle commented Jan 3, 2023

smockle commented Jan 10, 2023

jfuchs commented Jan 10, 2023

smockle commented Jan 10, 2023

jfuchs left a comment

Choose a reason for hiding this comment

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

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

smockle commented Dec 17, 2022 •

edited

smockle Dec 17, 2022 •

edited

smockle Dec 19, 2022 •

edited