Tests: Add basic a11 verification for the editor code using aXe #10695

gziolo · 2018-10-17T16:47:37Z

Description

Still needs some nice way to print violations or simply fix them :)

   + Array [
    +   Object {
    +     "description": "Ensures role attribute has an appropriate value for the element",
    +     "help": "ARIA role must be appropriate for the element",
    +     "helpUrl": "https://dequeuniversity.com/rules/axe/3.1/aria-allowed-role?application=axeAPI",
    +     "id": "aria-allowed-role",
    +     "impact": "minor",
    +     "nodes": Array [
    +       Object {
    +         "all": Array [],
    +         "any": Array [
    +           Object {
    +             "data": Array [
    +               "button",
    +             ],
    +             "id": "aria-allowed-role",
    +             "impact": "minor",
    +             "message": "role button  is not allowed for given element",
    +             "relatedNodes": Array [],
    +           },
    +         ],
    +         "failureSummary": "Fix any of the following:
    +   role button  is not allowed for given element",
    +         "html": "<input role=\"button\" aria-label=\"Add block\" class=\"editor-default-block-appender__content\" type=\"text\" readonly=\"\" value=\"Write your story\">",
    +         "impact": "minor",
    +         "none": Array [],
    +         "target": Array [
    +           ".editor-default-block-appender__content",
    +         ],
    +       },
    +     ],
    +     "tags": Array [
    +       "cat.aria",
    +       "best-practice",
    +     ],
    +   },
    +   Object {
    +     "description": "Ensures every form element has a label",
    +     "help": "Form elements must have labels",
    +     "helpUrl": "https://dequeuniversity.com/rules/axe/3.1/label?application=axeAPI",
    +     "id": "label",
    +     "impact": "critical",
    +     "nodes": Array [
    +       Object {
    +         "all": Array [],
    +         "any": Array [],
    +         "failureSummary": "Fix all of the following:
    +   Form element has explicit <label> that is hidden",
    +         "html": "<textarea id=\"post-title-0\" class=\"editor-post-title__input\" placeholder=\"Add title\" rows=\"1\" style=\"overflow: hidden; overflow-wrap: break-word; resize: none; height: 94px;\"></textarea>",
    +         "impact": "critical",
    +         "none": Array [
    +           Object {
    +             "data": null,
    +             "id": "hidden-explicit-label",
    +             "impact": "critical",
    +             "message": "Form element has explicit <label> that is hidden",
    +             "relatedNodes": Array [],
    +           },
    +         ],
    +         "target": Array [
    +           "#post-title-0",
    +         ],
    +       },
    +     ],
    +     "tags": Array [
    +       "cat.forms",
    +       "wcag2a",
    +       "wcag332",
    +       "wcag131",
    +       "section508",
    +       "section508.22.n",
    +     ],
    +   },
    + ]

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

gziolo · 2018-10-17T16:48:13Z

package.json

@@ -181,7 +182,7 @@
 		"publish:dev": "npm run build:packages && lerna publish --npm-tag next",
 		"publish:prod": "npm run build:packages && lerna publish",
 		"test": "concurrently \"npm run lint && npm run test-unit\" \"npm run test-mobile\"",
-		"pretest-e2e": "concurrently \"./bin/reset-e2e-tests.sh\" \"npm run build\"",
+		"pretest-e2e": "concurrently \"./bin/reset-e2e-tests.sh\"",


I need to revert it, it's painful to build for every run when hacking :)

Maybe we need an option to disable it! 😆

Maybe we need an option to disable it! 😆

It would be a nice addition. I've worked around this myself by copying, editing, and re-running the subcommand that triggers after the build, but it's not pretty 😅

e.g.

JEST_PUPPETEER_CONFIG=test/e2e/puppeteer.config.js npx wp-scripts test-unit-js --config test/e2e/jest.config.json --runInBand "test/e2e/specs/navigable-toolbar.test.js"

gziolo · 2018-10-17T17:06:59Z

My very naive try to fix violations:

