-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
misc: reorganize accessibility gatherer #12076
Conversation
const isNotApplicable = notApplicables.find(result => result.id === this.meta.id); | ||
if (isNotApplicable) { | ||
return { | ||
score: 1, | ||
score: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
threw this one in for #12075 (comment) and because technically I did this, when I was updating old rawValue: true
s to score: 1
s all over the place :)
// Simplify `nodes` and collect nodeDetails for each. | ||
const nodes = result.nodes.map(node => { | ||
const {html, target, failureSummary, element} = node; | ||
// TODO: with `elementRef: true`, `element` _should_ always be defined, but need to verify. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very debatable TODO. The type is {element?: HTMLElement}
, true, and we could be defensive, but AFAICT with elementRef: true
element
is always set.
- On the one hand I haven't been able to verify that it's always set (there doesn't seem to be a single place in the code that sets it, but maybe I'm missing it).
- On the other hand we've been assuming that it's always set since 2016.
So...worth marking it TODO or just a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we could generate a warning if this invariant fails (and when it does, just skip this node and not show it in the results / or keep and fake a nodeDetails / use the body node). would require a not-ideal restructure of these two functions, so maybe not worth it generating a warning.
am hesitant to have it fail the entire run, but it is an option to just throw an error if element does not exist..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we could generate a warning if this invariant fails (and when it does, just skip this node and not show it in the results / or keep and fake a nodeDetails / use the body node). would require a not-ideal restructure of these two functions, so maybe not worth it generating a warning.
what do you think of beaconing to Sentry? I assume we'd get some kind of Cannot read property 'whatever' of undefined
if we really did sometimes have no element, but I can't find any hint of that in Sentry for the accessibility gatherer or audits. But this way it would be explicit and we'd have as good a signal as any after a month that it really can't ever happen?
if (!returnedValue) { | ||
throw new Error('No axe-core results returned'); | ||
} | ||
if (!Array.isArray(returnedValue.violations)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we trust our evaluate()
by now and don't do these checks in other gatherers. No need to keep these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya
@@ -102,10 +102,9 @@ describe('Accessibility: axe-audit', () => { | |||
} | |||
const artifacts = { | |||
Accessibility: { | |||
passes: [{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a thing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but needs merge with master
* @param {import('axe-core/axe').Result} result | ||
* @return {LH.Artifacts.AxeRuleResult} | ||
*/ | ||
function createAxeRuleResultArtifact(result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice idea to pull this out to its own function
// Simplify `nodes` and collect nodeDetails for each. | ||
const nodes = result.nodes.map(node => { | ||
const {html, target, failureSummary, element} = node; | ||
// TODO: with `elementRef: true`, `element` _should_ always be defined, but need to verify. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we could generate a warning if this invariant fails (and when it does, just skip this node and not show it in the results / or keep and fake a nodeDetails / use the body node). would require a not-ideal restructure of these two functions, so maybe not worth it generating a warning.
am hesitant to have it fail the entire run, but it is an option to just throw an error if element does not exist..
if (!returnedValue) { | ||
throw new Error('No axe-core results returned'); | ||
} | ||
if (!Array.isArray(returnedValue.violations)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya
2772e13
to
b11ef6c
Compare
}, | ||
{ | ||
"id": "tabindex", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these changes are correct? looks like a lot of deletions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these changes are correct? looks like a lot of deletions
yes. It's a little annoying in the diff because incomplete
and notApplicable
switched places in the artifact, but it reads better in the gatherer in that order, and I figured preserving artifacts.json
order shouldn't negate that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, and lots of deletions because all we offer for notApplicable
is the id
:
lighthouse/types/artifacts.d.ts
Line 182 in 11d33bb
notApplicable: Array<Pick<AxeRuleResult, 'id'>>; |
so there's no need to keep the rest around
with the recent talk about the
accessibility
gatherer, I thought I would bring out this change which I had in another branch but is probably worth it on its own. This gatherer was written in ancient times so does some odd things, and it returns a bunch more stuff than we have inartifacts.d.ts
, which means it's not usable by audits and so is wasted.axe-core
also ships with types now, so we should use them :)The goal is that it should be easier to follow and clearer where every part of the artifact comes from. No smoke test changes.
Can update after #12075 lands.