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

[css-color-adjust-1] Spec currently breaks use of currentColor for SVG icons in WHCM #6310

Closed
AmeliaBR opened this issue May 25, 2021 · 9 comments
Labels
a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response. css-color-adjust-1 Current Work SVG

Comments

@AmeliaBR
Copy link
Contributor

Background:
For inline SVG icons that need to match the surrounding text color, the recommended technique is to use fill: currentColor or stroke: currentColor. In addition to being DRY-er than setting an explicit color, this makes sure that the color adjusts to match any forced color changed (e.g., Windows High Contrast Mode).

However, in other SVG, fill colour is part of the graphic and shouldn't automatically be adjusted by high contrast mode.

In #3855, we therefore agreed that forced-color-adjust: none should apply to all SVG, with the understanding that the currentColor technique for inline icons would still work because the inherited color from the surrounding HTML markup would already be adjusted.
https://drafts.csswg.org/css-color-adjust-1/#propdef-forced-color-adjust

However, because of other complications, in #4915 (comment) it was resolved the forced-color changes apply at used value time, which means they don't affect inherited values.
https://drafts.csswg.org/css-color-adjust-1/#forced-colors-properties

Chromium has shipped this change, and therefore web pages that previously had readable SVG icons in high-contrast mode now have broken icons that don't change to match the currentColor.
https://twitter.com/stommepoes/status/1397186894866288647
https://codepen.io/_mallory/pen/qBrjNpK
https://bugs.chromium.org/p/chromium/issues/detail?id=1213073

As an aside: they hypothetical example @alisonmaher brings up in #4915 (comment) doesn't seem like something that would be common (currentColor SVG text on top of an explicitly-coloured SVG background). In contrast, the example CodePen above uses a technique (currentColor for SVG with transparent background embedded within text) that has been widely advocated as the correct way to do accessible inline SVG icons!

@AmeliaBR AmeliaBR added a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response. SVG css-color-adjust-1 Current Work labels May 25, 2021
@AmeliaBR
Copy link
Contributor Author

Since it wasn't entirely clear from the above:

To avoid breaking existing accessible SVGs, currentColor and the system color keywords need to adjust by default for forced-color modes. However, other color values explicitly set on SVG content should not be adjusted.

It is less important if the color value adjusts, since it's not directly used in SVG rendering, but to avoid weird bugs it should probably continue to match currentColor.

The current spec handles the system color keywords as desired, but not currentColor.

There are two ways this could be achieved, that I see:

Given all the past discussion, I would lean towards the second option, adding a forced-color-adjust: color-only value.

@alisonmaher
Copy link
Collaborator

Thanks @AmeliaBR for bringing this one up - I do agree that the SVG icon use case is a more common scenario, and it would be helpful for developers to continue to use currentColor in that case. I like your proposal for a newly added forced-color-adjust: color-only value - it would be a clever way to achieve that behavior without disrupting how everything else is currently being forced, and I think that it would be straightforward to implement.

The only concern that I have is that if an ancestor of an SVG element has forced-color-adjust: none set, and the SVG element by default is set to forced-color-adjust: color-only, we would still end up forcing the color value on the SVG element even though its ancestor is not (which would probably result in an unexpected value for curerrentColor). But perhaps this isn't too concerning given that it's probably not a very likely scenario, and an author could additionally set the SVG to forced-color-adjust: none in this case?

(As a side note: authors can currently set forced-color-adjust: auto to ensure that an SVG icon's color value is forced today, so if for whatever reason we decide to keep the spec as-is, it would probably be a good idea to add an example around this to the spec.)

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-color-adjust-1] Spec currently breaks use of currentColor for SVG icons in WHCM, and agreed to the following:

  • RESOLVED: add color-only value as described in the issue and clarified by TabAtkins