diff --git a/packages/editor/src/components/default-block-appender/index.js b/packages/editor/src/components/default-block-appender/index.js
index afabf7c50..3a7fa5fd7 100644
--- a/packages/editor/src/components/default-block-appender/index.js
+++ b/packages/editor/src/components/default-block-appender/index.js
@@ -51,7 +51,6 @@ export function DefaultBlockAppender( {
 		<div data-root-client-id={ rootClientId || '' } className="editor-default-block-appender">
 			<BlockDropZone rootClientId={ rootClientId } layout={ layout } />
 			<input
-				role="button"
 				aria-label={ __( 'Add block' ) }
 				className="editor-default-block-appender__content"
 				type="text"
diff --git a/packages/editor/src/components/post-title/index.js b/packages/editor/src/components/post-title/index.js
index 718784b7b..7e252a879 100644
--- a/packages/editor/src/components/post-title/index.js
+++ b/packages/editor/src/components/post-title/index.js
@@ -115,7 +115,7 @@ class PostTitle extends Component {
 								'mod+shift+z': this.redirectHistory,
 							} }
 						>
-							<label htmlFor={ `post-title-${ instanceId }` } className="screen-reader-text">
+							<label htmlFor={ `post-title-${ instanceId }` }>
 								{ decodedPlaceholder || __( 'Add title' ) }
 							</label>
 							<Textarea

gziolo · 2018-10-17T17:17:37Z

We can use
https://github.com/marcysutton/empathy-driven-development/blob/master/test/utils.js#L24-L52
to have a nicer way to render violations.

tofumatt

So a nicer reporter would definitely be good here! Marcy's seems good, I guess we need to make our own reporter for that? (Hey, maybe we could publish it for others to use!)

I'm curious why role="button" was used for that text field. It seems fine to remove.

What does removing screen-reader-text className do to the visuals of that component? I'd imagine it changes it...

tofumatt · 2018-10-17T18:39:32Z

package.json

@@ -181,7 +182,7 @@
 		"publish:dev": "npm run build:packages && lerna publish --npm-tag next",
 		"publish:prod": "npm run build:packages && lerna publish",
 		"test": "concurrently \"npm run lint && npm run test-unit\" \"npm run test-mobile\"",
-		"pretest-e2e": "concurrently \"./bin/reset-e2e-tests.sh\" \"npm run build\"",
+		"pretest-e2e": "concurrently \"./bin/reset-e2e-tests.sh\"",


Maybe we need an option to disable it! 😆

aduth · 2018-10-17T19:41:25Z

test/e2e/specs/a11y.test.js

@@ -18,6 +23,23 @@ describe( 'a11y', () => {
 		await newPost();
 	} );

+	it( 'passes aXe Javascript Accessibility API checks', async () => {


Typo: Javascript -> JavaScript 😄

afercia · 2018-10-19T06:41:42Z

I'm curious why role="button" was used for that text field. It seems fine to remove.

@tofumatt I'd recommend to have a read at the related PR and issue, see #5742 / #4829 or maybe ask around could help answer your question.

Seems that for technical reasons the block appender must be an input field:

After some investigation the block appender can't be a button because of the writing flow, if it's a button, using arrows to navigate to it from the post title or previous block won't work. We could create a special case in the writing flow by targetting this button but I'd rather not do this and keep the component generic.

However, it gets activated with a click 😱it does something when activated so how it should be announced by assistive technologies? Screen readers announce input fields as "edit text":

Add block edit text doesn't sound appropriate
Add block button is better

I know this is, in a way, against the spec see #4829 (comment) but I think giving proper feedback to users is more important than an error thrown by a static analysis tool that can't evaluate the reasons behind it.

That said, if you can change the appender to a real <button> element, that would be excellent 🙂

tofumatt · 2018-11-16T11:59:06Z

Sorry, I'm just revisiting this now because I think it's a useful tool to add for further work on the repo.

I'd integrated a better reporter that worked with jest and did some tweaks to ignore the button failure... but then my Mac went weird and my Gutenberg repo was deleted. 😱

Sounds like the button test failure should be ignored, thanks for the context @afercia 👍

I'll take a look at the label issue, try to recreate my test suite work, and then we should get this merged. It's a handy thing to have 😄

gziolo · 2018-11-16T12:13:33Z

@tofumatt sure thing, be my guest. I'd love to see this merged. You can ping me when it's ready for review. By the way, I thought that we might want to make all elements with screen-reader-text class applied to be visible when performing the check to avoid the errors.

stephenmathieson · 2018-12-09T15:39:05Z

test/e2e/specs/a11y.test.js

@@ -18,6 +23,23 @@ describe( 'a11y', () => {
 		await newPost();
 	} );

+	it( 'passes aXe JavaScript Accessibility API checks', async () => {
+		const handle = await page.evaluateHandle( `


We've now got an axe-puppeteer for this, and we hope to get it working today (at WordCamp).

Awesome, as far as I can see it contains many useful helpers. That will make everything easier.

Happy WordCamp contributor day! This patch introduces accessibility testing using [`axe-puppeteer`](https://github.com/dequelabs/axe-puppeteer). Each one of the existing E2E tests now runs `AxePuppeteer#analyze()` on the relevant portion(s) of the DOM and outputs the returned a11y violations to stderr. In time, as these accessibility problems are fixed, the logging could be replaced with assertions, ensuring no "new" problems are introduced into the codebase. This patch is similar to WordPress#10695, but uses a more complete a11y testing tool and does not introduce new test failures.

gziolo · 2018-12-12T12:10:23Z

Let's close in favor of #12743 🎉

Tests: Add basic a11 verification for the editor code using aXe

ffac2b8

gziolo added this to the 4.2 - API freeze milestone Oct 17, 2018

gziolo requested review from tofumatt, youknowriad and aduth October 17, 2018 16:47

gziolo commented Oct 17, 2018

View reviewed changes

Testing: Narrow down aXe check to #editor

2cc9c57

tofumatt reviewed Oct 17, 2018

View reviewed changes

aduth reviewed Oct 17, 2018

View reviewed changes

gziolo removed this from the 4.2 milestone Oct 24, 2018

tofumatt added 2 commits November 12, 2018 22:54

chore: Capitalisation!

af6d7a4

chore: Revert test debugging

14456c1

tofumatt removed the [Status] In Progress Tracking issues with work in progress label Nov 15, 2018

tofumatt self-assigned this Nov 16, 2018

tofumatt added this to the WordPress 5.0.x Follow Ups milestone Nov 16, 2018

stephenmathieson reviewed Dec 9, 2018

View reviewed changes

stephenmathieson mentioned this pull request Dec 9, 2018

test: add a11y reporting using axe-puppeteer #12743

Closed

4 tasks

gziolo closed this Dec 12, 2018

gziolo deleted the add/axe-basic-test branch December 12, 2018 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests: Add basic a11 verification for the editor code using aXe #10695

Tests: Add basic a11 verification for the editor code using aXe #10695

gziolo commented Oct 17, 2018 •

edited

Loading

gziolo Oct 17, 2018

tofumatt Oct 17, 2018

aduth Oct 19, 2018

gziolo commented Oct 17, 2018

gziolo commented Oct 17, 2018

tofumatt left a comment •

edited

Loading

tofumatt Oct 17, 2018

aduth Oct 17, 2018

afercia commented Oct 19, 2018 •

edited

Loading

tofumatt commented Nov 16, 2018

gziolo commented Nov 16, 2018

stephenmathieson Dec 9, 2018

gziolo Dec 9, 2018

gziolo commented Dec 12, 2018

Tests: Add basic a11 verification for the editor code using aXe #10695

Tests: Add basic a11 verification for the editor code using aXe #10695

Conversation

gziolo commented Oct 17, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

gziolo Oct 17, 2018

Choose a reason for hiding this comment

tofumatt Oct 17, 2018

Choose a reason for hiding this comment

aduth Oct 19, 2018

Choose a reason for hiding this comment

gziolo commented Oct 17, 2018

gziolo commented Oct 17, 2018

tofumatt left a comment • edited Loading

Choose a reason for hiding this comment

tofumatt Oct 17, 2018

Choose a reason for hiding this comment

aduth Oct 17, 2018

Choose a reason for hiding this comment

afercia commented Oct 19, 2018 • edited Loading

tofumatt commented Nov 16, 2018

gziolo commented Nov 16, 2018

stephenmathieson Dec 9, 2018

Choose a reason for hiding this comment

gziolo Dec 9, 2018

Choose a reason for hiding this comment

gziolo commented Dec 12, 2018

gziolo commented Oct 17, 2018 •

edited

Loading

tofumatt left a comment •

edited

Loading

afercia commented Oct 19, 2018 •

edited

Loading