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

report: reorganize a11y audit groups #7129

Merged
merged 9 commits into from
Feb 20, 2019
Merged

report: reorganize a11y audit groups #7129

merged 9 commits into from
Feb 20, 2019

Conversation

robdodson
Copy link
Contributor

@robdodson robdodson commented Jan 31, 2019

Summary

Improve the categorization of accessibility audits.

  • Note: Also enables the accesskeys audit which was accidentally turned off in a previous PR.

Related Issues/PRs
fixes #7019

@robdodson robdodson changed the title Rename a11y audit categories. Fixes #7019 report: Rename a11y audit categories. Fixes #7019 Jan 31, 2019
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.

nice LGTM outside the potential scoring policy

{id: 'valid-lang', weight: 1, group: 'a11y-language'},
{id: 'video-caption', weight: 4, group: 'a11y-describe-contents'},
{id: 'video-description', weight: 3, group: 'a11y-describe-contents'},
{id: 'audio-caption', weight: 4, 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.

is the new sorting scheme alphabetical unless it's audio-caption? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I did that one first while I was still figuring out if the audits were sorted by id or group. Fixed

@@ -369,42 +369,42 @@ const defaultConfig = {
description: str_(UIStrings.a11yCategoryDescription),
manualDescription: str_(UIStrings.a11yCategoryManualDescription),
auditRefs: [
{id: 'accesskeys', weight: 3, group: 'a11y-navigation'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

@paulirish did we officially decide what our criteria for breaking changes was for v4 going forward? do we need to introduce this as weight: 0 to start or are we doing that starting v5?

Copy link
Member

Choose a reason for hiding this comment

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

do we need to introduce this as weight: 0 to start or are we doing that starting v5?

I think since this is not in the performance section it's less of a big deal (people might appreciate it, actually).

It was also a misunderstanding that disabled this audit, so we can treat the temporary weight: 0 as a bug that we're now fixing? :D

{id: 'button-name', weight: 10, group: 'a11y-element-names'},
{id: 'bypass', weight: 10, group: 'a11y-describe-contents'},
{id: 'button-name', weight: 10, group: 'a11y-names-labels'},
{id: 'bypass', weight: 10, group: 'a11y-navigation'},
{id: 'color-contrast', weight: 6, group: 'a11y-color-contrast'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

heh, is this really the only audit in the group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is! Sorry, it's an odd duck. But oh so important.

@brendankenny brendankenny changed the title report: Rename a11y audit categories. Fixes #7019 report: reorganize a11y audit groups Jan 31, 2019
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM from a proto round trip perspective.

But if we are changing a11y scoring weights would that be a major change? And changing the JSON group/audit keys might be considered breaking, its not crazy, but its definitely a change to the JSON structure.

@paulirish
Copy link
Member

Before / After:

image

(via lighthouse http://localhost:10200/a11y/a11y_tester.html)

@paulirish
Copy link
Member

@meggynw as fyi

@robdodson
Copy link
Contributor Author

I think all of the weights should be the same with the exception of accesskeys which was formerly listed as a manual audit. I could move it back to 0 weight and consider giving it a weight at a later, more breakage friendly time?

@@ -31,44 +31,44 @@ const UIStrings = {
diagnosticsGroupTitle: 'Diagnostics',
/** Description of the diagnostics section of the Performance category. Within this section are audits with non-imperative titles that provide more detail on the page's page load performance characteristics. Whereas the 'Opportunities' suggest an action along with expected time savings, diagnostics do not. Within this section, the user may read the details and deduce additional actions they could take. */
diagnosticsGroupDescription: 'More information about the performance of your application.',
/** Title of the Accessibility category of audits. This section contains audits focused on making web content accessible to users with disabilities. Also used as a label of a score gauge; try to limit to 20 characters. */
/** Title of the Accessibility category of audits. This section contains audits focused on making web content accessible to all users. Also used as a label of a score gauge; try to limit to 20 characters. */
Copy link
Member

Choose a reason for hiding this comment

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

👍 👍

@@ -369,42 +369,42 @@ const defaultConfig = {
description: str_(UIStrings.a11yCategoryDescription),
manualDescription: str_(UIStrings.a11yCategoryManualDescription),
auditRefs: [
{id: 'accesskeys', weight: 3, group: 'a11y-navigation'},
Copy link
Member

Choose a reason for hiding this comment

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

do we need to introduce this as weight: 0 to start or are we doing that starting v5?

I think since this is not in the performance section it's less of a big deal (people might appreciate it, actually).

It was also a misunderstanding that disabled this audit, so we can treat the temporary weight: 0 as a bug that we're now fixing? :D

@robdodson
Copy link
Contributor Author

Do I need to run that special command to regenerate the sample json?

@brendankenny
Copy link
Member

yeah, rerunning is probably a lot easier than dealing with the merge conflict. merge or rebase, delete or whatever any conflict in sample_v2.json, then rerun yarn update:sample-json

@robdodson
Copy link
Contributor Author

Updated PTAL

@brendankenny
Copy link
Member

there's a test snapshot that needs to be updated (it's in the travis logs, but you can run yarn run unit-cli:ci -u to automatically fix) and the smoke test expectations will need to be switched back to not a manual result (

'accesskeys': {
score: null,
scoreDisplayMode: 'manual',
},
)

@robdodson
Copy link
Contributor Author

Updated PTAL

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.

ok, a few more things, but otherwise LGTM.

Does anyone else want to have sign off on this?

@@ -230,8 +230,12 @@ module.exports = [
},
},
'accesskeys': {
score: null,
scoreDisplayMode: 'manual',
score: 0,
Copy link
Member

Choose a reason for hiding this comment

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

to fix the CI error, it looks like this might be

score: null,
scoreDisplayMode: 'notApplicable',

?

But did something change with the audit since removal in #7020?

/** Title of the best practices section of the Accessibility category. Within this section are audits with descriptive titles that highlight common accessibility best practices. */
a11yBestPracticesGroupTitle: 'Best practices',
/* Description of the best practices section within the Accessibility category. Within this section are audits with descriptive titles that highlight common accessibility best practices. */
a11yBestPracticesGroupDescription: 'These are opportunities to improve the interpretation of your content by users in different locales.',
Copy link
Member

Choose a reason for hiding this comment

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

not clear what different locales have to do with duplicate-id/meta-refresh/meta-viewport?

Copy link
Member

Choose a reason for hiding this comment

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

also seems like for almost all of these we could remove the beginning "These are", but maybe we want to wait for the efforts looking at all descriptions in the report

Copy link
Member

Choose a reason for hiding this comment

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

different locales

ah, looks like this may be a c/p holdover from the "Internationalization and localization" group

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

ditto on the bestpractices group description. but other than that... lgtm

<section>
<button id="accesskeys1" accesskey="s">Foo</button>
<button id="accesskeys2" accesskey="s">Bar</button>
</section>
Copy link
Member

Choose a reason for hiding this comment

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

👍 👍 thanks for adding this!

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

Successfully merging this pull request may close these issues.

Proposed better organization of a11y audits
5 participants