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

[SPIKE] Allow an app to respect system preference, or override them with app preference it would store #4921

Conversation

romaricpascal
Copy link
Member

@romaricpascal romaricpascal commented Apr 4, 2024

This PR:

A couple of notable things:

  • it regroups all code related to switching color scheme in the same Scss file rather than splitting it across the _govuk-frontend-properties.scss file for the 'defaults' and the _template.scss for the overriding classes. This provides a single place to manage all color-scheme related variables
  • it does not persist the app preference expressed by query parameter, as this is really a review app concern (and for services, down to their own implementation). We could explore that in a future PR if we wanted (POST to the app to set a cookie, for example, which should probably be left overridable by query parameter so we can quickly switch iframes from light to dark without having to change the whole site? 🤔 ).
  • it does not pass the preference expressed by query parameter to the example iframes. Same it's something we'll need to properly do when we implement dark mode rather than now for exploring the feasibility. We could also do that in a separate PR if we want.

Copy link

github-actions bot commented Apr 4, 2024

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 112.43 KiB
dist/govuk-frontend-development.min.js 42.21 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 87.21 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 81.95 KiB
packages/govuk-frontend/dist/govuk/all.mjs 4.17 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 112.41 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 42.2 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.55 KiB

Modules

File Size (bundled) Size (minified)
all.mjs 77.67 KiB 40.19 KiB
accordion.mjs 22.71 KiB 12.85 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 e920075

Copy link

github-actions bot commented Apr 4, 2024

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 5808bb920..e83928383 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
@@ -6,6 +6,7 @@
     --govuk-frontend-breakpoint-tablet: 40.0625rem;
     --govuk-frontend-breakpoint-desktop: 48.0625rem;
     --govuk-font-family: GDS Transport, arial, sans-serif;
+    color-scheme: light;
     --govuk-brand-colour: #1d70b8;
     --govuk-text-colour: #0b0c0c;
     --govuk-canvas-background-colour: #f3f2f1;
@@ -25,6 +26,38 @@
     --govuk-link-active-colour: #0b0c0c
 }
 
+@media screen and (prefers-color-scheme:dark) {
+    @supports (color-scheme:dark) {
+        :root:not(.govuk-theme--light) {
+            color-scheme: dark;
+            --govuk-canvas-background-colour: #333;
+            --govuk-body-background-colour: #222;
+            --govuk-text-colour: #fff;
+            --govuk-secondary-text-colour: #ccc;
+            --govuk-link-colour: #add8e6;
+            --govuk-link-visited-colour: #ffb6c1;
+            --govuk-link-hover-colour: #0ff;
+            --govuk-link-active-colour: #fff
+        }
+    }
+}
+
+@media screen {
+    @supports (color-scheme:dark) {
+        .govuk-theme--dark {
+            color-scheme: dark;
+            --govuk-canvas-background-colour: #333;
+            --govuk-body-background-colour: #222;
+            --govuk-text-colour: #fff;
+            --govuk-secondary-text-colour: #ccc;
+            --govuk-link-colour: #add8e6;
+            --govuk-link-visited-colour: #ffb6c1;
+            --govuk-link-hover-colour: #0ff;
+            --govuk-link-active-colour: #fff
+        }
+    }
+}
+
 @media print {
     :root {
         --govuk-text-colour: #000;
@@ -997,21 +1030,6 @@
     background-color: var(--govuk-body-background-colour)
 }
 
-@media screen {
-    .govuk-template--dark {
-        color-scheme: dark;
-        --govuk-canvas-background-colour: #333;
-        --govuk-hover-colour: #333;
-        --govuk-body-background-colour: #222;
-        --govuk-text-colour: #fff;
-        --govuk-secondary-text-colour: #ccc;
-        --govuk-link-colour: #add8e6;
-        --govuk-link-visited-colour: #ffb6c1;
-        --govuk-link-hover-colour: aqua;
-        --govuk-link-active-colour: #fff
-    }
-}
-
 .govuk-width-container {
     max-width: 960px;
     margin-right: 15px;

Action run for e920075

Copy link

github-actions bot commented Apr 4, 2024

Other changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/core/_all.scss b/packages/govuk-frontend/dist/govuk/core/_all.scss
index a9c87c554..202f48bec 100644
--- a/packages/govuk-frontend/dist/govuk/core/_all.scss
+++ b/packages/govuk-frontend/dist/govuk/core/_all.scss
@@ -1,4 +1,5 @@
 @import "govuk-frontend-properties";
+@import "theme";
 @import "links";
 @import "lists";
 @import "typography";
diff --git a/packages/govuk-frontend/dist/govuk/core/_govuk-frontend-properties.scss b/packages/govuk-frontend/dist/govuk/core/_govuk-frontend-properties.scss
index 1f7c648f7..2f5df20d6 100644
--- a/packages/govuk-frontend/dist/govuk/core/_govuk-frontend-properties.scss
+++ b/packages/govuk-frontend/dist/govuk/core/_govuk-frontend-properties.scss
@@ -9,28 +9,6 @@
   }
 
   --govuk-font-family: #{$govuk-font-family};
-  --govuk-brand-colour: #{$govuk-brand-colour};
-  --govuk-text-colour: #{$govuk-text-colour};
-  --govuk-canvas-background-colour: #{$govuk-canvas-background-colour};
-  --govuk-body-background-colour: #{$govuk-body-background-colour};
-  --govuk-print-text-colour: #{$govuk-print-text-colour};
-  --govuk-secondary-text-colour: #{$govuk-secondary-text-colour};
-  --govuk-focus-colour: #{$govuk-focus-colour};
-  --govuk-focus-text-colour: #{$govuk-focus-text-colour};
-  --govuk-error-colour: #{$govuk-error-colour};
-  --govuk-success-colour: #{$govuk-success-colour};
-  --govuk-border-colour: #{$govuk-border-colour};
-  --govuk-input-border-colour: #{$govuk-input-border-colour};
-  --govuk-hover-colour: #{$govuk-hover-colour};
-  --govuk-link-colour: #{$govuk-link-colour};
-  --govuk-link-visited-colour: #{$govuk-link-visited-colour};
-  --govuk-link-hover-colour: #{$govuk-link-hover-colour};
-  --govuk-link-active-colour: #{$govuk-link-active-colour};
-
-  @include govuk-media-query($media-type: print) {
-    --govuk-text-colour: #{$govuk-print-text-colour};
-    --govuk-font-family: #{$govuk-font-family-print};
-  }
 }
 
 /*# sourceMappingURL=_govuk-frontend-properties.scss.map */
