Set skip link linked element in init and put event listener behind flag #2467

hannalaakso · 2021-12-08T16:19:14Z

This PR solves the issue mentioned in #2450 (review).

Improve the readability and performance of the skip link script by:

checking for the linked element early as part of the initialisation
moving the event listener for the main content behind a flag so it's only set once
simplifying init() and getFragmentFromUrl()
removing redundant clickEventHandler()

This also doesn’t seem to play nice with the exported Components can be initialised test we have in all.test.js, I don’t fully understand right now whether it’s picked up on an issue or if the test is not working correctly. Was going to do some test debugging tomorrow anyway so could look at this then. Solved in #2470

Tested in

Operating system	Browser	Supports
Windows	Internet Explorer 11	x
	Edge (latest versions)	x
	Google Chrome (latest versions)	x
	Mozilla Firefox (latest versions)	x
macOS	Safari 12 and later	x
	Google Chrome (latest versions)	x
	Mozilla Firefox (latest versions)	x
iOS	Safari for iOS 12.1 and later
	Google Chrome (latest versions)
Android	Google Chrome (latest versions)
	Samsung Internet (latest versions)

36degrees · 2021-12-09T12:32:23Z

This also doesn’t seem to play nice with the exported Components can be initialised test we have in all.test.js, I don’t fully understand right now whether it’s picked up on an issue or if the test is not working correctly. Was going to do some test debugging tomorrow anyway so could look at this then.

I think I've managed to get to the bottom of this…

We're bailing out of the getLinkedElement if href strictly equals false.

govuk-frontend/src/govuk/components/skip-link/skip-link.js

Lines 39 to 42 in 91f6254

    
           // If the skip link does not have a href, return early 
        
           if (href === false) { 
        
             return false 
        
           }

If you try to get the href property of an element that does not have an href attribute, you'll get undefined – and undefined != false – so the function does not return and continues to try and find the indexOf('#') against a variable which is undefined.

BUT – why would the skip link be initialised on an element that doesn't have an href?! Especially in the context of that test, where we're only initialising modules within a particular part of the page – which does not include a skip link.

It looks like the error is actually coming from the test immediately before it:

govuk-frontend/src/govuk/all.test.js

Lines 61 to 76 in 429af61

    
           it('exported Components can be initialised', async () => { 
        
             await page.goto(baseUrl + '/', { waitUntil: 'load' }) 
        
             const GOVUKFrontendGlobal = await page.evaluate(() => window.GOVUKFrontend) 
        
             var components = Object.keys(GOVUKFrontendGlobal).filter(method => method !== 'initAll') 
        
             // Check that all the components on the GOV.UK Frontend global can be initialised 
        
             components.forEach(component => { 
        
               page.evaluate(component => { 
        
                 const Component = window.GOVUKFrontend[component] 
        
                 const $module = document.documentElement 
        
                 new Component($module).init() 
        
               }, component) 
        
             }) 
        
           })

There are a few unusual things going on in that test:

The test initialises each module whilst passing in document.documentElement as the $module, whereas the module JavaScript expects the element with the data-module attribute – so the skip link JS is being initialised against the <body> which obviously does not have an href!
If we wanted to initialise with the correct HTML for each component, the tests are being run against the 'home page' of the review app, which doesn't include the majority of them.
We do not wait for the calls to page.evaluate on line 70 to complete – those calls are 'fired and forgotten', so by the time the errors in the JS occur we've moved on to the next test – which is why the tests are surfacing in the wrong place.

You can make the error surface in the correct test by changing lines 69 to 75 so that we await the evaluation:

       // Check that all the components on the GOV.UK Frontend global can be initialised
-      components.forEach(component => {
-        page.evaluate(component => {
+      await page.evaluate((components) => {
+        components.forEach(component => {
           const Component = window.GOVUKFrontend[component]
           const $module = document.documentElement
           new Component($module).init()
-        }, component)
-      })
+        })
+      }, components)
     })

However, I am still not sure what we should do with the test… it's not testing what we think it does, and at the minute seems to offer little value. From talking to @lfdebrux, the suggestions so far are to either delete it, or to alter it so that it only checks that there is an init function for each component, and not try to execute it. Thoughts?

The test currently passes `document.documentElement` (the root `<html>` element) as the `$module` when initialising each component. This is also incorrect – every component expects `$module` to be the element with the corresponding data-module attribute. If we did want to initialise with the correct HTML for each component, we'd need to make substantial changes to the test. At the minute it runs in the context of the 'home page' of the review app, which doesn't include the majority of the components that have JavaScript. Given these constraints, replace the test with one that does not try to instantiate or initialise the components, but only check that they all have an `init` function. We were also not waiting for the Promises returned by `page.evaluate` to by fulfilled, so any errors thrown by the JS could cause unrelated tests that run later to fail. We were seeing this occur in #2467 when adding tests for the new skip link JavaScript. Fix this by moving the bulk of the test logic into a single `page.evaluate` call and using `await` to pause execution until the returned Promise is fulfilled.

