Hide Accordion content (again) during `.js-enabled` page load #3095

colinrotherham · 2022-12-12T13:48:17Z

This PR investigates an Accordion change to avoid a visual layout shift

During a slow page load (with js-enabled class on <body>) we now see Accordion sections jump from open to closed after component initialisation. We're not sure if this is preferred for users.

For example, shown when adding a 500ms delay before our code runs

<script src="/public/all.js"></script>
<script>
  // Add 500ms delay before our code runs
  // (For example, analytics might run first !!)
  setTimeout(function () {
    window.GOVUKFrontend.initAll()
  }, 500)
</script>

For discussion, I've included a quick tweak which keeps all the enhancements from:

Enhance the Accordion component with hidden='until-found' #3053

querkmachine · 2022-12-12T15:20:02Z

Still antsy about this, really, as although it reduces the layout shift on load, it's still shipping something that doesn't work properly if JavaScript fails. Doesn't feel very progressive enhancement of us.

As that's a bigger problem to fix across multiple components though (#999), and it being something we'll probably have to deal with soon when dropping JS support in IE, I'm happy to sit on my hands for now.

colinrotherham · 2022-12-12T15:41:24Z

@querkmachine Yeah, same thoughts here

If it helps, I'm thinking this PR solely serves to keep a consistent baseline for the next release

