alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.13k stars 319 forks source link

Refactor `govuk-visually-hidden` Sass #5063

Open querkmachine opened 2 weeks ago

querkmachine commented 2 weeks ago

Review app: https://govuk-frontend-pr-5063.herokuapp.com/

A neat little optimisation that we can probably make now that we've dropped support for older IE versions.

Currently, we have two mixins that share mostly identical styles: govuk-visually-hidden and govuk-visually-hidden-focusable, with a few of differences:

  1. govuk-visually-hidden uses pseudo-elements to add spaces before and after the visually-hidden content.
  2. govuk-visually-hidden-focusable applies the visually-hidden styles by default, but then attempts to undo these styles when the element receives keyboard focus or is in the active state.
  3. govuk-visually-hidden-focusable additionally omits a couple of the visually-hidden styles on the basis of not wanting to undo them upon focus or active states.

Having two almost-identical mixins isn't ideal when it comes to code duplication. Trying to manually revert styles upon a particular state is also quite untidy and evidently is something we've had problems doing before, given we've needed to explicitly omit some styles to not unintentionally revert the styles we do want.

Now that we've dropped support for Internet Explorer 8 and 9, we have the option of using the :not selector to only apply the visually-hidden styles when the element is not focused or active, allowing both mixins to use the same set of core styles and allowing for the complete removal of the manual revert code. 🛹

This would close #1597.

Changes

Thoughts

This uses two chained :not selectors to achieve the desired result: :not(:active):not(:focus). This could be just one selector, :not(:active, :focus), but IE 11 doesn't support this syntax and PostCSS doesn't appear to be converting it to the syntax that IE 11 understands.

I've named the internal mixin _govuk-visually-hidden-text. This is technically a misnomer as it could hide any element or set of elements, but simply calling it _govuk-visually-hidden seemed too likely to cause confusion. Alternative naming suggestions welcome.

github-actions[bot] commented 2 weeks ago

:clipboard: Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 112.79 KiB
dist/govuk-frontend-development.min.js 41.88 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 87.42 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 82.11 KiB
packages/govuk-frontend/dist/govuk/all.mjs 981 B
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 112.78 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 41.87 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.55 KiB
packages/govuk-frontend/dist/govuk/init.mjs 4.86 KiB

Modules

File Size (bundled) Size (minified)
all.mjs 79.24 KiB 39.84 KiB
accordion.mjs 23.5 KiB 12.39 KiB
button.mjs 5.98 KiB 2.69 KiB
character-count.mjs 22.4 KiB 9.92 KiB
checkboxes.mjs 5.83 KiB 2.83 KiB
error-summary.mjs 7.89 KiB 3.46 KiB
exit-this-page.mjs 17.1 KiB 9.26 KiB
header.mjs 4.46 KiB 2.6 KiB
notification-banner.mjs 6.26 KiB 2.62 KiB
password-input.mjs 15.15 KiB 7.25 KiB
radios.mjs 4.83 KiB 2.38 KiB
skip-link.mjs 4.39 KiB 2.18 KiB
tabs.mjs 10.13 KiB 6.11 KiB

View stats and visualisations on the review app


Action run for 1eef8431f9506324da139228c575607a27bb8527

github-actions[bot] commented 2 weeks ago

