Bug 58730 – currentColor not supported in CSS gradients

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Lea Verou]

See testcase. It should be a linear gradient from gray to red but instead the whole declaration is dropped. 

Gecko gets this right.

[David Barr]

Created attachment 93609 [details]
Patch

[Simon Fraser (smfr)]

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.

[David Barr]

Created attachment 93714 [details]
Patch

[Simon Fraser (smfr)]

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.

[David Barr]

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.

[David Barr]

Created attachment 96546 [details]
Revised patch

[David Barr]

Comment on attachment 96546 [details]
Revised patch

Revised patch - force initialization of CSSStyleSelector rather than work around the lack thereof

[Simon Fraser (smfr)]

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?

[David Barr]

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.

[David Barr]

Created attachment 96557 [details]
Patch

[David Barr]

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().

[David Barr]

Just noticed that the file summary in the WebCore ChangeLog is wrong, sorry.
I'm surprised that webkit-patch doesn't warn about that.

[David Barr]

Created attachment 96672 [details]
Patch

[David Barr]

Comment on attachment 96672 [details]
Patch

Re-generated ChangeLog entries to capture the methods affected.

[Simon Fraser (smfr)]

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.

[David Barr]

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.

[David Barr]

Created attachment 109122 [details]
Patch

[David Barr]

Comment on attachment 109122 [details]
Patch

Updated changelog with active description.

[WebKit Review Bot]

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

[David Barr]

Created attachment 109132 [details]
Patch

[David Barr]

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.

[David Barr]

Created attachment 109233 [details]
Patch

[David Barr]

Comment on attachment 109233 [details]
Patch

Updated test_expectations.txt change so that conflict is less likely to occur on the commit queue.

[Adam Barth]

Comment on attachment 109233 [details]
Patch

Forwarding simon.fraser's r+

[WebKit Review Bot]

Comment on attachment 109233 [details]
Patch

Clearing flags on attachment: 109233

Committed r96449: <http://trac.webkit.org/changeset/96449>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.