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.

Sign up for GitHub

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

Jump to bottom

Review accordion bug on mobile with summary focus state #2322

Closed
2 tasks
Tracked by #1706
hannalaakso opened this issue Aug 18, 2021 · 5 comments
Closed
2 tasks
Tracked by #1706

Review accordion bug on mobile with summary focus state #2322

hannalaakso opened this issue Aug 18, 2021 · 5 comments
Assignees
@hannalaakso @vanitabarrett
Labels
accordion

Comments

@hannalaakso
Copy link
Member

hannalaakso commented Aug 18, 2021 •
edited by Izabela-16
Loading

What

There is an issue with the focus state on mobile when there are multiple lines in the summary, where the focus state underline is rendered as a thin line

Screenshot 2021-08-19 at 18 36 39

This impacts Android and Samsung Internet as iOS doesn't show the focus style (but the issue can be replicated in desktop Chrome emulator too, for ease of debugging).

Why

We should align the focus state with the GOV.UK style, at the moment it looks a bit broken.

Who needs to know about this

Developers, designer

Done when

  • Focus state correctly rendered on Android
  • Focus state correctly rendered on Samsung Internet
@hannalaakso hannalaakso self-assigned this Aug 18, 2021
@hannalaakso hannalaakso changed the title Review accordion NVDA fix Review accordion bug on Android with summary focus state Aug 19, 2021
@hannalaakso
Copy link
Member Author

hannalaakso commented Aug 19, 2021 •
edited
Loading

It seems that the issue is the font size and line height set in the enclosing <h2> which very slightly seems to impact line heights inside of it, even when they're being set explicitly in the enclosed elements.

We explicitly set these properties to stop Elements, as well as browser default stylesheets, or other stylesheets, interfering with the h2 as it's not using the govuk-heading-m class. See #1796

@hannalaakso hannalaakso mentioned this issue Aug 25, 2021
Closed
62 tasks
@hannalaakso hannalaakso changed the title Review accordion bug on Android with summary focus state Review accordion bug on mobile with summary focus state Sep 20, 2021
@vanitabarrett
Copy link
Contributor

vanitabarrett commented Sep 21, 2021 •
edited
Loading

I've been playing around with removing the two explicitly set line-heights on the button and the toggle:

.govuk-accordion__section-button {
    @include govuk-text-colour;
    @include govuk-font($size: 24, $weight: bold, $line-height: 1.25);
    display: block;
    margin-bottom: 0;
    padding-top: govuk-spacing(3);
}

.govuk-accordion__section-toggle {
    @include govuk-font($size: 19, line-height: 1);
     color: $govuk-link-colour;
}

Note that line height is still set by the govuk-font mixin, we're just no longer overriding it.

The margin between the summary list and the heading and toggle needs increasing as a result so that there's a gap between each (in these screenshots, it's been increased to 8px), but this does seem to fix the issue with the focus state. See below for screenshots of before and after.

Before vs After

Screenshot 2021-09-21 at 16 51 55Screenshot 2021-09-21 at 16 56 22

Screenshot 2021-09-21 at 16 56 35Screenshot 2021-09-21 at 16 56 45

Before vs After in legacy mode

Screenshot 2021-09-21 at 17 15 17Screenshot 2021-09-21 at 17 15 25

Screenshot 2021-09-21 at 17 15 35Screenshot 2021-09-21 at 17 15 43

Before vs After of vertical-align: top on <sup>

Worth noting that some sections in the full page example use the <sup> tag within the summary line, which pushes things out of alignment. Don't think this is something for us to fix, but users may want to use vertical-align: top to stop things being pushed out of alignment.

Screenshot 2021-09-21 at 16 57 01Screenshot 2021-09-21 at 17 13 16

@vanitabarrett
Copy link
Contributor

Note: changes still need to be made and pushed to the accordion branch before this ticket can be closed.

vanitabarrett pushed a commit that referenced this issue Sep 22, 2021
Remove explicitly set line-height values
2f2fc58 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
@vanitabarrett
Copy link
Contributor

vanitabarrett commented Sep 22, 2021 •
edited
Loading

Fix pushed to 98f67a5 for testing

vanitabarrett pushed a commit that referenced this issue Sep 22, 2021
Remove explicitly set line-height values
98f67a5 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
vanitabarrett pushed a commit that referenced this issue Sep 22, 2021
Remove explicitly set line-height values
e11eab7 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
@hannalaakso
Copy link
Member Author

@vanitabarrett Great investigation, this seems to have fixed the issue 🎉 Tested this on Galaxy S21 on Chrome, FF and Samsung Internet.

hannalaakso pushed a commit that referenced this issue Sep 27, 2021
@hannalaakso
Remove explicitly set line-height values
632f400 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
hannalaakso pushed a commit that referenced this issue Oct 5, 2021
@hannalaakso
Remove explicitly set line-height values
5f48ba5 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
hannalaakso pushed a commit that referenced this issue Oct 13, 2021
@hannalaakso
Remove explicitly set line-height values
ec0bd7e 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
hannalaakso pushed a commit that referenced this issue Oct 13, 2021
@hannalaakso
Remove explicitly set line-height values
57718a0 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
hannalaakso pushed a commit that referenced this issue Oct 27, 2021
@hannalaakso
Remove explicitly set line-height to fix focus state on mobile
c7a796a 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
hannalaakso pushed a commit that referenced this issue Nov 1, 2021
@hannalaakso
Remove explicitly set line-height to fix focus state on mobile
795bda7 
We were explicitly setting line-heights on the button and the toggle - normally the
govuk-font mixin will do this for us and will choose a line-height value appropriate to
the font size.

We were seeing an issue with the focus state on smaller screens where there was
a thin black line between lines. See #2322
 for more detail.

This commit removes the explicitly set line-height so that the focus state displays as
expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hannalaakso hannalaakso

@vanitabarrett vanitabarrett

Labels
accordion
Projects
GOV.UK Design System cycle board
Archived in project
Milestone
No milestone
Development

No branches or pull requests
3 participants
@36degrees @hannalaakso @vanitabarrett