(So we've not made anything that could be considered worse for some users since the last release)

Similar to how we use font-display: fallback to prevent the "flash of unstyled content (FOUC)" we probably don't want a flash of unstyled Accordion either—but we should add a fallback soon!

owenatgov · 2022-12-13T11:17:14Z

I agree with @querkmachine that this isn't ideal. The risk of users not being able to see the content is, IMO, more damaging than them seeing a flash of content. We do do this elsewhere where we interact with js-enabled and sort of assume that the component will initialise so there's precedence for it but in those instances eg: the header. In that instance though it defaults the header menu to be open on mobile, meaning users can still access content.

After ticking this over I unfortunately don't have any quick alternatives. It is technically the same as v4.4.0 and it allows us to ship a neat enhancement, so whilst it's a problem it's not explicitly a regression. I'm gonna wait and see if anyone else has opinions but the options I see right now are:

Merge this and make a note on Components failing initialisation break due to .js-enabled styles #999 as additional evidence
Close this and revert Enhance the Accordion component with hidden='until-found' #3053

Im leading towards 1. If nobody has any other feelings I'll approve this in the afternoon.

I forgot to request a changelog entry

owenatgov

I'm not sure if this needs a changelog entry or not. I'm leaning towards "yes" unless somebody thinks otherwise.

colinrotherham · 2022-12-13T15:11:59Z

Thanks @owenatgov

Knowing that this could go either way we did a minor padding fix in this one:

Fix Accordion margin/padding inconsistencies #3094

(So should we keep the flicker, at least it's a beautiful flicker 😊)

Had a quick run through Tabs and there's another interesting middle ground. With slow-loading JS you can still click a tab and it'll (eventually) catch up via window.location.hash

Will be a shame to merge this PR but gives us more time to fully consider:

JavaScript off
JavaScript on but crashed
JavaScript on but skipped checks
JavaScript on but took too long

Components failing initialisation break due to .js-enabled styles #999

owenatgov · 2022-12-13T15:15:03Z

@colinrotherham That PR you've linked needs a changelog entry as well!

I initially wondered if this should have a change entry on the basis that this was technically a revision of an existing PR, however I'm not quite confident that it does.

colinrotherham · 2022-12-13T15:25:26Z

@colinrotherham That PR you've linked needs a changelog entry as well!

@owenatgov Ahh the padding fix will soon be hidden, can't do a change entry if the fix never gets rendered 😉

I'll do a quick one though for the mobile height, button margin and (maybe) trailing attribute space?

querkmachine · 2022-12-13T15:48:06Z

IMO this PR doesn't need a separate changelog entry as it's basically a continuation of the hidden="until-found" PR. Could reference both PRs against the same feature, I suppose?

colinrotherham · 2022-12-13T16:00:12Z

Some other related changes from the archive:

Fix flash of unstyled content with conditional reveals #885

colinrotherham · 2022-12-13T16:46:03Z

@owenatgov @querkmachine @claireashworth Can you check these two new commits for me please?

Or see the full CHANGELOG.md on this branch

owenatgov

A few comments on the changelog, mostly making the content leaner in line with how we generally write up changes. I'll let @claireashworth come in with further thoughts on the content, although it might make sense to instead do this as part of the complete 4.4.1 changelog review.

owenatgov · 2022-12-14T17:31:58Z

CHANGELOG.md

+
+Note: To improve the reading experience on slow loading pages we initially removed `display: none` from each Accordion section. To avoid a flicker of content, we've chosen to restore the current behaviour in [pull request #3095](https://github.com/alphagov/govuk-frontend/pull/3053).
+
+We’d like to take another look at how components load in: [issue #999: Components failing initialisation break due to `.js-enabled` styles](https://github.com/alphagov/govuk-frontend/issues/999).


I don't think this line is necessary. I think you're being considerate in including it, however we do reference this issue in this PR and users can come here to see the discourse if they are interested in the issue.

owenatgov · 2022-12-14T17:33:39Z

CHANGELOG.md

+- [#3053: Enhance the Accordion component with `hidden='until-found'`](https://github.com/alphagov/govuk-frontend/pull/3053)
+- [#3095: Hide Accordion content (again) during `.js-enabled` page load](https://github.com/alphagov/govuk-frontend/pull/3053)
+
+Note: To improve the reading experience on slow loading pages we initially removed `display: none` from each Accordion section. To avoid a flicker of content, we've chosen to restore the current behaviour in [pull request #3095](https://github.com/alphagov/govuk-frontend/pull/3053).


I don't think this line is necessary. You already reference this PR on line 16 and I think providing the link is enough.

owenatgov · 2022-12-14T17:34:34Z

CHANGELOG.md

+This was added in pull requests:
+
+- [#3053: Enhance the Accordion component with `hidden='until-found'`](https://github.com/alphagov/govuk-frontend/pull/3053)
+- [#3095: Hide Accordion content (again) during `.js-enabled` page load](https://github.com/alphagov/govuk-frontend/pull/3053)


Wonky link

Suggested change

- [#3095: Hide Accordion content (again) during `.js-enabled` page load](https://github.com/alphagov/govuk-frontend/pull/3053)

- [#3095: Hide Accordion content (again) during `.js-enabled` page load](https://github.com/alphagov/govuk-frontend/pull/3095)

owenatgov · 2022-12-15T11:58:52Z

We're pausing this PR in favour of getting a fix release out in order to get a critical chrome bug released and give ourselves a bit more time to discuss the tradeoff's of this PR and it's interaction with #3053.

When loading the Accordion component with JavaScript enabled, this change restores `display: none` on section content For context, we switched to `display: inherit` previously: #3053 But for slow-loading pages we saw a visual layout shift as each section flickered closed (from open) after initialisation We’d like to take another look at how components load in: #999

36degrees · 2023-01-10T16:52:33Z

We're pausing this PR in favour of getting a fix release out in order to get a critical chrome bug released and give ourselves a bit more time to discuss the tradeoff's of this PR and it's interaction with #3053.

We discussed this team today and agreed that we want to take a proper look at #999, but in the meantime we're happy to merge this as it basically maintains the 'status quo'.

owenatgov

Does the job 👍🏻

colinrotherham added 🔍 investigation sass / css accordion labels Dec 12, 2022

colinrotherham requested a review from querkmachine December 12, 2022 13:48

colinrotherham requested a review from owenatgov December 12, 2022 14:08

Base automatically changed from accordion-various-fixes to main December 12, 2022 15:48

colinrotherham marked this pull request as ready for review December 13, 2022 09:37

colinrotherham removed the 🔍 investigation label Dec 13, 2022

owenatgov previously approved these changes Dec 13, 2022

View reviewed changes

owenatgov reviewed Dec 13, 2022

View reviewed changes

colinrotherham changed the title ~~Hide accordion content during js-enabled page load~~ Hide Accordion content (again) during .js-enabled page load Dec 13, 2022

colinrotherham mentioned this pull request Dec 13, 2022

Components failing initialisation break due to .js-enabled styles #999

Open

colinrotherham force-pushed the accordion-default-visibility branch from 60f87a2 to 6d08bd2 Compare December 13, 2022 16:35

kellylee-gds added the Frontend squad label Dec 13, 2022

owenatgov reviewed Dec 14, 2022

View reviewed changes

36degrees added this to the [NEXT] milestone Dec 15, 2022

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3095 December 15, 2022 11:06 Inactive

colinrotherham force-pushed the accordion-default-visibility branch from 6d08bd2 to 9a93e5e Compare December 19, 2022 21:46

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3095 December 19, 2022 21:47 Inactive

colinrotherham force-pushed the accordion-default-visibility branch from 9a93e5e to 0b011a5 Compare December 22, 2022 17:22

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3095 December 22, 2022 17:22 Inactive

colinrotherham added 3 commits January 9, 2023 16:10

Add CHANGELOG note about accordion section visibility during load

340b4a9

Add CHANGELOG entry for accordion margin/padding fixes

ecca61d

colinrotherham force-pushed the accordion-default-visibility branch from 0b011a5 to ecca61d Compare January 9, 2023 16:11

colinrotherham requested a review from owenatgov January 11, 2023 15:06

owenatgov approved these changes Jan 13, 2023

View reviewed changes

colinrotherham merged commit 6280b88 into main Jan 16, 2023

colinrotherham deleted the accordion-default-visibility branch January 16, 2023 13:03

owenatgov mentioned this pull request Jan 31, 2023

Release v4.5.0 #3228

Merged

beclamide mentioned this pull request Aug 19, 2024

[Snyk] Upgrade govuk-frontend from 4.3.1 to 4.8.0 DEFRA/ffc-mpdp-frontend#157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide Accordion content (again) during `.js-enabled` page load #3095

Hide Accordion content (again) during `.js-enabled` page load #3095

colinrotherham commented Dec 12, 2022 •

edited

Loading

querkmachine commented Dec 12, 2022 •

edited

Loading

colinrotherham commented Dec 12, 2022

owenatgov commented Dec 13, 2022

owenatgov left a comment

colinrotherham commented Dec 13, 2022

owenatgov commented Dec 13, 2022

colinrotherham commented Dec 13, 2022

querkmachine commented Dec 13, 2022

colinrotherham commented Dec 13, 2022

colinrotherham commented Dec 13, 2022

owenatgov left a comment

owenatgov Dec 14, 2022

owenatgov Dec 14, 2022

owenatgov Dec 14, 2022

colinrotherham Dec 14, 2022

owenatgov commented Dec 15, 2022

36degrees commented Jan 10, 2023

owenatgov left a comment


		Note: To improve the reading experience on slow loading pages we initially removed `display: none` from each Accordion section. To avoid a flicker of content, we've chosen to restore the current behaviour in [pull request #3095](https://github.com/alphagov/govuk-frontend/pull/3053).

		We’d like to take another look at how components load in: [issue #999: Components failing initialisation break due to `.js-enabled` styles](https://github.com/alphagov/govuk-frontend/issues/999).

	- [#3095: Hide Accordion content (again) during `.js-enabled` page load](https://github.com/alphagov/govuk-frontend/pull/3053)
	- [#3095: Hide Accordion content (again) during `.js-enabled` page load](https://github.com/alphagov/govuk-frontend/pull/3095)

Hide Accordion content (again) during .js-enabled page load #3095

Hide Accordion content (again) during .js-enabled page load #3095

Conversation

colinrotherham commented Dec 12, 2022 • edited Loading

querkmachine commented Dec 12, 2022 • edited Loading

colinrotherham commented Dec 12, 2022

owenatgov commented Dec 13, 2022

owenatgov left a comment

Choose a reason for hiding this comment

colinrotherham commented Dec 13, 2022

owenatgov commented Dec 13, 2022

colinrotherham commented Dec 13, 2022

querkmachine commented Dec 13, 2022

colinrotherham commented Dec 13, 2022

colinrotherham commented Dec 13, 2022

owenatgov left a comment

Choose a reason for hiding this comment

owenatgov Dec 14, 2022

Choose a reason for hiding this comment

owenatgov Dec 14, 2022

Choose a reason for hiding this comment

owenatgov Dec 14, 2022

Choose a reason for hiding this comment

colinrotherham Dec 14, 2022

Choose a reason for hiding this comment

owenatgov commented Dec 15, 2022

36degrees commented Jan 10, 2023

owenatgov left a comment

Choose a reason for hiding this comment

Hide Accordion content (again) during `.js-enabled` page load #3095

Hide Accordion content (again) during `.js-enabled` page load #3095

colinrotherham commented Dec 12, 2022 •

edited

Loading

querkmachine commented Dec 12, 2022 •

edited

Loading