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

Add non-experimental axe tests. #1768

Merged
merged 7 commits into from
Mar 22, 2017
Merged

Add non-experimental axe tests. #1768

merged 7 commits into from
Mar 22, 2017

Conversation

robdodson
Copy link
Contributor

Big PR I know :P

This adds most of the axe tests with a few exceptions:

  • I removed some that seemed low priority, like testing for the <blink> tag and some stuff around image maps
  • I'm not using any of their experimental tests, though I think that would be cool at some point. I think it'd be nice to add an option to enable/disable experimental tests someday.
  • I'm not using their best-practice tests. Same as above, I'd like to add these someday but want to give folks an option to enable/disable. The exception to this is the tabindex test which is a simple test, and was already checked into lighthouse so I figure it's ok to leave it in place.

@paulirish
Copy link
Member

Screenshot of the report payload:
image

name: 'accesskeys',
description: 'All accesskeys in the document are unique.',
helpText: '`accesskey` attributes allow the user to quickly activiate or focus part ' +
'of the page.' +
Copy link
Member

Choose a reason for hiding this comment

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

need an extra space

@ebidel
Copy link
Contributor

ebidel commented Feb 24, 2017

Whoa that is HUUUUUUGE. Not sure what to do about it, but if 1-more audits fails that whole thing will be open. That'll probably be the case for most sites.

@paulirish
Copy link
Member

First up. This was some work! nice job wrangling it all together. I really like getting the helptext quickly as I don't understand all of these too well. :)

A few ideas and questions..

Combine into fewer audits

This is a larger issue.. but I think we can combine a bunch of these into fewer audits. As a result we won't have a wall of text when the Accessibility section is there. (as in the above screenshot)

So in this approach. You have a group like "Elements applying ARIA follow correct practices". Then if there is a failure, you open it up and see the results grouped by element. (Basically a list of elements and associated axe violations)

I spent some time trying to group them. I also ran the tests on a bunch of sites to make sure the ones that will fail a lot have a clear top-level description. So here's my proposal ....


  1. Color contrast of elements is satisfactory

    • "accessibility/color-contrast",
  2. Elements are well structured

    • "accessibility/definition-list",
      "accessibility/dlitem",
      "accessibility/duplicate-id",
      "accessibility/list",
      "accessibility/listitem",
  3. Elements avoid incorrect use of attributes

    • "accessibility/accesskeys",
      "accessibility/audio-caption",
      "accessibility/image-alt",
      "accessibility/input-image-alt",
      "accessibility/tabindex",
      "accessibility/td-headers-attr",
      "accessibility/th-has-data-cells",
  4. Elements applying ARIA follow correct practices

    • "accessibility/aria-allowed-attr",
      "accessibility/aria-required-attr",
      "accessibility/aria-required-children",
      "accessibility/aria-required-parent",
      "accessibility/aria-roles",
      "accessibility/aria-valid-attr-value",
      "accessibility/aria-valid-attr",
  5. Elements describe their contents well

    • "accessibility/document-title",
      "accessibility/frame-title",
      "accessibility/label",
      "accessibility/layout-table",
      "accessibility/object-alt",
      "accessibility/video-caption",
      "accessibility/video-description",
      "accessibility/bypass",
  6. The page's language is specified and valid.

    • "accessibility/html-has-lang",
      "accessibility/html-lang-valid",
      "accessibility/valid-lang",
  7. The document does not use <meta http-equiv="refresh">

    • "accessibility/meta-refresh",
  8. The <meta name="viewport"> element follows best practices.

    • "accessibility/meta-viewport",
  9. Links and buttons have discernable names.

    • "accessibility/link-name",
      "accessibility/button-name",

Top level aggregation

I'm working on a separate proposal around overall IA, but part of that proposal is that Accessibility breaks out into a separate top-level Aggregation. (So we'd have: PWA, Security, Accessibility, Performance, Best Practices). These would also be the selectable with checkboxes for people to only get what they want.

Language

I notice we don't always use language consistently. Note how we are referring to elements and attribute values:
image

We need to be consistent with uses of "All" and "Every" or the lackthereof. (I think the last option is probably best: [role] values are valid) ((I'm really keen on using the [attributename] convention.))

bypass question

is the bypass check satisfied by any <hN> element? If so, let's put "heading" first in the description before "skip link". :) (bypass is also the one audit i'm not entirely happy with my above taxomony with, but lmk wyt)

Audit feedback

Can you add a note on the lang ones that they accept valid BCP 47 tags?

Can you add something like this to the image-alt helptext?

"Ensure all informative elements have short, descriptive alternate text and all decorative elements have empty alt attributes (alt="")

Admittedly both of these changes won't belong in the helptext necessarily if we do the above combining.


that's a lot but that's what i'm thinkin.. wdyt?

@paulirish
Copy link
Member

FYI My proposed new IA for the report includes the above categorization for accessibility

image

@robdodson
Copy link
Contributor Author

I updated the help text language to be more consistent.

I may bug @ebidel or @brendankenny tomorrow when we're all back in the office to get their help combining the audits

@robdodson robdodson force-pushed the add-a11y-tests branch 2 times, most recently from 021a04f to 9c4c463 Compare March 21, 2017 02:37
@robdodson
Copy link
Contributor Author

OK I've added the aggregations. Here's what they look like (I closed the dropdowns in this screenshot)

image

@robdodson
Copy link
Contributor Author

@paulirish I think this is ready for review again, whenever you have a chance.

category: 'Accessibility',
name: 'accesskeys',
description: '`[accesskey]` values are unique.',
helpText: '`accesskey` attributes allow the user to quickly activiate or focus part ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

activate

@@ -0,0 +1,44 @@
/**
* @license
* Copyright 2016 Google Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017 for these new files

'use strict';

/**
* @fileoverview Ensures elements with an ARIA role that require child roles contain them.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence was hard to parse. Something like "Ensures elements contain a child role when their parent ARIA role requires it."

'use strict';

/**
* @fileoverview Ensures elements with an ARIA role that require parent roles are contained by them.
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment

category: 'Accessibility',
name: 'input-image-alt',
description: '`<input type="image">` elements have `[alt]` text.',
helpText: 'When an image is being used as an `<input>` button providing alternative text ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

button,

category: 'Accessibility',
name: 'valid-lang',
description: '`[lang]` attributes have a valid value.',
helpText: 'Specifying a valid BCP 47 language on elements helps ensure that text is ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

add a link: [BCP 47 language](LINK?)

'use strict';

/**
* @fileoverview Ensures every HTML document has a `lang` attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

update

Copy link
Contributor

@ebidel ebidel left a comment

Choose a reason for hiding this comment

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

LGTM. One more thing.

return {
category: 'Accessibility',
name: 'button-name',
description: 'buttons have an accessible name.',
Copy link
Contributor

Choose a reason for hiding this comment

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

spotted this one. The lower case looks weird:

screen shot 2017-03-22 at 11 34 08 am

How about buttons -> `<button>`s

@paulirish paulirish merged commit 2a12d5c into master Mar 22, 2017
@paulirish paulirish deleted the add-a11y-tests branch March 22, 2017 18:49
@paulirish
Copy link
Member

🎱🍐✨✨!

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.

3 participants