Preserve the state of conditional reveals when navigating 'back' in the browser #1842

36degrees · 2020-06-19T14:39:49Z

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') 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 do 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'. 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! This was fixed in Chromium 80.

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 (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.

Browser testing (against `82c5bf0`)

Fixes #1794
Fixes #1552

hannalaakso

This fixes the bug in Chrome 83 already (didn't get a chance to test others yet) which is good. Testing on Chrome 79 (on Browserstack) I'm still seeing the bug persisting.

Steps to replicate in Chrome 79:

Go to /components/checkboxes and then /components/checkboxes/with-conditional-items/preview
Tick one of the checkboxes, fill in the conditional input.
Press back button and then forward again. The item is checked but not expanded.

hannalaakso · 2020-06-22T18:23:36Z

src/govuk/components/checkboxes/checkboxes.js

-  }.bind(this))
+  })
+
+  if (document.readyState === 'complete') {


Should this be

document.addEventListener('readystatechange', this.syncState.bind(this))

Checkboxes.prototype.syncState = function () { if (document.readyState === 'complete') {

because at the moment it's only evaluated once (when it's loading) so complete is never hit. But I might be misunderstanding the reason for this check.

The intention of this logic is to handle cases where the script is being evaluated after the DOMContentLoaded event has already fired (in which case document.readyState would be complete).

This might be unnecessary – I am not sure if the way the script is included means this is guaranteed to run before the DOMContentLoadedevent fires. However, given this code is probably going to end up being included in ways we can't anticipate (e.g. as part of a React component) I'm thinking it might be safer to try and cater for these scenarios?

Having done a bit more reading, I think that I possibly need to flip this in order to handle readyState being interactive, like in this example in the MDN docs:

if (document.readyState === 'loading') { window.addEventListener('DOMContentLoaded', this.syncState.bind(this)) } else { this.syncState() }

Thanks for digging into this. It seems that older IE only support complete for readyState so according to that loading wouldn't ever be true for those browsers.

That page suggests that IE9-10 can fire the 'interactive' state too early, but also that they only support 'complete' – how can both of those be true?

It also associates the bug about Internet Explorer 9 and 10 with the IE11 block?

MDN seems to suggest that it's only IE8 that only supports complete: https://developer.mozilla.org/en-US/docs/Web/API/Document/readyState

Raised an issue about the misleading notes – Fyrd/caniuse#5489

So because of the bug in IE9-10 the readyState as being reported as interactive too early, which means that the state is not being restored correctly in those browsers.

I don't think it's safe to just wait for the DOMContentLoaded because we can't guarantee it hasn't already fired – an example might be if a user was initialising the checkboxes on a page that had already loaded, e.g. if they were added dynamically.

We could consider syncing within init, on DOMContentLoaded and on the pageshow event, but wondering if it's worth it given that we're only aiming for 'functional' support of IE9 and IE10 and this is something of an edge case?

At the very least, I'll need to revise the commit message and PR description as this it currently implies that it fixes IE8-11, which is not the case.

Actually, maybe the correct approach is to:

always sync within the init function

sync on the pageshow event OR the DOMContentLoaded if the browser does not support the pageshow event

Trying that now.

This sounds like a good solution, 🙌 for the investigation and write up @36degrees.

36degrees · 2020-06-23T08:11:43Z

Testing on Chrome 79 (on Browserstack) I'm still seeing the bug persisting.

Ah, sorry, I didn't make this clear at all! There's a bug in Chrome 79 where the form control state is not guaranteed to have been restored before the pageshow event fires – see https://bugs.chromium.org/p/chromium/issues/detail?id=1035662. It was fixed in Chrome 80.

So:

In Chrome 78 and below, the form control state is restored before the script is evaluated
In Chrome 79, the form control state is restored some time after the script is evaluated, and (potentially?) after the pageshow event fires.
In Chrome 80, a fix is introduced which means that the form control state is restored after the script is evaluated but before the pageshow event fires.

Based on the comments in the bug report, I don't think there's a reliable event we can listen for to fix the behaviour in Chrome 79. My current thinking is that we'd accept that it's broken in Chrome 79, given that Chrome 79's usage is already trending towards 0 (0.29% in May).

I'll update the commit message to make this clearer.

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

36degrees · 2020-06-23T09:28:46Z

I'll update the commit message to make this clearer.

Commit and PR description updated.

36degrees · 2020-06-23T09:32:48Z

Go to /components/checkboxes and then /components/checkboxes/with-conditional-items/preview

Tick one of the checkboxes, fill in the conditional input.

Press back button and then forward again. The item is checked but not expanded.

I've been using the feedback form ('Do you want a reply?') and what is your nationality ('Citizen of a different country') to test radios and checkboxes respectively, as you can submit the form and then hit back 👍

NickColley · 2020-06-24T14:19:06Z

src/govuk/components/checkboxes/checkboxes.js


  // Handle events
  $module.addEventListener('click', this.handleClick.bind(this))
 }

+Checkboxes.prototype.syncState = function () {
+  nodeListForEach(this.$inputs, function ($input) {


I can't unsubscribe from all notifications so sometimes I see interesting things, excellent write up, super interesting...

You might be able to simplify this to something like:

nodeListForEach(this.$inputs, this.setAttributes($input).bind(this))

Maybe not though, see ya later

🏃‍♂️ 💨

Thanks, @NickColley! 👋

I think it'd have to be:

nodeListForEach(this.$inputs, this.setAttributes.bind(this))

which is possibly a little less clear – any thoughts @hannalaakso?

alex-ju

Sorry for my delayed review of this. I wanted to have the time to test it properly via //discards. I'm unable to provide any suggestions for improvements. I think it's the best we can offer atm.

alex-ju · 2020-06-25T09:19:57Z

src/govuk/components/checkboxes/checkboxes.js

+  if (document.readyState === 'loading') {
+    window.addEventListener('DOMContentLoaded', this.syncState.bind(this))
+  } else {
+    this.syncState()
+  }


I like how these cover asynchronous loading too 👏

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)

36degrees · 2020-06-26T12:42:29Z

I think this is good to go, pending a review from @hannalaakso 🙏

hannalaakso

Good commit messages and write up 👍 thanks @36degrees 🕵️

36degrees requested a review from hannalaakso June 19, 2020 14:39

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1842 June 19, 2020 14:40 Inactive

36degrees changed the title ~~Preserve reveal states when navigating 'back'~~ Preserve the state of conditional reveals states when navigating 'back' in Internet Explorer, Chrome 79+, Edge 79+, Opera 66+ Jun 19, 2020

36degrees changed the title ~~Preserve the state of conditional reveals states when navigating 'back' in Internet Explorer, Chrome 79+, Edge 79+, Opera 66+~~ Preserve the state of conditional reveals when navigating 'back' in Internet Explorer, Chrome 79+, Edge 79+, Opera 66+ Jun 19, 2020

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1842 June 19, 2020 14:42 Inactive

hannalaakso reviewed Jun 22, 2020

View reviewed changes

36degrees force-pushed the conditional-reveals-back-button branch from e057475 to 152271c Compare June 23, 2020 09:27

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1842 June 23, 2020 09:27 Inactive

36degrees changed the title ~~Preserve the state of conditional reveals when navigating 'back' in Internet Explorer, Chrome 79+, Edge 79+, Opera 66+~~ Preserve the state of conditional reveals when navigating 'back' in all browsers (except Chrome 79, Edge 79 and Opera 66) Jun 23, 2020

36degrees changed the title ~~Preserve the state of conditional reveals when navigating 'back' in all browsers (except Chrome 79, Edge 79 and Opera 66)~~ Preserve the state of conditional reveals when navigating 'back' in the browser Jun 23, 2020

Document in CHANGELOG

aaff8f9

36degrees force-pushed the conditional-reveals-back-button branch from 152271c to aaff8f9 Compare June 23, 2020 10:55

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1842 June 23, 2020 10:55 Inactive

36degrees requested a review from alex-ju June 24, 2020 08:41

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1842 June 24, 2020 08:59 Inactive

36degrees force-pushed the conditional-reveals-back-button branch from 539a82c to aaff8f9 Compare June 24, 2020 08:59

36degrees mentioned this pull request Jun 24, 2020

Checked radio button with conditional reveal does not expand on page refresh in IE11 #1552

Closed

NickColley reviewed Jun 24, 2020

View reviewed changes

alex-ju approved these changes Jun 25, 2020

View reviewed changes

36degrees added 2 commits June 25, 2020 11:16

Clarify function names

d29b200

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1842 June 25, 2020 10:42 Inactive

Avoid unneccesary function

82c5bf0

govuk-design-system-ci deployed to govuk-frontend-review-pr-1842 June 25, 2020 12:19 View deployment

36degrees requested a review from hannalaakso June 26, 2020 12:42

This was referenced Jun 26, 2020

Improve documentation and consistency in radios and checkboxes components #1847

Merged

Character count can show incorrect count when going 'back', or if page is discarded and restored #1848

Closed

hannalaakso approved these changes Jun 30, 2020

View reviewed changes

36degrees merged commit ffc30f4 into master Jun 30, 2020

36degrees deleted the conditional-reveals-back-button branch June 30, 2020 10:40

vanitabarrett mentioned this pull request Jul 16, 2020

Preserve the state of character count when navigating 'back' #1868

Merged

14 tasks

vanitabarrett mentioned this pull request Jul 29, 2020

Release v3.8.0 #1886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve the state of conditional reveals when navigating 'back' in the browser #1842

Preserve the state of conditional reveals when navigating 'back' in the browser #1842

36degrees commented Jun 19, 2020 •

edited

Loading

hannalaakso left a comment

hannalaakso Jun 22, 2020

36degrees Jun 23, 2020

hannalaakso Jun 23, 2020

36degrees Jun 23, 2020

36degrees Jun 24, 2020

36degrees Jun 25, 2020

36degrees Jun 25, 2020

36degrees Jun 25, 2020

hannalaakso Jun 30, 2020

36degrees commented Jun 23, 2020 •

edited

Loading

36degrees commented Jun 23, 2020

36degrees commented Jun 23, 2020

NickColley Jun 24, 2020 •

edited

Loading

36degrees Jun 24, 2020

36degrees Jun 24, 2020

alex-ju left a comment

alex-ju Jun 25, 2020

36degrees commented Jun 26, 2020

hannalaakso left a comment

Preserve the state of conditional reveals when navigating 'back' in the browser #1842

Preserve the state of conditional reveals when navigating 'back' in the browser #1842

Conversation

36degrees commented Jun 19, 2020 • edited Loading

Firefox / Safari

Internet Explorer

Edge Legacy

Chromium based browsers (Chrome, Edge, Opera)

Browser testing (against 82c5bf0)

hannalaakso 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

36degrees commented Jun 23, 2020 • edited Loading

36degrees commented Jun 23, 2020

36degrees commented Jun 23, 2020

NickColley Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

36degrees commented Jun 26, 2020

hannalaakso left a comment

Choose a reason for hiding this comment

36degrees commented Jun 19, 2020 •

edited

Loading

Browser testing (against `82c5bf0`)

36degrees commented Jun 23, 2020 •

edited

Loading

NickColley Jun 24, 2020 •

edited

Loading