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

Enable automated accessibility testing #1151

Merged
merged 8 commits into from Sep 25, 2017
Merged

Enable automated accessibility testing #1151

merged 8 commits into from Sep 25, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Sep 21, 2017

  • Use aXe tests provided by govuk_publishing_components gem
  • Add poltergeist to enable integration tests with JS
  • Update gem config to explicitly include styles missing from Slimmer

Fix found accessibility failures:

  • Option select: Only add aria-controls if element is present
  • Option select: Declare list of checkboxes as checkbox group with label
  • GovSpeak: Text in bar charts has a contrast too low against light blue for WCAG
  • GovSpeak: Increase font on highlight answers for WCAG AA contrast
  • Bump govuk_publishing_components to 1.10 to avoid duplicate ID errors

Caveats:

  • Component JS isn't running on the integration tests because I can't easily include the jQuery dependency they need. Will tackle this in separate PR.
  • The visual regression tests produce screenshots that aren't always long enough (https://govuk-static-pr-1151.surge.sh/gallery.html)

Highlight answer

Updated to match https://www.gov.uk/bank-holidays and pass WCAG AA contrast ratio

https://govuk-static-pr-1151.herokuapp.com/component-guide/govspeak/highlight_answer

Before

screen shot 2017-09-21 at 17 10 06

After

screen shot 2017-09-21 at 17 09 58

Mobile

Before

screen shot 2017-09-21 at 17 12 21

After

screen shot 2017-09-21 at 17 04 11

Visual diff

screen shot 2017-09-21 at 17 48 04

Part of:
https://trello.com/c/Yq9FFia1/91-3-move-component-a11y-testing-from-application-into-gem-so-applications-get-the-testing-for-free

* Use aXe tests provided by govuk_publishing_components gem
* Add poltergeist to enable integration tests with JS
* Update gem config to explicitly include styles missing from Slimmer
@fofr fofr temporarily deployed to govuk-static-pr-1151 Sep 21, 2017 Inactive
@fofr fofr temporarily deployed to govuk-static-pr-1151 Sep 21, 2017 Inactive
@fofr fofr force-pushed the a11y-testing branch Sep 21, 2017
@fofr fofr temporarily deployed to govuk-static-pr-1151 Sep 21, 2017 Inactive
@fofr fofr force-pushed the a11y-testing branch Sep 21, 2017
@fofr fofr temporarily deployed to govuk-static-pr-1151 Sep 21, 2017 Inactive
@fofr fofr force-pushed the a11y-testing branch Sep 21, 2017
@fofr fofr temporarily deployed to govuk-static-pr-1151 Sep 21, 2017 Inactive
@fofr fofr requested a review from nickcolley Sep 21, 2017
fofr added 7 commits Sep 21, 2017
If the referenced aria-controls attribute points to the ID of an
element on the page that’s not present the attribute is invalid. Equally,
if JS fails to load the inputs will not be controlling anything.

* Dynamically add the attribute to all checkboxes if the referenced ID
is used and the element is on the page
* Update accessibility criteria to reference issue
* Fixes aXe error:
https://dequeuniversity.com/rules/axe/2.3/checkboxgroup?application=axeA
PI
* Generates a unique ID for the option select label
* ARIA group:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Tec
hniques/Using_the_group_role

On VoiceOver for Mac this announces the number of checkboxes in the
group when you tab into it. Following the first checkbox it does not
read out the group name each time.
19px text has a contrast too low against turquoise for WCAG AA.
Use bold font to increase contrast and fix aXe warning.

See lengthy discussion here:
alphagov/govuk_elements#200
Text in bar charts has a contrast too low against light blue
for WCAG AA. Use govuk-blue to increase contrast and fix aXe warning.
* Larger font-size for at least ratio of 3.5:0, stop failing aXe tests
* Add further examples and links to usage
* Update styles to more closely match the bank holidays view
30s isn’t long enough to compile all the static assets. Increase to 2
minutes to allow tests to run.

Switch from require phantomjs/poltergeist
(https://github.com/colszowka/phantomjs-gem/blob/master/lib/phantomjs/po
ltergeist.rb) to a modified version that specifies a timeout.
# TODO: Remove when PhantomJS updated on Puppet
# Use modern PhantomJS version via gem
# Based on: https://github.com/colszowka/phantomjs-gem/blob/master/lib/phantomjs/poltergeist.rb
Phantomjs.path

