Bug 54800 - SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in 'currentColor' handling
: SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in 'currentColor' handling
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To: Nikolas Zimmermann
:
Depends on: 55119
Blocks: 59941
  Show dependency treegraph
 
Reported: 2011-02-19 02:05 PST by Nikolas Zimmermann
Modified: 2011-05-04 13:55 PDT (History)
9 users (show)

See Also:


Attachments
Patch (58.41 KB, patch)
2011-02-19 02:19 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (58.47 KB, patch)
2011-02-19 03:24 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (136.55 KB, patch)
2011-02-23 12:25 PST, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff
Patch v3 (110.04 KB, patch)
2011-03-01 00:33 PST, Nikolas Zimmermann
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2011-02-19 02:05:07 PST
SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in 'currentColor' handling
Comment 1 Nikolas Zimmermann 2011-02-19 02:19:48 PST
Created attachment 83067 [details]
Patch
Comment 2 Nikolas Zimmermann 2011-02-19 02:21:26 PST
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.
Comment 3 Nikolas Zimmermann 2011-02-19 02:22:39 PST
Hm Platform detection doesn't seem to work too well.
Comment 4 Early Warning System Bot 2011-02-19 02:33:29 PST
Attachment 83067 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7938277
Comment 5 WebKit Review Bot 2011-02-19 02:35:51 PST
Attachment 83067 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7934476
Comment 6 Build Bot 2011-02-19 03:01:09 PST
Attachment 83067 [details] did not build on win:
Build output: http://queues.webkit.org/results/7939297
Comment 7 Nikolas Zimmermann 2011-02-19 03:24:41 PST
Created attachment 83068 [details]
Patch
Comment 8 Dirk Schulze 2011-02-19 03:43:28 PST
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 9 Dirk Schulze 2011-02-19 03:51:24 PST
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.
Comment 10 WebKit Review Bot 2011-02-19 03:57:14 PST
Attachment 83067 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7932555
Comment 11 WebKit Review Bot 2011-02-19 04:02:29 PST
Attachment 83067 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7939309
Comment 12 Nikolas Zimmermann 2011-02-22 01:39:21 PST
(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.
Comment 13 Nikolas Zimmermann 2011-02-23 12:25:44 PST
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 14 Dirk Schulze 2011-02-23 13:32:07 PST
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?
Comment 15 Nikolas Zimmermann 2011-02-24 00:30:09 PST
(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.
Comment 16 Rob Buis 2011-02-25 02:43:41 PST
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.
Comment 17 Rob Buis 2011-02-25 06:13:08 PST
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.
Comment 18 Nikolas Zimmermann 2011-03-01 00:33:13 PST
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.
Comment 19 Nikolas Zimmermann 2011-03-01 00:34:19 PST
(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.
Comment 20 Nikolas Zimmermann 2011-03-01 00:35:21 PST
*** Bug 38102 has been marked as a duplicate of this bug. ***
Comment 21 Antti Koivisto 2011-03-01 01:27:05 PST
Comment on attachment 84199 [details]
Patch v3

r=me
Comment 22 Nikolas Zimmermann 2011-03-01 02:53:55 PST
Committed r79985: <http://trac.webkit.org/changeset/79985>
Comment 23 Eric Seidel 2011-05-02 09:53:45 PDT
This is believed to have caused http://code.google.com/p/chromium/issues/detail?id=80880.
Comment 24 Nico Weber 2011-05-02 10:03:53 PDT
I filed https://bugs.webkit.org/show_bug.cgi?id=59941 for the regression that this caused.