| Summary: | SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in 'currentColor' handling | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | buildbot, dglazkov, eric, krit, rwlbuis, thakis, webkit-ews, webkit.review.bot, zimmermann | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Mac | ||||||||||||
| OS: | OS X 10.6 | ||||||||||||
| Bug Depends on: | 55119 | ||||||||||||
| Bug Blocks: | 59941 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Nikolas Zimmermann
2011-02-19 02:05:07 PST
Created attachment 83067 [details]
Patch
Comment on attachment 83067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83067&action=review > LayoutTests/ChangeLog:7 > + Oops, I was too fast when using webkit-patch. Here's one line missing: Add new test from SVG 1.1 2nd Edition covering currentColor support. Hm Platform detection doesn't seem to work too well. Attachment 83067 [details] did not build on qt: Build output: http://queues.webkit.org/results/7938277 Attachment 83067 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7934476 Attachment 83067 [details] did not build on win: Build output: http://queues.webkit.org/results/7939297 Created attachment 83068 [details]
Patch
Comment on attachment 83068 [details]
Patch
Before I start reviewing the patch. Could you please add a test, where we animate 'color' and use fill="currentColor" on the same object? I checked and it seems we do not have such a test:
<rect color="red" fill="currentColor" width="100" height="100">
<animate attributeName="color" from="red" to="green" dur="3s"/>
</rect>
Comment on attachment 83068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83068&action=review > Source/WebCore/rendering/style/SVGRenderStyle.h:73 > static float initialFillOpacity() { return 1.0f; } Should this be changed to return 1; in this patch? > Source/WebCore/rendering/style/SVGRenderStyle.h:77 > static float initialStrokeOpacity() { return 1.0f; } Ditto. > Source/WebCore/rendering/style/SVGRenderStyle.h:83 > static float initialStrokeMiterLimit() { return 4.0f; } > static float initialStopOpacity() { return 1.0f; } Ditto. I think when you are already changing something here, you could change following lines to match the style rule. Attachment 83067 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7932555 Attachment 83067 [details] did not build on mac: Build output: http://queues.webkit.org/results/7939309 (In reply to comment #8) > (From update of attachment 83068 [details]) > Before I start reviewing the patch. Could you please add a test, where we animate 'color' and use fill="currentColor" on the same object? I checked and it seems we do not have such a test: > > <rect color="red" fill="currentColor" width="100" height="100"> > <animate attributeName="color" from="red" to="green" dur="3s"/> > </rect> LayoutTests/svg/animations/animate-color-* cover this? var rect = createSVGElement("rect"); rect.setAttribute("id", "rect"); rect.setAttribute("width", "100"); rect.setAttribute("height", "100"); rect.setAttribute("color", "rgb(0,255,255)"); rect.setAttribute("fill", "currentColor"); rect.setAttribute("onclick", "executeTest()"); var animate = createSVGElement("animate"); animate.setAttribute("id", "animation"); animate.setAttribute("attributeName", "color"); animate.setAttribute("from", "rgb(255,0,0)"); animate.setAttribute("to", "rgb(0,255,255)"); animate.setAttribute("begin", "click"); animate.setAttribute("dur", "4s"); animate.setAttribute("calcMode", "discrete"); I'll add another test though, which doesn't use calcMode discrete. Created attachment 83522 [details]
Patch v2
Upon writing new testcases, I ran into problems as the SVGColor/SVGPaint API was not implemented correctly at all.
Took a while, but it's now fixed (except some issues, that can not be fixed atm because of our CSS OM Design, please read the ChangeLog carefully)
Comment on attachment 83522 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=83522&action=review See comments below. Anyway r=me. Great fix! > Source/WebCore/ChangeLog:27 > + It turns out the SVGPaint/SVGColor API is not implemented at all, fix that. Though the API doesn't work as expected (yet?) as > + we don't support mutable CSSValue derived objects for various reasons (our CSSValues are shared, we'd need copy-on-write support > + to make that work, and handle style invalidation, currently a CSSValue is not tied to a RenderStyle in any way, making dynamic updates impossible). > + So you can basically grab a SVGPaint/SVGColor object through getPropertyCSSValue(..), mutate it, that will all work as expected > + but the change won't take effect (see CSSPrimitiveValue::setCssText, for more reasons why this API is not implemented in WebKit). The first sentence sounds like a demand, rephrase it please. It seems to me that you're trying to fix two problems with one patch. Please avoid this. The API is not part of the bug description. And together with the color serialization fix you're fixing three bugs. Would make it much easier to review if you split the patches. Also from a logical point of view. The size of the patch grows from 58k to 136k. > Source/WebCore/svg/SVGPaint.cpp:51 > + break; > + case SVGPaint::SVG_PAINTTYPE_URI_RGBCOLOR: > + case SVGPaint::SVG_PAINTTYPE_RGBCOLOR: > + return SVGColor::SVG_COLORTYPE_RGBCOLOR; > + case SVGPaint::SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR: > + case SVGPaint::SVG_PAINTTYPE_RGBCOLOR_ICCCOLOR: > + return SVGColor::SVG_COLORTYPE_RGBCOLOR_ICCCOLOR; > + case SVGPaint::SVG_PAINTTYPE_URI_CURRENTCOLOR: > + case SVGPaint::SVG_PAINTTYPE_CURRENTCOLOR: > + return SVGColor::SVG_COLORTYPE_CURRENTCOLOR; > + } > I'd replace the break with return SVGColor::SVG_COLORTYPE_UNKNOWN; and add a ASSERT_NOT_REACHED after the switch. > Source/WebCore/svg/SVGPaint.cpp:143 > + case SVG_PAINTTYPE_CURRENTCOLOR: > + break; > + case SVG_PAINTTYPE_NONE: > return "none"; > - if (m_paintType == SVG_PAINTTYPE_CURRENTCOLOR) > - return "currentColor"; > - if (m_paintType == SVG_PAINTTYPE_URI) > + case SVG_PAINTTYPE_URI_NONE: > + return makeString("url(" + m_uri + ") none"); > + case SVG_PAINTTYPE_URI_CURRENTCOLOR: > + case SVG_PAINTTYPE_URI_RGBCOLOR: > + case SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR: > + return makeString("url(" + m_uri + ") ", SVGColor::cssText()); > + case SVG_PAINTTYPE_URI: > return "url(" + m_uri + ")"; > + }; > > return SVGColor::cssText(); Ditto. > Source/WebCore/svg/SVGPaint.cpp:163 > + break; > + case SVG_PAINTTYPE_URI_NONE: > + case SVG_PAINTTYPE_URI_CURRENTCOLOR: > + case SVG_PAINTTYPE_URI_RGBCOLOR: > + case SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR: > + case SVG_PAINTTYPE_URI: > + return referenceId == SVGURIReference::getTarget(m_uri); > + } > > - return referenceId == SVGURIReference::getTarget(m_uri); > + return false; Ditto. > LayoutTests/svg/animations/script-tests/animate-color-fill-currentColor.js:36 > + // Opera doesn't support getPropertyCSSValue - no way to compare to their SVGPaint/SVGColor objects :( This test doesn't work on opera anyway, right? (In reply to comment #14) > (From update of attachment 83522 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83522&action=review > > See comments below. Anyway r=me. Great fix! > > > Source/WebCore/ChangeLog:27 > > + It turns out the SVGPaint/SVGColor API is not implemented at all, fix that. Though the API doesn't work as expected (yet?) as > > + we don't support mutable CSSValue derived objects for various reasons (our CSSValues are shared, we'd need copy-on-write support > > + to make that work, and handle style invalidation, currently a CSSValue is not tied to a RenderStyle in any way, making dynamic updates impossible). > > + So you can basically grab a SVGPaint/SVGColor object through getPropertyCSSValue(..), mutate it, that will all work as expected > > + but the change won't take effect (see CSSPrimitiveValue::setCssText, for more reasons why this API is not implemented in WebKit). > > The first sentence sounds like a demand, rephrase it please. Fixed. > > It seems to me that you're trying to fix two problems with one patch. Please avoid this. The API is not part of the bug description. And together with the color serialization fix you're fixing three bugs. Would make it much easier to review if you split the patches. Also from a logical point of view. The size of the patch grows from 58k to 136k. You're absolutely right, I'm going to split up this beast. > > Source/WebCore/svg/SVGPaint.cpp:51 > > + break; > > + case SVGPaint::SVG_PAINTTYPE_URI_RGBCOLOR: > > + case SVGPaint::SVG_PAINTTYPE_RGBCOLOR: > > + return SVGColor::SVG_COLORTYPE_RGBCOLOR; > > + case SVGPaint::SVG_PAINTTYPE_URI_RGBCOLOR_ICCCOLOR: > > + case SVGPaint::SVG_PAINTTYPE_RGBCOLOR_ICCCOLOR: > > + return SVGColor::SVG_COLORTYPE_RGBCOLOR_ICCCOLOR; > > + case SVGPaint::SVG_PAINTTYPE_URI_CURRENTCOLOR: > > + case SVGPaint::SVG_PAINTTYPE_CURRENTCOLOR: > > + return SVGColor::SVG_COLORTYPE_CURRENTCOLOR; > > + } > > > > I'd replace the break with return SVGColor::SVG_COLORTYPE_UNKNOWN; and add a ASSERT_NOT_REACHED after the switch. Oh I had that before, but changed to this style, in order to avoid the ASSERT_NOT_REACHED. Hm, I think it's just a matter of preference, if you like, I'll change it. > > LayoutTests/svg/animations/script-tests/animate-color-fill-currentColor.js:36 > > + // Opera doesn't support getPropertyCSSValue - no way to compare to their SVGPaint/SVGColor objects :( > > This test doesn't work on opera anyway, right? Right :( No way to access SVGPaint/SVGColor objects at all. Hi Niko, I don't have much time until maybe sunday evening, so just a short reaction. I think the change is fine and having that shared CSSValue was always risky. > In order to fix to bug we have to resolve all currentColor values for SVGPaint objects, in SVGCSSStyleSelector, Spot the typo :) Also is bug 38102 fixed with this? Cheers, Rob. Hi Niko, Oh, and did you mean <g color="green"><rect fill="currentColor"> already worked fine in trunk instead of <g color="red"><rect fill="currentColor"/> already worked fine in trunk ? Just checking since we want to end up with green :) Cheers, Rob. Created attachment 84199 [details]
Patch v3
New patch, after SVGColor/SVGPaint API has been implemented and landed.
This fixes the currentColor bug, and makes SVGPaint/SVGColor changes take immediate affect, as demanded by the spec.
(In reply to comment #17) > Hi Niko, > > Oh, and did you mean > > <g color="green"><rect fill="currentColor"> already worked fine in trunk > > instead of > > <g color="red"><rect fill="currentColor"/> already worked fine in trunk > > ? > Just checking since we want to end up with green :) Well it's just an example. I only meant to say, that picking up the parents style color worked fine, but not the other way round. *** Bug 38102 has been marked as a duplicate of this bug. *** Comment on attachment 84199 [details]
Patch v3
r=me
Committed r79985: <http://trac.webkit.org/changeset/79985> This is believed to have caused http://code.google.com/p/chromium/issues/detail?id=80880. I filed https://bugs.webkit.org/show_bug.cgi?id=59941 for the regression that this caused. |