Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cookie banner issue (IE10) #2231

Merged
merged 1 commit into from
Aug 11, 2021
Merged

Fix cookie banner issue (IE10) #2231

merged 1 commit into from
Aug 11, 2021

Conversation

danacotoran
Copy link
Contributor

@danacotoran danacotoran commented Jul 26, 2021

It seems that IE10 does not support the hidden attribute.

This is causing some issues with the cookie banner component: when the confirmation banner is visible (upon rejecting/accepting cookies), the cookie choice panel should disappear, which does not happen in <IE10.

This attempts to fix the issue by using CSS to hide elements accordingly when the hidden attribute is present (as opposed to toggling display with JS).

Additionally, switch from using property dot notation to set hidden to true/false, to using setAttribute because although the former worked in other major browsers, IE10 seemed to ignore it (perhaps due to the distinction between properties and attributes).


More details and a lovely visual of the bug in action on the original issue.

@bevanloon bevanloon temporarily deployed to govuk-publis-cookie-ban-sjznpa July 26, 2021 17:31 Inactive
@@ -101,6 +101,7 @@ window.GOVUK.Modules = window.GOVUK.Modules || {};
CookieBanner.prototype.showConfirmationMessage = function () {
this.$cookieBannerMainContent = document.querySelector('.js-banner-wrapper')

this.$cookieBannerMainContent.style.display = 'none'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than doing this in the JavaScript, would adding:

[hidden] {
  display: none;
}

to the CSS work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should all already be handled by this code in GOV.UK Frontend:

https://github.com/alphagov/govuk-frontend/blob/72e9d6e43cffa7db79477343a2cf931812f9b7c7/src/govuk/components/cookie-banner/_index.scss#L20-L24

I think the issue is that you're showing and hiding the cookie banner by setting display: block rather than by toggling the hidden attribute. The display: block; inline styles is then overriding the CSS.

If you consistently use the hidden attribute then this should all 'just work'.

There's a 'test' client-side implementation here if that helps (it's a lot simpler as it doesn't actually read or store cookies):

https://github.com/alphagov/govuk-frontend/blob/72e9d6e43cffa7db79477343a2cf931812f9b7c7/app/views/full-page-examples/cookie-banner-client-side/index.njk#L184-L223

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to caniuse hidden isn't supported by IE10? https://caniuse.com/hidden

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE10 and older have no native support for the attribute, but the [hidden] attribute selector works just fine in CSS, which means we can make it work with just:

.govuk-cookie-banner[hidden] {
  display: none;
}

The implementation in GOV.UK Frontend was tested all the way back to IE8 as part of alphagov/govuk-frontend#2126.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks all, sounds like this needs some reworking!

@bevanloon bevanloon temporarily deployed to govuk-publis-cookie-ban-sjznpa July 28, 2021 17:08 Inactive
@danacotoran danacotoran marked this pull request as draft July 28, 2021 17:08
@danacotoran danacotoran marked this pull request as ready for review July 29, 2021 13:01
Copy link
Contributor

@injms injms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely - thanks for fixing this.

(Just needs a changelog entry.)

It seems that IE10 does not support the hidden attribute. This is
causing some issues with the cookie banner script: when the confirmation
banner is visible (upon rejecting/accepting cookies), the cookie choice
panel should disappear, which does not happen in IE10.
This fixes the issue by using CSS to hide elements accordingly when the
hidden attribute is present (as opposed to toggling display with JS).
Additionally, switch from using property dot notation to set hidden to
true/false, to using setAttribute because even though the former worked
in all major modern browsers, IE10 seemed to ignore it. This is perhaps
due to the distinction between properties and attributes.
@danacotoran danacotoran merged commit 044843e into master Aug 11, 2021
@danacotoran danacotoran deleted the cookie-banner-ie-10-bug branch August 11, 2021 13:40
injms added a commit that referenced this pull request Aug 12, 2021
Includes:

* Extend track click script (#2263)
* Fix cookie banner issue (IE10) (#2231)
* Extend layout for public with account components (#2255)
* Update search toggle tracking (#2262)
@injms injms mentioned this pull request Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants