Flag `aria-label` and `aria-labelledby` on non-interactive elements

[smockle]

What does this PR accomplish?

Fixes https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/issues/910

This PR indicates an error in markup like <span aria-label="Hello, world!">Hello</span>.

Why?

span and div elements implicitly have the generic role, and generic prohibits aria-label and aria-labelledby.

From the generic role spec:

The generic role is intended for use as the implicit role of generic elements in host languages (such as HTML div or span)

[…]

Prohibited States and Properties: aria-label, aria-labelledby

Implementation details

Previously, neither div nor span had an implicitly role defined in src/util/implicitRoles/, so the codepath in src/rules/role-supports-aria-props.js#L55-L63 was taken, which “assume[s] that the element can handle the global set of aria-* properties.” This is not a correct assumption, so I added src/util/implicitRoles/div.js and src/util/implicitRoles/span.js and re-exported them from the src/util/implicitRoles/index.js barrel.

Once the src/rules/role-supports-aria-props.js#L65-L70 codepath was taken, I discovered that the list of a role’s prohibitedProps was unused, so I concatenated it to the existing list of invalidAriaPropsForRole.

Next, I discovered that the list of validTests, tests expected to pass without errors, included e.g. <div role="superscript" aria-label />. This initially confused me, since aria-label is one of superscript’s prohibitedProps. I traced this to the superclass—roletype includes aria-label—then added a filter to remove them.

Finally, the addition of src/util/implicitRoles/div.js broke a few role-related tests that assumed div would not have an explicit or implicit role. I replaced div with a made-up custom element named custom-element, which functions the way div did before this PR (i.e. lacking explicit and implicit roles).

Testing details

I ran npm test locally, and all tests pass:

Test Suites: 65 passed, 65 total
Tests:       16078 passed, 16078 total
Snapshots:   0 total
Time:        40.463s, estimated 43s

[codecov[bot]]

Codecov Report

Merging #911 (b8d17cb) into main (4abc751) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #911   +/-   ##
=======================================
  Coverage   99.29%   99.30%           
=======================================
  Files         103      105    +2     
  Lines        1570     1574    +4     
  Branches      514      514           
=======================================
+ Hits         1559     1563    +4     
  Misses         11       11           

Impacted Files	Coverage Δ
src/rules/role-supports-aria-props.js	100.00% <100.00%> (ø)
src/util/implicitRoles/div.js	100.00% <100.00%> (ø)
src/util/implicitRoles/span.js	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[smockle]

I’m stumped about why the Node.js ≤11 checks are failing (example failing run). I didn’t change the language-tags package (or any build/transpilation config), which appears to be the source of the error:

689 /home/runner/work/eslint-plugin-jsx-a11y/eslint-plugin-jsx-a11y/node_modules/language-tags/lib/Tag.js:17
690 static ERR_DEPRECATED = 1;
691                       ^
692
693 SyntaxError: Unexpected token =
694
695    9 |
696   10 | import { propName, getLiteralPropValue } from 'jsx-ast-utils';
697 > 11 | import tags from 'language-tags';
698      | ^
699   12 | import { generateObjSchema } from '../util/schemas';
700   13 | import getElementType from '../util/getElementType';
701   14 |
702
703   at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:537:17)
704   at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:579:25)
705   at Object.<anonymous> (node_modules/language-tags/lib/index.js:11:11)
706   at Object.<anonymous> (src/rules/lang.js:11:1)
707   at Object.<anonymous> (__tests__/src/rules/lang-test.js:12:1)

[jessebeach]

@smockle thank you for the initiative here.

As a matter of principle, we don't want to store knowledge of WAI-ARIA in this plugin. That knowledge should be stored in aria-query: https://github.com/A11yance/aria-query

By doing so, we can leverage a store of all elements that map to specific roles. For example, there are 19 HTML elements that map to the generic role: https://github.com/A11yance/aria-query/blob/main/scripts/roles.json#L2961

The big complication here is that there is no constant indicator that determines if a role supports a label. Some interactive and some non-interactive roles will support a label and some roles have it outright prohibited. We get at this inconsistently right now with the nameFrom field on the Role definition in aria-query. But it hasn't been systematically filled out, so the data is incomplete. Here is the spec: https://github.com/A11yance/aria-query/blob/2d04e2928ed8f1f99950fa5f0075f1f67bfed9c7/scripts/roles.json#LL6C10-L6C10

Once we plumbed that data through aria-query, then we can leverage it in this plugin by filtering roles that prohibit a label on the role.

[smockle]

As a matter of principle, we don't want to store knowledge of WAI-ARIA in this plugin. That knowledge should be stored in aria-query: https://github.com/A11yance/aria-query

By doing so, we can leverage a store of all elements that map to specific roles. For example, there are 19 HTML elements that map to the generic role: https://github.com/A11yance/aria-query/blob/main/scripts/roles.json#L2961

Got it! This makes sense from an architecture perspective. Thanks for explaining, @jessebeach! I’ll close this PR, since its approach isn’t aligned with this. 👍

Out of curiosity, two related questions:

1. Does this mean that the src/util/implicitRoles/ directory will be removed in a future update? Or were you saying that src/util/implicitRoles/div.js and src/util/implicitRoles/span.js are categorically different from the other files there?
2. One of the 19 generic HTML elements in aria-query is a (scripts/roles.json#L2986-L2991). The a element only has the generic role when it lacks an href attribute, right? How does aria-query capture this—is that what concept.attributes is for?

[smockle]

…some non-interactive roles will support a label…

This is a good callout; I’d forgotten about cases like <div role="dialog" aria-labelledby="dialogHeading">. 👍

[jessebeach]

@smockle

Does this mean that the src/util/implicitRoles/ directory will be removed in a future update? Or were you saying that src/util/implicitRoles/div.js and src/util/implicitRoles/span.js are categorically different from the other files there?

This code is a vestige of the project before aria-query and axobject-query were introduced in early 2018. I just haven't had time to remove it :) I've been going through aria-query and axobject-query to get them both in alignment with ARIA 1.3, which will be going into recommendation soon-ish and then become evergreen.

One of the 19 generic HTML elements in aria-query is a (scripts/roles.json#L2986-L2991). The a element only has the generic role when it lacks an href attribute, right? How does aria-query capture this—is that what concept.attributes is for?

Exactly, the attributes define the shape of the concept, when attributes are required to match a role. You can see this in the AX Tree when using the Inspector in Chrome.

Many HTML elements look "semantic", but if they don't have a mapping to an AX Tree Node, then they have no semantic expression exposed to clients. That's what this tech stack is meant to formalize -- the reality of the semantic mappings from HTML (aria-query) and the browser's mapping (axobject-query), their overlap, and the subsequent map that gets exposed as the AX Tree. It's messy, inconsistent and highly idiosyncratic due to layers of history and calcified standards in decades-old tools :)