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

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

Merged
merged 3 commits into from
Dec 12, 2022

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Nov 29, 2022

until-found is a new value for the hidden attribute, that allows a user agent's find-in-page functionality to search the text inside of elements that are currently collapsed. It serves as a progressive enhancement to the existing hidden boolean attribute, falling back to the default behaviour in browsers that don't support the new value.

It was introduced at the same time as the beforematch JavaScript event, allowing us to see which element on the page the "found" text is contained within.

Currently the new attribute value and JavaScript event are only supported in Chromium-based browsers (including Google Chrome, Microsoft Edge, Opera, and Samsung Internet).

Linked to #2697. I previously did a small, hackier investigation into this at the start of August. See my previous comment for the outcome of that.

Changes in this spike

  • Replaces the hiding and showing of accordion sections via CSS, to instead conditionally add and remove the hidden="until-found" attribute using JavaScript.
    • This change means that the point that accordion content is hidden is slightly different. Formerly it was hidden (effectively) immediately, based on the presence of the js-enabled class. It now requires both the class and for the accordion's JavaScript to have been initialised before anything is hidden.
    • As a benefit, this is a little more resilient as it means the accordion's content is visible even when the accordion hasn't been initialised or errored whilst doing so, which wasn't the case previously.
  • Refactors the section content's padding to only apply when the section is expanded.
    • This is because an element with hidden="until-found" on it is not removed from the box model (it uses content-visibility: hidden, not display: none unlike boolean hidden) so the padding was always visible.
  • Adds a beforematch event handler and function to correctly change the state, styles and button labels in the event of a match by the user agent.
    • Without this, the user agent would automatically remove the hidden attribute upon a match, but would not change any other parts of the accordion.

Other thoughts

This would seem to be a non-breaking change in terms of service teams, unless a team has customised the styles of the section content box. As this element is no longer completely ignored when rendering the box model, any styles (such as borders, background colours, margins or padding) may become visible even when the section is collapsed.

This probably poses a larger issue for legacy Internet Explorer support. This is currently relying on user agent stylesheets to be clever and apply either content-visibility: hidden or display: none depending on the value (or lack) of the hidden attribute. IE 10 and below don't support the hidden attribute at all, so a display: none rule would need to be applied for those browsers without compromising those that support hidden="until-found". Resolved, see discussion below.

This functionality would seemingly want to be mirrored to the Details component, however we are relying solely on the user agent's own functionality to provide the expand/collapse feature in modern browsers.

@querkmachine querkmachine self-assigned this Nov 29, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3053 November 29, 2022 16:37 Inactive
@colinrotherham
Copy link
Contributor

Ah this is great, ties in nicely with the html-validate (issue details) discussion on:

Where HTML validation for hidden="until-found" was brought up too

@36degrees
Copy link
Contributor

IE 10 and below don't support the hidden attribute at all, so a display: none rule would need to be applied for those browsers without compromising those that support hidden="until-found".

I think we have prior art for this in the header component, and could do something similar here too?

&[hidden],
.js-enabled &[hidden] {
display: none;
}

@querkmachine
Copy link
Member Author

querkmachine commented Nov 30, 2022

Unfortunately, seemingly not.

Applying [hidden] { display: none } overrides the UA's content-visibility: hidden;, breaking the until-found functionality in supporting browsers.

Trying something fancy with negation (like [hidden]:not([hidden="until-found"]), or having two separate [hidden] and [hidden="until-found"] rules) also doesn't work, as IE still sees the until-found value, even if it doesn't understand it.

I think the solution here is to feature detect whether the browser supports until-found in JS and to apply different attribute values depending on whether it's supported or not. Going to experiment with that today.

@36degrees
Copy link
Contributor

Sounds sensible – from reading MDN it sounds like hidden="until-found" uses content-visibility: hidden so we might also be able to use @supports?

Something like

[hidden] {
  display :none;

  @supports (content-visibility: hidden) {
    content-visibility: hidden;  /* may not even be needed, as all browsers that support content-visibility: hidden probably have default UA styles? */
    display: inherit;
  }
}

We could also wait until 5.x when we no longer need to care about IE10!

@querkmachine
Copy link
Member Author

@supports seems to do it!

It even words in "tweener" browsers—i.e. versions of Chrome released between content-visibility being added in version 85 and hidden="until-found" being added in 102—without any obvious side-effects.

Previously, browsers that did not have a user agent style for the `hidden` attribute (such as IE 10 and below) would always show
section content, regardless of whether the section was expanded or collapsed.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3053 November 30, 2022 11:31 Inactive
@querkmachine querkmachine changed the title [SPIKE] Enhancing the Accordion component with [hidden='until-found'] Enhance the Accordion component with hidden='until-found' Nov 30, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3053 November 30, 2022 14:27 Inactive
@36degrees
Copy link
Contributor

