From 7cef20fd4e8c15310f5de70a08244d9ba4932803 Mon Sep 17 00:00:00 2001 From: Oscar Duignan <100650+oscarduignan@users.noreply.github.com> Date: Wed, 4 Dec 2024 22:57:25 +0000 Subject: [PATCH] fix flakey test (maybe) and try one method of introducing integrating fixes from adams path --- lib/browser-tests/puppeteer-helpers.js | 23 +- src/accessible-autocomplete.scss | 12 + .../accessibility-patches.browser.test.js | 237 ++++++++++++++++++ .../accessible-autocomplete.js | 34 ++- .../account-menu.nojs.browser.test.js | 2 +- src/components/back-link-helper/example.njk | 1 + .../timeout-dialog.browser.test.js | 89 ++++--- 7 files changed, 347 insertions(+), 51 deletions(-) create mode 100644 src/components/accessible-autocomplete/accessibility-patches.browser.test.js diff --git a/lib/browser-tests/puppeteer-helpers.js b/lib/browser-tests/puppeteer-helpers.js index f8c83db3..f845d596 100644 --- a/lib/browser-tests/puppeteer-helpers.js +++ b/lib/browser-tests/puppeteer-helpers.js @@ -41,6 +41,7 @@ export function withHmrcStylesAndScripts(body) { + ${preloadGovukFonts} @@ -49,6 +50,7 @@ export function withHmrcStylesAndScripts(body) { ${body} + `; @@ -56,12 +58,19 @@ export function withHmrcStylesAndScripts(body) { export async function render(page, body, options) { await page.setRequestInterception(true); - page.once('request', (req) => { - req.respond({ contentType: 'text/html', body }); - page.setRequestInterception(false); - }); - await page.goto('http://localhost:3000/', options); // if ever flaky, waitUntil networkidle0 (js loaded) - await page.evaluateHandle('document.fonts.ready'); - await page.bringToFront(); + const interceptPageRender = (req) => { + if (req.url() === 'http://localhost:3000/') { + return req.respond({ contentType: 'text/html', body }); + } + return Promise.resolve().then(() => req.continue()).catch(() => {}); + }; + page.on('request', interceptPageRender); + try { + await page.goto('http://localhost:3000/', options); // if ever flaky, waitUntil networkidle0 (js loaded) + await page.bringToFront(); + } finally { + page.off('request', interceptPageRender); + await page.setRequestInterception(false); + } return page; } diff --git a/src/accessible-autocomplete.scss b/src/accessible-autocomplete.scss index 0fa2efa3..4a2d85a7 100644 --- a/src/accessible-autocomplete.scss +++ b/src/accessible-autocomplete.scss @@ -11,3 +11,15 @@ $govuk-include-default-font-face: false; .autocomplete__hint { @include govuk-font($size: 19); } + +// the following is a bit more targeted, wouldn't work in ie11, and might be a bit brittle + +.govuk-form-group--error div:has(+ .govuk-select--error[data-module='hmrc-accessible-autocomplete']) > .autocomplete__wrapper .autocomplete__input { + border-color: #d4351c; +} + +// the following is more compatible, and probably unlikely to style unintended stuff + +//.govuk-form-group--error .autocomplete__wrapper .autocomplete__input { +// border-color: #d4351c; +//} diff --git a/src/components/accessible-autocomplete/accessibility-patches.browser.test.js b/src/components/accessible-autocomplete/accessibility-patches.browser.test.js new file mode 100644 index 00000000..47c9dd84 --- /dev/null +++ b/src/components/accessible-autocomplete/accessibility-patches.browser.test.js @@ -0,0 +1,237 @@ +import { + delay, + render, + withHmrcStylesAndScripts, +} from '../../../lib/browser-tests/puppeteer-helpers'; + +const adamsPolyfill = ` +// Note - updated to work with the HMRC Frontend implementation +// https://github.com/hmrc/play-frontend-hmrc#adding-accessible-autocomplete-css-and-javascript + +if (typeof HMRCAccessibleAutocomplete != 'undefined' && document.querySelector('[data-module="hmrc-accessible-autocomplete"]') != null) { + var originalSelect = document.querySelector('[data-module="hmrc-accessible-autocomplete"]'); + // load autocomplete - now handled by the HMRC component wrapper in Twirl + // accessibleAutocomplete.enhanceSelectElement({ + // selectElement: originalSelect, + // showAllValues: true + // }); + + // ===================================================== + // Polyfill autocomplete once loaded + // ===================================================== + var checkForLoad = setInterval(checkForAutocompleteLoad, 50); + var parentForm = upTo(originalSelect, 'form'); + + function polyfillAutocomplete(){ + var combo = parentForm.querySelector('[role="combobox"]'); + + // ===================================================== + // Update autocomplete once loaded with fallback's aria attributes + // Ensures hint and error are read out before usage instructions + // ===================================================== + if(originalSelect && originalSelect.getAttribute('aria-describedby') > ""){ + if(parentForm){ + if(combo){ + combo.setAttribute('aria-describedby', originalSelect.getAttribute('aria-describedby') + ' ' + combo.getAttribute('aria-describedby')); + } + } + } + // ===================================================== + // Update autocomplete once loaded with error styling if needed + // This won't work if the autocomplete css is loaded after the frontend library css because + // the autocomplete's border will override the error class's border (they are both the same specificity) + // but we can use the class assigned to build a more specific rule + // ===================================================== + setErrorClass(); + function setErrorClass(){ + if(originalSelect && originalSelect.classList.contains("govuk-select--error")){ + if(parentForm){ + if(combo){ + combo.classList.add("govuk-input--error"); + // Also set up an event listener to check for changes to input so we know when to repeat the copy + combo.addEventListener('focus', function(){setErrorClass()}); + combo.addEventListener('blur', function(){setErrorClass()}); + combo.addEventListener('change', function(){setErrorClass()}); + } + } + } + } + + // ===================================================== + // Ensure when user replaces valid answer with a non-valid answer, then valid answer is not retained + // ===================================================== + var holdSubmit = true; + parentForm.addEventListener('submit', function(e){ + if(holdSubmit){ + e.preventDefault() + if(originalSelect.querySelectorAll('[selected]').length > 0 || originalSelect.value > ""){ + + var resetSelect = false; + + if(originalSelect.value){ + if(combo.value != originalSelect.querySelector('option[value="' + originalSelect.value +'"]').text){ + resetSelect = true; + } + } + if(resetSelect){ + originalSelect.value = ""; + if(originalSelect.querySelectorAll('[selected]').length > 0){ + originalSelect.querySelectorAll('[selected]')[0].removeAttribute('selected'); + } + } + } + + holdSubmit = false; + //parentForm.submit(); + HTMLFormElement.prototype.submit.call(parentForm); // because submit buttons have id of "submit" which masks the form's natural form.submit() function + } + }) + + } + function checkForAutocompleteLoad(){ + if(parentForm.querySelector('[role="combobox"]')){ + clearInterval(checkForLoad) + polyfillAutocomplete(); + } + } + + +} + + +// Find first ancestor of el with tagName +// or undefined if not found +function upTo(el, tagName) { + tagName = tagName.toLowerCase(); + + while (el && el.parentNode) { + el = el.parentNode; + if (el.tagName && el.tagName.toLowerCase() == tagName) { + return el; + } + } + + // Many DOM methods return null if they don't + // find the element they are searching for + // It would be OK to omit the following and just + // return undefined + return null; +} +`; + +describe('Patched accessible autocomplete', () => { + describe('has an aria-describedby attribute linking to the hint and error associated with the original select', () => { + it('should add hint and error before the usage instructions', async () => { + await render(page, withHmrcStylesAndScripts(` +
+ +
+ This can be different to where you went before +
+

+ Error: Select a location +

+ +
+ `)); + + const element = await page.$('#location'); + const tagName = await element.evaluate((el) => el.tagName.toLowerCase()); + const ariaDescribedBy = await element.evaluate((el) => el.getAttribute('aria-describedby')); + + expect(tagName).not.toBe('select'); // or select element was not enhanced to be an autocomplete component + expect(ariaDescribedBy).toBe('location-hint location-error location__assistiveHint'); + }); + + it('should not be possible for them to be added twice if page is still using adams patch', async () => { + await render(page, withHmrcStylesAndScripts(` +
+
+ +
+ This can be different to where you went before +
+

+ Error: Select a location +

+ +
+
+ `)); + + await page.evaluate(adamsPolyfill); + await delay(100); // because it takes ~50ms for adam's polyfill to apply + + const element = await page.$('#location'); + const tagName = await element.evaluate((el) => el.tagName.toLowerCase()); + const ariaDescribedBy = await element.evaluate((el) => el.getAttribute('aria-describedby')); + + expect(tagName).not.toBe('select'); // or select element was not enhanced to be an autocomplete component + expect(ariaDescribedBy).toBe('location-hint location-error location__assistiveHint'); + }); + }); + describe('original select has an error', () => { + it('should have the border colour of a gov.uk input with errors', async () => { + await render(page, withHmrcStylesAndScripts(` +
+ +
+ This can be different to where you went before +
+

+ Error: Select a location +

+ +
+ `)); + + const element = await page.$('#location'); + const tagName = await element.evaluate((el) => el.tagName.toLowerCase()); + const borderColor = await element.evaluate((el) => getComputedStyle(el).getPropertyValue("border-color")); + + // await jestPuppeteer.debug(); + + expect(tagName).not.toBe('select'); // or select element was not enhanced to be an autocomplete component + expect(borderColor).toBe('rgb(212, 53, 28)'); + }); + }); +}); diff --git a/src/components/accessible-autocomplete/accessible-autocomplete.js b/src/components/accessible-autocomplete/accessible-autocomplete.js index 676d8185..1ecea922 100644 --- a/src/components/accessible-autocomplete/accessible-autocomplete.js +++ b/src/components/accessible-autocomplete/accessible-autocomplete.js @@ -6,20 +6,21 @@ function AccessibleAutoComplete($module, window, document) { AccessibleAutoComplete.prototype.init = function init() { if (this.$module) { + const selectElement = this.$module; const showAllValues = (this.$module.getAttribute('data-show-all-values') === 'true'); const autoselect = (this.$module.getAttribute('data-auto-select') === 'true'); const defaultValue = this.$module.getAttribute('data-default-value'); const minLength = this.$module.getAttribute('data-min-length'); const configurationOptions = { - selectElement: this.$module, + selectElement, showAllValues, autoselect, defaultValue, minLength, }; - const language = this.$module.getAttribute('data-language') || 'en'; + const language = selectElement.getAttribute('data-language') || 'en'; if (language === 'cy') { configurationOptions.tAssistiveHint = () => 'Pan fydd canlyniadau awtogwblhau ar gael, defnyddiwch y saethau i fyny ac i lawr i’w hadolygu a phwyswch y fysell ’enter’ i’w dewis.' @@ -34,7 +35,36 @@ AccessibleAutoComplete.prototype.init = function init() { }; } + const selectElementOriginalId = selectElement.id; + const selectElementAriaDescribedBy = selectElement.getAttribute('aria-describedby'); + window.HMRCAccessibleAutocomplete.enhanceSelectElement(configurationOptions); + + const autocompleteElement = document.getElementById(selectElementOriginalId); + const autocompleteElementAriaDescribedBy = autocompleteElement && autocompleteElement.getAttribute('aria-describedby'); + + const autocompleteElementMissingAriaDescribedAttrs = ( + autocompleteElement + && autocompleteElement.tagName !== 'select' + && autocompleteElementAriaDescribedBy + && selectElementAriaDescribedBy + && !autocompleteElementAriaDescribedBy.includes(selectElementAriaDescribedBy) + ); + if (autocompleteElementMissingAriaDescribedAttrs) { + // if there is a hint and/or error then the autocomplete element + // needs to be aria-describedby these, which it isn't be default + // we need to check if it hasn't already been done to avoid + autocompleteElement.setAttribute( + 'aria-describedby', + `${selectElementAriaDescribedBy} ${autocompleteElementAriaDescribedBy}`, + ); + // and in case page is still using adam's patch, this should stop + // the select elements aria described by being added to the + // autocomplete element twice when that runs (though unsure if a + // screen reader would actually announce the elements twice if same + // element was listed twice in the aria-describedby attribute) + selectElement.setAttribute('aria-describedby', ''); + } } }; diff --git a/src/components/account-menu/account-menu.nojs.browser.test.js b/src/components/account-menu/account-menu.nojs.browser.test.js index 83058990..49a4f118 100644 --- a/src/components/account-menu/account-menu.nojs.browser.test.js +++ b/src/components/account-menu/account-menu.nojs.browser.test.js @@ -3,7 +3,7 @@ import { examplePreview } from '../../../lib/url-helpers'; describe('/components/account-menu', () => { const defaultAccountMenu = examplePreview('account-menu/default'); - async function displayStyle(selector) { + function displayStyle(selector) { return page.$eval(selector, (el) => window.getComputedStyle(el).display); } diff --git a/src/components/back-link-helper/example.njk b/src/components/back-link-helper/example.njk index da5017e7..89f7379f 100644 --- a/src/components/back-link-helper/example.njk +++ b/src/components/back-link-helper/example.njk @@ -8,6 +8,7 @@ + {{ govukBackLink({ attributes: { "data-module": "hmrc-back-link" diff --git a/src/components/timeout-dialog/timeout-dialog.browser.test.js b/src/components/timeout-dialog/timeout-dialog.browser.test.js index a07a64ed..96788f44 100644 --- a/src/components/timeout-dialog/timeout-dialog.browser.test.js +++ b/src/components/timeout-dialog/timeout-dialog.browser.test.js @@ -77,7 +77,7 @@ describe('/components/timeout-dialog', () => { `); await clockTickSeconds(page, 900); await page.waitForNavigation({ timeout: 500 }); - await expect(page.url()).toMatch('/timeout-reached'); + expect(page.url()).toMatch('/timeout-reached'); }); it('should not sign you out when the countdown runs out if you chose to stay signed in', async () => { @@ -91,7 +91,7 @@ describe('/components/timeout-dialog', () => { await expect(page).toClick('button', { text: 'Stay signed in' }); await clockTickSeconds(page, 1); await delay(500); - await expect(page.url()).not.toMatch('/timeout-reached'); + expect(page.url()).not.toMatch('/timeout-reached'); await expect(page).not.toShowTimeoutDialog(); }); @@ -122,7 +122,7 @@ describe('/components/timeout-dialog', () => { await expect(page).toClick('button', { text: 'Stay signed in' }); await clockTickSeconds(page, 900); await page.waitForNavigation({ timeout: 500 }); - await expect(page.url()).toMatch('/timeout-reached'); + expect(page.url()).toMatch('/timeout-reached'); }); it('should let you extend your session repeatedly', async () => { @@ -141,7 +141,7 @@ describe('/components/timeout-dialog', () => { await expect(page.url()).not.toMatch('/timeout-reached'); await clockTickSeconds(page, 900); await page.waitForNavigation({ timeout: 500 }); - await expect(page.url()).toMatch('/timeout-reached'); + expect(page.url()).toMatch('/timeout-reached'); }); it('should let you sign out early', async () => { @@ -154,7 +154,7 @@ describe('/components/timeout-dialog', () => { await clockTickSeconds(page, 800); await expect(page).toClick('a', { text: 'Sign out' }); await page.waitForNavigation({ timeout: 500 }); - await expect(page.url()).toMatch('/timeout-reached'); + expect(page.url()).toMatch('/timeout-reached'); }); it('should let you specify a separate timeout url', async () => { @@ -178,7 +178,7 @@ describe('/components/timeout-dialog', () => { await clockTickSeconds(page, 800); await expect(page).toClick('a', { text: 'Sign out' }); await page.waitForNavigation({ timeout: 500 }); - await expect(page.url()).toMatch('/signed-out-early'); + expect(page.url()).toMatch('/signed-out-early'); }); // TODO at the moment it doesn't, should we change test or implementation? @@ -193,19 +193,22 @@ describe('/components/timeout-dialog', () => { `); await clockTickSeconds(page, 899); await page.setRequestInterception(true); - await page.once('request', (req) => { - setTimeout(() => req.respond({ - contentType: 'text/plain', - body: 'simulate slow response to signing out', - }), 2000); - page.setRequestInterception(false); - }); - await page.evaluate(() => { - document.getElementById('hmrc-timeout-sign-out-link').click(); - window.clock.tick(1000); // to reach timeout while sign out page is still loading - }); - await page.waitForNavigation({ timeout: 500 }); - await expect(page.url()).toMatch('/signed-out-early'); + try { + page.once('request', (req) => { + setTimeout(() => req.respond({ + contentType: 'text/plain', + body: 'simulate slow response to signing out', + }), 2000); + }); + await page.evaluate(() => { + document.getElementById('hmrc-timeout-sign-out-link').click(); + window.clock.tick(1000); // to reach timeout while sign out page is still loading + }); + await page.waitForNavigation({ timeout: 500 }); + expect(page.url()).toMatch('/signed-out-early'); + } finally { + await page.setRequestInterception(false); + } }); function takeTextContentEachSecondForAMinute(page, selector) { @@ -255,7 +258,7 @@ describe('/components/timeout-dialog', () => { await clockTickSeconds(page, 1); await page.waitForNavigation({ timeout: 500 }); - await expect(page.url()).toMatch('/timeout-reached'); + expect(page.url()).toMatch('/timeout-reached'); }); function twentyTimes(value) { return Array.from({ length: 20 }, () => value); } @@ -292,7 +295,7 @@ describe('/components/timeout-dialog', () => { await clockTickSeconds(page, 1); await page.waitForNavigation({ timeout: 500 }); - await expect(page.url()).toMatch('/timeout-reached'); + expect(page.url()).toMatch('/timeout-reached'); }); }); @@ -306,28 +309,31 @@ describe('/components/timeout-dialog', () => { let completeSlowTimeoutRequest; const slowTimeout = new Promise((resolve) => { completeSlowTimeoutRequest = resolve; }); await page.setRequestInterception(true); - page.once('request', (req) => { - slowTimeout.then(() => { - req.respond({ - contentType: 'text/plain', - body: 'timeout page reached', + try { + page.once('request', (req) => { + slowTimeout.then(() => { + req.respond({ + contentType: 'text/plain', + body: 'timeout page reached', + }); }); - page.setRequestInterception(false); }); - }); - const [visibleMessage, audibleMessage] = await page.evaluate(() => { - window.clock.tick(10000); // bring up warning - window.clock.tick(30000); // overrun timeout by 20s, the redirect will hang until resolved - return [ - document.querySelector('.hmrc-timeout-dialog__message').textContent, - document.querySelector('#hmrc-timeout-message').textContent, - ]; - }); - expect(visibleMessage).toBe('For your security, we will sign you out in 0 seconds.'); - expect(audibleMessage).toBe('For your security, we will sign you out in 20 seconds.'); - completeSlowTimeoutRequest(); - await page.waitForNavigation({ timeout: 500 }); - await expect(page).toMatchTextContent('timeout page reached'); + const [visibleMessage, audibleMessage] = await page.evaluate(() => { + window.clock.tick(10000); // bring up warning + window.clock.tick(30000); // overrun timeout by 20s, the redirect will hang until resolved + return [ + document.querySelector('.hmrc-timeout-dialog__message').textContent, + document.querySelector('#hmrc-timeout-message').textContent, + ]; + }); + expect(visibleMessage).toBe('For your security, we will sign you out in 0 seconds.'); + expect(audibleMessage).toBe('For your security, we will sign you out in 20 seconds.'); + completeSlowTimeoutRequest(); + await page.waitForNavigation({ timeout: 500 }); + await expect(page).toMatchTextContent('timeout page reached'); + } finally { + await page.setRequestInterception(false); + } }); it('should display the time remaining as a whole number of seconds', async () => { @@ -404,5 +410,6 @@ describe('/components/timeout-dialog', () => { await expect(background).toShowTimeoutDialog(); expect(await visualCountdownFrom(background)) .toBe('For your security, we will sign you out in 10 seconds.'); + await session.close(); }); });