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-4] Make system color keywords compute to themselves #3847

Closed
tabatkins opened this issue Apr 18, 2019 · 12 comments
Closed

[css-color-4] Make system color keywords compute to themselves #3847

tabatkins opened this issue Apr 18, 2019 · 12 comments

Comments

@tabatkins
Copy link
Member

In order to make system colors reflect the color-scheme, which can change throughout the tree, we need these colors to be able to inherit as keywords whose used value can change depending on color-scheme. (For Web-compatibility, however, they will need to still have a resolved value of a particular rgb value.) So we need to update css-color-4 to say that they compute to themselves, but that their resolved value is an rgb value.

@tabatkins tabatkins added the css-color-4 Current Work label Apr 18, 2019
aarongable pushed a commit to chromium/chromium that referenced this issue Aug 26, 2019
System colors should resolve differently based on the used color-scheme
value. See [1]. Ideally they should compute to the keyword to make
inheritance work and have different used values for system colors based
on color-scheme, but that needs to be resolved[2].

System colors are used for forms rendering. This CL is preparing for
color-scheme specific forms rendering.

Some places we use initial color-scheme in contexts where we have no
ComputedStyle. These are typically offscreen canvas not connected to a
particular element. It's possible we should use the root element
computed value instead.

The color interpolation needs to be fixed and I've added a TODO. Will
follow up in another CL.

No tests as there are no behavioral changes.

[1] w3c/csswg-drafts#3804
[2] w3c/csswg-drafts#3847

Bug: 929098
Change-Id: Ie734e399c0dba82c59b221222c8e7ef365cae79b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1768139
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690375}
aarongable pushed a commit to chromium/chromium that referenced this issue Aug 27, 2019
We currently store a bool saying if the StyleColor represents
currentcolor or not since currentcolor computes to itself and will
inherit as currentcolor. If [1] is resolved, we need to do the same for
system colors. Store a CSSValueID in preparation for that.

This CL should not have any behavioral changes.

[1] w3c/csswg-drafts#3847

Bug: 939811
Change-Id: I5908a09b311c549c9d194bdf4180e24ba7baae5c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1771413
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690897}
aarongable pushed a commit to chromium/chromium that referenced this issue Sep 4, 2019
We currently compute system colors as their rgba values based on
color-scheme. However, color-scheme was not a high priority property, so
changing color-scheme on the same element as the system color was set
did not always yield the correct result as the color-scheme might have
been applied too late.

There is an open issue[1] for having system colors computing to
themselves and resolve to an rgba value for the used value. That would
mean we will have to change the internal storage for colors in
ComputedStyle and possibly store a StyleColor with system color keywords
in addition to currentcolor which currently computes to the keyword.

For now, instead of introducing an even higher priority level, make
color-scheme a high priority property, and delay application of the
cascaded color property value to the ComputedStyle until we have applied
other high priority properties.

[1] w3c/csswg-drafts#3847

Bug: 939811
Change-Id: I91ce04a55886507ee20aace9b2470a5ac5b43fbd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1773073
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#693151}
@upsuper
Copy link
Member

upsuper commented Oct 30, 2019

It sounds this would make color animation more complicated. You may need to be able to express a computed color like 10% currentcolor + 20% Canvas + 30% Text + ... + 5% red, which... doesn't sound fun.

@heycam
Copy link
Contributor

heycam commented Oct 30, 2019

Either that or we wouldn't support interpolation between system colors.

@heycam
Copy link
Contributor

heycam commented Oct 30, 2019

Or we have special rules to define that animations between colors use their resolved values.

@svgeesus
Copy link
Contributor

Or we have special rules to define that animations between colors use their resolved values.

That seems preferable, given the whole forced colors thing.

@svgeesus
Copy link
Contributor

So we need to update css-color-4 to say that they compute to themselves, but that their resolved value is an rgb value

