760345 - change 'currentColor' keyword to be a computed value (except for 'color' property)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[David Baron :dbaron:]

At the Hamburg Face-to-Face meeting earlier this month the CSS WG agreed to change currentColor so that it's a computed value (by changing the computed value lines of the properties that accept it).

We should change our implementation to match.

This is easy for:
 * border-color
 * outline-color
 * column-rule-color
 * text-decoration-color
since we already have an implementation of this concept under a different name.  It'll require a drop more work for things already stored in some sort of union or structure:
 * fill
 * stroke
 * gradient color stops
 * shadow colors
and a little more still for the things we just store as nscolor:
 * background-color 
 *-moz-border-*-colors
 * flood-color
 * lighting-color
 * stop-color

This is a pretty useful simplification for the first four properties, though a bit more work for the others.

Might want to wait until after bug 629882 lands.

Comment 1

[David Baron :dbaron:]

The minutes from the meeting were:
http://lists.w3.org/Archives/Public/www-style/2012May/0541.html
and point to:
http://www.w3.org/Style/CSS/Tracker/issues/244
which in turn points to:
http://lists.w3.org/Archives/Public/www-style/2012Jan/0521.html
http://lists.w3.org/Archives/Public/www-style/2012Jan/0936.html

Comment 3

[David Baron :dbaron:]

A few notes:
 - we've already done this for text-emphasis-color, text-decoration-color, and -webkit-text-fill-color
 - we already have an implementation of this concept under another name (-moz-use-text-color) for border-color
 - it requires additional per-property code (like for the above properties) for all other CSS properties that take colors

Comment 4

[David Baron :dbaron:]

I think we probably shouldn't make any more properties do this until we have the ability to interpolate between currentcolor and real colors.  Otherwise we risk making the web depend on them *not* being interpolable, when we actually want them to be interpolable.  See bug 1260543 and https://lists.w3.org/Archives/Public/www-style/2016Apr/0284.html .

Comment 5

[Bobby Holley (:bholley)]

This blocks stylo per the comment at https://github.com/servo/servo/blob/f16aac0e5d53fce673b884476a68b9439f6042ba/components/style/properties/gecko.mako.rs#L232

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Hmmmm... currentcolor is so hard...

I think we have three choices:
1. make stylo follow how gecko handle it currently (so access nsStyleColor when needed)
2. follow the WebKit way (which was implemented in bug 1260543 comment 15) and turn all currentcolor (except for 'color') to the real used-value time currentcolor
3. implement an internal version of blend() function (and push CSSWG to standardize it)

The thrid way needs lots of work, but might be the best thing to do in long term.

dbaron, what do you think?

Comment 7

[David Baron :dbaron:]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> Hmmmm... currentcolor is so hard...
> 
> I think we have three choices:
> 1. make stylo follow how gecko handle it currently (so access nsStyleColor
> when needed)
> 2. follow the WebKit way (which was implemented in bug 1260543 comment 15)
> and turn all currentcolor (except for 'color') to the real used-value time
> currentcolor
> 3. implement an internal version of blend() function (and push CSSWG to
> standardize it)
> 
> The thrid way needs lots of work, but might be the best thing to do in long
> term.

So I believe current specs call for (2).

Is the reason you're suggesting (3) so that animation works better with 'currentcolor' values?  (That is a known risk for switching to what the spec wants.)

But for animation, what's wrong with the standardized version at https://drafts.csswg.org/css-color-4/#blend-adjuster other than complexity?

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Per discussion with dbaron on IRC:

The current spec calls for making currentcolor not interpolatable with other color value, which isn't what we want.

We can follow the WebKit way to treat currentcolor as a static value of "color" property at a given moment, which should not cause bigger compatibility issue in the future than our current behavior. But before we start implementing that way, we should bring it up in www-style to let other vendors know that we are going this way.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

I'm going to work on this.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

It looks like Blink implements the full interpolation of currentcolor. I'm going to go that way as well. I think we can implement that without fully implementing blend() function. (The only issue is getComputedStyle(). But I guess it's fine to make resolved value of color properties to be used value later.)

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #7)
> But for animation, what's wrong with the standardized version at
> https://drafts.csswg.org/css-color-4/#blend-adjuster other than complexity?

It seems the spec still doesn't say anything about how currentcolor should be interpolated with a numeric color value.

The current interpolation approach for color value defined in CSS Transitions doesn't use this blenda function. Instead, it just does a linear interpolation on premultipled color channels. That means if we use blenda for interpolation between currentcolor value and numeric color value, we would have inconsistent behavior.

In addition, it doesn't seem to me that the blenda function is something stable enough in the spec. There is still an issue calls for potential better formula [1] (as the current formula of blenda was determined empirically without any theory support), and I don't quite agree with some part of the current algorithm (the part how alpha channel is computed).

I'd probably go with linear interpolation on premultipled color channels, then.


[1] https://github.com/w3c/csswg-drafts/issues/305

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Looks like Blink is using linear interpolation on premultipled color, FWIW.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

This is not a stylo bug.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> This is not a stylo bug.

Oh I see, I didn't read the bug history carefully enough. Filed bug 1345709 for the stylo work item.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 760345 - Remove the last usage of lossy currentcolor. r=heycam

We don't have lossy currentcolor in the style system anymore, except for a
single property -moz-font-smoothing-background-color.

I could've converted it into a proper StyleColor and thread down all the
necessary information to the font metrics code.

But it doesn't really seem worth it given it's not exposed to the web, so I just
did the simplest thing, which is making currentcolor compute to transparent to
that specific property.

This patch also removes the stores_complex_colors_lossily code and related,
since now we always can cache computed colors.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 760345 - Make color and text-decoration-line not early properties. r=#style

The text-decoration-line is a drive-by thing, but I can put it in a separate
patch if you want. It was needed for Servo (to compute
-servo-text-decorations-in-effect), but I moved Servo away from that model a
while ago.

Comment 17

[Cameron McCormack (:heycam)]

It's OK for now, but is the kind of thing that helps reviewers (well, me at least) determine which changes are not related to the others.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ea4b3582033
Remove the last usage of lossy currentcolor. r=heycam

Comment 19

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 1ea4b3582033 (Bug 760345) for build bustages at ia2AccessibleComponent.cpp.

Backout: https://hg.mozilla.org/integration/autoland/rev/dd4d744643a31e8817f15b07fe610bc0fbe0b671

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=1ea4b3582033d4d9777590af85c7299f68861ffd&selectedJob=238313749

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238313749&repo=autoland&lineNumber=29993

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0550767ec778
Remove the last usage of lossy currentcolor. r=heycam

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08f256aa338a
Make color and text-decoration-line not early properties. r=heycam

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/0550767ec778
https://hg.mozilla.org/mozilla-central/rev/08f256aa338a

Comment 23

[Rachel Andrew]

Added to the release notes for 68.