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(audits): remove audio-caption accessibility audit #10453

Merged

Conversation

beaufortfrancois
Copy link
Member

@beaufortfrancois beaufortfrancois commented Mar 12, 2020

This PR removes the audio caption accessibility audit from Lighthouse as it has been disabled/deprecated since 2018. See dequelabs/axe-core#1071

Note that a web.dev PR is in progress to remove https://web.dev/audio-caption as well.

@beaufortfrancois beaufortfrancois requested a review from a team as a code owner March 12, 2020 08:54
@beaufortfrancois beaufortfrancois changed the title Remove audio-caption accessibility audit core(audits): Remove audio-caption accessibility audit Mar 12, 2020
@patrickhulce patrickhulce self-assigned this Mar 12, 2020
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

Thanks @beaufortfrancois! Right in time for 6.0 breaking change too :)

For tests to pass you'll also need to update

assert.equal(collisions, 17, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings}`);

and our smoke test assertions
https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-cli/test/smokehouse/test-definitions/a11y/expectations.js

@@ -459,7 +458,6 @@ const defaultConfig = {
{id: 'aria-toggle-field-name', weight: 3, group: 'a11y-aria'},
{id: 'aria-valid-attr-value', weight: 10, group: 'a11y-aria'},
{id: 'aria-valid-attr', weight: 10, group: 'a11y-aria'},
{id: 'audio-caption', weight: 10, group: 'a11y-audio-video'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're still running this audit because of its wcag2a tag though, so I think we'll want to explicitly say enabled: false in our accessibility gatherer.

rules: {
'tabindex': {enabled: true},
'accesskeys': {enabled: true},
'heading-order': {enabled: true},
'meta-viewport': {enabled: true},
'duplicate-id': {enabled: false},
'table-fake-caption': {enabled: false},
'td-has-header': {enabled: false},
'marquee': {enabled: false},
'area-alt': {enabled: false},
'aria-dpub-role-fallback': {enabled: false},
'html-xml-lang-mismatch': {enabled: false},
'blink': {enabled: false},
'server-side-image-map': {enabled: false},
'identical-links-same-purpose': {enabled: false},
'no-autoplay-audio': {enabled: false},
'svg-img-alt': {enabled: false},
},

@patrickhulce
Copy link
Collaborator

cc @robdodson I'm assuming you're already looped in here, but any thoughts on audits/axe-rules we should be adding to be the spiritual successor here?

@beaufortfrancois beaufortfrancois changed the title core(audits): Remove audio-caption accessibility audit core(audits): remove audio-caption accessibility audit Mar 12, 2020
@connorjclark
Copy link
Collaborator

Note that a web.dev PR is in progress to remove web.dev/audio-caption as well.

Shouldn't we keep it, but just add a deprecated banner?

@patrickhulce
Copy link
Collaborator

thanks @beaufortfrancois ! in addition to @connorjclark 's web.dev comment (👍) , a heads up that there are still some failing smoketests that will need assertion updates in https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-cli/test/smokehouse/test-definitions/a11y/expectations.js

@robdodson
Copy link
Contributor

cc @robdodson I'm assuming you're already looped in here, but any thoughts on audits/axe-rules we should be adding to be the spiritual successor here?

I don't think they've created a replacement for this specific audit. They do have some newer audits that seem cool like no-autoplay-audio. Their list of audits is here for what it's worth. If any of them seem interesting for Lighthouse we can try to get some docs written on the web.dev side.

@patrickhulce
Copy link
Collaborator

Friendly ping here @beaufortfrancois to see if you think you'll have time to get to this. We'd love to get it in before 6.0 final since it's a breaking change :)

@robdodson
Copy link
Contributor

btw the recommendation on the web.dev side was to add a banner to the article stating that the audit is deprecated. Similar to what we did with heading-level. That's the easiest solution that doesn't cause us to 404 or redirect an existing url.

@beaufortfrancois
Copy link
Member Author

I've added the banner to the web.dev audit article.
I'm failing to understand what needs to be done in this PR to fix tests sadly. Any help?

@patrickhulce
Copy link
Collaborator

This PR removed some nodes in lighthouse-cli/test/fixtures/a11y/a11y_tester.html which shifts the position of all following nodes in the tree. We need to update the test expectations in lighthouse-cli/test/smokehouse/test-definitions/a11y/expectations.js to match the new values. The new expected values are printed in the error message in travis.

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.

thanks for wading through all 30 things that need to be updated just to remove a single audit @beaufortfrancois!

@brendankenny
Copy link
Member

@patrickhulce not trying to steal your review :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM too, thanks for sticking with this @beaufortfrancois!

@patrickhulce patrickhulce merged commit ff4f82f into GoogleChrome:master Mar 27, 2020
@beaufortfrancois beaufortfrancois deleted the remove-audio-caption branch March 30, 2020 06:05
@beaufortfrancois
Copy link
Member Author

:)

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.

6 participants