From 6d331914a001b5243ab5006e1a6a3a1bf2e6373d Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Fri, 19 Jun 2020 13:59:43 +0100 Subject: [PATCH 1/5] Preserve conditional reveal state when going back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the user navigates back to a previous page that includes a conditional reveal, the visible state of the conditionally revealed content is currently only preserved in some browsers. Firefox / Safari ---------------- Recent versions of Firefox and Safari use a Back-Forward Cache ('bfcache')[1] which preserves the entire page, which means the JavaScript is not re-evaluated but the browser 'remembers' that the conditional reveal was visible. Internet Explorer ----------------- In Internet Explorer the state of the form controls has not been restored at the point we currently call the init function, and so the conditional reveal state is not preserved. To fix this, wait for the `DOMContentLoaded` event before syncing the visibility of the conditional reveals to their checkbox / radio state. As the checkbox state in IE8-11 has been restored before the `DOMContentLoaded` event fires, this fixes the preservation of the reveal state in Internet Explorer. We already polyfill document.addEventListener and the DOMContentLoaded event for IE8, so we don't have to treat it as a special case. Edge Legacy ----------- In Edge 18, the state of the form controls does not seem to be restored at all when navigating back, so there is nothing to sync the conditional reveal state to 😢 Chromium based browsers (Chrome, Edge, Opera) --------------------------------------------- In browsers based on Chromium 78 or older, the state of the form controls has been restored at the point we invoke the init function, and so the reveal state currently displays correctly, even without waiting for the `DOMContentLoaded` event. In Chromium 79, the 'timing of restoring control state was changed so that it is done as a task posted just after DOMContentLoaded' [2]. This means that even after the `DOMContentLoaded` event the form state has not yet been restored. The recommended approach seems to be to wait for the `pageshow` event, however in Chromium 79 the form control state has not been restored at the point this event fires either [3]! This was fixed in Chromium 80 [4]. So: - in Chrome ≤ 78, the form control state is restored before the script is evaluated, before both the `DOMContentLoaded` and `pageshow` events. - in Chrome = 79, the form control state is restored after both the `DOMContentLoaded` and `pageshow` events. - in Chrome ≥ 80, the form control state is restored after the `DOMContentLoaded` but before the `pageshow` event. Syncing the conditional reveal state after the `pageshow` event preserves the conditional reveal state except for Chromium 79 where it remains broken. Given that Chrome 79's usage is already trending towards 0 [5] (0.29% in May 2020) and there's seemingly no other event we can listen for that'll guarantee the state is restored, we'll accept that it'll remain broken in this specific version (affecting Chrome 79, Edge 79 and Opera 66). The `pageshow` event is not supported in older browsers including IE8-10 so we need to listen for both. This means that we 'sync' twice in browsers that support `pageshow`, but the performance impact should be minimal. [1]: https://www.chromestatus.com/feature/5815270035685376 [2]: https://chromium.googlesource.com/chromium/src.git/+/069ad65654655b7bdfb9b760f188395840bc4be4 [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1035662 [4]: https://crrev.com/069ad65654655b7bdfb9b760f188395840bc4be4 [5]: https://gs.statcounter.com/browser-version-market-share --- src/govuk/components/checkboxes/checkboxes.js | 26 +++++++++++++++-- src/govuk/components/radios/radios.js | 29 ++++++++++++++++--- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/govuk/components/checkboxes/checkboxes.js b/src/govuk/components/checkboxes/checkboxes.js index 3f883d966c..d9c7243fba 100644 --- a/src/govuk/components/checkboxes/checkboxes.js +++ b/src/govuk/components/checkboxes/checkboxes.js @@ -1,5 +1,6 @@ import '../../vendor/polyfills/Function/prototype/bind' -import '../../vendor/polyfills/Event' // addEventListener and event.target normaliziation +// addEventListener, event.target normalization and DOMContentLoaded +import '../../vendor/polyfills/Event' import '../../vendor/polyfills/Element/prototype/classList' import { nodeListForEach } from '../../common' @@ -29,13 +30,32 @@ Checkboxes.prototype.init = function () { // If we have content that is controlled, set attributes. $input.setAttribute('aria-controls', controls) $input.removeAttribute('data-aria-controls') - this.setAttributes($input) - }.bind(this)) + }) + + if (document.readyState === 'loading') { + window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) + } else { + this.syncState() + } + + // When the page is restored after navigating 'back' in some browsers the + // state of form controls is not restored until *after* the DOMContentLoaded + // event is fired, so we need to sync after the pageshow event is fired as + // well. + // + // (Older browsers don't have a pageshow event, so we do both.) + window.addEventListener('pageshow', this.syncState.bind(this)) // Handle events $module.addEventListener('click', this.handleClick.bind(this)) } +Checkboxes.prototype.syncState = function () { + nodeListForEach(this.$inputs, function ($input) { + this.setAttributes($input) + }.bind(this)) +} + Checkboxes.prototype.setAttributes = function ($input) { var inputIsChecked = $input.checked $input.setAttribute('aria-expanded', inputIsChecked) diff --git a/src/govuk/components/radios/radios.js b/src/govuk/components/radios/radios.js index b4af3dcad4..353b2d3c83 100644 --- a/src/govuk/components/radios/radios.js +++ b/src/govuk/components/radios/radios.js @@ -1,15 +1,17 @@ import '../../vendor/polyfills/Function/prototype/bind' -import '../../vendor/polyfills/Event' // addEventListener and event.target normaliziation +// addEventListener, event.target normalization and DOMContentLoaded +import '../../vendor/polyfills/Event' import '../../vendor/polyfills/Element/prototype/classList' import { nodeListForEach } from '../../common' function Radios ($module) { this.$module = $module + this.$inputs = $module.querySelectorAll('input[type="radio"]') } Radios.prototype.init = function () { var $module = this.$module - var $inputs = $module.querySelectorAll('input[type="radio"]') + var $inputs = this.$inputs /** * Loop over all items with [data-controls] @@ -28,13 +30,32 @@ Radios.prototype.init = function () { // If we have content that is controlled, set attributes. $input.setAttribute('aria-controls', controls) $input.removeAttribute('data-aria-controls') - this.setAttributes($input) - }.bind(this)) + }) + + if (document.readyState === 'loading') { + window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) + } else { + this.syncState() + } + + // When the page is restored after navigating 'back' in some browsers the + // state of form controls is not restored until *after* the DOMContentLoaded + // event is fired, so we need to sync after the pageshow event is fired as + // well. + // + // (Older browsers don't have a pageshow event, so we do both.) + window.addEventListener('pageshow', this.syncState.bind(this)) // Handle events $module.addEventListener('click', this.handleClick.bind(this)) } +Radios.prototype.syncState = function () { + nodeListForEach(this.$inputs, function ($input) { + this.setAttributes($input) + }.bind(this)) +} + Radios.prototype.setAttributes = function ($input) { var $content = document.querySelector('#' + $input.getAttribute('aria-controls')) From aaff8f94aefbb796b34ee5645f0847bf4164d09e Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Fri, 19 Jun 2020 15:42:03 +0100 Subject: [PATCH 2/5] Document in CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d32032e1d7..49fd013d21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ We’ve made fixes to GOV.UK Frontend in the following pull requests: - [#1838: Correctly camel case SVG attributes in the header and footer](https://github.com/alphagov/govuk-frontend/pull/1838) +- [#1842: Preserve the state of conditional reveals when navigating 'back' in the browser](https://github.com/alphagov/govuk-frontend/pull/1842) ## 3.7.0 (Feature release) From 44ecae6007704170c96d3998580a5756dfb2e432 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Thu, 25 Jun 2020 11:16:52 +0100 Subject: [PATCH 3/5] Always sync on either pageshow or DOMContentLoaded Because of a bug in IE9-10 the readyState as being reported as interactive when we're checking it, which means that the DOMContentLoaded event handler isn't set up, and we try to sync the conditional reveals before the form control state has been restored. Instead: - sync on the pageshow event OR the DOMContentLoaded if the browser does not support the pageshow event - always sync within the init function (to handle cases where init is called after both the pageshow and DOMContentLoaded events have already fired) --- src/govuk/components/checkboxes/checkboxes.js | 21 ++++++++++--------- src/govuk/components/radios/radios.js | 21 ++++++++++--------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/govuk/components/checkboxes/checkboxes.js b/src/govuk/components/checkboxes/checkboxes.js index d9c7243fba..13f0df9c38 100644 --- a/src/govuk/components/checkboxes/checkboxes.js +++ b/src/govuk/components/checkboxes/checkboxes.js @@ -32,19 +32,20 @@ Checkboxes.prototype.init = function () { $input.removeAttribute('data-aria-controls') }) - if (document.readyState === 'loading') { - window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) + // When the page is restored after navigating 'back' in some browsers the + // state of form controls is not restored until *after* the DOMContentLoaded + // event is fired, so we need to sync after the pageshow event in browsers + // that support it. + if ('onpageshow' in window) { + window.addEventListener('pageshow', this.syncState.bind(this)) } else { - this.syncState() + window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) } - // When the page is restored after navigating 'back' in some browsers the - // state of form controls is not restored until *after* the DOMContentLoaded - // event is fired, so we need to sync after the pageshow event is fired as - // well. - // - // (Older browsers don't have a pageshow event, so we do both.) - window.addEventListener('pageshow', this.syncState.bind(this)) + // Although we've set up handlers to sync state on the pageshow or + // DOMContentLoaded event, init could be called after those events have fired, + // for example if they are added to the page dynamically, so sync now too. + this.syncState() // Handle events $module.addEventListener('click', this.handleClick.bind(this)) diff --git a/src/govuk/components/radios/radios.js b/src/govuk/components/radios/radios.js index 353b2d3c83..3fe98207c4 100644 --- a/src/govuk/components/radios/radios.js +++ b/src/govuk/components/radios/radios.js @@ -32,19 +32,20 @@ Radios.prototype.init = function () { $input.removeAttribute('data-aria-controls') }) - if (document.readyState === 'loading') { - window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) + // When the page is restored after navigating 'back' in some browsers the + // state of form controls is not restored until *after* the DOMContentLoaded + // event is fired, so we need to sync after the pageshow event in browsers + // that support it. + if ('onpageshow' in window) { + window.addEventListener('pageshow', this.syncState.bind(this)) } else { - this.syncState() + window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) } - // When the page is restored after navigating 'back' in some browsers the - // state of form controls is not restored until *after* the DOMContentLoaded - // event is fired, so we need to sync after the pageshow event is fired as - // well. - // - // (Older browsers don't have a pageshow event, so we do both.) - window.addEventListener('pageshow', this.syncState.bind(this)) + // Although we've set up handlers to sync state on the pageshow or + // DOMContentLoaded event, init could be called after those events have fired, + // for example if they are added to the page dynamically, so sync now too. + this.syncState() // Handle events $module.addEventListener('click', this.handleClick.bind(this)) From d29b20018e0e61f404596e700c4950fbfd2d9d48 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Thu, 25 Jun 2020 11:26:58 +0100 Subject: [PATCH 4/5] Clarify function names --- src/govuk/components/checkboxes/checkboxes.js | 14 +++++++------- src/govuk/components/radios/radios.js | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/govuk/components/checkboxes/checkboxes.js b/src/govuk/components/checkboxes/checkboxes.js index 13f0df9c38..7ccce7021b 100644 --- a/src/govuk/components/checkboxes/checkboxes.js +++ b/src/govuk/components/checkboxes/checkboxes.js @@ -37,27 +37,27 @@ Checkboxes.prototype.init = function () { // event is fired, so we need to sync after the pageshow event in browsers // that support it. if ('onpageshow' in window) { - window.addEventListener('pageshow', this.syncState.bind(this)) + window.addEventListener('pageshow', this.syncAll.bind(this)) } else { - window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) + window.addEventListener('DOMContentLoaded', this.syncAll.bind(this)) } // Although we've set up handlers to sync state on the pageshow or // DOMContentLoaded event, init could be called after those events have fired, // for example if they are added to the page dynamically, so sync now too. - this.syncState() + this.syncAll() // Handle events $module.addEventListener('click', this.handleClick.bind(this)) } -Checkboxes.prototype.syncState = function () { +Checkboxes.prototype.syncAll = function () { nodeListForEach(this.$inputs, function ($input) { - this.setAttributes($input) + this.syncWithInputState($input) }.bind(this)) } -Checkboxes.prototype.setAttributes = function ($input) { +Checkboxes.prototype.syncWithInputState = function ($input) { var inputIsChecked = $input.checked $input.setAttribute('aria-expanded', inputIsChecked) @@ -74,7 +74,7 @@ Checkboxes.prototype.handleClick = function (event) { var isCheckbox = $target.getAttribute('type') === 'checkbox' var hasAriaControls = $target.getAttribute('aria-controls') if (isCheckbox && hasAriaControls) { - this.setAttributes($target) + this.syncWithInputState($target) } } diff --git a/src/govuk/components/radios/radios.js b/src/govuk/components/radios/radios.js index 3fe98207c4..459af0e679 100644 --- a/src/govuk/components/radios/radios.js +++ b/src/govuk/components/radios/radios.js @@ -37,27 +37,27 @@ Radios.prototype.init = function () { // event is fired, so we need to sync after the pageshow event in browsers // that support it. if ('onpageshow' in window) { - window.addEventListener('pageshow', this.syncState.bind(this)) + window.addEventListener('pageshow', this.syncAll.bind(this)) } else { - window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) + window.addEventListener('DOMContentLoaded', this.syncAll.bind(this)) } // Although we've set up handlers to sync state on the pageshow or // DOMContentLoaded event, init could be called after those events have fired, // for example if they are added to the page dynamically, so sync now too. - this.syncState() + this.syncAll() // Handle events $module.addEventListener('click', this.handleClick.bind(this)) } -Radios.prototype.syncState = function () { +Radios.prototype.syncAll = function () { nodeListForEach(this.$inputs, function ($input) { - this.setAttributes($input) + this.syncWithInputState($input) }.bind(this)) } -Radios.prototype.setAttributes = function ($input) { +Radios.prototype.syncWithInputState = function ($input) { var $content = document.querySelector('#' + $input.getAttribute('aria-controls')) if ($content && $content.classList.contains('govuk-radios__conditional')) { @@ -87,7 +87,7 @@ Radios.prototype.handleClick = function (event) { // In radios, only radios with the same name will affect each other. var hasSameName = ($input.name === $clickedInput.name) if (hasSameName && hasSameFormOwner) { - this.setAttributes($input) + this.syncWithInputState($input) } }.bind(this)) } From 82c5bf010b920d3771a8f30007173f179a43edbc Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Thu, 25 Jun 2020 13:18:46 +0100 Subject: [PATCH 5/5] Avoid unneccesary function --- src/govuk/components/checkboxes/checkboxes.js | 4 +--- src/govuk/components/radios/radios.js | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/govuk/components/checkboxes/checkboxes.js b/src/govuk/components/checkboxes/checkboxes.js index 7ccce7021b..14c2950efd 100644 --- a/src/govuk/components/checkboxes/checkboxes.js +++ b/src/govuk/components/checkboxes/checkboxes.js @@ -52,9 +52,7 @@ Checkboxes.prototype.init = function () { } Checkboxes.prototype.syncAll = function () { - nodeListForEach(this.$inputs, function ($input) { - this.syncWithInputState($input) - }.bind(this)) + nodeListForEach(this.$inputs, this.syncWithInputState.bind(this)) } Checkboxes.prototype.syncWithInputState = function ($input) { diff --git a/src/govuk/components/radios/radios.js b/src/govuk/components/radios/radios.js index 459af0e679..c0f1652eb4 100644 --- a/src/govuk/components/radios/radios.js +++ b/src/govuk/components/radios/radios.js @@ -52,9 +52,7 @@ Radios.prototype.init = function () { } Radios.prototype.syncAll = function () { - nodeListForEach(this.$inputs, function ($input) { - this.syncWithInputState($input) - }.bind(this)) + nodeListForEach(this.$inputs, this.syncWithInputState.bind(this)) } Radios.prototype.syncWithInputState = function ($input) {