Bump aria-query and update to fix tests #448

khiga8 · 2023-07-10T17:21:34Z

Follow-up to #447 (comment)
Fixes: https://github.com/github/accessibility/issues/3966

We see breaking tests for our role-supports-aria-props rule with the latest aria-query bump. This is because the latest aria-query bump includes update to the element to role mapping.

It took me a bit to fully understand the logic in our role-supports-aria-props rule. In short, we're:

Creating a new object map based off the map returned by elementRoleMap-test. We remove some constraints from the key which isn't useful to us.
We take the node we are given, and construct a new key.
With this key, we check if there's a match in the object map we constructed.
If there isn't a match, the role is undefined, and we just skip the node in the rule.
If there is a match, we check if there are any prohibited attributes being used.

It looks like our current code does not account for the various new constraints introduced in the latest aria-query. This results in incorrect roles being returned, and the tests for our rule breaking. In addition, some of the key to role mappings have changed.

I recommend reviewing by commit.

The changes in this PR include:

Don't delete the attributes constraints because it will influence the mapping output.
Remove aria-expanded from the disambiguating attributes on summary. This has been removed in these lines of code.
Update the disambiguating attributes to include aria-label and aria-labelledby (restricted to section, aside)
Update the disambiguating attributes to include name restricted to form
Update the disambiguating attributes to include alt restricted to img
Update tests based on changes in aria-query.
Extract logic into a getRole helper and add tests.

In a follow-up PR, I will rename this rule to a11y-role-supports-aria-props to ensure it's tracked by the scorecard.

Other thoughts

Interestingly, the jsx-a11y library hard-codedly defines the logic for a lot of tags in implicitRoles. The downside of that approach is that it gets out of date, and isn't utilize the mapping in aria-query which gets more frequently updated.

The downside of the approach we're taking by constructing a key/map and using the mapping as the source of truth, is that the key we're constructing might stop returning a match depending on updates to aria-query. For example, more recent aria-query updates introduced a bunch of constraints which previously weren't there. I tried to protect against brittleness by adding more tests for getRole which I've extracted. I think we can revisit this later.

- generic does not list `aria-checked`.

- according to https://www.w3.org/TR/html-aam-1.0/, link tag doesn't map to anything even with an href. - therefore, it no longer makes sense to try to determine role and evaluate, so it should be skipped. - accordingly, tests are deleted since we don't want to evaluate it.

- see https://www.w3.org/TR/html-aria/ and https://www.w3.org/TR/html-aam-1.0/

Add additional dis-ambiguating attributes. When `aria-label` or `aria-labelledby` is set at all, constraints should be set.

khiga8 · 2023-07-10T21:00:52Z

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

