Bug 58730

Summary: currentColor not supported in CSS gradients
Product: WebKit Reporter: Lea Verou <lea>
Component: CSSAssignee: David Barr <davidbarr>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, mathias, mikelawther, shanestephens, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
URL: http://jsfiddle.net/leaverou/TdYzS/
Attachments:
Description Flags
Patch
none
Patch
none
Revised patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Lea Verou 2011-04-16 05:27:54 PDT
See testcase. It should be a linear gradient from gray to red but instead the whole declaration is dropped. 

Gecko gets this right.
Comment 1 David Barr 2011-05-15 23:02:06 PDT
Created attachment 93609 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-05-16 07:56:40 PDT
Comment on attachment 93609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93609&action=review

> LayoutTests/fast/css/linear-gradient-currentcolor.html:5
> +    color:red;

Please don't have visible red in a passing test. It would be better to use green, and maybe put two color stops next to eachother to get a sharp edge that's less likely to suffer from platform differences.

Spaces after colons please.

> Source/WebCore/ChangeLog:8
> +        https://bugs.webkit.org/show_bug.cgi?id=58730
> +
> +        Test: fast/css/linear-gradient-currentcolor.html

This needs some text saying how you fixed the issue.

> Source/WebCore/css/CSSGradientValue.cpp:159
> +            stops[i].color = renderer->document()->styleSelector()->getColorFromPrimitiveValue(color);

I don't get it. getColorFromPrimitiveValue() already looks for CSSValueCurrentcolor.
Comment 3 David Barr 2011-05-16 16:55:25 PDT
Created attachment 93714 [details]
Patch
Comment 4 Simon Fraser (smfr) 2011-05-16 17:01:27 PDT
Comment on attachment 93714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93714&action=review

> Source/WebCore/css/CSSGradientValue.cpp:159
> -        stops[i].color = renderer->document()->styleSelector()->getColorFromPrimitiveValue(stop.m_color.get());
> +        CSSPrimitiveValue* color = stop.m_color.get();
> +        if (color->getIdent() == CSSValueCurrentcolor)
> +            stops[i].color = style->visitedDependentColor(CSSPropertyColor);
> +        else
> +            stops[i].color = renderer->document()->styleSelector()->getColorFromPrimitiveValue(color);

I still don't get this part. getColorFromPrimitiveValue() handles currentCOlor already.
Comment 5 David Barr 2011-05-16 22:37:15 PDT
Comment on attachment 93714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93714&action=review

>> Source/WebCore/css/CSSGradientValue.cpp:159
>> +            stops[i].color = renderer->document()->styleSelector()->getColorFromPrimitiveValue(color);
> 
> I still don't get this part. getColorFromPrimitiveValue() handles currentCOlor already.

In this context, CSSStyleSelector::m_style is NULL rather than equal to the local var 'style'.
This change works around that issue pending a change in the path to getColorFromPrimitiveValue().
Maybe a FIXME is warranted? This feature addition exposes a lurking NULL-deref bug.
Comment 6 David Barr 2011-06-08 22:06:42 PDT
Created attachment 96546 [details]
Revised patch
Comment 7 David Barr 2011-06-08 22:24:04 PDT
Comment on attachment 96546 [details]
Revised patch

Revised patch - force initialization of CSSStyleSelector rather than work around the lack thereof
Comment 8 Simon Fraser (smfr) 2011-06-08 23:15:36 PDT
Comment on attachment 96546 [details]
Revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96546&action=review

> Source/WebCore/css/CSSGradientValue.cpp:153
> +    // FIXME Force initialization to support CSSValueCurrentcolor - should this happen earlier?
> +    renderer->document()->styleSelector()->setStyle(style);

It seems like the style selector needs "start using for style" and "stop using for style" methods. Are there other places that need to do the same thing?
Comment 9 David Barr 2011-06-08 23:53:19 PDT
I've just discovered that this is the tip of the iceberg - there are a issues with caching the computed image too. New patch soon to come.