Really nice improvement – It'd be great to get this shipped 🙌🏻

Given this uses content-visibility: hidden we should just double check what impact (if any) this has on the accessibility tree / assistive tech. A quick Google suggests there might be some gotchas to watch out for:

So it might be worth comparing the behaviour with and without this change to check if anything's changed?

This functionality would seemingly want to be mirrored to the Details component, however we are relying solely on the user agent's own functionality to provide the expand/collapse feature in modern browsers.

In my testing, the details element already takes care of this for us – if you search for terms hidden inside a details element, it'll expand automatically to reveal the content. Because our styling hooks on to the open attribute everything else just seems to work.

@querkmachine
Copy link
Member Author

@36degrees My earlier poke at this seemed to show that the accessibility tree was rendering and being updated as expected, though admittedly that was only checked in latest Chrome. The issues described in those articles were (seemingly) fixed in Chrome 90.

@querkmachine querkmachine marked this pull request as ready for review December 7, 2022 10:24
@owenatgov
Copy link
Contributor

owenatgov commented Dec 7, 2022

A general comment to say that once this is ready for review (which I think it is now?) I think this is good to go from my perspective I was late! I'll give this another review plus QA ASAP.

I'll do one final pass against browsers to be belt and bracers once this is moved out of draft but based on the chat in the PR so far I'm feeling positive.

@owenatgov
Copy link
Contributor

Percy is being a bit odd. On edge it shows the accordions as default open. I'm unsure what could be causing this? Maybe some sort of version mish mash. I personally don't think this is a blocker for live but if we're able to figure out why this is happening that'd be neat.

@querkmachine
Copy link
Member Author

@owenatgov I think what I wrote in the initial content-y bit was my best guess as to why Percy is returning different:

This change means that the point that accordion content is hidden is slightly different. Formerly it was hidden (effectively) immediately, based on the presence of the js-enabled class. It now requires both the class and for the accordion's JavaScript to have been initialised before anything is hidden.

Is Percy taking its screenshots before the bulk of Frontend's JS has kicked in? In the past, the accordion will already have been collapsed at this point (via CSS and JS inlined in the template), but now it won't until the accordion has been initialised.

@owenatgov
Copy link
Contributor

@querkmachine That'll be it. I don't think this is a showstopper but it probably is something for us to consider if we want to continue using percy and run into other cases where we move away from relying on js-enabled.

Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

Code looking good.

Working across browsers.

Accessibility wise this seems to be operating as expected and from a bit of research, Chrome 90 is on the extreme low end of use.

Cracking work 💪🏻

Something worth recording is that in IE9 the accordion looks a little odd with a big border at the bottom. See screenshot below. This was already there however so it's not a regression and it's not stopping users from reaching the content. It may even be captured somewhere else.

Screenshot 2022-12-07 at 14 20 07

@colinrotherham
Copy link
Contributor

Is Percy taking its screenshots before the bulk of Frontend's JS has kicked in? In the past, the accordion will already have been collapsed at this point (via CSS and JS inlined in the template), but now it won't until the accordion has been initialised.

@querkmachine It's been fab reading through this PR 🎉

Percy wise, we've just got the 100ms default networkIdleTimeout I believe

Can try dropping in a .percy.js config into the project root, seeing if a longer timeout helps settle things down? Does this mean users will see the same flicker through as sections close after init?

@querkmachine
Copy link
Member Author

querkmachine commented Dec 7, 2022

@colinrotherham

Does this mean users will see the same flicker through as sections close after init?

Yes, but I personally think this isn't a problem and is actually an improvement.

Previously, if the stylesheet loaded but the network request for all.js (or equivalent) failed, the accordion's content would be hidden and inaccessible. You'd only have the section headings visible, and clicking on them doesn't do anything. You can emulate this by blocking the all.js request in browser devtools in the review app.

Screenshot 2022-12-07 at 14 43 54

Now, the accordion's content remains visible up until the component JS has successfully downloaded and initialised.

@colinrotherham
Copy link
Contributor

Ah I see the benefit, I just know Google unfavourably ranks websites with high "cumulative layout shift" as users prefer a stable layout when JavaScript kicks in. Having unclickable section headings is still a valid point though

colinrotherham added a commit that referenced this pull request Dec 13, 2022
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
colinrotherham added a commit that referenced this pull request Dec 19, 2022
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
colinrotherham added a commit that referenced this pull request Dec 22, 2022
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
colinrotherham added a commit that referenced this pull request Jan 9, 2023
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
@owenatgov owenatgov mentioned this pull request Jan 31, 2023
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.

[Spike] Investigate updating the accordion to use the hidden="until-found" attribute
5 participants