That seemed straightforward to me when I first read this issue, and was about to just add it (but I hadn't considered animation when I thought that)

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Oct 30, 2019

Any impacts regarding animations also affect currentColor as an interpolation end point, since currentColor is also spec'd to compute/inherit as a keyword. All browsers currently support animating currentColor, as far as I know, so we can't break that. Firefox also successfully inherits it as a keyword, even when animating, but Chrome (and I think WebKit) do not.

Test: https://codepen.io/AmeliaBR/pen/VwwMEqy?editors=1100

Note that, in Firefox, the inherited background color on the div animates between currentColor and tan, even though it has a different currentColor (hotPink) than the parent which defines the animation (alternating navy and red).

Or we have special rules to define that animations between colors use their resolved values.

I'm not sure how this would work for the cases in the demo, where the reference colors change during the animation, and where the animated value is inherited.

I'd rather just jump ahead and introduce an interpolation function to describe the intermediary computed state of interpolate(50%, currentColor, tan).

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Make system color keywords compute to themselves, and agreed to the following:

  • RESOLVED: Make system color keywords behave like currentColor and inherit as keywords
The full IRC log of that discussion <dael> Topic: Make system color keywords compute to themselves
<dael> github: https://github.com//issues/3847
<dael> TabAtkins: Back in the day system colros could become rgb at computed value time. Now that we can switch user themes using color-scheme you can turn it on and off at subtree. That changes what system colors resolve to
<dael> TabAtkins: Thus they should act like currentColor
<dael> TabAtkins: Talked to our impl and it's fine
<dael> chris: Issue about animation, but can animate currentColor
<dael> AmeliaBR: Don't have interop on currentColor
<dael> AmeliaBR: As far as making system color keywords inherit as keywords we want them same as currentColor. We just need impl to go through details of making currentColor and these inherit as keywords
<dael> AmeliaBR: Right now FF does it. Chrome and I think WK do not
<dael> TabAtkins: Chrome appears to. I tested yesterday. We're doing it sufficiently close to look correct in a test
<dael> AmeliaBR: DOesn't when you animate or you're newer chromium version
<dael> TabAtkins: Might be jsut passin gmy straightforward test
<tantek> interesting, thought we had enough currentColor tests to get interop. curious about the test cases that demonstrate non-interop
<fantasai> dbaron, getComputedStyle would return a resolved color
<dael> AmeliaBR: Complexities with animations; if we still agree currentColor spec is right those same rules would apply to making system colors inherit as keywords
<dael> chris: Makes sense to me
<dael> myles: Does that mean fingerprinting is no longer necessary?
<dael> TabAtkins: Nope because they always show as rgb in gCS
<dbaron> s/fingerprinting/fingerprinting stuff we just resolved on/
<dael> TabAtkins: THat's hard compat requirements, people have been relying on it
<dael> chris: Need animation on current and system colors?
<dael> TabAtkins: Sure
<AmeliaBR> s/animation/animation tests/
<dael> AmeliaBR: With the cavaet that we need more tests, any obj to making system color keywords behave like currentColor and inherit as keywords?
<tantek> inherit as keywords makes sense to me
<dael> RESOLVED: Make system color keywords behave like currentColor and inherit as keywords
<fantasai> +1, we need this for them to behave correctly in the presence of forced-color-adjust / color-scheme

@fantasai
Copy link
Collaborator

Fixed in 1263778 however there's an open question of how the deprecated system colors should be handled; tagged that to #3873.

@upsuper
Copy link
Member

upsuper commented Oct 30, 2019

So what's the solution for animation? I think Firefox and Chrome both support transition with currentcolor (as far as I tested previously), not sure what is non-interop here. Requiring to animate resolved value would certainly break that. Not sure whether that's important, though.

@AmeliaBR
Copy link
Contributor

The solution for animation is "do what Firefox does for currentColor", at least as far as rendering goes.

But you're right: when it comes to serialization / TypedOM representation of the mid-animation computed value (not resolved value), I think we do still need a syntax for describing the interpolation between two keyword computed color values.

Maybe that should be a new issue?

@upsuper
Copy link
Member

upsuper commented Oct 30, 2019

What I meant to say in #3847 (comment) is that Firefox's approach is not scalable to more keywords. It was done under the assumption that only currentcolor would behave that way. It's not touching serialization / Typed OM, but about the fact that animating with multiple parts which can change during animation could be challenging.

But I might be wrong because most complexity of animating currentcolor comes from the fact that color itself can animate as well, so there might be some smart way to handle all the system color keywords which can only discretely switch (AFAIU) without adding too much complexity to implementation, but I'm not sure.

Serialization / Typed OM is indeed something we need to consider, but I don't think that has or would introduce inherent complexity as far as we allow only linear combination of colors and keywords. But animation may have such complexity by itself.

@upsuper
Copy link
Member

upsuper commented Oct 31, 2019

Hmmm... color can have system color keyword as well... That may or may not make the situation more complicated...

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 30, 2021
…re. r=dholbert

For that, add `.dark` version of the browser.display* prefs that control
the light version of these colors.

The default for background/foreground colors are taken from the
GenericDarkColors used in LookAndFeel.

The defaults for links are based on this discussion:

  whatwg/html#5426 (comment)

(So they effectively match Chrome).

Whether the dark colors should be exposed in about:preferences (like the
light colors are) is TBD.

With this patch, we pass all the tests in:

  /html/semantics/document-metadata/the-meta-element/color-scheme/

Use the colors to paint the default canvas background and the default
colors.

There are three "regressions", though they are really progressions: we
now render the reference as the test expects (before we rendered a light
canvas background even for the reference).

Apart of these iframe tests (which we should look into, I filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1738380), there are three
remaining test failures.

Two of them are due to `color: initial` not changing based on the
color-scheme. Safari also fails these tests, and the thing they're
really testing is whether system colors are preserved at computed-value
time:

  w3c/csswg-drafts#3847

Regarding that change, I'm not so sure the trade-offs there are worth
it, as that not only complicates interpolation (we wouldn't be able to
use system colors in color-mix among others, see
w3c/csswg-drafts#5780) plus it changes
inheritance behavior in sorta unexpected ways, see:

  w3c/csswg-drafts#6773

Which I just filed because apparently no browser implements this
correctly. So for now will punt on those (keep matching Safari).

There's an svg-as-image test:

  https://searchfox.org/mozilla-central/rev/f8576fec48d866c5f988baaf1fa8d2f8cce2a82f/testing/web-platform/tests/css/css-color-adjust/rendering/dark-color-scheme/svg-as-image.html

Which isn't using the feature at all and I'm not sure why is it supposed
to pass (why prefers-color-scheme: dark is supposed to match that SVG
image). This test fails in all browsers apparently:

  https://wpt.fyi/results/css/css-color-adjust/rendering/dark-color-scheme/svg-as-image.html?label=master&label=experimental&aligned

I sent web-platform-tests/wpt#31407 to remove
it and hopefully get it reviewed by some Chromium folks.

Differential Revision: https://phabricator.services.mozilla.com/D129746
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Nov 1, 2021
…re. r=dholbert

For that, add `.dark` version of the browser.display* prefs that control
the light version of these colors.

The default for background/foreground colors are taken from the
GenericDarkColors used in LookAndFeel.

The defaults for links are based on this discussion:

  whatwg/html#5426 (comment)

(So they effectively match Chrome).

Whether the dark colors should be exposed in about:preferences (like the
light colors are) is TBD.

With this patch, we pass all the tests in:

  /html/semantics/document-metadata/the-meta-element/color-scheme/

Use the colors to paint the default canvas background and the default
colors.

There are three "regressions", though they are really progressions: we
now render the reference as the test expects (before we rendered a light
canvas background even for the reference).

Apart of these iframe tests (which we should look into, I filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1738380), there are three
remaining test failures.

Two of them are due to `color: initial` not changing based on the
color-scheme. Safari also fails these tests, and the thing they're
really testing is whether system colors are preserved at computed-value
time:

  w3c/csswg-drafts#3847

Regarding that change, I'm not so sure the trade-offs there are worth
it, as that not only complicates interpolation (we wouldn't be able to
use system colors in color-mix among others, see
w3c/csswg-drafts#5780) plus it changes
inheritance behavior in sorta unexpected ways, see:

  w3c/csswg-drafts#6773

Which I just filed because apparently no browser implements this
correctly. So for now will punt on those (keep matching Safari).

There's an svg-as-image test:

  https://searchfox.org/mozilla-central/rev/f8576fec48d866c5f988baaf1fa8d2f8cce2a82f/testing/web-platform/tests/css/css-color-adjust/rendering/dark-color-scheme/svg-as-image.html

Which isn't using the feature at all and I'm not sure why is it supposed
to pass (why prefers-color-scheme: dark is supposed to match that SVG
image). This test fails in all browsers apparently:

  https://wpt.fyi/results/css/css-color-adjust/rendering/dark-color-scheme/svg-as-image.html?label=master&label=experimental&aligned

I sent web-platform-tests/wpt#31407 to remove
it and hopefully get it reviewed by some Chromium folks.

Differential Revision: https://phabricator.services.mozilla.com/D129746
Loirooriol pushed a commit to Loirooriol/servo that referenced this issue May 30, 2023
For that, add `.dark` version of the browser.display* prefs that control
the light version of these colors.

The default for background/foreground colors are taken from the
GenericDarkColors used in LookAndFeel.

The defaults for links are based on this discussion:

  whatwg/html#5426 (comment)

(So they effectively match Chrome).

Whether the dark colors should be exposed in about:preferences (like the
light colors are) is TBD.

With this patch, we pass all the tests in:

  /html/semantics/document-metadata/the-meta-element/color-scheme/

Use the colors to paint the default canvas background and the default
colors.

There are three "regressions", though they are really progressions: we
now render the reference as the test expects (before we rendered a light
canvas background even for the reference).

Apart of these iframe tests (which we should look into, I filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1738380), there are three
remaining test failures.

Two of them are due to `color: initial` not changing based on the
color-scheme. Safari also fails these tests, and the thing they're
really testing is whether system colors are preserved at computed-value
time:

  w3c/csswg-drafts#3847

Regarding that change, I'm not so sure the trade-offs there are worth
it, as that not only complicates interpolation (we wouldn't be able to
use system colors in color-mix among others, see
w3c/csswg-drafts#5780) plus it changes
inheritance behavior in sorta unexpected ways, see:

  w3c/csswg-drafts#6773

Which I just filed because apparently no browser implements this
correctly. So for now will punt on those (keep matching Safari).

There's an svg-as-image test:

  https://searchfox.org/mozilla-central/rev/f8576fec48d866c5f988baaf1fa8d2f8cce2a82f/testing/web-platform/tests/css/css-color-adjust/rendering/dark-color-scheme/svg-as-image.html

Which isn't using the feature at all and I'm not sure why is it supposed
to pass (why prefers-color-scheme: dark is supposed to match that SVG
image). This test fails in all browsers apparently:

  https://wpt.fyi/results/css/css-color-adjust/rendering/dark-color-scheme/svg-as-image.html?label=master&label=experimental&aligned

I sent web-platform-tests/wpt#31407 to remove
it and hopefully get it reviewed by some Chromium folks.

Differential Revision: https://phabricator.services.mozilla.com/D129746
Loirooriol pushed a commit to Loirooriol/servo that referenced this issue May 30, 2023
For that, add `.dark` version of the browser.display* prefs that control
the light version of these colors.

The default for background/foreground colors are taken from the
GenericDarkColors used in LookAndFeel.

The defaults for links are based on this discussion:

  whatwg/html#5426 (comment)

(So they effectively match Chrome).

Whether the dark colors should be exposed in about:preferences (like the
light colors are) is TBD.

With this patch, we pass all the tests in:

  /html/semantics/document-metadata/the-meta-element/color-scheme/

Use the colors to paint the default canvas background and the default
colors.

There are three "regressions", though they are really progressions: we
now render the reference as the test expects (before we rendered a light
canvas background even for the reference).

Apart of these iframe tests (which we should look into, I filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1738380), there are three
remaining test failures.

Two of them are due to `color: initial` not changing based on the
color-scheme. Safari also fails these tests, and the thing they're
really testing is whether system colors are preserved at computed-value
time:

  w3c/csswg-drafts#3847

Regarding that change, I'm not so sure the trade-offs there are worth
it, as that not only complicates interpolation (we wouldn't be able to
use system colors in color-mix among others, see
w3c/csswg-drafts#5780) plus it changes
inheritance behavior in sorta unexpected ways, see:

  w3c/csswg-drafts#6773

Which I just filed because apparently no browser implements this
correctly. So for now will punt on those (keep matching Safari).

There's an svg-as-image test:

  https://searchfox.org/mozilla-central/rev/f8576fec48d866c5f988baaf1fa8d2f8cce2a82f/testing/web-platform/tests/css/css-color-adjust/rendering/dark-color-scheme/svg-as-image.html

Which isn't using the feature at all and I'm not sure why is it supposed
to pass (why prefers-color-scheme: dark is supposed to match that SVG
image). This test fails in all browsers apparently:

  https://wpt.fyi/results/css/css-color-adjust/rendering/dark-color-scheme/svg-as-image.html?label=master&label=experimental&aligned

I sent web-platform-tests/wpt#31407 to remove
it and hopefully get it reviewed by some Chromium folks.

Differential Revision: https://phabricator.services.mozilla.com/D129746
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants