Bug 18568 – background: currentColor fails

• Component: New Bugs
• Status: RESOLVED FIXED

[Anantha Keesara]

Go to
http://www.hixie.ch/tests/adhoc/css/color/currentColor/001.html

Issue:
current color basic test fails. A green square is not showing.

Other browsers:
IE7 : not ok
FF3 beta : ok
Safari 3.1 : not ok
Opera 9.24 Build 8816 : not  ok

Nightly tested: r31446

[Mark Rowe (bdash)]

currentColor is part of the CSS 3 color module:  http://www.w3.org/TR/css3-color/#currentcolor

[Rob Buis]

Created attachment 20849 [details]
First attempt

This brings support for currentColor. I moved it from svg css values to
the normal css values. One issue may be that I thought currentColor would be matched case sensitively, but it does not seem that way. FF3 treats it case-insensitively, Opera uses case-sensitive here.
Let me know whether the test case needs improvement, as there are a lot more
css properties than background that can have colors and therefore can use crrentColor.
Cheers,

Rob.

Comment on attachment 20849 [details]
First attempt

You have an unrelated test and result in the patch.

Since you have text in the render tree dump, the expected results should go into a platform-specific directory. As alternatives, you could make a text-only test using getComputedStyle, or just remove all text from the test.

CSS3 says "If the 'currentColor' keyword is set on the 'color' property itself, it is treated as 'color:inherit' at parse time". I don't understand how the patch satisfies this. I think it should be included in the test (maybe only testable by having a cascade where the first rule sets color: to something different from the parent's and the second sets it to inherit).

[Rob Buis]

Created attachment 20916 [details]
Improved testcase

I made the testcase into a text-only one. I hope I did enough to test the color:currentColor case, if not let me know how to fix it :)
Cheers,

Rob.

Comment on attachment 20916 [details]
Improved testcase

Here is the case I had in mind. I think the text should be green, but with the patch it is blue. What do you think?

<style>
	span { color: blue; }
</style>
<div style="color: green;">
	<span style="color: currentColor;">This should be green</span>
</div>

[Rob Buis]

Created attachment 20917 [details]
Incorporated cascade test

This incorporates the code + testcase part for mitzpettel's extra cascade test.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 20917 [details]
Incorporated cascade test

Two comments:

1.  This deserves a comment:
+    case CSSPropertyColor:
+        if (primitiveValue && primitiveValue->getIdent() == CSSValueCurrentcolor)
+            isInherit = true;

(about how the spec says to treat currentColor for the "color" property as though it means inherit.)

2.  Does getComputedStyle().color correctly return "currentColor" instead of "currentcolor" ?  Since I think in CSS3 they always refer to it in CamelCase, even though we accept any case (it seems).

Otherwise it looks fine.

[Rob Buis]

Hi Eric,

(In reply to comment #7)
> (From update of attachment 20917 [details] [edit])
> Two comments:
> 
> 1.  This deserves a comment:
> +    case CSSPropertyColor:
> +        if (primitiveValue && primitiveValue->getIdent() ==
> CSSValueCurrentcolor)
> +            isInherit = true;
> 
> (about how the spec says to treat currentColor for the "color" property as
> though it means inherit.)

I now did that locally.

> 2.  Does getComputedStyle().color correctly return "currentColor" instead of
> "currentcolor" ?  Since I think in CSS3 they always refer to it in CamelCase,
> even though we accept any case (it seems).

The computed style will always return a rgb string like rgb(0,128,0) AFAIK. Did you mean something else, like getPropertyCSSValue?
I am postponing landing until much later today, so there is time to resolve point 2. Also postponing since I need a clean testrun.
Cheers,

Rob.

[Eric Seidel (no email)]

re: 1.  Great to hear.

re: 2.  Yes, whatever way there could be to get a string "currentColor" out of WebCore, we should return it the same case as the spec, IMO.  That's a separate issue and can be addressed later.  If you could spend 5m investigating before you land, that would be great.  If it takes more than 5m, don't worry about it.

Land away!

[Rob Buis]

Landed in r32836.

[Robert Blaut]

*** Bug 17762 has been marked as a duplicate of this bug. ***

[Robert Blaut]

More tests for currentColor are available in duplicate bug 17762