Bug 54800 – SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in 'currentColor' handling

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in 'currentColor' handling

[Nikolas Zimmermann]

Created attachment 83067 [details]
Patch

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Hm Platform detection doesn't seem to work too well.

[Early Warning System Bot]

Attachment 83067 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7938277

[WebKit Review Bot]

Attachment 83067 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7934476

[Build Bot]

Attachment 83067 [details] did not build on win:
Build output: http://queues.webkit.org/results/7939297

[Nikolas Zimmermann]

Created attachment 83068 [details]
Patch

[Dirk Schulze]

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>

[Dirk Schulze]

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.

[WebKit Review Bot]

Attachment 83067 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7932555

[WebKit Review Bot]

Attachment 83067 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7939309

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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)

[Dirk Schulze]

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?

[Nikolas Zimmermann]

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

[Rob Buis]

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.

[Rob Buis]

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.

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

*** Bug 38102 has been marked as a duplicate of this bug. ***

[Antti Koivisto]

Comment on attachment 84199 [details]
Patch v3

r=me

[Nikolas Zimmermann]

Committed r79985: <http://trac.webkit.org/changeset/79985>

[Eric Seidel (no email)]

This is believed to have caused http://code.google.com/p/chromium/issues/detail?id=80880.

[Nico Weber]

I filed https://bugs.webkit.org/show_bug.cgi?id=59941 for the regression that this caused.