-
Notifications
You must be signed in to change notification settings - Fork 334
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
Fix GOV.UK logo disappearing on light background in Windows High Contrast Mode #2237
Conversation
// But remove it if there's nothing after the logo to keep hover and focus | ||
// states neat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it wasn't changed in this PR, but this comment is kind of confusing now that there is stuff between setting and removing margin-right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch. I think there's a couple of options…
We could move the color adjust properties to after the pseudo-selector. However, I think we generally try and avoid having nested selectors in the middle of a ruleset.
.govuk-header__logotype {
display: inline-block;
// Add a gap between logo and any product name
margin-right: govuk-spacing(1);
// But remove it if there's nothing after the logo to keep hover and focus
// states neat
&:last-child {
margin-right: 0;
}
// Prevent readability backplate from obscuring underline in Windows
// High Contrast Mode
forced-color-adjust: none;
@media (forced-colors: active) {
color: linktext;
}
}
We could also just update the comment to make it less confusing, but the two related declarations are still a little divorced from each other:
.govuk-header__logotype {
display: inline-block;
// Add a gap after the logo in case it's followed by a product name. This
// gets removed later if the logotype is a :last-child.
margin-right: govuk-spacing(1);
// Prevent readability backplate from obscuring underline in Windows
// High Contrast Mode
forced-color-adjust: none;
@media (forced-colors: active) {
color: linktext;
}
// Remove the gap after the logo if there's no product name to keep hover
// and focus states neat
&:last-child {
margin-right: 0;
}
}
Or… something else I haven't thought of.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second option looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the rule about nested declarations purely a stylistic thing, or does the location affect the generated CSS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, it's just a stylistic thing – specifically, it's easy to miss any additional rules that come after the nested selectors, as it's easy to assume we've 'finished' setting the rules for the element and have moved on to it's children / pseudo-selectors.
I'm not sure if this is worth fixing, but I tested this with IE11 and the visited state had a different colour to the unvisited state 🤔 |
I think that's probably fine, and the behaviour in browsers other than IE matches the non-HCM version where the logo in the header does not have a distinct visited state either. |
In 6f90027 I updated the logo in the header to use `forced-color-adjust: none;` in order to fix an issue where the readability backplate obscured the underline on the link. However, I neglected to then handle the color adjustment correctly – the logo is now always white in high contrast mode, regardless of the user’s colour preferences. This means that the logo can be invisible or hard to perceive when the user’s colour scheme has a lighter background. To fix this, set the colour of the logo to the system color `linktext` when forced colors are active, which means it will appear in the user’s preferred colour for unvisited links, matching the appearance of the other links in the header.
These comments only really make sense when one immediately follows the other, but since the rules around forced colors were added they’ve been separated. Tweak the comments so they work when each rule stands alone.
6b578c2
to
cf53b67
Compare
In 6f90027 I updated the logo in the header to use
forced-color-adjust: none;
in order to fix an issue where the readability backplate obscured the underline on the link.However, I neglected to then handle the color adjustment correctly – the logo is now always white in high contrast mode, regardless of the user’s colour preferences. This means that the logo can be invisible or hard to perceive when the user’s colour scheme has a lighter background.
To fix this, set the colour of the logo to the system color
linktext
when forced colors are active, which means it will appear in the user’s preferred colour for unvisited links, matching the appearance of the other links in the header.This only affects Chrome and Edge (and presumably Opera as that's also built on Chromium, but that's not available to test with in Assistiv Labs) – as Firefox doesn't support
forced-color-adjust
.