smfr: My feeling is that the proposed methods ought to be called in WebCore::StyleGeneratedImage::image() before and after the call to GeneratedImage::image().
I'm don't know about other callers.
Comment 10 David Barr 2011-06-09 00:07:39 PDT
Created attachment 96557 [details]
Patch
Comment 11 David Barr 2011-06-09 00:10:50 PDT
Comment on attachment 96557 [details]
Patch

Ok, more an ice-cube than iceberg - moved initialization of CSSStyleSelector to  WebCore::StyleGeneratedImage::image() and added additional constraint to CSSGradientValue::isCacheable().
Comment 12 David Barr 2011-06-09 00:20:46 PDT
Just noticed that the file summary in the WebCore ChangeLog is wrong, sorry.
I'm surprised that webkit-patch doesn't warn about that.
Comment 13 David Barr 2011-06-09 17:36:58 PDT
Created attachment 96672 [details]
Patch
Comment 14 David Barr 2011-06-09 17:40:24 PDT
Comment on attachment 96672 [details]
Patch

Re-generated ChangeLog entries to capture the methods affected.
Comment 15 Simon Fraser (smfr) 2011-06-28 10:41:49 PDT
Comment on attachment 96672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96672&action=review

> Source/WebCore/ChangeLog:8
> +        currentColor not supported in CSS gradients
> +        https://bugs.webkit.org/show_bug.cgi?id=58730
> +
> +        Test: fast/css/linear-gradient-currentcolor.html

This needs to describe what was broken, and what you did to fix it.
Comment 16 David Barr 2011-06-28 11:37:49 PDT
The primary outstanding concern with this change is clarifying the contract with CSSStyleSelector.
The patch implements the parsing and style logic for currentColor support in CSS gradient color stops.
Comment 17 David Barr 2011-09-28 22:34:39 PDT
Created attachment 109122 [details]
Patch
Comment 18 David Barr 2011-09-28 22:37:30 PDT
Comment on attachment 109122 [details]
Patch

Updated changelog with active description.
Comment 19 WebKit Review Bot 2011-09-28 23:23:40 PDT
Comment on attachment 109122 [details]
Patch

Rejecting attachment 109122 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/css/CSSGradientValue.cpp
patching file Source/WebCore/css/CSSParser.cpp
Hunk #1 succeeded at 5903 (offset -2 lines).
patching file Source/WebCore/rendering/style/StyleGeneratedImage.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Fraser', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/9888418
Comment 20 David Barr 2011-09-29 00:54:55 PDT
Created attachment 109132 [details]
Patch
Comment 21 David Barr 2011-09-29 07:12:04 PDT
I added some detail to the ChangeLog and resolved the conflict
encountered by the commit queue.

However, it is likely to conflict again due to the high rate of appends
to LayoutTests/platform/chromium/test_expectations.txt.
Is it simply a matter of rebasing repeatedly until it lands?
Maybe we could use a merge driver for test_expectations.txt.
Comment 22 David Barr 2011-09-29 18:35:30 PDT
Created attachment 109233 [details]
Patch
Comment 23 David Barr 2011-09-29 18:36:52 PDT
Comment on attachment 109233 [details]
Patch

Updated test_expectations.txt change so that conflict is less likely to occur on the commit queue.
Comment 24 Adam Barth 2011-09-30 18:27:14 PDT
Comment on attachment 109233 [details]
Patch

Forwarding simon.fraser's r+
Comment 25 WebKit Review Bot 2011-09-30 18:42:26 PDT
Comment on attachment 109233 [details]
Patch

Clearing flags on attachment: 109233

Committed r96449: <http://trac.webkit.org/changeset/96449>
Comment 26 WebKit Review Bot 2011-09-30 18:42:32 PDT
All reviewed patches have been landed.  Closing bug.