deps(axe-core): upgrade to 4.1.2 #12075
Conversation
connorjclark
requested review from
patrickhulce
and removed request for
a team
| { | ||
| "impact": "critical", | ||
| "html": "<img src=\"blob:http://localhost:10200/54001a08-60c6-4ea1-aeec-ca1831fd81f1\">", | ||
| "html": "<img src=\"blob:http://localhost:10200/65381135-47e9-4deb-9f84-bfc18dbf2534\">", |
There was a problem hiding this comment.
These urls are random for each run.
Still glad to have done the rebaseline by itself, otherwise it wouldn't be clear that there is no difference in this version from 4.1.1.
| ], | ||
| "Accessibility": { | ||
| "violations": [ | ||
| { |
There was a problem hiding this comment.
I'm thinking this went away because the smoke test page has since been "fixed" of a color contrast issue. Or maybe the axe violation check was modified since last time this ran.
Do we want to resolve this?
There was a problem hiding this comment.
Given it was a consequence of us rebaselining and not of this version bump, I'm not concerned. IMO, the goal here of sample artifacts is to test at least some of the axe-audits and it's not particularly important which ones get exercised.
There was a problem hiding this comment.
it is still failing on stable but not in Canary. Looks like the root cause is the text color styling via /deep/ but that not being supported anymore:
lighthouse/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
Lines 109 to 111 in c6e14d7
There was a problem hiding this comment.
Nice find! Do you agree it's not a blocker here @brendankenny ?
There was a problem hiding this comment.
Yes. We should probably update at some point just because that /deep/ does nothing (I think we also previously removed the deprecation notice it used to generate because Chrome doesn't even bother anymore), but agreed that's more a smoke test thing, not the purpose of the sample artifacts/sample_v2
| "id": "aria-progressbar-name", | ||
| "title": "ARIA `progressbar` elements have accessible names", | ||
| "description": "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).", | ||
| "score": 1, |
There was a problem hiding this comment.
It seems these were trivially passing w/ score 1 because aria-progressbar-name check was never found in the axe results, so when LH sees that it just spits out score: 1. https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/accessibility/axe-audit.js#L101
side note: https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/accessibility/axe-audit.js#L37 why is score 1 if this is marked n/a? Shouldn't we do null, because that's what the audit result will end up being anyway?
There was a problem hiding this comment.
Extra explanation: I scratched my head for a second, but in order for us to mark as notApplicable if the audit isn't "informative" we need to explicitly find it in axe's inapplicable set.
why is score 1 if this is marked n/a? Shouldn't we do null, because that's what the audit result will end up being anyway?
Yeah we should. My guess is that while authoring "not applicable" feels closest to "this is passing, don't worry about it" and then 1 gets used.
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
LGTM though we're both operating on the same source of advice here :)
| ], | ||
| "Accessibility": { | ||
| "violations": [ | ||
| { |
There was a problem hiding this comment.
Given it was a consequence of us rebaselining and not of this version bump, I'm not concerned. IMO, the goal here of sample artifacts is to test at least some of the axe-audits and it's not particularly important which ones get exercised.
| "id": "aria-progressbar-name", | ||
| "title": "ARIA `progressbar` elements have accessible names", | ||
| "description": "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).", | ||
| "score": 1, |
There was a problem hiding this comment.
Extra explanation: I scratched my head for a second, but in order for us to mark as notApplicable if the audit isn't "informative" we need to explicitly find it in axe's inapplicable set.
why is score 1 if this is marked n/a? Shouldn't we do null, because that's what the audit result will end up being anyway?
Yeah we should. My guess is that while authoring "not applicable" feels closest to "this is passing, don't worry about it" and then 1 gets used.
| /** @type {import('axe-core/axe')} */ | ||
| // @ts-expect-error axe defined by axeLibSource | ||
| const axe = window.axe; | ||
| axe.configure({ |
|
|
||
| /** @param {import('axe-core/axe').Result} result */ | ||
| const augmentAxeNodes = result => { | ||
| result.helpUrl = result.helpUrl.replace(application, 'lighthouse'); |
There was a problem hiding this comment.
👍 carrying this uid to the url seemed odd. good call.
There was a problem hiding this comment.
do we use helpUrl anywhere?
There was a problem hiding this comment.
seems we don't even use this field but w.e.
| // Augment the node objects with outerHTML snippet & custom path string | ||
| axeResults.violations.forEach(augmentAxeNodes); | ||
| axeResults.incomplete.forEach(augmentAxeNodes); | ||
| axeResults.inapplicable.forEach(augmentAxeNodes); |
There was a problem hiding this comment.
why is this needed now? if something's inapplicable, it shouldn't have any nodes at all or it would be applicable...
There was a problem hiding this comment.
help url change. Didn't want to recreate the looping stuff for just that.
| application, | ||
| }, | ||
| // @ts-ignore | ||
| noHtml: true, |
There was a problem hiding this comment.
I guess this is fine since Accessibility isn't in our PublicGathererArtifacts, but we also ship back our snippets which except for eliding are the same thing verbatim, so we aren't exactly creating Fort Knox over here :)
is this a description of what you did or a request? (just noticed the force pushes which you usually don't do :) |
|
Don't want to squash the PR, the first commit is just a rebaseline and the second is the actual upgrade. |
Is there a reason this change is different than usual? We rebaseline artifacts at the same time as updating gatherers lots of times, often when the artifact is hilariously out of date. My vote is don't open the pandoras box for different merge workflows (changes can always go in separate PRs if it's helpful to separate, and smaller PRs are always a bonus :) |
This workflow is equivalent to me doing a PR (upgrade axe) on top of the first PR (rebaseline) and waiting for both to land in sequence, except it's simpler to update this way imo. Reviewer can separate the diffs themselves in the commit selector. I don't do this often, I know @patrickhulce does sometimes, I've never found it problematic wrt reviewing. |
And a squashed commit is equivalent to updating axe and running Landing separate commits for preserving people's work (e.g. they do the majority of a PR but can't finish it) or preserving git but otherwise keeping it simple seems worth it. Just my opinion. The PR is R: @patrickhulce. |
|
re: squash
|
6 participants
Do not squash.