Stylesheets changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
index ceac53d84..17f05ea65 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
@@ -5130,18 +5130,6 @@ screen and (forced-colors:active) {
 }

 .govuk-skip-link {
-    position: absolute !important;
-    width: 1px !important;
-    height: 1px !important;
-    margin: 0 !important;
-    overflow: hidden !important;
-    clip: rect(0 0 0 0) !important;
-    -webkit-clip-path: inset(50%) !important;
-    clip-path: inset(50%) !important;
-    white-space: nowrap !important;
-    -webkit-user-select: none;
-    -ms-user-select: none;
-    user-select: none;
     font-family: GDS Transport, arial, sans-serif;
     -webkit-font-smoothing: antialiased;
     -moz-osx-font-smoothing: grayscale;
@@ -5154,20 +5142,21 @@ screen and (forced-colors:active) {
     padding: 10px 15px
 }

-.govuk-skip-link:active,
-.govuk-skip-link:focus {
-    position: static !important;
-    width: auto !important;
-    height: auto !important;
-    margin: inherit !important;
-    overflow: visible !important;
-    clip: auto !important;
-    -webkit-clip-path: none !important;
-    clip-path: none !important;
-    white-space: inherit !important;
-    -webkit-user-select: text;
-    -ms-user-select: text;
-    user-select: text
+.govuk-skip-link:not(:active):not(:focus) {
+    position: absolute !important;
+    width: 1px !important;
+    height: 1px !important;
+    margin: 0 !important;
+    padding: 0 !important;
+    overflow: hidden !important;
+    clip: rect(0 0 0 0) !important;
+    -webkit-clip-path: inset(50%) !important;
+    clip-path: inset(50%) !important;
+    border: 0 !important;
+    white-space: nowrap !important;
+    -webkit-user-select: none;
+    -ms-user-select: none;
+    user-select: none
 }

 @media print {
@@ -6201,37 +6190,23 @@ screen and (-ms-high-contrast:active) {
     content: " "
 }

-.govuk-visually-hidden-focusable {
+.govuk-visually-hidden-focusable:not(:active):not(:focus) {
     position: absolute !important;
     width: 1px !important;
     height: 1px !important;
     margin: 0 !important;
+    padding: 0 !important;
     overflow: hidden !important;
     clip: rect(0 0 0 0) !important;
     -webkit-clip-path: inset(50%) !important;
     clip-path: inset(50%) !important;
+    border: 0 !important;
     white-space: nowrap !important;
     -webkit-user-select: none;
     -ms-user-select: none;
     user-select: none
 }

-.govuk-visually-hidden-focusable:active,
-.govuk-visually-hidden-focusable:focus {
-    position: static !important;
-    width: auto !important;
-    height: auto !important;
-    margin: inherit !important;
-    overflow: visible !important;
-    clip: auto !important;
-    -webkit-clip-path: none !important;
-    clip-path: none !important;
-    white-space: inherit !important;
-    -webkit-user-select: text;
-    -ms-user-select: text;
-    user-select: text
-}
-
 .govuk-\!-display-inline {
     display: inline !important
 }

Action run for 1eef8431f9506324da139228c575607a27bb8527

github-actions[bot] commented 2 weeks ago

Other changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/helpers/_visually-hidden.scss b/packages/govuk-frontend/dist/govuk/helpers/_visually-hidden.scss
index 1bd4bf53d..604da3122 100644
--- a/packages/govuk-frontend/dist/govuk/helpers/_visually-hidden.scss
+++ b/packages/govuk-frontend/dist/govuk/helpers/_visually-hidden.scss
@@ -2,7 +2,7 @@
 /// @group helpers/accessibility
 ////

-/// Hide an element visually, but have it available for screen readers
+/// Helper function containing the common code for the following two mixins
 ///
 /// @link https://snook.ca/archives/html_and_css/hiding-content-for-accessibility
 ///   - Hiding Content for Accessibility, Jonathan Snook, February 2011
@@ -11,23 +11,11 @@
 ///
 /// @param {Boolean} $important [true] - Whether to mark as `!important`
 ///
-/// @access public
+/// @access private

-@mixin govuk-visually-hidden($important: true) {
+@mixin _govuk-visually-hide-content($important: true) {
   position: absolute if($important, !important, null);

-  // Absolute positioning has the unintended consequence of removing any
-  // whitespace surrounding visually hidden text from the accessibility tree.
-  // Insert a space character before and after visually hidden text to separate
-  // it from any visible text surrounding it.
-  &::before {
-    content: "\00a0";
-  }
-
-  &::after {
-    content: "\00a0";
-  }
-
   width: 1px if($important, !important, null);
   height: 1px if($important, !important, null);
   // If margin is set to a negative value it can cause text to be announced in
@@ -36,6 +24,8 @@
   padding: 0 if($important, !important, null);

   overflow: hidden if($important, !important, null);
+
+  // `clip` is needed for IE11 support
   clip: rect(0 0 0 0) if($important, !important, null);
   -webkit-clip-path: inset(50%) if($important, !important, null);
           clip-path: inset(50%) if($important, !important, null);
@@ -55,62 +45,40 @@
           user-select: none;
 }

-/// Hide an element visually, but have it available for screen readers whilst
-/// allowing the element to be focused when navigated to via the keyboard (e.g.
-/// for the skip link)
-///
-/// This is slightly less opinionated about borders and padding to make it
-/// easier to style the focussed element.
+/// Hide an element visually, but have it available for screen readers
 ///
 /// @param {Boolean} $important [true] - Whether to mark as `!important`
 ///
 /// @access public

-@mixin govuk-visually-hidden-focusable($important: true) {
-  position: absolute if($important, !important, null);
-
-  width: 1px if($important, !important, null);
-  height: 1px if($important, !important, null);
-  // If margin is set to a negative value it can cause text to be announced in
-  // the wrong order in VoiceOver for OSX
-  margin: 0 if($important, !important, null);
-
-  overflow: hidden if($important, !important, null);
-  clip: rect(0 0 0 0) if($important, !important, null);
-  -webkit-clip-path: inset(50%) if($important, !important, null);
-          clip-path: inset(50%) if($important, !important, null);
-
-  // For long content, line feeds are not interpreted as spaces and small width
-  // causes content to wrap 1 word per line:
-  // https://medium.com/@jessebeach/beware-smushed-off-screen-accessible-text-5952a4c2cbfe
-  white-space: nowrap if($important, !important, null);
-
-  // Prevent users from selecting or copying visually-hidden text. This prevents
-  // a user unintentionally copying more text than they intended and needing to
-  // manually trim it down again.
-  -webkit-user-select: none;
-      -ms-user-select: none;
-          user-select: none;
-
-  &:active,
-  &:focus {
-    position: static if($important, !important, null);
+@mixin govuk-visually-hidden($important: true) {
+  @include _govuk-visually-hide-content($important: $important);

-    width: auto if($important, !important, null);
-    height: auto if($important, !important, null);
-    margin: inherit if($important, !important, null);
+  // Absolute positioning has the unintended consequence of removing any
+  // whitespace surrounding visually hidden text from the accessibility tree.
+  // Insert a space character before and after visually hidden text to separate
+  // it from any visible text surrounding it.
+  &::before {
+    content: "\00a0";
+  }

-    overflow: visible if($important, !important, null);
-    clip: auto if($important, !important, null);
-    -webkit-clip-path: none if($important, !important, null);
-            clip-path: none if($important, !important, null);
+  &::after {
+    content: "\00a0";
+  }
+}

-    white-space: inherit if($important, !important, null);
+/// Hide an element visually, but have it available for screen readers whilst
+/// allowing the element to be focused when navigated to via the keyboard (e.g.
+/// for the skip link)
+///
+/// @param {Boolean} $important [true] - Whether to mark as `!important`
+///
+/// @access public

-    // Allow the text to be selectable now it's visible
-    -webkit-user-select: text;
-        -ms-user-select: text;
-            user-select: text;
+@mixin govuk-visually-hidden-focusable($important: true) {
+  // IE 11 doesn't support the combined `:not(:active, :focus)` syntax.
+  &:not(:active):not(:focus) {
+    @include _govuk-visually-hide-content($important: $important);
   }
 }

Action run for 1eef8431f9506324da139228c575607a27bb8527

querkmachine commented 2 days ago

I've renamed the internal mixin to _govuk-visually-hide-content, to make it a bit more distinct from the others. Helps to understand it by making it more verb-y too, I guess?

selfthinker commented 1 day ago

@selfthinker would you prefer having further testing with a wider range of browser/assistive tech combinations?

I would prefer to test more. That's mostly because I know we had issues with the visually hidden class before that would only happen in a specific screen reader / browser combination. Better to be safe than sorry.

Where can I best test this? I can do that tomorrow.

romaricpascal commented 1 day ago

Where can I best test this? I can do that tomorrow.

@querkmachine may have better options

The govuk-visually-hidden-focusable mixin is used by the Skip Link (for comparison, here's the Skip Link example for main)

We have quite a few applications of the govuk-visually-hidden class in the different components and examples.

selfthinker commented 1 day ago

Where can I best test this? I can do that tomorrow.

@romaricpascal, sorry, I meant where on the internet. Is this PR deployed somewhere? I could potentially also copy the CSS and create an example page that uses it in several different scenarios.

querkmachine commented 1 day ago

https://govuk-frontend-pr-5063.herokuapp.com/

There's an automated deployment under the 'View deployment' button (currently) a few comments above this one. GitHub's interface be at it again.

selfthinker commented 1 day ago

There's an automated deployment under the 'View deployment' button

I thought it would be deployed automatically, and I was looking for such a link or button. But either I didn't see it or I didn't understand that that is what it did. Thanks for pointing that out.

selfthinker commented 20 hours ago

I've just tested the three examples suggested by Romaric and they behave exactly like before, so everything looks good to me. :+1:

(And while I was doing that I noticed that it's not a good idea to visually hide table headers as most screen readers ignore them when navigating data cells. But that was already an issue before and doesn't have anything to do with this implementation.)