core(a11y): add relatedNodes to accessibility audits #13193

connorjclark · 2021-10-07T23:47:25Z

Grabs the relatedNodes from axe's CheckResults, and displays them as subitems for every axe audit.

Demo: https://googlechrome.github.io/lighthouse/viewer/?gist=8c8747c376d7b592427ee5da8c954f19

Duplicate id audits:

And others!

paulirish · 2021-10-09T00:58:22Z

lighthouse-core/gather/gatherers/accessibility.js

@@ -87,10 +87,23 @@ function createAxeRuleResultArtifact(result) {
    // @ts-expect-error - getNodeDetails put into scope via stringification
    const nodeDetails = getNodeDetails(/** @type {HTMLElement} */ (element));

+    const relatedNodeDetails = [];
+    for (const checkResult of [...node.any, ...node.all, ...node.none]) {


i've never understood any/all/none and reading https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#results-object doesnt help me out.

but i'm not confident we want relatedNodes from ALL of them. we sure about that? maybe we ask our axe friends to explain it?

They simply control whether the associated axe rule fails: a axe rule consists of checks split between "any", "all", "none". Which one a check is in is totally of no interest to us.

https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#:~:text=This%20is%20a%20list%20of%20checks%20that%2C%20if%20none%20%22pass%22%2C%20will%20generate%20a%20violation

Agree with @paulirish that the docs there are pretty opaque. Are they positive tests vs negative tests? Are the related nodes always offending nodes or are some just related and possibly related nodes actually doing the right thing?

Maybe it would help to give some example axe audits that have related elements in one of node.any, node.all, and node.none?

I guess I had imagined that what "related" meant would vary per accessibility test, so we'd have to turn it on for some but it wouldn't be relevant for others. It would be great if this works out instead, but we do need to be sure.

there are a couple of weird ones in the demo report, like contrast appears to just be the same element:

and the node audit details fallback in a subitem ends up kind of confusing:

Are all of the related nodes going to be relevant to the failure? I'm worried that the 3 nodes we surface are not going to be as important as nodes further down the list.

Maybe sorting the checks by impact could be useful?

@straker could you help us out with this? The motivation: #10396

From what i can tell, axe 'rules' are fairly equivalent to lighthouse 'audits'. However axe rules don't have actual implementation of their own, they are just metadata and depend on 'checks'. Checks have 2 components: the 'evaluate' (which is like Lighthouse gatherers) and the 'after` which is like LH audit implementation).

The any/all/none bit in the results, I presume, matches how the rule is defined. I don't totally grok this part. But.. maybe we don't need to know. :p

If we want to surface all the relatedNodes that are relevant to the specific node, should we just look within all 3 arrays? For all the duplicate-* rules, this seems fine, but I wanted a gut-check from you on others to make sure it's sensible. :)

We rely on indentation for subitems to visually make sense...perhaps we should change how they look? Maybe some thick border-left adornment would help?

Maybe it would help to give some example axe audits that have related elements in one of node.any, node.all, and node.none?

here's a none with the list rule:

the rule sez 'only put <li>s inside of lists', but then this is a <button> as a child of a <ul>. makes sense. this qualifies as "related" for sure. :)

color-contrast

The relatedNodes i'm seeing here are often the parent container. On the LH github page, the rule flags some of the bash commands (gray on gray) and the relatedNode is the <pre>. (seems... OK). But then it also flags the page's footer links, and the relatedNode is the <body>. (I guess the related node is who owns the background-color.) including these are probably useful, but.. it'll certainly be more noisy

some all with the aria-hidden-focus rule

The each node has 1 related node and it's always the same DOM element. (at least for me on this page).

So that's a case that we'd probably want to filter out.

but i'll also just add that the axe Chrome extension presents these related nodes basically in the same way. it appears it includes all of them from any/all/none.

So for a rule, the any, all, and none arrays dictate how the rule passes / fails. The any array means that at least one check must return true for the rule to pass. The all array means all checks must return true for the rule to pass. The none array means that none of the checks must return true.

From what I understand, the relatedNodes property should be used when an element's failure relates to another node (in the case of unique-id and the list rule). That doesn't appear to be how it's used in all cases, so I would say it's safe to not use the relatedNodes if it's the same element as the failure node. color-contrast will add the relatedNode to which element applies the background, but also if it returns as needs review (incomplete) it will point to the element that caused it to do so (like an overlapping element or an ancestor with a pseudo element).

As for getting all the relatedNodes from all arrays, that should be fine. We do some filtering of which check result get added to the final result, so they should only be for the relevant checks that caused the failure.

@straker okay. super useful and that aligns with my understanding and expectations. thanks!

lighthouse-core/gather/gatherers/accessibility.js

brendankenny

nice! I think this will be a life saver for the confusing duplicate * accessibility audits

lighthouse-core/gather/gatherers/accessibility.js

brendankenny · 2021-10-12T15:27:04Z

lighthouse-core/gather/gatherers/accessibility.js

@@ -87,10 +87,23 @@ function createAxeRuleResultArtifact(result) {
    // @ts-expect-error - getNodeDetails put into scope via stringification
    const nodeDetails = getNodeDetails(/** @type {HTMLElement} */ (element));

+    const relatedNodeDetails = [];
+    for (const checkResult of [...node.any, ...node.all, ...node.none]) {


Agree with @paulirish that the docs there are pretty opaque. Are they positive tests vs negative tests? Are the related nodes always offending nodes or are some just related and possibly related nodes actually doing the right thing?

Maybe it would help to give some example axe audits that have related elements in one of node.any, node.all, and node.none?

I guess I had imagined that what "related" meant would vary per accessibility test, so we'd have to turn it on for some but it wouldn't be relevant for others. It would be great if this works out instead, but we do need to be sure.

lighthouse-core/test/audits/accessibility/axe-audit-test.js

lighthouse-core/audits/accessibility/axe-audit.js

adamraine · 2021-10-12T15:54:42Z

lighthouse-core/gather/gatherers/accessibility.js

+    for (const checkResult of [...node.any, ...node.all, ...node.none]) {
+      for (const relatedNode of checkResult.relatedNodes || []) {
+        // Prevent overloading the report with way too many nodes.
+        if (relatedNodeDetails.length >= 3) break;


You don't have to do it, but a label could be used to break the outer loop.

adamraine · 2021-10-12T16:09:02Z

lighthouse-core/gather/gatherers/accessibility.js

@@ -87,10 +87,23 @@ function createAxeRuleResultArtifact(result) {
    // @ts-expect-error - getNodeDetails put into scope via stringification
    const nodeDetails = getNodeDetails(/** @type {HTMLElement} */ (element));

+    const relatedNodeDetails = [];
+    for (const checkResult of [...node.any, ...node.all, ...node.none]) {


Are all of the related nodes going to be relevant to the failure? I'm worried that the 3 nodes we surface are not going to be as important as nodes further down the list.

Maybe sorting the checks by impact could be useful?

…nodes

lighthouse-core/gather/gatherers/accessibility.js

brendankenny · 2021-10-12T20:12:14Z

all the subitems appear to be missing from sample_v2 now :)

edit: oh, ha, they were all empty before. I guess that works :)

connorjclark · 2021-10-12T20:17:18Z

yea, un-fortch dbw has no a11y things, they are all in the a11y_tester. Shrug.

connorjclark · 2021-10-20T19:44:38Z

anything else?

lighthouse-core/gather/gatherers/accessibility.js

…nodes

core(a11y): add relatedNodes to accessibility audits

1c6926d

connorjclark requested a review from a team as a code owner October 7, 2021 23:47

connorjclark requested review from adamraine and removed request for a team October 7, 2021 23:47

google-cla bot added the cla: yes label Oct 7, 2021

devtools-bot assigned adamraine Oct 7, 2021

devtools-bot added the waiting4reviewer label Oct 7, 2021

json

b240121

vercel bot deployed to Preview October 7, 2021 23:50 View deployment

real json update

36c5d06

vercel bot deployed to Preview October 7, 2021 23:52 View deployment

limit

eef5b77

vercel bot deployed to Preview October 8, 2021 18:31 View deployment

connorjclark added 2 commits October 8, 2021 14:34

ugh

a0112e7

axetest

5d47eec

vercel bot deployed to Preview October 8, 2021 21:42 View deployment

bit more

b03db74

vercel bot deployed to Preview October 8, 2021 21:44 View deployment

paulirish reviewed Oct 9, 2021

View reviewed changes

brendankenny reviewed Oct 12, 2021

View reviewed changes

adamraine reviewed Oct 12, 2021

View reviewed changes

connorjclark added 2 commits October 12, 2021 11:28

smoke

22a9aca

update

1550ee9

vercel bot deployed to Preview October 12, 2021 18:30 View deployment

connorjclark added 2 commits October 12, 2021 11:30

Merge remote-tracking branch 'origin/master' into axe-audits-related-…

d405289

…nodes

do not show same el twice

10cee43

vercel bot deployed to Preview October 12, 2021 18:34 View deployment

sort

0a47f1c

vercel bot deployed to Preview October 12, 2021 18:51 View deployment

connorjclark commented Oct 12, 2021

View reviewed changes

lighthouse-core/gather/gatherers/accessibility.js Show resolved Hide resolved

brendankenny reviewed Oct 12, 2021

View reviewed changes

lighthouse-core/gather/gatherers/accessibility.js Outdated Show resolved Hide resolved

pr

b019118

vercel bot deployed to Preview October 12, 2021 20:26 View deployment

connorjclark added the 9.0 label Oct 12, 2021

adamraine reviewed Oct 21, 2021

View reviewed changes

lighthouse-core/gather/gatherers/accessibility.js Outdated Show resolved Hide resolved

lol

ec5d317

vercel bot deployed to Preview October 21, 2021 18:38 View deployment

adamraine approved these changes Oct 21, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into axe-audits-related-…

9b7433b

…nodes

vercel bot deployed to Preview October 21, 2021 18:54 View deployment

connorjclark merged commit 8577fd6 into master Oct 21, 2021

connorjclark deleted the axe-audits-related-nodes branch October 21, 2021 19:52

connorjclark mentioned this pull request Mar 25, 2024

Better error messages in a11y test #12282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(a11y): add relatedNodes to accessibility audits #13193

core(a11y): add relatedNodes to accessibility audits #13193

connorjclark commented Oct 7, 2021

paulirish Oct 9, 2021

connorjclark Oct 9, 2021 •

edited

brendankenny Oct 12, 2021 •

edited

brendankenny Oct 12, 2021

adamraine Oct 12, 2021

paulirish Oct 12, 2021

connorjclark Oct 12, 2021

paulirish Oct 12, 2021

straker Oct 12, 2021 •

edited

paulirish Oct 12, 2021

brendankenny left a comment

brendankenny Oct 12, 2021 •

edited

adamraine Oct 12, 2021

adamraine Oct 12, 2021

brendankenny commented Oct 12, 2021 •

edited

connorjclark commented Oct 12, 2021

connorjclark commented Oct 20, 2021

core(a11y): add relatedNodes to accessibility audits #13193

core(a11y): add relatedNodes to accessibility audits #13193

Conversation

connorjclark commented Oct 7, 2021

Choose a reason for hiding this comment

connorjclark Oct 9, 2021 • edited

Choose a reason for hiding this comment

brendankenny Oct 12, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

here's a none with the list rule:

color-contrast

some all with the aria-hidden-focus rule

straker Oct 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Oct 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Oct 12, 2021 • edited

connorjclark commented Oct 12, 2021

connorjclark commented Oct 20, 2021

connorjclark Oct 9, 2021 •

edited

brendankenny Oct 12, 2021 •

edited

here's a `none` with the `list` rule:

some `all` with the `aria-hidden-focus` rule

straker Oct 12, 2021 •

edited

brendankenny Oct 12, 2021 •

edited

brendankenny commented Oct 12, 2021 •

edited