Page MenuHomePhabricator

Make CSS-only icons compatible with CSS variables
Closed, ResolvedPublic5 Estimated Story Points

Description

Summary

When running the sandbox (npm run dev) for https://gerrit.wikimedia.org/r/c/design/codex/+/993056 , CSS-only icons are broken:

image.png (134×308 px, 9 KB)

image.png (290×235 px, 13 KB)

The icons appear as pure black (#000), but they should be showing as off white (#f8f9fa) since that is the value of var( --color-base ) in this demo.

Potential solutions

This appears to happen because CSS variables don't work in data URI SVGs like this. We'll need to find a different way to do change the color:

  • We can't just use currentColor for the SVG fill because there's no way for the URL to know what the currentColor is
  • We could use mask-image and background-color, which is what we currently do for CSS icons within buttons. This would allow us to change the color outside of the SVG URL (via background-color). However, mask-image is not supported in Firefox v39-52, which are in our basic support tier. In those browsers, for icons within buttons, we just set the icon to either @color-base or white, whichever has more contrast. We'd need to do something similar for all icons to ensure a visible icon in old Firefox versions.

Acceptance criteria

  • Change the CSS icon mixin so that tokens that are CSS variables work for setting the icon color
  • Ensure this works when you pass in a raw CSS variable to the CSS icon mixin (instead of a Less token)
  • Ensure this reaches an acceptable level of support in all of our supported browsers

Event Timeline

Change 997983 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] Use mask-image for all CSS icons

https://gerrit.wikimedia.org/r/997983

AnneT changed the task status from Open to In Progress.EditedFeb 6 2024, 11:47 PM

We can't use fill="currentColor" for the same reason that we can't use a CSS variable - the URL has no idea of its context so currentColor doesn't meant anything to it.

We can use the mask-image technique. This will degrade the experience for users of old versions of Firefox, however, potentially more than they already experience with icons within buttons. At least in those cases, we set the icon to either @color-base or white, depending on which contrasts more, because we reliably know the background color the icon is contrasting with. For icons outside of buttons, we will need to devise a reliable way to determine if the icon should be dark or light. Either way, users with old versions of Firefox will no longer see any icon colors other than the ones we handle in the mixin (so no more progressive or destructive icon colors, for example).

I've opened a patch starting this work, but it doesn't resolve the issue I just described, and needs some cleanup.

Volker_E subscribed.

A bit of a concern might be, after doing the obligatory look at caniuse mask image, were the mobile browser support landscape with prefixed values necessary (Android) and Firefox mobile just supporting this since Jan 2024. We should have a closer look at these platforms here. CSS-only speaks to basic support in my conviction in our environment.
Is a color-base destructive icon-only button sufficient?

Firefox mobile just supporting this since Jan 2024.

I'm not sure what's up with the caniuse chart, but according to MDN it's been supported in FF for Android since v53 in 2017.

CSS-only speaks to basic support in my conviction in our environment.

I agree. My interpretation of this for when I originally built the CSS icon mixin and needed to handle icons within buttons was that all users need to be able to clearly see the icon, but it didn't necessarily need to be the right color. So we landed on ensuring that the icon had appropriate contrast depending on the button's weight and state, which can change the background color. But this is open for interpretation, especially now that we're considering it for all icons.

Is a color-base destructive icon-only button sufficient?

This is how it currently functions - if we think it's necessary to change the color of CSS-only icons in buttons in old browsers, we should open a new task to handle that. For this task, I'm concerned about icons outside of buttons, because the solution to the issue defined in the task is probably to use mask-image for all CSS icons, rather than just the ones in buttons. What level of support do we need to reach for users of browsers that don't support mask-image for all icons? These are the potential levels of support I've thought of:

  1. Showing all icons in @color-base
  2. Showing all icons either in @color-base or white (or something close to white), depending on which has more contrast
  3. Supporting @color-base, white, @color-progressive, and @color-destructive only
  4. Supporting any color (including custom colors set by the dev user)

Anything beyond level 1 will require figuring out how to determine which color needs to be used in the mixin code so we can hardcode the hex value for the SVG background image. I think we should try to balance providing basic support for users of browsers that don't support mask image with avoiding a huge amount of additional complexity in this already complex code.

I think that this is another situation where we could update our "basic support" baseline somewhat and avoid having to resort to complex workarounds and hacks.

