Bug 18568 - background: currentColor fails
Summary: background: currentColor fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://www.hixie.ch/tests/adhoc/css/c...
Keywords:
: 17762 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-04-17 22:21 PDT by Anantha Keesara
Modified: 2008-05-03 11:17 PDT (History)
1 user (show)

See Also:


Attachments
First attempt (17.58 KB, patch)
2008-04-27 07:24 PDT, Rob Buis
mitz: review-
Details | Formatted Diff | Diff
Improved testcase (5.88 KB, patch)
2008-05-01 12:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Incorporated cascade test (6.49 KB, patch)
2008-05-01 13:54 PDT, Rob Buis
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anantha Keesara 2008-04-17 22:21:13 PDT
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
Comment 1 Mark Rowe (bdash) 2008-04-18 00:34:32 PDT
currentColor is part of the CSS 3 color module:  http://www.w3.org/TR/css3-color/#currentcolor
Comment 2 Rob Buis 2008-04-27 07:24:20 PDT
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 3 mitz 2008-04-28 15:42:11 PDT
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).
Comment 4 Rob Buis 2008-05-01 12:45:22 PDT
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 5 mitz 2008-05-01 12:59:19 PDT
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>
Comment 6 Rob Buis 2008-05-01 13:54:58 PDT
Created attachment 20917 [details]
Incorporated cascade test

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

Rob.
Comment 7 Eric Seidel (no email) 2008-05-01 19:45:06 PDT
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.
Comment 8 Rob Buis 2008-05-01 23:59:53 PDT
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.
Comment 9 Eric Seidel (no email) 2008-05-02 14:42:10 PDT
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!
Comment 10 Rob Buis 2008-05-02 23:49:08 PDT
Landed in r32836.
Comment 11 Robert Blaut 2008-05-03 11:15:24 PDT
*** Bug 17762 has been marked as a duplicate of this bug. ***
Comment 12 Robert Blaut 2008-05-03 11:17:20 PDT
More tests for currentColor are available in duplicate bug 17762