|Summary:||currentColor not supported in CSS gradients|
|Product:||WebKit||Reporter:||Lea Verou <lea>|
|Component:||CSS||Assignee:||David Barr <davidbarr>|
|Severity:||Normal||CC:||hyatt, mathias, mikelawther, shanestephens, simon.fraser, webkit.review.bot|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
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 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 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 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 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 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 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 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 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.