Right now Grade C support includes FF version 39 (from 2014). How many users are we losing if we bump this to v53 (2017)?

The introduction of color themes and night mode represents a major change in our UI, it makes sense that supporting this would require an across-the-board update to our browser compatibility baseline.

Maybe we need some kind of very basic "legacy skin" (which contains minimal styling, and simply ensures that content is visible) that we can serve to users on really old browsers in order to free us up to pursue a more modern solution for everyone else.

Change 998451 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Revert "Color subtle should be a CSS variable"

https://gerrit.wikimedia.org/r/998451

Change 998451 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Revert "Color subtle should be a CSS variable"

https://gerrit.wikimedia.org/r/998451

CSS-only speaks to basic support in my conviction in our environment.

I agree. My interpretation of this for when I originally built the CSS icon mixin and needed to handle icons within buttons was that all users need to be able to clearly see the icon, but it didn't necessarily need to be the right color. So we landed on ensuring that the icon had appropriate contrast depending on the button's weight and state, which can change the background color. But this is open for interpretation, especially now that we're considering it for all icons.

I still think this interpretation holds and would be fine to apply to all icons.

Is a color-base destructive icon-only button sufficient?

This is how it currently functions - if we think it's necessary to change the color of CSS-only icons in buttons in old browsers, we should open a new task to handle that. For this task, I'm concerned about icons outside of buttons, because the solution to the issue defined in the task is probably to use mask-image for all CSS icons, rather than just the ones in buttons. What level of support do we need to reach for users of browsers that don't support mask-image for all icons? These are the potential levels of support I've thought of:

  1. Showing all icons in @color-base
  2. Showing all icons either in @color-base or white (or something close to white), depending on which has more contrast
  3. Supporting @color-base, white, @color-progressive, and @color-destructive only
  4. Supporting any color (including custom colors set by the dev user)

Anything beyond level 1 will require figuring out how to determine which color needs to be used in the mixin code so we can hardcode the hex value for the SVG background image. I think we should try to balance providing basic support for users of browsers that don't support mask image with avoiding a huge amount of additional complexity in this already complex code.

Per @egardner's comment above, we can also try to bump up the browser versions in Grade C, but I don't think that should hold up this task. Level 1 seems perfectly acceptable as color should not be the primary means of conveying information.

Per @egardner's comment above, we can also try to bump up the browser versions in Grade C, but I don't think that should hold up this task. Level 1 seems perfectly acceptable as color should not be the primary means of conveying information.

I'm afraid this won't be sufficient when an icon is placed on a dark background, either because it's in night mode or because the background is dark for another reason. So I think we will need to go for at least level 2. I've implemented a way to do this proposed by Roan in my patch. I personally think level 2 is fine, especially if we can drop support for browsers that don't support mask-image soonish.

Per @egardner's comment above, we can also try to bump up the browser versions in Grade C, but I don't think that should hold up this task. Level 1 seems perfectly acceptable as color should not be the primary means of conveying information.

I'm afraid this won't be sufficient when an icon is placed on a dark background, either because it's in night mode or because the background is dark for another reason. So I think we will need to go for at least level 2. I've implemented a way to do this proposed by Roan in my patch. I personally think level 2 is fine, especially if we can drop support for browsers that don't support mask-image soonish.

Oh, I think I misunderstood then. I was thinking that @color-base would also get inverted for night mode. Level 2 sounds fine as well.

Oh, I think I misunderstood then. I was thinking that @color-base would also get inverted for night mode. Level 2 sounds fine as well.

Yeah sorry, that's a confusing thing about the icon mixin - we can't rely on the actual variable @color-base because it's going to be a CSS custom property, which won't work as a fill color in the background image URL. So instead we have to hardcode a color hex value in the mixin.

Change 997983 merged by jenkins-bot:

[design/codex@main] Icon, styles: Use mask-image for all CSS icons

https://gerrit.wikimedia.org/r/997983

Change 1005171 had a related patch set uploaded (by VolkerE; author: VolkerE):

[mediawiki/core@master] Update Codex from v1.3.2 to v1.3.3

https://gerrit.wikimedia.org/r/1005171

Change 1005171 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v1.3.2 to v1.3.3

https://gerrit.wikimedia.org/r/1005171