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
: WebKit
SVG
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To:
:
:
: 55119
: 59941
  Show dependency treegraph
 
Reported: 2011-02-19 02:05 PST by
Modified: 2011-05-04 13:55 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-02-19 02:05:07 PST
SVG 1.1 2nd Edition color-prop-05-t.svg exposes bug in 'currentColor' handling
------- Comment #1 From 2011-02-19 02:19:48 PST -------
Created an attachment (id=83067) [details]
Patch
------- Comment #2 From 2011-02-19 02:21:26 PST -------
(From update of attachment 83067 [details])
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 From 2011-02-19 02:22:39 PST -------
Hm Platform detection doesn't seem to work too well.
------- Comment #4 From 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 From 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 From 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 From 2011-02-19 03:24:41 PST -------
Created an attachment (id=83068) [details]
Patch
------- Comment #8 From 2011-02-19 03:43:28 PST -------
(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>
------- Comment #9 From 2011-02-19 03:51:24 PST -------
(From update of attachment 83068 [details])
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 From 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 From 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 From 2011-02-22 01:39:21 PST -------
(In reply to comment #8)
> (From update of attachment 83068 [details] [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 From 2011-02-23 12:25:44 PST -------
Created an attachment (id=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 From 2011-02-23 13:32:07 PST -------
(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.

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 From 2011-02-24 00:30:09 PST -------
(In reply to comment #14)
> (From update of attachment 83522 [details] [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 From 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 From 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 From 2011-03-01 00:33:13 PST -------
Created an attachment (id=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 From 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 From 2011-03-01 00:35:21 PST -------
*** Bug 38102 has been marked as a duplicate of this bug. ***
------- Comment #21 From 2011-03-01 01:27:05 PST -------
(From update of attachment 84199 [details])
r=me
------- Comment #22 From 2011-03-01 02:53:55 PST -------
Committed r79985: <http://trac.webkit.org/changeset/79985>
------- Comment #23 From 2011-05-02 09:53:45 PST -------
This is believed to have caused http://code.google.com/p/chromium/issues/detail?id=80880.
------- Comment #24 From 2011-05-02 10:03:53 PST -------
I filed https://bugs.webkit.org/show_bug.cgi?id=59941 for the regression that this caused.