The full IRC log of that discussion <dael> Topic: [css-color-adjust-1] Spec currently breaks use of currentColor for SVG icons in WHCM
<dael> github: https://github.com//issues/6310
<dael> alisonmaher: This is around handling currentColor. AmeliaBR noted that SVG icons will not inherit appropriate forced-color through currentColor.
<dael> alisonmaher: 2 solutions. 1 undo the resolution to use forced colors at used value time. 2) intoduce a color only value that only adjusted color. Make that default for SVGs. b/c color only effects svg through currentColor works well for icon
<TabAtkins> q+
<dael> alisonmaher: Only possible unexpected is an svg ancestor set forced-color to none the svg would still set to currentCOlor. This seems rare so in favor of color-only. Looking for other opinions
<Rossen_> ack TabAtkins
<dael> TabAtkins: Definitely need to fix this. Only consideration to fix issue you raised could the value act as none if inheritied value is none but act as color if it's auto.
<dael> TabAtkins: Then it still works as expected
<dael> alisonmaher: Good idea. Will need to look into how to impl but could be good way to handle
<dael> TabAtkins: Seems reasonable to add, I'm in value
<dael> s/value/favor
<dael> Rossen_: Prop to fix the issue through a spec clarification and not adding color-only value?
<dael> TabAtkins: NO, still add value. The value acts as alisonmaher and AmeliaBR spec in auto case. If inherited is none it would act as none. It would automatically opt-out
<dael> Rossen_: I see
<dael> Rossen_: Other opinions or suggestions?
<dael> Rossen_: alisonmaher do you want a resolution now? Or do you prefer to go back and figure out if implementable?
<dael> alisonmaher: Can resolve and come back if impl is tricky. Seems adding color-only value we can resolve on
<dael> Rossen_: Objections to add color-only value as described in the issue and clarified by TabAtkins
<dael> RESOLVED: add color-only value as described in the issue and clarified by TabAtkins

@fantasai
Copy link
Collaborator

fantasai commented Jun 9, 2021

Reflagging for the agenda, because I don't think we quite got this situation correct.

Consider an SVG where the author has set 'color' to an explicit value on the <SVG> element and is using ''currentColor'' throughout to reference that color. This isn't the case we're trying to solve here: the SVG is not trying to inherit the color from the context. They're using an explicit color, likely in coordination with other colors, and using 'color' as a convenient variable. If we apply forced-colors only to 'color' in this case, and not to the rest of the colors in the SVG, we might end up breaking the SVG.

So to target just the case we need to address (icons wanting to inherit the surrounding context's text color) I think what we really want to do is limit the forced-color application to when 'color' is inherited from the SVG's context, and not to the use of the 'color' property in general. Something like:

preserve-parent-color
If the 'color' property’s specified value is ''currentColor'',
then it computes to the used color of its parent’s 'color' value.
In all other respects, behaves the same as ''none''.

@alisonmaher
Copy link
Collaborator

That's a great point, if an SVG has set a color, currentColor would fail to pick up the correct value using the forced-color-adjust: color-only proposal. So I think scoping this down to only when color is inherited makes sense.

@fantasai I may be misinterpreting the wording, but I was curious how preserve-parent-color would work for the original SVG icon case given the following definition:

preserve-parent-color
If the 'color' property’s specified value is ''currentColor'',
then it computes to the used color of its parent’s 'color' value.
In all other respects, behaves the same as ''none''.

color has a default value of black in Chromium, so if an author doesn't set color: currentColor on an SVG icon, would this work?

I'm also wondering if this were to be done at computed value time, would this re-raise the issue of handling transitions in forced colors mode: #5419?

@melanierichards
Copy link

Was thinking about this a bit more as well, and wanted to flag something for the group.

Adding a value to forced-color-adjust seems like a simple/elegant/implementable solution, and doesn't add much work for authors who are aware of and want their users to have a great experience with forced color modes. What we do lose as of previous resolutions (and don't regain with the current solution) is the ability to propagate the appropriate color across color schemes in cases where the author has not put any care or consideration into forced color modes.

Take this common practice, for example: https://codepen.io/somelaniesaid/pen/Yzqxogg. "Light mode" is the default style, where the SVG parts are set to currentColor. Contextual colors are updated in "dark mode", and propagate automagically to the SVG part. In previous implementations, no work at all is required in forced color modes to pix up the contextual semantic colors. The result of this behavior is that people who use forced color modes can have a better experience with (very common) simple SVGs across more web applications, built by FCM-unaware authors.

Given that SVGs can be used in so many different ways, it might not be possible to find a solution that appropriately balances potential breakages between use cases without adding just a tiny bit more work for authors. Adding a new value might still be the best way to go at this point if it minimizes breakage.* I just want to make sure that in considering the paths forward, we're aware that SVG icons will be broken on some sites given more recent currentColor behaviors, and are likely to stay broken.

