From 181d95e6b355b3bf7b549148d677f4e02c6b2bfd Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 6 Dec 2023 11:14:07 +0000 Subject: [PATCH 1/7] Move Skip link `getLinkedElement()` into constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This copies other components where “find it once” elements are queried and checked in the constructor --- .../src/govuk/components/skip-link/skip-link.mjs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs index fbac41f9a9..82a22bfcf0 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs @@ -36,18 +36,7 @@ export class SkipLink extends GOVUKFrontendComponent { } this.$module = $module - this.$linkedElement = this.getLinkedElement() - this.$module.addEventListener('click', () => this.focusLinkedElement()) - } - - /** - * Get linked element - * - * @private - * @returns {HTMLElement} $linkedElement - Target of the skip link - */ - getLinkedElement() { const linkedElementId = getFragmentFromUrl(this.$module.hash) // Check for link hash fragment @@ -68,7 +57,9 @@ export class SkipLink extends GOVUKFrontendComponent { }) } - return $linkedElement + this.$linkedElement = $linkedElement + + this.$module.addEventListener('click', () => this.focusLinkedElement()) } /** From 158d9f578b9d7dc485f27c48c70f3a8d9ab11e56 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 6 Dec 2023 12:42:33 +0000 Subject: [PATCH 2/7] Update Exit this page example to avoid `initAll()` --- .../exit-this-page-with-skiplink/index.njk | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/govuk-frontend-review/src/views/examples/exit-this-page-with-skiplink/index.njk b/packages/govuk-frontend-review/src/views/examples/exit-this-page-with-skiplink/index.njk index 898d54a3fe..419c2a6e57 100644 --- a/packages/govuk-frontend-review/src/views/examples/exit-this-page-with-skiplink/index.njk +++ b/packages/govuk-frontend-review/src/views/examples/exit-this-page-with-skiplink/index.njk @@ -20,3 +20,19 @@ redirectUrl: "https://www.gov.uk/" }) }} {% endblock %} + +{% block bodyEnd %} + + +{% endblock %} From e2163d39e68deef5190763f54ae7d46ca2ccd108 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 6 Dec 2023 15:15:28 +0000 Subject: [PATCH 3/7] Clarify Skip link URL error with `href=` attribute value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some pages can have multiple Skip link components and our error messages don’t explain which one has the problem The error message now includes the exact `getAttribute('href')` HTML attribute value to help identify the invalid link --- .../src/govuk/components/skip-link/skip-link.mjs | 7 +++++-- .../govuk/components/skip-link/skip-link.puppeteer.test.js | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs index 82a22bfcf0..e38d739d86 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs @@ -37,12 +37,15 @@ export class SkipLink extends GOVUKFrontendComponent { this.$module = $module - const linkedElementId = getFragmentFromUrl(this.$module.hash) + const hash = this.$module.hash + const href = this.$module.getAttribute('href') ?? '' + + const linkedElementId = getFragmentFromUrl(hash) // Check for link hash fragment if (!linkedElementId) { throw new ElementError( - 'Skip link: Root element (`$module`) attribute (`href`) has no URL fragment' + `Skip link: Target link (\`href="${href}"\`) has no hash fragment` ) } diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js index 55b0ae2ce6..51f54a3f23 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js @@ -144,7 +144,7 @@ describe('Skip Link', () => { cause: { name: 'ElementError', message: - 'Skip link: Root element (`$module`) attribute (`href`) has no URL fragment' + 'Skip link: Target link (`href="this-element-does-not-exist"`) has no hash fragment' } }) }) From 4e5cb523eddc990a12a48773a3e8c6e767b68687 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 6 Dec 2023 15:55:30 +0000 Subject: [PATCH 4/7] Check Skip link URL and return early for external URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t need to proceed with event listeners and focus handling if the target isn’t on the same page For example, Exit this page secondary link https://design-system.service.gov.uk/components/exit-this-page/#adding-the-secondary-link --- .../src/govuk/components/skip-link/skip-link.mjs | 14 ++++++++++++++ .../skip-link/skip-link.puppeteer.test.js | 9 +++++++++ 2 files changed, 23 insertions(+) diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs index e38d739d86..399d052ef9 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs @@ -39,6 +39,12 @@ export class SkipLink extends GOVUKFrontendComponent { const hash = this.$module.hash const href = this.$module.getAttribute('href') ?? '' + const url = new window.URL(this.$module.href) + + // Check for external link URL and return early + if (url.origin !== window.location.origin) { + return + } const linkedElementId = getFragmentFromUrl(hash) @@ -73,6 +79,10 @@ export class SkipLink extends GOVUKFrontendComponent { * @private */ focusLinkedElement() { + if (!this.$linkedElement) { + return + } + if (!this.$linkedElement.getAttribute('tabindex')) { // Set the element tabindex to -1 so it can be focused with JavaScript. this.$linkedElement.setAttribute('tabindex', '-1') @@ -101,6 +111,10 @@ export class SkipLink extends GOVUKFrontendComponent { * @private */ removeFocusProperties() { + if (!this.$linkedElement) { + return + } + this.$linkedElement.removeAttribute('tabindex') this.$linkedElement.classList.remove('govuk-skip-link-focused-element') } diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js index 51f54a3f23..7f397e45f4 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js @@ -67,6 +67,15 @@ describe('Skip Link', () => { }) describe('errors at instantiation', () => { + it('can return early without errors for external href', async () => { + await render(page, 'skip-link', { + context: { + text: 'Exit this page', + href: 'https://www.bbc.co.uk/weather' + } + }) + }) + it('can throw a SupportError if appropriate', async () => { await expect( render(page, 'skip-link', examples.default, { From b58d90950b81046707b2173f8f8252854a3f959f Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 6 Dec 2023 12:23:32 +0000 Subject: [PATCH 5/7] Check Skip link URL is valid by parsing with `new URL()` --- .../govuk/components/skip-link/skip-link.mjs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs index 399d052ef9..4e69ce1ff6 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs @@ -39,7 +39,24 @@ export class SkipLink extends GOVUKFrontendComponent { const hash = this.$module.hash const href = this.$module.getAttribute('href') ?? '' - const url = new window.URL(this.$module.href) + + /** @type {URL | undefined} */ + let url + + /** + * Check for valid link URL + * + * {@link https://caniuse.com/url} + * {@link https://url.spec.whatwg.org} + * + */ + try { + url = new window.URL(this.$module.href) + } catch (error) { + throw new ElementError( + `Skip link: Target link (\`href="${href}"\`) is invalid` + ) + } // Check for external link URL and return early if (url.origin !== window.location.origin) { From 74e3a6c2dec0308c1208eacde1afa198a13f03b2 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 6 Dec 2023 16:24:38 +0000 Subject: [PATCH 6/7] Ensure Skip link URL matches `location.pathname` --- .../govuk/components/skip-link/skip-link.mjs | 6 ++++-- .../skip-link/skip-link.puppeteer.test.js | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs index 4e69ce1ff6..e533925339 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs @@ -66,9 +66,11 @@ export class SkipLink extends GOVUKFrontendComponent { const linkedElementId = getFragmentFromUrl(hash) // Check for link hash fragment - if (!linkedElementId) { + if (!linkedElementId || url.pathname !== window.location.pathname) { throw new ElementError( - `Skip link: Target link (\`href="${href}"\`) has no hash fragment` + !linkedElementId + ? `Skip link: Target link (\`href="${href}"\`) has no hash fragment` + : `Skip link: Target link (\`href="${href}"\`) must stay on page (\`${window.location.pathname}\`)` ) } diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js index 7f397e45f4..8d96f8d36b 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js @@ -157,5 +157,22 @@ describe('Skip Link', () => { } }) }) + + it('throws when the href links to another page', async () => { + await expect( + render(page, 'skip-link', { + context: { + text: 'Skip to main content', + href: '/somewhere-else#main-content' + } + }) + ).rejects.toMatchObject({ + cause: { + name: 'ElementError', + message: + 'Skip link: Target link (`href="/somewhere-else#main-content"`) must stay on page (`/components/skip-link/preview`)' + } + }) + }) }) }) From 5cf5c3289a4ab85b2360bcff763fa9404cc58062 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Thu, 7 Dec 2023 14:35:09 +0000 Subject: [PATCH 7/7] Return early for internal URLs too For example when linking to other service pages (e.g. `/clear-session-data`) as described in the Exit this page guidance: https://design-system.service.gov.uk/components/exit-this-page/#consider-what-to-do-with-user-session-data Co-authored-by: Oliver Byford --- .../govuk/components/skip-link/skip-link.mjs | 15 +++--- .../skip-link/skip-link.puppeteer.test.js | 48 +++++++++++-------- 2 files changed, 37 insertions(+), 26 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs index e533925339..c81f2e8eab 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs @@ -58,19 +58,20 @@ export class SkipLink extends GOVUKFrontendComponent { ) } - // Check for external link URL and return early - if (url.origin !== window.location.origin) { + // Return early for external URLs or links to other pages + if ( + url.origin !== window.location.origin || + url.pathname !== window.location.pathname + ) { return } const linkedElementId = getFragmentFromUrl(hash) - // Check for link hash fragment - if (!linkedElementId || url.pathname !== window.location.pathname) { + // Check link path matching current page + if (!linkedElementId) { throw new ElementError( - !linkedElementId - ? `Skip link: Target link (\`href="${href}"\`) has no hash fragment` - : `Skip link: Target link (\`href="${href}"\`) must stay on page (\`${window.location.pathname}\`)` + `Skip link: Target link (\`href="${href}"\`) has no hash fragment` ) } diff --git a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js index 8d96f8d36b..cd3b7f8cc1 100644 --- a/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js +++ b/packages/govuk-frontend/src/govuk/components/skip-link/skip-link.puppeteer.test.js @@ -76,6 +76,33 @@ describe('Skip Link', () => { }) }) + it('can return early without errors when linking to another page (without hash fragment)', async () => { + await render(page, 'skip-link', { + context: { + text: 'Exit this page', + href: '/clear-session-data' + } + }) + }) + + it('can return early without errors when linking to another page (with hash fragment)', async () => { + await render(page, 'skip-link', { + context: { + text: 'Skip to main content', + href: '/somewhere-else#main-content' + } + }) + }) + + it('can return early without errors when linking to the current page (with hash fragment)', async () => { + await render(page, 'skip-link', { + context: { + text: 'Skip to main content', + href: '#content' + } + }) + }) + it('can throw a SupportError if appropriate', async () => { await expect( render(page, 'skip-link', examples.default, { @@ -146,31 +173,14 @@ describe('Skip Link', () => { render(page, 'skip-link', { context: { text: 'Skip to main content', - href: 'this-element-does-not-exist' - } - }) - ).rejects.toMatchObject({ - cause: { - name: 'ElementError', - message: - 'Skip link: Target link (`href="this-element-does-not-exist"`) has no hash fragment' - } - }) - }) - - it('throws when the href links to another page', async () => { - await expect( - render(page, 'skip-link', { - context: { - text: 'Skip to main content', - href: '/somewhere-else#main-content' + href: '/components/skip-link/preview' } }) ).rejects.toMatchObject({ cause: { name: 'ElementError', message: - 'Skip link: Target link (`href="/somewhere-else#main-content"`) must stay on page (`/components/skip-link/preview`)' + 'Skip link: Target link (`href="/components/skip-link/preview"`) has no hash fragment' } }) })