fix: improve cumulative layout shift

[millnut]

I've been investigating a CLS (Cumulative Layout Shift) in PageSpeed and ways to improve it; after some debugging I found it is coming from the alert banner module.

My understanding of how it renders from looking at the code is as follows;

1. A hidden class is added in localgov_alert_banner.module here
2. In the frontend, we find each alert banner, remove the hidden class, and then if it's in the user's cookie hide the alert here.

Due to this, the use of hidden in step 1 and removing the class in step 2 is redundant and instead causes a large CLS chained onto the elements below it where we are hiding by default and then unhiding all the alert banners and then hiding again, this PR resolves this and doesn't hide the alerts unless they are in the user's cookie.

I've attached the before and after below;

Before

After

cc: @markconroy to confirm my findings on this.

[andybroomfield]

Thanks for looking @millnut
Did you solve the issue mentioned in #125?
There is an issue with the internal page cache (anon users get the same html) that I seem to remember at the time was why it had to go the other way round. Also how will this effect moving the cookie to localstorage #271?

[millnut]

Thanks for looking @millnut Did you solve the issue mentioned in #125? There is an issue with the internal page cache (anon users get the same html) that I seem to remember at the time was why it had to go the other way round. Also how will this effect moving the cookie to localstorage #271?

Thanks for the context @andybroomfield those two issues are interesting, in #125 if we go serverside for display handling and read the cookie there and hide before it gets to the client side we can then remove a lot of the cookie checking handling from the JS side and will no longer need to look at switching to localstorage.

With #125 and how it is currently handled you still get a flash of banners due to it being hidden on the backend for all banners and then showing them all briefly before the frontend cookie checks kicks in.

I'll do some further investigation on shifting the rendering to serverside and see where I get and update on #125.

[millnut]

@andybroomfield I'll roll a separate PR for discussion on the backend handling, I've got it working locally so will push it up this evening once I've replaced the jQuery dependencies with pure javascript. We can then discuss cases I might have missed when implementing backend handling around caches.

[andybroomfield]

A couple of tests with this branch, first as logged in user (so non anon cache)

Alert.banner.show.by.deafault.mp4

And then logged out, with the internal page cache.

Alert.banner.show.by.default.anon.mp4

Seems to work ok on page refresh too. I notice we're just changing which way round we decide to show / hide the banner (show by deafult). This means that.

• Users who do hide the banner will now see brief flash of the hidden banner and layout shift.
• Those without javascript will now see the banners, instead of them being hidden.

[millnut]

Thanks @andybroomfield those tests were helpful could you do the same test for this WIP PR which uses the backend on checks #306 you'll likely have to add the {% if not alert_hidden %} condition to the localgov_base theme template override.

It's very work-in-progress that PR, but so far I've;

• Moved cookie check to backend
• Switch out jQuery document ready to Drupal behaviours

[millnut]

A couple of tests with this branch, first as logged in user (so non anon cache)
Alert.banner.show.by.deafault.mp4

And then logged out, with the internal page cache.
Alert.banner.show.by.default.anon.mp4

Seems to work ok on page refresh too. I notice we're just changing which way round we decide to show / hide the banner (show by deafult). This means that.

* Users who do hide the banner will now see brief flash of the hidden banner and layout shift.

* Those without javascript will now see the banners, instead of them being hidden.

Correct there are likely to be more anonymous users that visit a website whether they are real users or bots such as Google via search console and PageSpeed, so improving CLS will help with search ranking scores and other similar metrics around performance and page rendering, especially on the likes of homepages. So these are the cases;

• Users that do hide a banner will see a brief flash before it hides it. This probably already happens as the current implementation hides by a hidden class, then when it loops through all banners it removes all hidden classes and then does a hide; so this would still have a flash at some point dependent on the number of alerts shown on a single page. This is the main cause of the CLS issue which I would say is up for discussion as to what side we improve CLS for - people that hide alerts or people that leave them visible.

• Looking at the current implementation on https://demo.localgovdrupal.org/ if the user has no javascript it looks like the banners are all hidden, do we want banners to be hidden if the user does not have javascript? The body does have a no-js class so we could do a specific CSS rule to make them show with no JS. Sorry if this point has already been discussed in the past

[andybroomfield]

I'm discussing this with our digital content team internally to check what would work for them.

[finnlewis]

I know at Greenwich we definitely don't want a flash on page load.

@ekes is suggesting can we use bigpipe , and what about https://www.drupal.org/project/big_pipe_sessionless ? Would this help?

[markconroy]

Without basically repeating things others are saying, I don’t have much to add here.

I guess it gets down to:

• Are alert banners important? If so, they should be visible to those without JS
• Are we happy to show all alert banners if no JS? Probably yes
• If JS is available, do we hide all alert banners first and then loop through the cookies to see which ones we want to show? I think yes. In which case we flip the logic in the JS (if "not" in cookies list, show; rather than if "is" in cookies list , hide)
• • Does this cause CLS? Probably, so we're back to square one.

I guess then we have to ask ourselves what's more important, a small accessibility improvement or a small CLS improvement?

[andybroomfield]

From Brigthon Digital Content team: We would prefer to not have the flash on the banner after it is closed. They also mentioned that this could be an accessbility issue for triggering a flash on each page load.

So it looks like we should keep the current method for now, unless we can make this an option. We could also look at Big Pipe mentioned above or trying to work out a way of caching by cookie for the page cache to get #306 working.

[millnut]

Thanks @andybroomfield let's close this PR, I may work on some PageSpeed best practice documentation around CLS and add a note that the more alerts shown on a page the greater the CLS impact is