* As active members of the CSSWG are probably aware, I pivoted to some other projects before some of the more recent resolutions and haven't been part of the discussions for awhile. So I will to defer to Alison, who is still active in this work and may have data/insights that I don't. :)

@fantasai
Copy link
Collaborator

@melanierichards The idea in this issue was to make this the UA's default value for forced-color-adjust on SVG, instead of none. See @AmeliaBR's original proposal in #6310 (comment)

@melanierichards
Copy link

Oh goodness, seems I misread/missed a very key element of this proposal. 🤦‍♀️ Thanks for pointing that out, @fantasai! Seems this is headed in the right direction then, please ignore my comment.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Color Adjust, and agreed to the following:

  • RESOLVED: Add new keyword to forced-color-adjust as described above, apply it in UA default stylesheet for SVG root element
The full IRC log of that discussion <fantasai> Topic: Color Adjust
<fantasai> github: https://github.com//issues/6310
<fantasai> https://github.com//issues/6310#issuecomment-858112357
<fremy> fantasai: the current proposal is not gonna get us in good shape in all cases
<fremy> fantasai: currently our resolution would not work if the svg has set its color explicitly
<fremy> fantasai: so my new proposal is that if the color is orginating from outside the svg then we recolor, but if not then we keep it
<fremy> fantasai: proposal is to add a new keyword for that magic behavior which depends on the origin of the value of color
<alisonmaher> q+
<iank_> I'm getting very warbled audio - is this is the for others?
<fremy> fantasai: (if you are not inheriting from outside, then we don't reset the color)
<fantasai> alisonmaher: I agree with the proposal
<astearns> ack alisonmaher
<fantasai> alisonmaher: only problem I wanted to ask about was whether we can do at computed value time
<fantasai> alisonmaher: if we take the used value of the color, might expose the color where otherwise wouldn't
<fantasai> TabAtkins: :visited won't be exposed that way
<fantasai> TabAtkins: that's done by selector-hacking
<fantasai> TabAtkins: and the rest of colors are already automatically exposed via getComputedStyle
<fantasai> TabAtkins: because gCS returns used value of color
<Rossen_> q
<fantasai> TabAtkins: so not sure there's info leakage problem
<fantasai> Rossen_: While we're on the topic, wrt TAG review
<fantasai> Rossen_: We convinced ourselves that having a grouping of color values that we essentially return just defaults for, and lie about the computed or used values
<fantasai> Rossen_: colors used for fingerprinting
<fantasai> Rossen_: could be a good path forward
<fantasai> Rossen_: These are magic values, you won't get the actual values though gCS
<fantasai> Rossen_: benefit of user for privacy
<astearns> ack fantasai
<fremy> fantasai: this is a different issue though
<fantasai> fantasai: That's a separate issue, here https://github.com//issues/5710
<fremy> fantasai: in that issue there are further comments there that says why it's probably not possible
<fremy> fantasai: but this is not linked to our current issue here
<fantasai> astearns: If we end up doing anything special for particular sources of colors
<fantasai> astearns: if we add this new mechanism, we'd have to do whatever magic here, too
<fantasai> fantasai: You'd also have to taint Canvas, and a lot of other things, yeah.
<fantasai> astearns: alisonmaher is it enough to note that, whatever protections would end up happening here also?
<fantasai> alisonmaher: We're storing [?] in chrome, so would be possible to do at used-value time
<fremy> fantasai: the issue is that would have to track this down the tree
<fremy> fantasai: and this type of tracking is usually done via inheritance
<fremy> fantasai: I don't think implementations have a special inherit for colors
<fantasai> alisonmaher: We inherit that info separately in Chromium
<fantasai> astearns: So what do we resolve to deal with this
<fremy> fantasai: we need to add a new value
<fantasai> fantasai: We need to a new value to forced-color-adjust
<fantasai> fantasai: as described in https://github.com//issues/6310#issuecomment-858112357
<fantasai> fantasai: We can call it something else, but need something that behaves like that
<fantasai> astearns: New value, not something to add to default?
<fantasai> TabAtkins: Yes, absolutely
<fantasai> TabAtkins: and needs to be set in default UA stylesheet for SVG root element
<fantasai> astearns: exposed to author styles?
<fantasai> TabAtkins: yes; don't want it to be special unspecifiable magic
<fantasai> astearns: So proposed resolution is to add this keyword
<fantasai> astearns: and to add that to the defautl UA stylesheet
<fantasai> RESOLVED: Add new keyword to forced-color-adjust as described above, apply it in UA default stylesheet for SVG root element
<fremy> fantasai: other than this issue
<fantasai> https://github.com/w3c/csswg-drafts/labels/css-color-adjust-1
<fremy> fantasai: we have no other remaining issue for the spec
<fremy> fantasai: so our plan is to make the edit
<fremy> fantasai: publish a new draft
<fremy> fantasai: and ask for last comments before trying to propose to make a recommendation
<fremy> fantasai: so, heads up
<fremy> astearns: and get wide review?
<fremy> fantasai: we already have sent an email (... missed)
<fantasai> https://github.com//issues/5768
<fantasai> in December
<fremy> astearns: sounds like a good plan

36degrees added a commit to alphagov/govuk-frontend that referenced this issue Jul 9, 2021
When forced color mode was introduced in Chrome 89, the default user agent styles for SVGs were set to `forced-color-adjust: none` in line with the CSS Color Adjustment specification at the time [1]. Unfortunately, this means that using currentColor for stroke and fill in SVGs no longer works as expected in forced color mode.

As per the comment in Chromium bug #1164162 [2]:

> This is the result of one of the recent spec changes. The spec was updated to force colors at used value time rather than computed value time, which means elements that have "forced-color-adjust: none" set (like svg elements) will inherit their non-forced color values, resulting in a different currentcolor used for stroke and fill in this case than you would get if forcing was done at computed value time.
>
> See spec issue resolution for more details: w3c/csswg-drafts#4915

It looks like this has since been flagged as an issue with the CSS working group [3] and the spec has been updated to resolve it [4] but it’s going to take a while before that change is reflected in browsers.

In the meantime, we can explicitly set `forced-color-adjust: auto` on the OGL logo in order for it to correctly inherit the color from the parent [5].

Once Chromium has been updated so that the default UA styles for SVGs use the new `forced-color-adjust: preserve-parent-color` keyword, and traffic to older versions has dropped off, we can then consider removing this.

[1]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210520/#forced-color-adjust-prop
[2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1164162#c4
[3]: w3c/csswg-drafts#6310
[4]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210616/#forced-color-adjust-prop
[5]: w3c/csswg-drafts#6310 (comment)
36degrees added a commit to alphagov/govuk-frontend that referenced this issue Jul 9, 2021
When forced color mode was introduced in Chrome 89, the default user agent styles for SVGs were set to `forced-color-adjust: none` in line with the CSS Color Adjustment specification at the time [1]. Unfortunately, this means that using currentColor for stroke and fill in SVGs no longer works as expected in forced color mode.

As per the comment in Chromium bug #1164162 [2]:

> This is the result of one of the recent spec changes. The spec was updated to force colors at used value time rather than computed value time, which means elements that have "forced-color-adjust: none" set (like svg elements) will inherit their non-forced color values, resulting in a different currentcolor used for stroke and fill in this case than you would get if forcing was done at computed value time.
>
> See spec issue resolution for more details: w3c/csswg-drafts#4915

It looks like this has since been flagged as an issue with the CSS working group [3] and the spec has been updated to resolve it [4] but it’s going to take a while before that change is reflected in browsers.

In the meantime, we can explicitly set `forced-color-adjust: auto` on the OGL logo in order for it to correctly inherit the color from the parent [5].

Once Chromium has been updated so that the default UA styles for SVGs use the new `forced-color-adjust: preserve-parent-color` keyword, and traffic to older versions has dropped off, we can then consider removing this.

[1]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210520/#forced-color-adjust-prop
[2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1164162#c4
[3]: w3c/csswg-drafts#6310
[4]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210616/#forced-color-adjust-prop
[5]: w3c/csswg-drafts#6310 (comment)
36degrees added a commit to alphagov/govuk-frontend that referenced this issue Jul 15, 2021
When forced color mode was introduced in Chrome 89, the default user agent styles for SVGs were set to `forced-color-adjust: none` in line with the CSS Color Adjustment specification at the time [1]. Unfortunately, this means that using currentColor for stroke and fill in SVGs no longer works as expected in forced color mode.

As per the comment in Chromium bug #1164162 [2]:

> This is the result of one of the recent spec changes. The spec was updated to force colors at used value time rather than computed value time, which means elements that have "forced-color-adjust: none" set (like svg elements) will inherit their non-forced color values, resulting in a different currentcolor used for stroke and fill in this case than you would get if forcing was done at computed value time.
>
> See spec issue resolution for more details: w3c/csswg-drafts#4915

It looks like this has since been flagged as an issue with the CSS working group [3] and the spec has been updated to resolve it [4] but it’s going to take a while before that change is reflected in browsers.

In the meantime, we can explicitly set `forced-color-adjust: auto` on the chevron SVG in order for it to correctly inherit the color from the parent [5]. This mimics the fix for the OGL logo in the footer made in 850c0b7.

Once Chromium has been updated so that the default UA styles for SVGs use the new `forced-color-adjust: preserve-parent-color` keyword, and traffic to older versions has dropped off, we can then consider removing this.

[1]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210520/#forced-color-adjust-prop
[2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1164162#c4
[3]: w3c/csswg-drafts#6310
[4]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210616/#forced-color-adjust-prop
[5]: w3c/csswg-drafts#6310 (comment)
36degrees added a commit to alphagov/govuk-frontend that referenced this issue Jul 15, 2021
When forced color mode was introduced in Chrome 89, the default user agent styles for SVGs were set to `forced-color-adjust: none` in line with the CSS Color Adjustment specification at the time [1]. Unfortunately, this means that using currentColor for stroke and fill in SVGs no longer works as expected in forced color mode.

As per the comment in Chromium bug #1164162 [2]:

> This is the result of one of the recent spec changes. The spec was updated to force colors at used value time rather than computed value time, which means elements that have "forced-color-adjust: none" set (like svg elements) will inherit their non-forced color values, resulting in a different currentcolor used for stroke and fill in this case than you would get if forcing was done at computed value time.
>
> See spec issue resolution for more details: w3c/csswg-drafts#4915

It looks like this has since been flagged as an issue with the CSS working group [3] and the spec has been updated to resolve it [4] but it’s going to take a while before that change is reflected in browsers.

In the meantime, we can explicitly set `forced-color-adjust: auto` on the chevron SVG in order for it to correctly inherit the color from the parent [5]. This mimics the fix for the OGL logo in the footer made in 850c0b7.

Once Chromium has been updated so that the default UA styles for SVGs use the new `forced-color-adjust: preserve-parent-color` keyword, and traffic to older versions has dropped off, we can then consider removing this.

[1]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210520/#forced-color-adjust-prop
[2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1164162#c4
[3]: w3c/csswg-drafts#6310
[4]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210616/#forced-color-adjust-prop
[5]: w3c/csswg-drafts#6310 (comment)
36degrees added a commit to alphagov/govuk-frontend that referenced this issue Jul 16, 2021
When forced color mode was introduced in Chrome 89, the default user agent styles for SVGs were set to `forced-color-adjust: none` in line with the CSS Color Adjustment specification at the time [1]. Unfortunately, this means that using currentColor for stroke and fill in SVGs no longer works as expected in forced color mode.

As per the comment in Chromium bug #1164162 [2]:

> This is the result of one of the recent spec changes. The spec was updated to force colors at used value time rather than computed value time, which means elements that have "forced-color-adjust: none" set (like svg elements) will inherit their non-forced color values, resulting in a different currentcolor used for stroke and fill in this case than you would get if forcing was done at computed value time.
>
> See spec issue resolution for more details: w3c/csswg-drafts#4915

It looks like this has since been flagged as an issue with the CSS working group [3] and the spec has been updated to resolve it [4] but it’s going to take a while before that change is reflected in browsers.

In the meantime, we can explicitly set `forced-color-adjust: auto` on the chevron SVG in order for it to correctly inherit the color from the parent [5]. This mimics the fix for the OGL logo in the footer made in 850c0b7.

Once Chromium has been updated so that the default UA styles for SVGs use the new `forced-color-adjust: preserve-parent-color` keyword, and traffic to older versions has dropped off, we can then consider removing this.

[1]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210520/#forced-color-adjust-prop
[2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1164162#c4
[3]: w3c/csswg-drafts#6310
[4]: https://www.w3.org/TR/2021/WD-css-color-adjust-1-20210616/#forced-color-adjust-prop
[5]: w3c/csswg-drafts#6310 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response. css-color-adjust-1 Current Work SVG
Projects
None yet
Development

No branches or pull requests

5 participants