-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
new_audit(accessibility): add accessibility manual audits #3834
Conversation
f4255f4
to
374cc85
Compare
374cc85
to
cd86368
Compare
lighthouse-core/config/default.js
Outdated
@@ -199,6 +209,10 @@ module.exports = { | |||
title: 'Meta Tags Used Properly', | |||
description: 'These are opportunities to improve the user experience of your site.', | |||
}, | |||
'manual-a11y-checks': { | |||
title: 'Manual checks to verify', | |||
description: 'These checks address areas which an automated testing tool cannot cover. They do not affect your score but it\'s important that you verify them manually. Learn more in our guide on [conducting an accessibility review](https://developers.google.com/web/fundamentals/accessibility/how-to-review).' |
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.
I think this is a little wordy, though i don't have any particular suggestions. Between this and the overall a11y description you want to take another pass?
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.
I chopped out the middle sentence if that helps?
static get meta() { | ||
return Object.assign({ | ||
name: 'focusable-controls', | ||
helpText: 'All interactive controls should aim to be keyboard focusable and display a focus indicator. [Learn more](https://developers.google.com/web/fundamentals/accessibility/how-to-review#start_with_the_keyboard).', |
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.
Some inconsistency between:
- custom controls
- interactive controls
- custom interactive controls
can we use the same term?
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.
Settled on custom interactive controls.
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.
Sorry meant to add:
In some spots I still use the term "controls" because i mean different things. Controls may include custom controls (turning a div into a button). But sometimes I specifically refer to custom interactive controls because that term excludes built-in interactive controls. For example, aXe will already test if you have an unlabeled built-in interactive control. But it can't test if you've made a custom interactive control (like a div button or a custom element).
lighthouse-core/config/default.js
Outdated
@@ -199,6 +209,10 @@ module.exports = { | |||
title: 'Meta Tags Used Properly', | |||
description: 'These are opportunities to improve the user experience of your site.', | |||
}, | |||
'manual-a11y-checks': { | |||
title: 'Manual checks 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.
i know you were just copying what we already had but.... wdyt about a diff title for these?
like
Additional checks to manually verify
Additional items to manually check
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.
sgtm
lighthouse-core/config/default.js
Outdated
{id: 'custom-controls-roles', weight: 0, group: 'manual-a11y-checks'}, | ||
{id: 'focus-traps', weight: 0, group: 'manual-a11y-checks'}, | ||
{id: 'focusable-controls', weight: 0, group: 'manual-a11y-checks'}, | ||
{id: 'heading-levels', weight: 0, group: 'manual-a11y-checks'}, |
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.
is the order of these deliberate?
seems like its mostly: ARIA, Focus, Order and Exclusion
i dont really want to add groupings inside of Manual, but i figure they should be mostly grouped by topic. :)
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.
good call it should be. They're split amongst keyboard, screen reader, and landmarks but I think i may have just alphabetized them cuz my brain was shutting down. I can resort
Reworded stuff to try to make it more concise. @paulirish PTAL. |
Adds a set of manual checks for a11y and language to indicate that lighthouse's automated tests to not ensure total accessibility.
edited by paulirish: add new screenshot using proper manual grouping