+          'multiple',
+          'scope',
+          'name',
+        ]) {


I know this doesn't consider ALL constraints, but I think it's fine for now since it will cover most cases we're concerned about (e.g. <span aria-label>.

Whatever isn't matched should be skipped and we should avoid raising false positives.

accessibility-bot · 2023-07-10T21:01:22Z

👋 Hello and thanks for pinging us! You've entered our first responder queue. 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.

khiga8 · 2023-07-10T21:03:19Z

tests/role-supports-aria-props.js

-    // this will have global
-    {code: '<a aria-checked />'},


This is now treated as generic

khiga8 · 2023-07-10T21:04:30Z

tests/role-supports-aria-props.js

@@ -78,30 +75,6 @@ ruleTester.run('role-supports-aria-props', rule, {
    {code: '<area href="#" aria-owns />'},
    {code: '<area href="#" aria-relevant />'},

-    // this will have global
-    {code: '<area aria-checked />'},


This is now treated as generic

khiga8 · 2023-07-10T21:05:04Z

tests/role-supports-aria-props.js

-    // this will have global
-    {code: '<area aria-checked />'},
-
-    // LINK TESTS - implicit role is `link`


<link> maps to nothing now so I'm deleting these tests.

khiga8 · 2023-07-10T21:05:44Z

tests/role-supports-aria-props.js

+      errors: [getErrorMessage('aria-expanded', 'generic')],
+    },
+    {
+      code: '<section aria-label="something" aria-expanded />',


If <section aria-label>, we treat it as region.

khiga8 · 2023-07-11T15:40:43Z

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

@@ -37,27 +36,8 @@ module.exports = {
  create(context) {
    return {
      JSXOpeningElement(node) {
-        // Assemble a key for looking-up the element’s role in the `elementRolesMap`


I'm extracting this entire section a new helper called getRole, with tests.

khiga8 · 2023-07-11T15:41:50Z

tests/utils/get-element-type.js

 const mocha = require('mocha')
 const describe = mocha.describe
 const it = mocha.it
 const expect = require('chai').expect

-function mockJSXAttribute(prop, propValue) {


I'm extracting this into a helper.

khiga8 · 2023-07-11T16:05:10Z

lib/utils/get-role.js

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


I would appreciate @smockle eyes on this! I extracted the code into this helper, and made updates to fix the bug.

khiga8 · 2023-07-11T16:06:04Z

tests/utils/helpers.js

@@ -0,0 +1,26 @@
+function mockJSXAttribute(prop, propValue) {


This is used in multiple tests now, so extracted it into this file.

Maybe we can call this file mocks? -- I am more likely to explore mocks for testing then helpers for testing

khiga8 · 2023-07-11T17:43:15Z

lib/utils/get-role.js

+// Clean-up `elementRoles` from `aria-query`
+const elementRolesMap = new ObjectMap()


This code is constructing a cleaned up version of elementRoles. Below, we'll be constructing a key and grabbing the value from elementRoles.

khiga8 · 2023-07-11T17:53:16Z

There won't always be a matching role returned but I think that's fine, as long as we cover the majority of cases.

jfuchs

Thank you for this! If you haven't already, it might be nice to verify in a branch that if you rebase this dependabot update on top of this this PR, tests pass.

khiga8 · 2023-07-11T19:53:36Z

@jfuchs this PR already includes the same aria-query bump as in the dependabot PR, so I think we should be good!!

smockle · 2023-07-12T19:34:23Z

lib/utils/get-role.js

+  // - Remove unused `constraints` key
+  delete key.constraints


Is this line still necessary?

Oooh nice catch! It looks like removing this constraints can result in incorrect mappings in some scenarios...

For example, elementRoles includes:

[{"constraints": ["ancestor table element has table role"], "name": "td"}, ["cell"]] [{"constraints": ["ancestor table element has grid role", "ancestor table element has treegrid role"], "name": "td"}, ["gridcell"]]

If we remove the constraints key, we're left with:

[{"name": "td"}, ["cell"]] [{"name": "td"}, ["gridcell"]]

Since we cannot always accurately parse out the role of an ancestor in a linter context, I think we'd want to return undefined instead of potentially returning a false positive role.

I will follow-up!

I deleted this line in 8fb6419!

However, there were two scenarios where it seemed appropriate to assume the role despite the constraints defined in aria-query. I opted to hard-code those with a comment on why! You can see the commit for more information.

I've added additional tests in aaca700#diff-a6b7fa0d4a2be411ccfcc6e6db5466c135100a1a045599f5c211bb91c37b839dR180-R190.

smockle

This looks good to me!

The basic approach—normalizing an element key, retrieving a role, finding (dis)allowed props for that role—hasn’t changed. 👍
Incorporating (new) constraints from aria-query improves disambiguation. (And I agree this covers most cases.) 👍
The added/updated tests assert roles that look correct. 👍
Tests are passing against the updated version of aria-query (related: chore(deps): bump the all-dependencies group with 7 updates #452). 👍
Extracting chunks of logic into separate functions/files improves readability/testability. 👍

Nice work @khiga8! The changes that landed in aria-query were tricky to decipher; I appreciate you tackling the complexity! ✨

…pite constraints

khiga8 · 2023-07-12T21:15:11Z

I moved the map constructing code into a function with additional code comments in 40c0b2b.

khiga8 · 2023-07-12T21:56:56Z

I will merge this PR and #454 in tomorrow morning (EST)!!!

khiga8 added 5 commits July 10, 2023 13:18

bump aria-query to 5.3.0

951feea

Don't delete all of attributes

95c60cf

a and area without href are now generic,

762f7e1

- generic does not list `aria-checked`.

body now maps to generic role

d21a25e

- see https://www.w3.org/TR/html-aria/ and https://www.w3.org/TR/html-aam-1.0/

khiga8 force-pushed the kh-bump-aria-query branch from ff41732 to d21a25e Compare July 10, 2023 18:12

khiga8 added 3 commits July 10, 2023 15:21

summary does not map to anything anymore

281fa95

Add aria-label and aria-labelledby and alt and name

33fe0b0

Add additional dis-ambiguating attributes. When `aria-label` or `aria-labelledby` is set at all, constraints should be set.

update tests to reflect truth

985fa01

khiga8 commented Jul 10, 2023

View reviewed changes

khiga8 marked this pull request as ready for review July 10, 2023 21:01

khiga8 requested a review from a team as a code owner July 10, 2023 21:01

khiga8 requested review from theinterned, smockle and a team July 10, 2023 21:01

khiga8 commented Jul 10, 2023

View reviewed changes

khiga8 marked this pull request as draft July 10, 2023 21:07

This comment was marked as outdated.

Sign in to view

khiga8 added 3 commits July 11, 2023 11:36

Extract test helpers

247eea2

Extract getRole into its own helper

5c98a44

Add tests for getRole

ba52aca

khiga8 force-pushed the kh-bump-aria-query branch from 39d5d98 to ba52aca Compare July 11, 2023 15:39

khiga8 commented Jul 11, 2023

View reviewed changes

Add more tests for getRole

b12cda5

khiga8 marked this pull request as ready for review July 11, 2023 15:56

khiga8 commented Jul 11, 2023

View reviewed changes

Add more test

9136431

khiga8 force-pushed the kh-bump-aria-query branch from 3d343b4 to 9136431 Compare July 11, 2023 16:44

khiga8 commented Jul 11, 2023

View reviewed changes

jfuchs approved these changes Jul 11, 2023

View reviewed changes

jfuchs mentioned this pull request Jul 11, 2023

chore(deps): bump the all-dependencies group with 7 updates #452

Closed

Remove more forgotten, unnecessary code

2fa5eca

khiga8 force-pushed the kh-bump-aria-query branch from 9cc41d8 to 2fa5eca Compare July 12, 2023 13:57

theinterned removed their request for review July 12, 2023 14:43

Rename helpers to mocks

b2ed30e

khiga8 force-pushed the kh-bump-aria-query branch from 4d67b61 to b2ed30e Compare July 12, 2023 16:40

smockle reviewed Jul 12, 2023

View reviewed changes

smockle approved these changes Jul 12, 2023

View reviewed changes

khiga8 added 4 commits July 12, 2023 16:53

Delete constraints, and hard-code roles we benefit from enforcing des…

8fb6419

…pite constraints

update tests

5a66033

add more test coverage of roles

aaca700

Move map constructing code into a function

40c0b2b

This was referenced Jul 12, 2023

Rename role-supports-aria-props to a11y- #454

Merged

Make sure recommended.js is up-to-date and dependecies are up-to-date primer/eslint-plugin-primer-react#56

Closed

khiga8 merged commit 8d691db into main Jul 13, 2023
6 checks passed

khiga8 deleted the kh-bump-aria-query branch July 13, 2023 13:10

khiga8 mentioned this pull request Jul 17, 2023

Set config override for false positive rule #460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump aria-query and update to fix tests #448

Bump aria-query and update to fix tests #448

khiga8 commented Jul 10, 2023 •

edited

khiga8 Jul 10, 2023 •

edited

accessibility-bot commented Jul 10, 2023

khiga8 Jul 10, 2023

khiga8 Jul 10, 2023

khiga8 Jul 10, 2023 •

edited

khiga8 Jul 10, 2023

This comment was marked as outdated.

khiga8 Jul 11, 2023 •

edited

khiga8 Jul 11, 2023

khiga8 Jul 11, 2023 •

edited

khiga8 Jul 11, 2023

kendallgassner Jul 12, 2023

khiga8 Jul 11, 2023

khiga8 commented Jul 11, 2023

jfuchs left a comment

khiga8 commented Jul 11, 2023

smockle Jul 12, 2023

khiga8 Jul 12, 2023

khiga8 Jul 12, 2023

smockle left a comment

khiga8 commented Jul 12, 2023

khiga8 commented Jul 12, 2023

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

		@@ -0,0 +1,26 @@
		function mockJSXAttribute(prop, propValue) {

		// Clean-up `elementRoles` from `aria-query`
		const elementRolesMap = new ObjectMap()

Bump aria-query and update to fix tests #448

Bump aria-query and update to fix tests #448

Conversation

khiga8 commented Jul 10, 2023 • edited

Other thoughts

khiga8 Jul 10, 2023 • edited

Choose a reason for hiding this comment

accessibility-bot commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 Jul 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

khiga8 Jul 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khiga8 Jul 11, 2023 • 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 11, 2023

jfuchs left a comment

Choose a reason for hiding this comment

khiga8 commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smockle left a comment

Choose a reason for hiding this comment

khiga8 commented Jul 12, 2023

khiga8 commented Jul 12, 2023

khiga8 commented Jul 10, 2023 •

edited

khiga8 Jul 10, 2023 •

edited

khiga8 Jul 10, 2023 •

edited

khiga8 Jul 11, 2023 •

edited

khiga8 Jul 11, 2023 •

edited