Add axe accessibility testing #33

NickColley · 2017-08-17T15:16:43Z

These tests will look for '[data-module="test-a11y"]' and run axe against them,
it'll then throw an error to be caught in dev tools or better in an applications'
integration tests.

Adds Jasmine as a development dependency to test this behaviour, we add require jasmine to the dummy application
so that the default rake build will execute successfully.

government-frontend integration test failure:

Chrome DevTools example:

NickColley · 2017-08-17T15:55:38Z

WIP, need to figure out a good way to test this behaviour

fofr · 2017-08-17T18:00:43Z

I don't like that jQuery will get included twice. Once from component guide and once from static.

NickColley · 2017-08-18T11:03:32Z

@fofr I was thinking of not including jQuery at all but was thinking of the cases where we're testing the guide against older browsers, that said might be able to just use the static dependency, will check.

NickColley · 2017-08-22T16:06:51Z

Trying to add the jasmine tests to the default rake command results in

Don't know how to build task 'app:jasmine:ci' (see --tasks)

I'm not currently sure how to resolve this.

fofr · 2017-08-23T08:37:41Z

app/views/govuk_publishing_components/component_guide/preview.html.erb

+    locals: {
+      component_doc: @component_doc,
+      fixture: fixture,
+      flush: true


What does "flush" mean?

As in 'flush fit'. It's not so obvious, maybe there's an alternative.

'full_bleed'? https://en.wikipedia.org/wiki/Bleed_(printing)

Might be a way to change this so the borders wrap the internal component, will try something.

fofr · 2017-08-23T10:37:26Z

@NickColley I get the error locally too:

vagrant@development:/var/govuk/govuk_publishing_components$ bundle exec rake
rake aborted!
Don't know how to build task 'app:jasmine:ci' (see --tasks)
/usr/lib/rbenv/versions/2.3.1/bin/bundle:23:in `load'
/usr/lib/rbenv/versions/2.3.1/bin/bundle:23:in `<main>'

NickColley · 2017-08-24T09:47:07Z

I'm going to redo this PR to tidy up the commits.

NickColley · 2017-08-24T09:50:25Z

Actually just gonna squash this one together 👍

fofr · 2017-08-24T10:42:28Z

app/assets/javascripts/govuk_publishing_components/accessibility-test.js

+  // http://responsivenews.co.uk/post/18948466399/cutting-the-mustard
+  if (document.addEventListener) {
+    document.addEventListener('DOMContentLoaded', function () {
+      AccessibilityTest('.js-test-a11y', function (err, results) {


Why not use data-module="test-a11y" pattern?

fofr · 2017-08-24T10:44:14Z

app/assets/javascripts/govuk_publishing_components/accessibility-test.js

+    var axeOptions = {
+      restoreScroll: true,
+      include: [selector]
+    }


Whitespace here: run through standardjs linter?

Added some space 👍

fofr · 2017-08-24T10:44:35Z

app/assets/javascripts/govuk_publishing_components/accessibility-test.js

+      restoreScroll: true,
+      include: [selector]
+    }
+    axe.run(axeOptions, function (err, results, blah, boo) {


blah, boo 🤔

fofr · 2017-08-24T10:48:49Z

spec/javascripts/govuk_publishing_components/AccessibilityTestSpec.js

+  })
+
+  it('should throw if there\'s a contrast issue', function (done) {
+    addToDom('<a href="#">Low contrast</a>', 'a { background: white; color: #ddd }')


fofr · 2017-08-24T10:50:24Z

test/dummy/config/application.rb

@@ -7,6 +7,7 @@
 require 'action_cable/engine'
 require 'rails/test_unit/railtie'
 require 'sprockets/railtie'
+require 'jasmine'


Given the pain caused by this – is it worth adding a comment or more of a length description in commit?

fofr · 2017-08-24T10:51:34Z

Are the screenshots in the PR still accurate?

NickColley · 2017-08-24T11:31:06Z

@fofr screenshots are accurate yes

These tests will look for '[data-module="test-a11y"]' and run axe against them, it'll then throw an error to be caught in dev tools or better in an applications' integration tests. Adds Jasmine as a development dependency to test this behaviour, we add require jasmine to the dummy application so that the default rake build will execute successfully.

fofr · 2017-08-24T11:48:41Z

spec/javascripts/govuk_publishing_components/AccessibilityTestSpec.js

@@ -0,0 +1,136 @@
+/* global describe, afterEach, it, expect */
+
+var TEST_SELECTOR = '.js-test-a11y'


Does this need updating too?

I had some troubles with using a consistent data attribute, I don't think axe likes it as much when outputting the targeting.

Since this spec test the behaviour rather than how it works in the guide, this is fine.

fofr · 2017-08-24T12:59:22Z

spec/spec_helper.rb

+
+WebMock.disable_net_connect!(allow_localhost: true)
+
+# Allow us to test Javascript for example thrown errors


"for errors thrown by examples"?

fofr · 2017-08-24T13:01:24Z

spec/component_guide/component_guide_spec.rb

@@ -83,12 +84,18 @@
    end
  end

+  it 'guide throws errors if component has a11y issues' do


"accessibility"

fofr · 2017-08-24T13:02:42Z

spec/spec_helper.rb

@@ -3,6 +3,12 @@
 require 'webmock/rspec'
 require 'slimmer/rspec'
 require 'capybara/rails'
+require 'capybara/poltergeist'
+
+WebMock.disable_net_connect!(allow_localhost: true)


Does this stop external scripts from loading?

This was copied from government-frontend https://github.com/alphagov/government-frontend/blob/master/test/support/webmock.rb

Makes sure a JavaScript error is thrown on pages where there is an accessibility issue.

Since we're using the alpha release, the help urls generated are not live yet. This commit fixes that by pointing at the 2.3.x release, and adds a message that'll be noticed if someone updated axe in the future.

See vcr/vcr#637 (comment)

fofr · 2017-08-25T08:29:56Z

Regarding 46c86e3, I'm not sure why CI was running the tests with Ruby 2.4 when 2.3.1 is specified in the .ruby-version file.

Edit: We test against 2.2, 2.3 and 2.4:
https://github.com/alphagov/govuk-puppet/blob/master/modules/govuk_jenkins/files/var/lib/jenkins/groovy_scripts/govuk_jenkinslib.groovy#L513-L528

fofr · 2017-08-25T08:33:22Z

This branch fails locally on 2.3.1 and 2.4:

Failures:

  1) Component guide guide throws errors if component has an accessibility issue
     Failure/Error:
       expect { visit '/component-guide/test-component-with-a11y-issue' }
         .to raise_error(Capybara::Poltergeist::JavascriptError)
     
       expected Capybara::Poltergeist::JavascriptError but nothing was raised
     # ./spec/component_guide/component_guide_spec.rb:88:in `block (2 levels) in <top (required)>'

Tests were running without running `assets:precompile` and this led javascript to be unavailable in the test app. They passed on CI because we run the assets:precompile task as part of the standard CI suite. They only failed locally.

We want to know if there are any errors in our dummy app.

fofr · 2017-08-25T10:59:14Z

Fixed local tests in 5636164

fofr

Have fixed the issue with local tests.

Top work 👍 💯

* Add aXe accessibility testing javascript to component guide pages (PR #33)

* Add aXe accessibility testing javascript (PR #33) * Mark strings in YAML fixtures as HTML safe (PR #36) * Refactor internal structs to use classes (PR #34)

NickColley mentioned this pull request Aug 17, 2017

Test component guide alphagov/government-frontend#452

Merged

NickColley changed the title ~~Add axe accessibility testing~~ WIP: Add axe accessibility testing Aug 17, 2017

NickColley force-pushed the add-axe-accessibility-testing branch from 470d52a to 8ab3503 Compare August 22, 2017 16:10

NickColley changed the title ~~WIP: Add axe accessibility testing~~ Add axe accessibility testing Aug 22, 2017

NickColley force-pushed the add-axe-accessibility-testing branch 3 times, most recently from 64d429b to 575a364 Compare August 22, 2017 17:13

fofr reviewed Aug 23, 2017

View reviewed changes

NickColley force-pushed the add-axe-accessibility-testing branch from 70bfd7d to 807a738 Compare August 23, 2017 17:29

NickColley closed this Aug 24, 2017

NickColley reopened this Aug 24, 2017

NickColley force-pushed the add-axe-accessibility-testing branch 4 times, most recently from 3d454df to 36d8a7a Compare August 24, 2017 10:31

fofr reviewed Aug 24, 2017

View reviewed changes

NickColley force-pushed the add-axe-accessibility-testing branch from 36d8a7a to 68b3f78 Compare August 24, 2017 10:48

fofr reviewed Aug 24, 2017

View reviewed changes

NickColley force-pushed the add-axe-accessibility-testing branch from 68b3f78 to cbb8b95 Compare August 24, 2017 11:30

NickColley force-pushed the add-axe-accessibility-testing branch from cbb8b95 to 3bd7524 Compare August 24, 2017 11:33

fofr reviewed Aug 24, 2017

View reviewed changes

NickColley force-pushed the add-axe-accessibility-testing branch from 57e95c1 to 885783f Compare August 24, 2017 13:02

fofr reviewed Aug 24, 2017

View reviewed changes

NickColley force-pushed the add-axe-accessibility-testing branch from 885783f to efc7237 Compare August 24, 2017 13:03

Add integration test for thrown errors in a11y tests

2ecfaac

Makes sure a JavaScript error is thrown on pages where there is an accessibility issue.

NickColley force-pushed the add-axe-accessibility-testing branch from efc7237 to 2ecfaac Compare August 24, 2017 13:10

Add formatting to the helpUrl to avoid broken links

50453e5

Since we're using the alpha release, the help urls generated are not live yet. This commit fixes that by pointing at the 2.3.x release, and adds a message that'll be noticed if someone updated axe in the future.

NickColley force-pushed the add-axe-accessibility-testing branch from 0f7d1fe to 50453e5 Compare August 24, 2017 14:22

Update webmock to avoid Ruby errors

46c86e3

See vcr/vcr#637 (comment)

fofr added 2 commits August 25, 2017 11:56

Don't use asset digest in test

5636164

Tests were running without running `assets:precompile` and this led javascript to be unavailable in the test app. They passed on CI because we run the assets:precompile task as part of the standard CI suite. They only failed locally.

Enable errors on missing translations

40bd6cd

We want to know if there are any errors in our dummy app.

fofr approved these changes Aug 25, 2017

View reviewed changes

fofr merged commit 588faf0 into master Aug 25, 2017

fofr deleted the add-axe-accessibility-testing branch August 25, 2017 11:04

fofr added a commit that referenced this pull request Aug 25, 2017

Version 0.9.0

7388d4d

* Add aXe accessibility testing javascript to component guide pages (PR #33)

fofr added a commit that referenced this pull request Aug 25, 2017

Version 0.9.0

032a38c

* Add aXe accessibility testing javascript (PR #33) * Mark strings in YAML fixtures as HTML safe (PR #36) * Refactor internal structs to use classes (PR #34)

fofr mentioned this pull request Aug 25, 2017

Version 0.9.0 #38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add axe accessibility testing #33

Add axe accessibility testing #33

NickColley commented Aug 17, 2017 •

edited

Loading

NickColley commented Aug 17, 2017

fofr commented Aug 17, 2017 •

edited

Loading

NickColley commented Aug 18, 2017

NickColley commented Aug 22, 2017

fofr Aug 23, 2017

NickColley Aug 23, 2017

fofr Aug 23, 2017

NickColley Aug 23, 2017

fofr commented Aug 23, 2017

NickColley commented Aug 24, 2017

NickColley commented Aug 24, 2017

fofr Aug 24, 2017

fofr Aug 24, 2017

NickColley Aug 24, 2017

fofr Aug 24, 2017

NickColley Aug 24, 2017

fofr Aug 24, 2017

fofr Aug 24, 2017

fofr commented Aug 24, 2017

NickColley commented Aug 24, 2017

fofr Aug 24, 2017

NickColley Aug 24, 2017 •

edited

Loading

fofr Aug 24, 2017

fofr Aug 24, 2017

fofr Aug 24, 2017

NickColley Aug 24, 2017

fofr commented Aug 25, 2017 •

edited

Loading

fofr commented Aug 25, 2017

fofr commented Aug 25, 2017

fofr left a comment

		@@ -0,0 +1,136 @@
		/* global describe, afterEach, it, expect */

		var TEST_SELECTOR = '.js-test-a11y'


		WebMock.disable_net_connect!(allow_localhost: true)

		# Allow us to test Javascript for example thrown errors

Add axe accessibility testing #33

Add axe accessibility testing #33

Conversation

NickColley commented Aug 17, 2017 • edited Loading

NickColley commented Aug 17, 2017

fofr commented Aug 17, 2017 • edited Loading

NickColley commented Aug 18, 2017

NickColley commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fofr commented Aug 23, 2017

NickColley commented Aug 24, 2017

NickColley commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fofr commented Aug 24, 2017

NickColley commented Aug 24, 2017

Choose a reason for hiding this comment

NickColley Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fofr commented Aug 25, 2017 • edited Loading

fofr commented Aug 25, 2017

fofr commented Aug 25, 2017

fofr left a comment

Choose a reason for hiding this comment

NickColley commented Aug 17, 2017 •

edited

Loading

fofr commented Aug 17, 2017 •

edited

Loading

NickColley Aug 24, 2017 •

edited

Loading

fofr commented Aug 25, 2017 •

edited

Loading