This comment has been minimized.

@nickcolley

nickcolley Sep 22, 2017
Contributor

Is this intentional?

This comment has been minimized.

@fofr

fofr Sep 22, 2017
Author Contributor

Yes, see link above.

OptionSelect.prototype.setCheckboxAriaControlsAttributes = function setCheckboxAriaControlsAttributes(){
var controls = this.$optionSelect.data('input-aria-controls');
if (typeof controls === "string" && $('#' + controls).length > 0) {
this.$optionSelect.find('input[type="checkbox"]').each(function() {

This comment has been minimized.

@nickcolley

nickcolley Sep 22, 2017
Contributor

Do we know how long this takes to execute? I'll give it a quick profile

This comment has been minimized.

@fofr

fofr Sep 22, 2017
Author Contributor

It'll vary based on the number of checkboxes. I optimised somewhat by moving logic onto parent and checking logic only once as each checkbox has the same attribute.

This comment has been minimized.

@nickcolley

nickcolley Sep 22, 2017
Contributor

Before
57.10ms
46.03ms
51.65ms

After
56.92ms
45.76ms
55.98ms

At CPU 6x slowdown

description: |
Used on:
* [https://gov.uk/vat-rates](https://gov.uk/vat-rates)

This comment has been minimized.

@nickcolley

nickcolley Sep 22, 2017
Contributor

👍 nice

# Tests aren't going through Slimmer so we need to explicitly include styles
GovukPublishingComponents.configure do |c|
c.application_print_stylesheet = "core-layout-print"
c.application_stylesheet = "core-layout"

This comment has been minimized.

@nickcolley

nickcolley Sep 22, 2017
Contributor

do we need the grid? could we use header-and-footer-only?

nevermind: core-layout includes the components too 👼

This comment has been minimized.

@fofr

fofr Sep 22, 2017
Author Contributor

I'm using the same thing that the guide uses when you view it in a browser.

</div>
<div class='options-container' id='<%= options_container_id %>'>
<div class='js-auto-height-inner'>
<div role="group" aria-labelledby="<%= title_id %>" class="options-container" id="<%= options_container_id %>">

This comment has been minimized.

@nickcolley

nickcolley Sep 22, 2017
Contributor

Do we need to check this change in different A.T?

This comment has been minimized.

@fofr

fofr Sep 22, 2017
Author Contributor

Yes. I've only been able to test with VoiceOver on Mac and iOS so far. (It tested well)

Copy link
Contributor

@nickcolley nickcolley left a comment

This is excellent work, let's test the update to the option select with more assistive tech since it's fairly complex before we merge this.

(I know you're working from home, sorry!)

@fofr
Copy link
Contributor Author

@fofr fofr commented Sep 25, 2017

Changes in 715a9cd are picked up by JAWS 17 and 18. They make the reading of each checkbox more verbose by reading the group name before the checkbox name each time. Unlike on VoiceOver which does it the first time but not the following times. This may slow down users when navigating through a list of items in a group with a long name.

NVDA behaves the same before and after, ie it doesn't support the aria group role.

JAWS 17 and 18:

Market sector. Aerospace checkbox. Not checked
Market sector. Agriculture checkbox. Not checked

NVDA:

Aerospace checkbox. Not checked

VoiceOver on Mac:

Market sector. Aerospace checkbox. Not checked
Agriculture checkbox. Not checked

VoiceOver on iOS:

Aerospace checkbox. Not checked

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Sep 25, 2017

Thanks for the testing results.

The extra verbosity worries me since I wonder if it was intentionally left out when building this component originally, but it should not create barriers, what do you think?

@fofr
Copy link
Contributor Author

@fofr fofr commented Sep 25, 2017

Confirmed that 0a70e29 works as expected on VoiceOver for Mac and iOS (checkbox changes read out the loading/results element).

Confirmed this continues to work on JAWS 14/Windows 7

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Sep 25, 2017

Talked to Paul, this change is seems good and does not introduce barriers, so we'll move this forwards and get some opinions on verbosity of re-reading of the label with the accessibility mailing list.

@fofr fofr merged commit f7fc777 into master Sep 25, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
security/snyk No new issues
Details
@fofr fofr deleted the a11y-testing branch Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants