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

Flag aria-label and aria-labelledby on non-interactive elements #911

Closed
wants to merge 2 commits into from
Closed

Flag aria-label and aria-labelledby on non-interactive elements #911

wants to merge 2 commits into from

Conversation

smockle
Copy link

@smockle smockle commented Dec 6, 2022

What does this PR accomplish?

Fixes #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
Copy link

codecov bot commented Dec 6, 2022

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
Copy link
Author

smockle commented Dec 6, 2022

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
Copy link
Collaborator

@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
Copy link
Author

smockle commented Dec 7, 2022

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
Copy link
Author

smockle commented Dec 7, 2022

…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">. 👍

@smockle smockle closed this Dec 7, 2022
@jessebeach
Copy link
Collaborator

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

Screen Shot 2022-12-07 at 10 34 46 AM

Screen Shot 2022-12-07 at 10 35 37 AM

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

role-suports-aria-props is not flagging prohibited accessible name usage on generic elements
2 participants