diff --git a/packages/govuk-frontend/dist/govuk/core/_theme.scss b/packages/govuk-frontend/dist/govuk/core/_theme.scss
new file mode 100644
index 000000000..ae33b69dc
--- /dev/null
+++ b/packages/govuk-frontend/dist/govuk/core/_theme.scss
@@ -0,0 +1,71 @@
+// This would likely go in a proper 'settings' file
+// and default to `false`.
+$govuk-theme-from-system: true !default;
+
+@mixin _govuk-theme-light() {
+  color-scheme: light;
+
+  --govuk-brand-colour: #{$govuk-brand-colour};
+  --govuk-text-colour: #{$govuk-text-colour};
+  --govuk-canvas-background-colour: #{$govuk-canvas-background-colour};
+  --govuk-body-background-colour: #{$govuk-body-background-colour};
+  --govuk-print-text-colour: #{$govuk-print-text-colour};
+  --govuk-secondary-text-colour: #{$govuk-secondary-text-colour};
+  --govuk-focus-colour: #{$govuk-focus-colour};
+  --govuk-focus-text-colour: #{$govuk-focus-text-colour};
+  --govuk-error-colour: #{$govuk-error-colour};
+  --govuk-success-colour: #{$govuk-success-colour};
+  --govuk-border-colour: #{$govuk-border-colour};
+  --govuk-input-border-colour: #{$govuk-input-border-colour};
+  --govuk-hover-colour: #{$govuk-hover-colour};
+  --govuk-link-colour: #{$govuk-link-colour};
+  --govuk-link-visited-colour: #{$govuk-link-visited-colour};
+  --govuk-link-hover-colour: #{$govuk-link-hover-colour};
+  --govuk-link-active-colour: #{$govuk-link-active-colour};
+}
+
+@mixin _govuk-theme-dark() {
+  @supports (color-scheme: dark) {
+    color-scheme: dark;
+
+    --govuk-canvas-background-colour: #333333;
+    --govuk-body-background-colour: #222222;
+    --govuk-text-colour: #ffffff;
+    --govuk-secondary-text-colour: #cccccc;
+    --govuk-link-colour: #add8e6;
+    --govuk-link-visited-colour: #ffb6c1;
+    --govuk-link-hover-colour: #00ffff;
+    --govuk-link-active-colour: #ffffff;
+  }
+}
+
+@include govuk-exports("govuk/core/theme") {
+  :root {
+    @include _govuk-theme-light;
+
+    @if $govuk-theme-from-system {
+      @include govuk-media-query($media-type: screen, $and: "(prefers-color-scheme: dark)") {
+        &:not(.govuk-theme--light) {
+          @include _govuk-theme-dark;
+        }
+      }
+    }
+  }
+
+  // Prevent dark theme to leak into print if some variables don't get overridden
+  // reducing the risk of printing big areas of black
+  @include govuk-media-query($media-type: screen) {
+    .govuk-theme--dark {
+      @include _govuk-theme-dark;
+    }
+  }
+
+  @include govuk-media-query($media-type: print) {
+    :root {
+      --govuk-text-colour: #{$govuk-print-text-colour};
+      --govuk-font-family: #{$govuk-font-family-print};
+    }
+  }
+}
+
+/*# sourceMappingURL=_theme.scss.map */
diff --git a/packages/govuk-frontend/dist/govuk/objects/_template.scss b/packages/govuk-frontend/dist/govuk/objects/_template.scss
index 85c8eeb2e..4664d2f0e 100644
--- a/packages/govuk-frontend/dist/govuk/objects/_template.scss
+++ b/packages/govuk-frontend/dist/govuk/objects/_template.scss
@@ -48,23 +48,6 @@
     // Set the overall body of the page back to the typical background colour.
     background-color: var(--govuk-body-background-colour);
   }
