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

Update accordion to use new WCAG 2.1 compliant focus style #1324

Merged
merged 9 commits into from May 13, 2019

Conversation

@nickcolley
Copy link
Contributor

commented May 8, 2019

Updates the accordion component to use the new WCAG 2.1 compliant focus styles.

GIF demonstrations

IE8

accordion-ie8

IE9

accordion-ie9

IE10

accordion-ie10

IE11

accordion-ie11

Firefox with changed colours

accordion-firefox-changed-colours

Firefox

accordion-firefox

Safari

accordion-safari

Chrome

accordion-chrome

Chrome mobile

accordion-chrome-mobile

Safari mobile

accordion-safari-mobile

Closes #1307

@nickcolley nickcolley changed the base branch from master to v3 May 8, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 8, 2019 Inactive

@nickcolley nickcolley force-pushed the new-focus-accordion branch from bdfcf6d to 339cd19 May 9, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 9, 2019 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 9, 2019 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 9, 2019 Inactive

@nickcolley nickcolley force-pushed the new-focus-accordion branch from 7790dd2 to bdfcb67 May 9, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 9, 2019 Inactive

@nickcolley nickcolley marked this pull request as ready for review May 9, 2019

@nickcolley nickcolley added this to Needs review in Design System Sprint Board via automation May 9, 2019

@nickcolley nickcolley added this to the v3.0.0 milestone May 9, 2019

@nickcolley
Copy link
Contributor Author

left a comment

Needs a CHANGELOG entry

@@ -5,6 +5,10 @@

@include govuk-exports("govuk/component/accordion") {

$govuk-accordion-link-colour: govuk-colour("blue");

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

I think these should probably reference $govuk-link-colour / $govuk-link-hover-colour?

EDIT: As Hanna pointed out in #1326, ``$govuk-link-hover-colouris dark-blue, so that doesn't work, but$govuk-link-colour` I think makes sense.

border-width: 0;
color: $govuk-link-colour;
background: none;
cursor: pointer;
-webkit-appearance: none;

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Should we use appearance: none here and let autoprefixer do the rest?

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 10, 2019

Author Contributor

I don't think that's actually real CSS right now? In our button CSS we use the webkit prefix too...

https://drafts.csswg.org/css-ui-4/#appearance-switching

I'd raise an issue to update all of them if you think it's worth doing.

@include govuk-not-ie8 {
border-top-color: $govuk-focus-colour;
}
outline: $govuk-focus-width solid transparent;

This comment was marked as resolved.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Might be good to comment explaining what this is for

@media (hover: none) {
background-color: initial;
// For devices that can't hover such as touch devices,
// remove hover state as it can be stuck in that state (iOS).

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

The comment doesn't seem to match what the code's doing any more?

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 10, 2019

Author Contributor

It's still a hover state that can be stuck and needs to be removed, any ideas on how to improve it?

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Sorry, I think I mis-read this, makes sense.

padding-top: govuk-spacing(3);
padding-bottom: 0;
padding-left: 0;
padding: 0;
border-width: 0;
// Headings in section headers have link colours as an additional affodance

This comment was marked as resolved.

Copy link
@36degrees

36degrees May 10, 2019

Contributor
Suggested change
// Headings in section headers have link colours as an additional affodance
// Headings in section headers have link colours as an additional affordance
}
}

