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

core(a11y): upgrade axe-core to 4.1.1, update a11y audits #11661

Merged
merged 33 commits into from
Dec 15, 2020
Merged

Conversation

@connorjclark connorjclark requested a review from a team as a code owner November 12, 2020 22:55
@connorjclark connorjclark requested review from Beytoven and removed request for a team November 12, 2020 22:55
@google-cla google-cla bot added the cla: yes label Nov 12, 2020
docs/scoring.md Outdated

<!-- TODO: need a commit first to update url ... -->
The accessibility score is a weighted average. The specific weights, at the time of publishing, are [as follows](https://github.com/GoogleChrome/lighthouse/blob/080c6b4b9fec6dfcaf8e0cd8d09c3224465e4fd3/lighthouse-core/config/default-config.js#L450-L491):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just link to master?

// @ts-expect-error
const augmentAxeNodes = result => {
if (result.error instanceof Error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added types here. I suppose .error is not part of the handwritten, public api.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should verify, I don't think there are any tests for this.

@brendankenny
Copy link
Member

It looks like 4.1 (scheduled for next Monday) is where all their big size improvements are going to be landing:

We've been making great strides in our file size for our 4.1 release, having reduced the total size down to 108 kB minified and gzipped (down from 140 kB).

dequelabs/axe-core#1935 (comment).

@connorjclark connorjclark changed the title misc: upgrade axe-core to 4.0.2 misc: upgrade axe-core to 4.1.0 Nov 16, 2020
@connorjclark
Copy link
Collaborator Author

4.1 landed just now. new rules to consider https://github.com/dequelabs/axe-core/releases/tag/v4.1.0

@connorjclark
Copy link
Collaborator Author

connorjclark commented Nov 16, 2020

All these new rules deal with accessible names, just like aria-toggle-field-name, each for a specific element or role. Should we combine all of these into 1) one audit or 2) one base audit, each a separate audit or 3) separate audits as usual ?

With 1 and (maybe 2) we could reuse the same i18n strings and web.dev docs.

To do 1 we'd need to determine if all of these are of similar weight.

@eps1lon
Copy link

eps1lon commented Nov 16, 2020

To do 1 we'd need to determine if all of these are of similar weight.

The rules have tags for the WCAG level they fail (wcag2a for level A, wcag2aa for level AA etc.) or just "best-practice". So you could weigh them by their WCAG level or whether they're "just" best-practice.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Nov 16, 2020

Thanks for the note @eps1lon

We decided:

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@@ -8,8 +8,7 @@
/* global window, document, getNodeDetails */

const Gatherer = require('./gatherer.js');
const fs = require('fs');
const axeLibSource = fs.readFileSync(require.resolve('axe-core/axe.min.js'), 'utf8');
const axeLibSource = require('../../lib/axe.js').source;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was scared of the changes in this file until I did "Hide whitespace changes". Nice cleanup! Much more readable.

@@ -0,0 +1,29 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment. L26

ha, I guess I missed it, sorry :)

docs/scoring.md Outdated Show resolved Hide resolved
Co-authored-by: Brendan Kenny <bckenny@gmail.com>
@patrickhulce
Copy link
Collaborator

@connorjclark is the status of this PR correct that you're awaiting another review?

@connorjclark
Copy link
Collaborator Author

yes. @Beytoven do you have time to review this?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (needs a rebase/merge)

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

Successfully merging this pull request may close these issues.

7 participants