-
-
-  @include govuk-media-query($media-type: screen) {
-    .govuk-template--dark {
-      color-scheme: dark;
-
-      --govuk-canvas-background-colour: #333;
-      --govuk-hover-colour: #333;
-      --govuk-body-background-colour: #222;
-      --govuk-text-colour: #fff;
-      --govuk-secondary-text-colour: #ccc;
-      --govuk-link-colour: lightblue;
-      --govuk-link-visited-colour: lightpink;
-      --govuk-link-hover-colour: aqua;
-      --govuk-link-active-colour: #fff;
-    }
-  }
 }
 
 /*# sourceMappingURL=_template.scss.map */

Action run for e920075

@romaricpascal romaricpascal changed the title [SPIKE] Allow an app to respect system preference, or override them with user preference it would store [SPIKE] Allow an app to respect system preference, or override them with app preference it would store Apr 4, 2024
@@ -0,0 +1,62 @@
@mixin _govuk-theme-light() {
color-scheme: light;
Copy link
Member Author

Choose a reason for hiding this comment

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

note We need the color-scheme set in each mixin setting the theme custom properties. If we'd rely on the light dark value, which picks up system preferences without a media query, we'd then get light variables with a dark color scheme if app preferences say 'light' but the system preferences say 'dark'.

Screenshot of a file upload component with a dark themed button in a light page context

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting side effect from the color-scheme property in Firefox. It'll change the value of the prefers-color-scheme media query inside iframes. This means they automatically get the right color scheme there. Chrome and Safari, do not though.

We do need the property for styling native controls like the file inputs with the right colour scheme. I don't think the behaviour with iframes is a deal breaker, more something of a suprise if you expect some level of isolation from being in an iframe and something to make sure to handle if you use iframe so Chrome and Safari users have the right theme. Certainly something that would impact us for the Design System site. For services, not sure how many iframes they use (for payment fields, maybe?).

Side by side comparison of Chrome and Firefox on the File upload page of GOV.UK Frontend review app. Console show the different value for window.matchMedia('(prefers-color-scheme: false)') in Firefox

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@owenatgov owenatgov Apr 8, 2024

Choose a reason for hiding this comment

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

Anecdata here but the most common users of ours also using iframes seem to be other government design systems that consume govuk-frontend. That will probably need some verification (ask Pay maybe?) but considering it's a fairly small scope, maybe the answer here is to just write guidance advising that it's a thing to be aware of and how to fix it. Part of the 'how to fix it' bit of course is tied to if we should bother offering a solution since vendors are still discussing it.

@include _govuk-theme-light;

@include govuk-media-query($media-type: screen, $and: "(prefers-color-scheme: dark)") {
&:not(.govuk-theme--light) {
Copy link
Member Author

Choose a reason for hiding this comment

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

note This adds some specificity, but avoids re-declaring all the variables making the light theme in a .govuk-theme--light class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks to the screen media query, the print styles will not need extra specificity for overriding them.

// Prevent dark theme to leak into print if some variables don't get overridden
// reducing the risk of printing big areas of black
@include govuk-media-query($media-type: screen) {
.govuk-theme--dark {
Copy link
Member Author

Choose a reason for hiding this comment

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

note For consistency with .govuk-theme--light we could have that as :root.govuk-theme--dark rather than just .govuk-theme--dark. That'd make both selectors controlling the theme have the same specificity, which is likely better for overriding in app code.

}

@include govuk-exports("govuk/core/theme") {
:root {
Copy link
Member Author

Choose a reason for hiding this comment

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

note Minifiers will regroup this selector with the :root in _govuk-frontend-properties.

@@ -0,0 +1,62 @@
@mixin _govuk-theme-light() {
Copy link
Member Author

Choose a reason for hiding this comment

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

note There'd be a couple of extra decision around these mixins for an actual implementation:

  • public or private?

    Don't think we need them to be public unless we plan to support setting themes on only part of the page (for ex. through not root level .govuk-theme--light and .govuk-theme--dark classes that could be used on any element)

  • 'theme' or 'colour-scheme'?

    Went with 'theme' to avoid confusion with a mixin that would only control the 'color-scheme' property

  • separate mixins or single mixin with argument (_govuk-theme(light) or _govuk-theme(dark)), possibly pulling from a map ($govuk-themes: (light: ( brand: $govuk-brand-colour, /* ... */), dark: (/* ... */)))?

    Don't think we need the extra complexity of receiving arguments unless we plan to let users customise parts of the theme (in Sass at least, as they can already override the variables by setting their own values on :root anyway).

Copy link
Contributor

Choose a reason for hiding this comment

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

In order:

  • I'm in favour of private for the reasons you outlined
  • Agree with theme. Whilst color-scheme feels nicer because it's more in line with the spec, as this is covering a much broader scope than just that attribute, I feel like theme is a sensible classification.
  • I take your point on added complexity but if we keep this private then it might be neater for us. I think this question hinges on if our own code is more performant or readable if it's one mixin vs if its 2, or if it doesn't make a difference in which case I think 1 with params saves more space.

@romaricpascal romaricpascal force-pushed the spike-css-custom-properties-user-preference branch from fe2c802 to e920075 Compare April 8, 2024 10:09
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4921 April 8, 2024 10:09 Inactive
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.

Left a couple of comments but overall feel that this spike does the job. I've additionally tested this against chrome and firefox as per your recommendations in the PR description and everything works as expected. I think the hierarchy of system settings -> vendor settings -> class on the page makes sense as a mental model for how to operate a prospective dark mode.

The iframe thing is a bit of a bother but I don't think it's a problem for us to solve right now. As I commented, this seems to be something that vendors themselves are figuring out.

As an aside I think this verifies a similar suggestion Colin put forward for toggling feature flags in the review app which we could take back to that issue.

@@ -0,0 +1,62 @@
@mixin _govuk-theme-light() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In order:

  • I'm in favour of private for the reasons you outlined
  • Agree with theme. Whilst color-scheme feels nicer because it's more in line with the spec, as this is covering a much broader scope than just that attribute, I feel like theme is a sensible classification.
  • I take your point on added complexity but if we keep this private then it might be neater for us. I think this question hinges on if our own code is more performant or readable if it's one mixin vs if its 2, or if it doesn't make a difference in which case I think 1 with params saves more space.

@@ -0,0 +1,62 @@
@mixin _govuk-theme-light() {
color-scheme: light;
Copy link
Contributor

@owenatgov owenatgov Apr 8, 2024

Choose a reason for hiding this comment

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

Anecdata here but the most common users of ours also using iframes seem to be other government design systems that consume govuk-frontend. That will probably need some verification (ask Pay maybe?) but considering it's a fairly small scope, maybe the answer here is to just write guidance advising that it's a thing to be aware of and how to fix it. Part of the 'how to fix it' bit of course is tied to if we should bother offering a solution since vendors are still discussing it.

@romaricpascal romaricpascal marked this pull request as ready for review April 9, 2024 12:15
@romaricpascal romaricpascal merged commit ebbfca6 into spike-css-custom-properties Apr 9, 2024
39 of 45 checks passed
@romaricpascal romaricpascal deleted the spike-css-custom-properties-user-preference branch April 9, 2024 12:38
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.

3 participants