.govuk-accordion__section:not(.govuk-accordion__section--expanded) .govuk-accordion__section-button:focus:not(:active) {

This comment was marked as resolved.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Complex selector – worth a comment?

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Great comment.

@@ -70,15 +74,24 @@
// This is styled to look like a link not a button
.govuk-accordion__open-all {
@include govuk-font($size: 16);
@include govuk-link-common;
display: inline;
position: relative;

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

I am wondering whether the open all button meets colour contrast if the top-most section is already open when you go to focus it.

Screenshot 2019-05-10 at 09 45 05BST

I think in this case the black underline 'blends in' with the blue top border, so I'm not sure it'd be perceivable, and the yellow alone does not meet contrast against the white page background.

@dashouse what do you think?

This comment has been minimized.

Copy link
@dashouse

dashouse May 10, 2019

Contributor

@36degrees Blue to Black is 3.78:1. We're somewhat relying on this change being ok elsewhere. It also has the underline removed which is exclusive to the focus state. Go with it and see what comes up in the audit?

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Sounds good – I should've checked the contrast rather than just assuming, surprised it's that high! 👍

&:focus {
background: none;
// Remove default button focus outline in Firefox
&::-moz-focus-inner {

This comment was marked as resolved.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Good catch – wonder if we should list this bug fix (and the similar fix for section headings) separately in the changelog?

@nickcolley nickcolley force-pushed the new-focus-accordion branch from bdfcb67 to 9610671 May 10, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 10, 2019 Inactive

@nickcolley nickcolley force-pushed the new-focus-accordion branch from 9610671 to d0bd0b1 May 10, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 10, 2019 Inactive

@nickcolley nickcolley changed the title New focus accordion Update accordion to use new WCAG 2.1 compliant focus style May 10, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 10, 2019 Inactive

@nickcolley nickcolley force-pushed the new-focus-accordion branch from 765d3f6 to 3fb3de6 May 10, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 10, 2019 Inactive


- Update footer links to use new focus style

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

I think this has got lost in the rebase?

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 10, 2019

Author Contributor

Thanks, I had a bit of a mare in the rebase on this one

@media (hover: none) {
background-color: initial;
// For devices that can't hover such as touch devices,
// remove hover state as it can be stuck in that state (iOS).

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Sorry, I think I mis-read this, makes sense.

}
}

.govuk-accordion__section:not(.govuk-accordion__section--expanded) .govuk-accordion__section-button:focus:not(:active) {

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

Great comment.

@hannalaakso
Copy link
Member

left a comment

Great work. Just left a couple of small comments.

@include govuk-link-common;
display: inline;
position: relative;
z-index: 1;

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso May 10, 2019

Member

It looks like this is used to float the button over the component so that the focus state is visible. In order to avoid setting z-index, could we add a few pixels of bottom margin to .govuk-accordion__controls so that it doesn't overlap the component?

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 10, 2019

Author Contributor

I think Dave made these changes to intentionally overlap the accordion by the looks?

This comment has been minimized.

Copy link
@dashouse

dashouse May 10, 2019

Contributor

Yeah, wanted this to overlap the top border and line up. It felt a bit untidy when it was almost overlapping.

This comment has been minimized.

Copy link
@hannalaakso
display: inline;
position: relative;
z-index: 1;
margin: 0;

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso May 10, 2019

Member

I'm not sure if this is needed. Or is it guarding against some edge case?

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 10, 2019

Author Contributor

If you don't do this the open all button focus state renders below the border

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso May 10, 2019

Member

Sorry my comment refers to margin: 0, not the z-index. I still can't see setting margin: 0 making any difference. But this is non-blocking

This comment has been minimized.

Copy link
@36degrees

36degrees May 10, 2019

Contributor

I think it's just being explicit rather than relying on the user agent styles. The open all button is just using the default user agent padding at the minute, for example. Even if most (all?) browsers use margin: 0 it's still good to be clear that that's intentional.

This comment has been minimized.

Copy link
@hannalaakso
border-width: 0;
// Headings in section headers have link colours as an additional affodance
color: $govuk-link-colour;
color: inherit;

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso May 10, 2019

Member

Non-blocking: Would it be more robust to set the colour directly on the button?

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 10, 2019

Author Contributor

The color is set on the header element which lets you group the CSS together so it's easier to understand

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 10, 2019

Author Contributor

Also the intent of our styling here is to make the button turn into the header, so inheriting feels more correct towards that effect.

This comment has been minimized.

Copy link
@hannalaakso
@include govuk-not-ie8 {
&:focus {
outline: none;
background: none;

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso May 10, 2019

Member

Non-blocking: I can't see these two styles having any effect. Might be worth adding a comment?

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 10, 2019

Author Contributor

Sure thing

This comment has been minimized.

Copy link
@nickcolley

nickcolley May 13, 2019

Author Contributor

I think this was left around before we had the new focus styles so can be removed thanks :)

@nickcolley nickcolley force-pushed the new-focus-accordion branch from 3fb3de6 to 53c5a59 May 10, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 10, 2019 Inactive

@nickcolley nickcolley force-pushed the new-focus-accordion branch from 53c5a59 to 97eee6c May 13, 2019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1324 May 13, 2019 Inactive

@nickcolley nickcolley merged commit 8bc4d9c into v3 May 13, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Design System Sprint Board automation moved this from Needs review to Done May 13, 2019

@nickcolley nickcolley deleted the new-focus-accordion branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.