Implement automated accessibility testing using Axe#3294
Implement automated accessibility testing using Axe#3294swissspidy merged 17 commits intodevelopfrom
Conversation
|
Even having these tests in has already highlight some issues we have. It will also help with future work. I would recommend fixing the issues that are found in the PR in another PR. Once that is complete, merge this as is and we can tweak it later. |
|
@spacedmonkey Can you create individual issues for all the things flagged here so we can better track them? |
44fafd5 to
052ca8a
Compare
| tStatusQueryTooShort, | ||
| tStatusSelectedOption, | ||
| tStatusResults, | ||
| ariaLabel, |
There was a problem hiding this comment.
Sneaked in some very small accessibility fixes here to make the tests pass.
| // Temporary disabled rules to enable initial integration. | ||
| // See: https://github.com/WordPress/gutenberg/pull/15018. | ||
| disabledRules: [ | ||
| 'aria-allowed-role', |
There was a problem hiding this comment.
These rules are disabled because of the errors in gutenberg. I would love for this ignored rules to be removed.
| // Since the selected block is already at the back, the 'Back' and 'Backward' buttons should be disabled. | ||
| expect( page ).toMatchElement( '.amp-story-controls-send-back[aria-disabled="true"]' ); | ||
| expect( page ).toMatchElement( '.amp-story-controls-send-backward[aria-disabled="true"]' ); | ||
| expect( page ).toMatchElement( '.amp-story-controls-send-backwards[aria-disabled="true"]' ); |
There was a problem hiding this comment.
This is a fix for e2e to pass. Not sure how this was passing before.
barklund
left a comment
There was a problem hiding this comment.
I'd like to see a reference to which exact rule(s), file and line, that should be enabled for each of the "child" issues from this one (#3301, #3305, #3307).
And just to confirm, if those three tickets are solved (and the corresponding rules enabled), AMP Story Editor is as ARIA-compliant (based on automated testing alone) as Gutenberg itself, right?
Otherwise this looks good to me!
| * | ||
| * @return {?Promise} Promise resolving once Axe texts are finished. | ||
| */ | ||
| async function runAxeTestsForBlockEditor() { |
There was a problem hiding this comment.
Can we rename this to runAxeTestsForStoriesEditor and use a different selector to detect it?
There was a problem hiding this comment.
@swissspidy Why? The tests are passing for both. Any new e2e added for plugin should also run these axe tests.
There was a problem hiding this comment.
The block editor and stories editor will have different configurations though. The exclude config already has a stories-specific config in it as well.
Also, eventually we should eventually add these tests for the plugin settings screen as well, where we're going to have yet another set of configuration options.
Having three separate sets would make maintenance easier.
There was a problem hiding this comment.
The selector in runAxeTestsForStoriesEditor is still the same though.
What I meant was having separate functions with separate configurations:
runAxeTestsForBlockEditorwith the current.block-editorselectorrunAxeTestsForStoriesEditorwith a#amp-story-editorselector- Eventually:
runAxeTestsForSettingsScreenwith a#amp-settingsselector - Eventually:
runAxeTestsForValidationScreenwith a.post-type-amp_validated_urlselector
There was a problem hiding this comment.
The selector in
runAxeTestsForStoriesEditoris still the same though.What I meant was having separate functions with separate configurations:
runAxeTestsForBlockEditorwith the current.block-editorselectorrunAxeTestsForStoriesEditorwith a#amp-story-editorselector- Eventually:
runAxeTestsForSettingsScreenwith a#amp-settingsselector- Eventually:
runAxeTestsForValidationScreenwith a.post-type-amp_validated_urlselector
What are you asking to change here? Just change the selector, like this runAxeTestsForStoriesEditor with a #amp-story-editor selector
There was a problem hiding this comment.
Yes. And have a second function, runAxeTestsForBlockEditor, with a different selector that only targets the block editor but not the stories editor. That function would then not exclude '.amp-story-page-attachment-close-button' as it's irrelevant in the standalone block editor.
If it's unclear I can also change this myself.
There was a problem hiding this comment.
And of course, let me know if you think it doesn't make sense. Then we can just do the #amp-story-editor selector change for now.
Co-Authored-By: Pascal Birchler <pascalb@google.com>
Good point @barklund . Regarding these other issues.
As suggested by @westonruter , existing fails were ignored where possible and other that couldn't be ignored were fixed. Those two issues will go into the backlog and will like not be looked at until after 2.0 release. |
assets/src/stories-editor/components/font-family-picker/index.js
Outdated
Show resolved
Hide resolved
Also fixes some tests and an a11y issue in the `Autocomplete` component.
* tag '1.3.0': (318 commits) Bump 1.3.0 Add inline styles for custom fonts (#3345) Limit deeply-nesting test to 200 to fix Xdebug error (#3341) Bump 1.3-RC2 (#3335) Sanitize invalid children of amp-story and amp-story-page elements to prevent white story of death (#3336) Remove unused Travis deploy stage (#3340) Implement automated accessibility testing using Axe (#3294) Only add all Google Font style rules in editor context Prevent adding AMP query var to Story URLs in Compatibility Tool Prevent attempting to redirect Stories with rejected validation errors Ensure all AMP scripts (including v0.js) get moved to the head Make sure that media picker is background types are filter correctly. Normalize style[type] attribute quote style after r46164 in WP core Fix phpunit covers tags Bump version to 1.3-RC1 Strip 100% width/height from layout=fill elements Fix issue with cut (#3246) Remove unused Google Fonts SVGs (#3289) Fix resize for non-fit text box (#3259) Use template_dir consistently as signal for transitional mode ...
First pass at adding AXE into e2e testing.
Related: #3249
Fixes #3301