-
Notifications
You must be signed in to change notification settings - Fork 232
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
Adds simple aXe testing to the Design System #784
Conversation
You can preview this change here: Built with commit 5d28805 https://deploy-preview-784--govuk-design-system-preview.netlify.com |
f952340
to
88df654
Compare
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.
Looks good, a few comments...
src/components/radios/index.md.njk
Outdated
@@ -58,7 +58,7 @@ When there are more than 2 options, radios should be stacked, like so: | |||
|
|||
If there are only 2 options, you can either stack the radios or display them inline, like so: | |||
|
|||
{{ example({group: "components", item: "radios", example: "default", html: true, nunjucks: true, open: false, size: "s", id: "default-2"}) }} | |||
{{ example({group: "components", item: "radios", example: "default", exampleSuffix: 'second' ,html: true, nunjucks: true, open: false, size: "s", id: "default-2"}) }} |
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.
Nice, did you check all examples that have 'id: default-2'? Or just the ones covered in these tests...
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 only checked the radio example.
done() | ||
}) | ||
|
||
describe('Accessibility Audit', () => { |
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.
Al and I paired on this a bit, so would appreciate someone else to review this bit.
views/partials/_example.njk
Outdated
@@ -23,7 +24,7 @@ | |||
example in a new window | |||
</a> | |||
</span> | |||
<iframe title="{{ exampleTitle }}" data-module="app-example-frame" class="app-example__frame app-example__frame--resizable{% if params.size %} app-example__frame--{{ params.size }}{% endif %}" src="{{ exampleURL }}" frameBorder="0"></iframe> | |||
<iframe title="{{ exampleTitle }}{{ exampleTitleSuffix }} example" data-module="app-example-frame" class="app-example__frame app-example__frame--resizable{% if params.size %} app-example__frame--{{ params.size }}{% endif %}" src="{{ exampleURL }}" frameBorder="0"></iframe> |
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.
- Would it be simpler to just allow the
exampleTitle
to be passed in as a param, rather than suffixing? This partial's already got quite a lot going on, so anything we can do to avoid adding more complexity is a win in my eyes. - Is it intentional that we're adding 'example' to every title?
a6b18e6
to
6c6184d
Compare
I have addressed @36degrees concerns of complexity by making the logic to generate the iframe ids dependant on the titles. This means since titles need to be unique as well as ids, we only need to have a way to change the title. I have chosen to use a |
Instead of testing all the pages in the service, it was decided that it would be best to check page templates instead of specific components which should be tested in GOV.UK Frontend. 1. Home page - layout template ✅ 2. Component page - layout-pane template ✅ 3. Patterns - Gender and Sex page - layout-pane template ✅ 4. Cookie page - layout-single-page-prose template ✅ 5. Get in touch Page - layout-single-page template ✅ 6. Sitemap page - layout-sitemap template ✅
Since we're now using the title to generate the ID, add a suffix the title and avoid having to set a new ID manually.
6c6184d
to
5d28805
Compare
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.
LGTM, thanks both 🙌
@@ -33,7 +33,7 @@ If this is not possible, you should hide the back link when JavaScript is not av | |||
|
|||
There are 2 ways to use the back link component. You can use HTML or, if you are using [Nunjucks](https://mozilla.github.io/nunjucks/) or the [GOV.UK Prototype Kit](https://govuk-prototype-kit.herokuapp.com), you can use the Nunjucks macro. | |||
|
|||
{{ example({group: "components", item: "back-link", example: "default", html: true, nunjucks: true, open: false, id: "default-2"}) }} | |||
{{ example({group: "components", item: "back-link", example: "default", html: true, nunjucks: true, open: false, titleSuffix: "second"}) }} |
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.
Non blocking suggestion: I wonder if calling it idSuffix
would be more accurate.
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 since we're adding a suffix to the title it makes sense as is, people using this partial no longer need to understand how the id works?
// axe reports there is "no label associated with the text field", when there is one. | ||
['#app-site-search__input'], | ||
// axe reports that the phase banner is not inside a landmark, which is intentional. | ||
['.app-phase-banner__wrapper'] |
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.
Just an observation: I wasn't able to reproduce this error locally when I removed ['.app-phase-banner__wrapper']
from thingsToExclude
(unlike ['#app-site-search__input']
which threw up an error when removed from thingsToExclude
).
But I can see the error when running Axe in the browser.
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'll investigate and do a follow up PR if it makes sense to thanks Hanna.
This reverts commit dc6c2c4.
This reverts commit dc6c2c4.
Looks like this came in by mistake in #784: https://github.com/alphagov/govuk-design-system/pull/784/files#diff-7883ebc5245f0aeaea5ee02bb00d6d56L23
Fixes the accessibility audit tests so we can exclude multiple different selectors from tests. Previously the selector `.app-phase-banner__wrapper` was in the list of things to exclude, but was not being excluded (see [comment from @hannalaakso in PR #784][1]). It isn't clear from the `axe-puppeteer` documentation, but `AxePuppeteer.exclude` accepts only one argument, which is either a string or a list of strings. If a list of strings is provided, this is used to select an element within an iframe [[2]]. So to exclude several different elements, we need to call `.exclude` multiple times. The easiest way to call `.exclude` multiple times for each test is to create a function (replacing the `thingsToExclude` object), which this commit does. This is required to upgrade `axe-core` to version 3.5.0 or greater. The tests we passing before this commit because the rule ['all page content must be contained by landmarks'][3] was not working fully: it treated regions as being part of the `html` element (and thus not covered by `.include('body')`) [[4]]. [1]: https://github.com/alphagov/govuk-design-system/pull/784/files#r260285048 [2]: https://deque.com/axe/core-documentation/api-documentation/#context-parameter [3]: https://dequeuniversity.com/rules/axe/4.3/region [4]: dequelabs/axe-core#1980
Instead of testing all the pages in the service, it was decided that it
would be best to check page templates instead of specific components which
should be tested in GOV.UK Frontend.
fixes #620