Use text colour on focus for better contrast #982

NickColley · 2018-09-06T11:41:30Z

Closes #944

This pull request updates the focus styles of links in GOV.UK Frontend so they pass WCAG contrast requirements.

You can compare https://govuk-frontend-review-pr-982.herokuapp.com/examples/links vs https://govuk-frontend-review.herokuapp.com/examples/links

Screenshots

back link

Before	After

breadcrumbs

Before	After

details

Before	After

skip link

Before	After

header

Before	After

govuk-link

Before	After

https://trello.com/c/XQPpwofN/1381-1-modify-focus-state-to-use-text-colour

NickColley · 2018-09-06T12:00:04Z

src/helpers/_links.scss

+
+  // When focussed, the text colour needs to be darker to ensure that colour
+  // contrast is still acceptable
+  &:focus {


I tried changing the focusable mixin and that led to ordering problems since focus was not the last selector.

Do you think this would be easy to fix?

It just felt fragile having to move lots of code around to get the ordering right, we can explore it more, it would be nice to have it in the mixin.

I think one to do together if we decide to explore that.

36degrees · 2018-09-06T12:44:56Z

'Active' links in the header component are still showing as light blue on yellow:

36degrees · 2018-09-06T12:47:10Z

Through doing this I found that the .govuk-list core styles styles anchors, I have updated these to extend the govuk-link selector instead.

I have checked our documentation and we do not recommend our users do this, so maybe this should deprecated and removed instead, or in the future?

Yeah, we don't do it in any other component that might include links, so I agree we should be thinking about removing this.

NickColley · 2018-09-06T15:16:19Z

@36degrees I have added an additional commit to address your feedback about the header component

36degrees · 2018-09-06T15:17:50Z

app/views/examples/typography/index.njk

@@ -137,18 +137,18 @@
      <section class="govuk-!-margin-top-8">
        <h2 class="govuk-heading-l govuk-!-padding-bottom-2" style="border-bottom: 4px solid;">Lists</h2>
        <h3 class="govuk-heading-m">govuk-list</h3>
-        <ul class="govuk-list govuk">


If we're not sure about keeping the list link styling, do we want to keep these classes for now?

36degrees · 2018-09-06T15:20:21Z

src/components/details/_details.scss

@@ -42,6 +42,9 @@
    // -1px offset fixes gap between background and outline in Firefox


Non-blocking: I wonder if we should make it possible to pass an offset to the focusable helper, to avoid this duplication? As far as I can tell, the 1px offset is the only reason this isn't using the helper.

I think the width of the outline itself is different too... I think best for another pull request.

36degrees · 2018-09-06T15:20:53Z

src/components/error-summary/_error-summary.scss

      color: $govuk-error-colour;
-      text-decoration: underline;


Is this just for consistency with the link styles, relying on the browser default?

As far as I could tell, this did nothing?

36degrees · 2018-09-06T15:21:06Z

src/components/header/_header.scss

    &:focus {
-      color: govuk-colour("black");
+      color: $govuk-text-colour;


Actually, I'm wondering if this should be a different application? Thinking about (admittedly future) things like whitelabelling, coupling focus text colour to always being the same as the text colour might not always be what users want to do? For example, depending on the focus colour, white might be a better choice.

$govuk-focus-text-colour: govuk-colour("black") !default;

Yeah I like that, we should definitely do that.

Hmm, maybe we could do this later to be honest, I've done a commit but wonder if thinking about theming holistically with everything else would be better than adding it piecemeal.

Also removes `text-decoration: underline` since it is not being used.

36degrees

Have tested by fixing the linting error and running the app locally and this is looking good – just a couple of minor things to address and then I think we can merge this 🎉

36degrees · 2018-09-07T07:35:46Z

src/settings/_colours-applied.scss

@@ -127,4 +136,4 @@ $govuk-link-hover-colour: govuk-colour("light-blue") !default;
 /// @type Colour
 /// @access public

-$govuk-link-active-colour: govuk-colour("light-blue") !default;
+$govuk-link-active-colour: govuk-colour("light-blue") !default;


Minor, but looks like you've removed the new line from the end of this file.

Looks like the linter's picked up on this as well, which I think is why the tests (and the review app) are now failing.

All hail the linter

src/settings/_colours-applied.scss

+/// Focused text colour
+///
+/// Ensure that the contrast between the text and background colour passes
+/// WCAG Level AA contrast requirements.


NickColley · 2018-09-07T09:40:29Z

@36degrees updated as per your feedback, thanks Ollie.

NickColley · 2018-09-07T14:12:59Z

I think I've broken the 'no visited state', definitely have, so block merging until I resolve that.

NickColley · 2018-09-10T09:32:24Z

I managed to solve the issues by duplicating the pseudo selectors, which is unfortunate but I think is the simplest way for now, this is consistent with the other modifiers at least.

Makes this class consistent with the other selectors. Unfortunately due to specificity we need to duplicate the pseudo selectors. This also fixes the hover state for the no visited state links.

36degrees

Great work 👏

paroxp · 2018-09-10T10:26:25Z

👏

This reverts commit 6a007a7. The issue in question has been beautifully handled by the Design System Team[1] and no longer needs to exist in our codebase. Going back to the standard GOV.UK Frontend for consistency and easier upgrades. [1]: alphagov/govuk-frontend#982

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-982 September 6, 2018 11:41 Inactive

NickColley changed the title ~~[Do not merge] Use text colour on focus~~ [Do not merge] Use text colour on focus for better contrast Sep 6, 2018

NickColley commented Sep 6, 2018

View reviewed changes

NickColley force-pushed the use-text-colour-on-focus branch from c6d93db to 74ab674 Compare September 6, 2018 12:25

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-982 September 6, 2018 12:25 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-982 September 6, 2018 14:08 Inactive

36degrees reviewed Sep 6, 2018

View reviewed changes

NickColley force-pushed the use-text-colour-on-focus branch from ca5b1e0 to 29b2404 Compare September 6, 2018 15:35

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-982 September 6, 2018 15:35 Inactive

NickColley added this to the v2.0.0 milestone Sep 6, 2018

NickColley added the breaking change label Sep 6, 2018

NickColley added 3 commits September 6, 2018 16:54

Add darker focus color for details component

3c78f95

Add darker focus colour for error summary links

0125d65

Also removes `text-decoration: underline` since it is not being used.

Add darker focus colour for default link styles

a1922cb

NickColley force-pushed the use-text-colour-on-focus branch from 29b2404 to de1c426 Compare September 6, 2018 15:54

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-982 September 6, 2018 15:54 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-982 September 6, 2018 16:00 Inactive

NickColley changed the title ~~[Do not merge] Use text colour on focus for better contrast~~ Use text colour on focus for better contrast Sep 6, 2018

36degrees reviewed Sep 7, 2018

View reviewed changes

NickColley force-pushed the use-text-colour-on-focus branch from a7c354e to 16a6272 Compare September 7, 2018 09:34

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-982 September 7, 2018 09:35 Inactive

This comment has been minimized.

Sign in to view

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-982 September 10, 2018 09:29 Inactive

Add darker focus colour to header component

37421a5

NickColley added 2 commits September 10, 2018 10:33

Add a new applied variable for focus text colour

45b6d06

Add no visited state mixin with new focus styles

2c1271d

Makes this class consistent with the other selectors. Unfortunately due to specificity we need to duplicate the pseudo selectors. This also fixes the hover state for the no visited state links.

NickColley force-pushed the use-text-colour-on-focus branch from 28f6aee to 2c1271d Compare September 10, 2018 09:34

govuk-design-system-ci deployed to govuk-frontend-review-pr-982 September 10, 2018 09:34 View deployment

36degrees approved these changes Sep 10, 2018

View reviewed changes

NickColley merged commit 5ce12b9 into master Sep 10, 2018

NickColley deleted the use-text-colour-on-focus branch September 10, 2018 10:19

paroxp mentioned this pull request Apr 17, 2019

Bump GOV.UK Frontend to v2.2.0 alphagov/paas-admin#104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use text colour on focus for better contrast #982

Use text colour on focus for better contrast #982

NickColley commented Sep 6, 2018 •

edited

Loading

NickColley Sep 6, 2018

36degrees Sep 6, 2018

NickColley Sep 6, 2018

36degrees commented Sep 6, 2018

36degrees commented Sep 6, 2018

NickColley commented Sep 6, 2018

36degrees Sep 6, 2018

36degrees Sep 6, 2018

NickColley Sep 6, 2018

36degrees Sep 6, 2018

36degrees Sep 6, 2018

NickColley Sep 6, 2018

36degrees Sep 6, 2018

36degrees Sep 6, 2018

NickColley Sep 6, 2018

NickColley Sep 6, 2018

36degrees left a comment

36degrees Sep 7, 2018

36degrees Sep 7, 2018

NickColley Sep 7, 2018

This comment was marked as resolved.

NickColley commented Sep 7, 2018

NickColley commented Sep 7, 2018

This comment has been minimized.

NickColley commented Sep 10, 2018

36degrees left a comment

paroxp commented Sep 10, 2018

		@@ -42,6 +42,9 @@
		// -1px offset fixes gap between background and outline in Firefox

Use text colour on focus for better contrast #982

Use text colour on focus for better contrast #982

Conversation

NickColley commented Sep 6, 2018 • edited Loading

Screenshots

back link

breadcrumbs

details

skip link

header

govuk-link

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

36degrees commented Sep 6, 2018

36degrees commented Sep 6, 2018

NickColley commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

36degrees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

NickColley commented Sep 7, 2018

NickColley commented Sep 7, 2018

This comment has been minimized.

NickColley commented Sep 10, 2018

36degrees left a comment

Choose a reason for hiding this comment

paroxp commented Sep 10, 2018

NickColley commented Sep 6, 2018 •

edited

Loading