The test currently passes `document.documentElement` (the root `<html>` element) as the `$module` when initialising each component. This is also incorrect – every component expects `$module` to be the element with the corresponding data-module attribute. If we did want to initialise with the correct HTML for each component, we'd need to make substantial changes to the test. At the minute it runs in the context of the 'home page' of the review app, which doesn't include the majority of the components that have JavaScript. Given these constraints, replace the test with one that does not try to instantiate or initialise the components, but only check that they all have an `init` function. We were also not waiting for the Promises returned by `page.evaluate` to by fulfilled, so any errors thrown by the JS could cause unrelated tests that run later to fail. We were seeing this occur in #2467 when adding tests for the new skip link JavaScript. Fix this by moving the bulk of the test logic into a single `page.evaluate` call and using `await` to pause execution until the returned Promise is fulfilled. (It would likely make more sense to run these tests against the classes directly, rather than using Puppeteer to assert things about the `window.GOVUKFrontend` global object in a browser. However, we don't currently have a good way to test the component JavaScript directly. When we do, we should revisit this test.)

36degrees · 2021-12-09T17:24:33Z

I've raised a PR to update the test (#2470)

The test currently passes `document.documentElement` (the root `<html>` element) as the `$module` when initialising each component. This is also incorrect – every component expects `$module` to be the element with the corresponding data-module attribute. If we did want to initialise with the correct HTML for each component, we'd need to make substantial changes to the test. At the minute it runs in the context of the 'home page' of the review app, which doesn't include the majority of the components that have JavaScript. Given these constraints, replace the test with one that does not try to instantiate or initialise the components, but only check that they all have an `init` function. We were also not waiting for the Promises returned by `page.evaluate` to by fulfilled, so any errors thrown by the JS could cause unrelated tests that run later to fail. We were seeing this occur in #2467 when adding tests for the new skip link JavaScript. Fix this by moving the bulk of the test logic into a single `page.evaluate` call and using `await` to pause execution until the returned Promise is fulfilled. (It would likely make more sense to run these tests against the classes directly, rather than using Puppeteer to assert things about the `window.GOVUKFrontend` global object in a browser. However, we don't currently have a good way to test the component JavaScript directly. When we do, we should revisit this test.)

The test currently passes `document.documentElement` (the root `<html>` element) as the `$module` when initialising each component. This is not what the component expects – `$module` should be the element with the corresponding `data-module` attribute, for example `<div data-module="govuk-button">`. If we did want to initialise with the correct HTML for each component, we'd need to make substantial changes to the test. At the minute it runs in the context of the 'home page' of the review app, which doesn't include the majority of the components that have JavaScript. Given these constraints, replace the test with one that does not try to instantiate or initialise the components, but only check that they all have an `init` function. We were also not waiting for the Promises returned by `page.evaluate` to by fulfilled, so any errors thrown by the JS could cause unrelated tests that run later to fail. We were seeing this occur in #2467 when adding tests for the new skip link JavaScript. Fix this by moving the bulk of the test logic into a single `page.evaluate` call and using `await` to pause execution until the returned Promise is fulfilled. (It would likely make more sense to run these tests against the classes directly, rather than using Puppeteer to assert things about the `window.GOVUKFrontend` global object in a browser. However, we don't currently have a good way to test the component JavaScript directly. When we do, we should revisit this test.)

36degrees

I think this approach makes sense – nice work 👍🏻 I pushed a commit to fix the issue in my previous comment, hope that's OK.

There's a couple of other things that I think we might need to look at, and a couple of suggestions.

src/govuk/components/skip-link/skip-link.js

Improve the readability and performance of the skip link script by: - checking for the linked element early as part of the initialisation - moving the event listener for the main content behind a flag so it's only set once - simplifying init() and getFragmentFromUrl() - removing redundant clickEventHandler()

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2467 December 8, 2021 16:19 Inactive

36degrees mentioned this pull request Dec 9, 2021

Fix flawed 'components can be initialised' test #2470

Merged

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2467 December 10, 2021 10:56 Inactive

36degrees reviewed Dec 10, 2021

View reviewed changes

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2467 December 13, 2021 19:54 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2467 December 13, 2021 20:10 Inactive

hannalaakso marked this pull request as ready for review December 13, 2021 20:10

hannalaakso changed the title ~~Set linked element and event listener in skip link on initialisation~~ Set skip link linked element in init and put event listener behind flag Dec 13, 2021

36degrees reviewed Dec 14, 2021

View reviewed changes

src/govuk/components/skip-link/skip-link.js Outdated Show resolved Hide resolved

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2467 December 14, 2021 11:52 Inactive

36degrees approved these changes Dec 14, 2021

View reviewed changes

hannalaakso force-pushed the update-skip-link-event-listener branch from 3a9e1e3 to cf7e9ad Compare December 14, 2021 17:25

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2467 December 14, 2021 17:25 Inactive

hannalaakso merged commit 70e3d31 into main Dec 14, 2021

hannalaakso deleted the update-skip-link-event-listener branch December 14, 2021 17:28

vanitabarrett mentioned this pull request Dec 15, 2021

After clicking "Skip to main content", focus isn't sent to the content so screenreader can't read from there #2187

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set skip link linked element in init and put event listener behind flag #2467

Set skip link linked element in init and put event listener behind flag #2467

hannalaakso commented Dec 8, 2021 •

edited

Loading

36degrees commented Dec 9, 2021

36degrees commented Dec 9, 2021

36degrees left a comment

Set skip link linked element in init and put event listener behind flag #2467

Set skip link linked element in init and put event listener behind flag #2467

Conversation

hannalaakso commented Dec 8, 2021 • edited Loading

36degrees commented Dec 9, 2021

36degrees commented Dec 9, 2021

36degrees left a comment

Choose a reason for hiding this comment

hannalaakso commented Dec 8, 2021 •

edited

Loading