Add button shim and setup Puppeteer #572

alex-ju · 2018-03-05T13:30:18Z

This PR:

adds shim-links-with-button-role.js from govuk_frontend_toolkit
rewrites the code in plain javascript
updates the code to use common/generic functions (to deal with polyfill; e.g. addEvent, preventDefault). this functions would ideally sit in a common.js/core.js/lib.js that any component can reference.
sets up Puppeteer for integration testing and add tests to make sure the shim works in the browser

Manual tests
Browsers

IE8+
Latest Safari, Chrome, Firefox, Edge

AT testing

VoiceOver
JAWS
NDVA
Dragon

Automated tests

Jest integration tests written

Background on why we need this shim
For links that have role="button" we need to add an event handler so that it can be activated with the space bar alphagov/govuk_elements#272 (comment)

Notes to reviewer(s)
Left the commits separated so we can compare the govuk-frontend shim with the govuk_frontend_toolkit one.

Trello card

36degrees · 2018-03-05T13:53:41Z

src/components/button/button.js

+    },
+
+    init: function () {
+      var buttons = document.querySelectorAll('[role="button"]')


This will only work for buttons that are present on document ready, not buttons that are loaded in afterwards (e.g. via an AJAX request). This would be a 'regression' from what we've got in Frontend Toolkit, where we use a delegated event handler.

This might be OK, but we might want to consider:

documenting this limitation

allow init to be run more than once (i.e. ensure we only ever bind to the buttons that we haven't already handled)?

I believe jQuery uses global document event handling to work around this.

e.g.

document.addEventListener('click', event => { if (event.target === rolebutton) { } })

@36degrees yep, it's something that I'm looking into.
I was thinking that we can either reinitialise on DOMContentLoaded on document
addEvent(document, 'DOMContentLoaded', init) or watch for the keydown event at the document level as @NickColley described.

36degrees · 2018-03-05T13:54:32Z

src/components/button/button.js

@@ -0,0 +1,70 @@
+// javascript 'shim' to trigger the click event of element(s)


Being a little bit fussy here, but can take this opportunity to wrap comments consistently and use the proper capitalisation?

36degrees · 2018-03-05T13:56:31Z

src/components/button/button.js

+
+    eventHandler: function (event) {
+      // if the keyCode (which) is 32 it's a space, let's simulate a click.
+      if (GOVUK_FRONTEND.buttons.charCode(event) === 32) {


Would be good to copy what we've got in the details polyfill and make the space character code a constant.

36degrees · 2018-03-05T14:02:19Z

src/components/button/button.js

+  global.GOVUK_FRONTEND = GOVUK_FRONTEND
+
+  // auto-initialise
+  GOVUK_FRONTEND.buttons.init()


If we're immediately initialising, and this JS only binds to buttons present at the time the script loads, am I right in thinking we're then relying on the user including this JS at the end of their body, rather than in the <head>? That seems like it might be quite fragile in use.

Should we either run init on document ready, or use a delegated event handler?

kr8n3r · 2018-03-08T12:09:41Z

package.json

@@ -22,7 +22,8 @@
    "build:dist": "node bin/check-nvmrc.js && gulp build:dist --destination 'dist' && npm run test:build:dist",
    "test": "standard && gulp test && npm run test:app && npm run test:components && npm run test:generate:readme",
    "test:app": "jest app/__tests__/app.test.js --forceExit # express server fails to end process",
-    "test:components": "jest src/",
+    "test:components": "jest src/**/template.test.js --env=jsdom",
+    "test:components:integration": "jest src/button/button.test.js",


how are we going to scale that to multiple components?

kr8n3r · 2018-03-08T12:11:55Z

I get this

 FAIL  src/button/button.test.js
  /components/button
    /components/button/link
      ✕ renders a link button (15ms)
      ✕ triggers the click event when the space key is pressed (9ms)

  ● /components/button › /components/button/link › renders a link button

    net::ERR_CONNECTION_REFUSED

      at navigate (node_modules/puppeteer/lib/Page.js:520:37)

  ● /components/button › /components/button/link › triggers the click event when the space key is pressed

    Protocol error (Runtime.callFunctionOn): Cannot find context with specified idundefined

      at Promise (node_modules/puppeteer/lib/Connection.js:196:56)
      at CDPSession.send (node_modules/puppeteer/lib/Connection.js:195:12)
      at ExecutionContext.evaluateHandle (node_modules/puppeteer/lib/ExecutionContext.js:67:75)
      at ExecutionContext.evaluate (node_modules/puppeteer/lib/ExecutionContext.js:46:31)
      at Frame.evaluate (node_modules/puppeteer/lib/FrameManager.js:299:20)

Test Suites: 1 failed, 1 total
Tests:       2 failed, 2 total
Snapshots:   0 total
Time:        1.084s
Ran all test suites matching /src\/button\/button.test.js/i.
Teardown Puppeteer
Teardown Test Environment.

36degrees

Have we looked into expect-puppeteer at all? Looks like we could use it to expect a click rather than relying on side effects (the page location changing)

36degrees · 2018-03-08T14:33:29Z

lib/puppeteer_environment.js

+
+const DIR = path.join(os.tmpdir(), 'jest_puppeteer_global_setup')
+
+class PuppeteerEnvironment extends NodeEnvironment {


Would be really good to get some comments throughout this file, especially in the setup function.

36degrees · 2018-03-08T14:33:42Z

lib/puppeteer_environment.js

+
+class PuppeteerEnvironment extends NodeEnvironment {
+  async setup () {
+    console.log(chalk.yellow('Setup Test Environment.'))


Are these comments still useful, or should they be removed now?

I thought would be good to know what happens and when.

Can remove them all.

Happy to keep them in if they are useful :)

36degrees · 2018-03-08T14:34:09Z

lib/puppeteer_environment.js

+    await super.teardown()
+  }
+
+  runScript (script) {


Do we need to define this function if all we're doing is calling the parent method? Does it not just inherit it?

It's not being used now, but gives the space to allowing extra code before running the script. Can remove.

36degrees · 2018-03-08T14:34:54Z

lib/puppeteer_setup.js

+
+module.exports = async function () {
+  console.log(chalk.green('Setup Puppeteer'))
+  const browser = await puppeteer.launch({args: ['--no-sandbox']})


Again, comments here would be good. For example, what is --no-sandbox doing? Why is it needed?

36degrees · 2018-03-08T14:36:05Z

src/button/button.js

+      }
+    },
+
+    eventHandler: function (e) {


Would be ace to document this in the same way the other functions are documented.

36degrees · 2018-03-08T14:36:43Z

src/button/button.js

@@ -0,0 +1,85 @@
+/**
+ * JavaScript 'shim' to trigger the click event of element(s) when the space key is pressed.


Comments in this file are generally 💯. Is the doc block style new? Are there new conventions we're establishing that need to be documented?

We started using them on the lib jest helpers, seems like a good thing to try to do everywhere it makes sense.

36degrees · 2018-03-08T14:40:42Z

src/button/button.test.js

+
+      const href = await page.evaluate(() => document.body.getElementsByTagName('a')[0].getAttribute('href'))
+
+      await page.keyboard.press('Tab')


Would it be clearer / more explicit to use page.focus('a') here?

Sure. I was replicating my manual testing, but it makes sense to use focus for this.

alex-ju · 2018-03-08T14:59:04Z

@igloosi the server wasn't synchronised with puppeteer. last commit fixes that.

alex-ju · 2018-03-08T15:01:07Z

@36degrees no we haven't looked into it since this PR doesn't rely on expect-puppeteer or jest-puppeteer as a dependency. we only use puppeteer, so the documentation that I've been looking at was GoogleChrome/puppeteer. if we want to, we can change dependencies, rely on jest-puppeteer and update the tests accordingly.

NickColley · 2018-03-14T12:55:49Z

.travis.yml

 language: node_js
+node_js:


See if we can remove these changes and see if it still works?

sure thing. updated. works as expected.

alex-ju · 2018-03-14T14:31:16Z

Thanks everyone for feedback. I tried to discuss every comment online or offline.
It's now updated and ready for review.

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 5, 2018 13:30 Inactive

alex-ju changed the title ~~WIP: Add button shim~~ [WIP] Add button shim Mar 5, 2018

36degrees reviewed Mar 5, 2018

View reviewed changes

alex-ju force-pushed the add-button-shim branch from 89e5446 to e411946 Compare March 5, 2018 14:53

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 5, 2018 14:53 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 5, 2018 15:41 Inactive

alex-ju force-pushed the add-button-shim branch from 6a94640 to de7136c Compare March 5, 2018 16:14

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 5, 2018 16:14 Inactive

alex-ju force-pushed the add-button-shim branch from de7136c to 0a2b3a3 Compare March 7, 2018 14:46

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 7, 2018 14:46 Inactive

alex-ju force-pushed the add-button-shim branch from 0a2b3a3 to 6ab55d7 Compare March 7, 2018 14:47

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 7, 2018 14:47 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 8, 2018 10:47 Inactive

alex-ju changed the title ~~[WIP] Add button shim~~ Add button shim Mar 8, 2018

kr8n3r reviewed Mar 8, 2018

View reviewed changes

alex-ju changed the title ~~Add button shim~~ [DNM] Add button shim Mar 8, 2018

36degrees reviewed Mar 8, 2018

View reviewed changes

alex-ju force-pushed the add-button-shim branch from 0f6176a to c12e25d Compare March 8, 2018 14:47

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 8, 2018 14:47 Inactive

alex-ju changed the title ~~[DNM] Add button shim~~ Add button shim and setup Puppeteer Mar 8, 2018

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 8, 2018 15:04 Inactive

alex-ju force-pushed the add-button-shim branch from f603fd5 to c12e25d Compare March 8, 2018 15:08

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 8, 2018 15:08 Inactive

alex-ju force-pushed the add-button-shim branch from c12e25d to 14e1000 Compare March 8, 2018 15:13

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 8, 2018 15:13 Inactive

alex-ju added 2 commits March 8, 2018 15:24

Add button shim from govuk_frontend_toolkit

613d751

Rewrite in plain JavaScript

2e42674

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 12, 2018 17:31 Inactive

alex-ju force-pushed the add-button-shim branch from 6b30a15 to d30e84e Compare March 13, 2018 08:51

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 13, 2018 08:51 Inactive

alex-ju force-pushed the add-button-shim branch from d30e84e to 50c3c38 Compare March 13, 2018 09:04

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 13, 2018 09:04 Inactive

alex-ju force-pushed the add-button-shim branch from 50c3c38 to 2b5541d Compare March 13, 2018 09:11

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 13, 2018 09:11 Inactive

alex-ju force-pushed the add-button-shim branch from 2b5541d to 848c289 Compare March 13, 2018 09:22

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 13, 2018 09:22 Inactive

alex-ju force-pushed the add-button-shim branch from 848c289 to ffb7bc2 Compare March 13, 2018 09:34

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 13, 2018 09:34 Inactive

alex-ju force-pushed the add-button-shim branch from ffb7bc2 to b0041eb Compare March 13, 2018 11:58

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 13, 2018 11:58 Inactive

Listen to keydown at the document level

0338f95

alex-ju force-pushed the add-button-shim branch from b0041eb to 1cf7827 Compare March 14, 2018 09:49

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 14, 2018 09:49 Inactive

alex-ju changed the title ~~[WIP] Add button shim and setup Puppeteer~~ Add button shim and setup Puppeteer Mar 14, 2018

NickColley reviewed Mar 14, 2018

View reviewed changes

Setup puppeteer

1bc68cf

alex-ju force-pushed the add-button-shim branch from 1cf7827 to 4018c2d Compare March 14, 2018 12:59

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-572 March 14, 2018 12:59 Inactive

Add button test

3119acd

alex-ju force-pushed the add-button-shim branch from 4018c2d to 3119acd Compare March 14, 2018 14:24

govuk-design-system-ci deployed to govuk-frontend-review-pr-572 March 14, 2018 14:24 View deployment

NickColley approved these changes Mar 14, 2018

View reviewed changes

alex-ju merged commit a7488f7 into master Mar 14, 2018

alex-ju deleted the add-button-shim branch March 14, 2018 19:35

This was referenced Apr 11, 2018

Update changelog with JS Polyfill PRs #647

Merged

Release v0.0.27-alpha #648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add button shim and setup Puppeteer #572

Add button shim and setup Puppeteer #572

alex-ju commented Mar 5, 2018 •

edited

Loading

36degrees Mar 5, 2018

NickColley Mar 5, 2018

alex-ju Mar 5, 2018 •

edited

Loading

36degrees Mar 5, 2018

36degrees Mar 5, 2018

36degrees Mar 5, 2018

kr8n3r Mar 8, 2018

kr8n3r commented Mar 8, 2018

36degrees left a comment

36degrees Mar 8, 2018

36degrees Mar 8, 2018

alex-ju Mar 8, 2018

36degrees Mar 8, 2018

36degrees Mar 8, 2018

alex-ju Mar 8, 2018

36degrees Mar 8, 2018

36degrees Mar 8, 2018

36degrees Mar 8, 2018

NickColley Mar 8, 2018

36degrees Mar 8, 2018

alex-ju Mar 8, 2018

alex-ju commented Mar 8, 2018

alex-ju commented Mar 8, 2018 •

edited

Loading

NickColley Mar 14, 2018

alex-ju Mar 14, 2018

alex-ju commented Mar 14, 2018

		@@ -0,0 +1,70 @@
		// javascript 'shim' to trigger the click event of element(s)


		const DIR = path.join(os.tmpdir(), 'jest_puppeteer_global_setup')

		class PuppeteerEnvironment extends NodeEnvironment {

		@@ -0,0 +1,85 @@
		/**
		* JavaScript 'shim' to trigger the click event of element(s) when the space key is pressed.


		const href = await page.evaluate(() => document.body.getElementsByTagName('a')[0].getAttribute('href'))

		await page.keyboard.press('Tab')

Add button shim and setup Puppeteer #572

Add button shim and setup Puppeteer #572

Conversation

alex-ju commented Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju Mar 5, 2018 • 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

kr8n3r commented Mar 8, 2018

36degrees left a 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

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

alex-ju commented Mar 8, 2018

alex-ju commented Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju commented Mar 14, 2018

alex-ju commented Mar 5, 2018 •

edited

Loading

alex-ju Mar 5, 2018 •

edited

Loading

alex-ju commented Mar 8, 2018 •

edited

Loading