Bug 15012 – SVGColor dom interface is not created for color property in SVG elements

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rob Buis]

When inspecting css properties using getPresentationAttribute it becomes clear that color properties
are of type CSSPrimitiveValue whereas the svg spec says they should be SVGColor.

[Rob Buis]

Created attachment 16026 [details]
Testcase

Quick testcase showing the problem. Try in Opera to verify that it should report SVGColor, not CSSPrimitiveValue.

[Rob Buis]

Created attachment 16117 [details]
First attempt

My first shot at this. It could need some more ENABLE(SVG) sections, anyway I think we need something like this to support things like SVGColor as a proper CSSValue interface.
Cheers,

Rob.

[Maciej Stachowiak]

I am uncomfortable with having an "SVG mode" for the CSS parser. It makes things confusing when mixing SVG with other XML languages. Also, the way the patch does this, it would apply to stylesheets references with a style element, but not to ones using a processing instruction: <http://www.w3.org/TR/xml-stylesheet/>. Perhaps the actual right thing to do is to special-case the color property in the implementation of getPresentationAttribute. Then getPresentationAttribute would do the right thing for colors set by stylesheets included from an XHTML <link> element or an xml-stylesheet processing instruction. Is there a reason that wouldn't work?

[Eric Seidel (no email)]

Comment on attachment 16117 [details]
First attempt

I'm also not a fan of a separate parsing mode.  r-'ing after Maciej's comments.

[Dirk Schulze]

Rob, I still see CSSPrimitiveValue on the example. But there was something dropped around SVGColor IIRC. Can you check if this is still a valid bug report? And maybe fix it? :D

[Rob Buis]

Hi Dirk,

(In reply to comment #5)
> Rob, I still see CSSPrimitiveValue on the example. But there was something dropped around SVGColor IIRC. Can you check if this is still a valid bug report? And maybe fix it? :D

I'll have a look this weekend, sure.
Cheers,

Rob.

[Rob Buis]

Created attachment 134981 [details]
Patch

[Dirk Schulze]

Comment on attachment 134981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134981&action=review

I would like to have some more eyes on that. But it looks strange to me that just 'color' should be affected.

> Source/WebCore/ChangeLog:8
> +        Create SVGColor instances for the color property when in parsing mode SVGAttributeMode.

So it should only be SVGColor for asking presentation attributes? Can you add a link to the spec text please?

> Source/WebCore/css/CSSParser.cpp:339
> +    if (cssParserMode == SVGAttributeMode && propertyId == CSSPropertyColor) {

Why just for 'color' and not 'stop-color', 'flood-color' or 'fill' and 'stroke' when they have colors?

> Source/WebCore/css/CSSParser.cpp:342
> +        if (!CSSParser::fastParseColor(color, string, strict && string[0] != '#'))
> +            return false;

This gets called some lines later, sure that you should't move the exception for SVG there?

[Rob Buis]

Hi Dirk,

(In reply to comment #8)
> (From update of attachment 134981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134981&action=review
> 
> I would like to have some more eyes on that. But it looks strange to me that just 'color' should be affected.

Ok. Read below :)

> > Source/WebCore/ChangeLog:8
> > +        Create SVGColor instances for the color property when in parsing mode SVGAttributeMode.
> 
> So it should only be SVGColor for asking presentation attributes? Can you add a link to the spec text please?

How do you mean? Do you think this patch is not fixing the bug?

> > Source/WebCore/css/CSSParser.cpp:339
> > +    if (cssParserMode == SVGAttributeMode && propertyId == CSSPropertyColor) {
> 
> Why just for 'color' and not 'stop-color', 'flood-color' or 'fill' and 'stroke' when they have colors?

color is special since it is there in both normal and 'svg' css, whereas those others are svg only and handled by SVGCSSParser with a different code path already producing SVGColors, normal CSSParser doesn't know them.

> > Source/WebCore/css/CSSParser.cpp:342
> > +        if (!CSSParser::fastParseColor(color, string, strict && string[0] != '#'))
> > +            return false;
> 
> This gets called some lines later, sure that you should't move the exception for SVG there?

We could, but then we'd have to work around the validPrimitiveValue, SVGColor is not a CSSPrimitiveValue.
Cheers,

Rob.

[Rob Buis]

The test also needs to check other color properties like flood-color etc. R- until the next patch...

[Rob Buis]

Created attachment 135812 [details]
Patch

[Rob Buis]

My latest patch now should test all color properties of SVG.

[Rob Buis]

Ok, now I get better what Dirk meant by "But there was something dropped around SVGColor IIRC.", I just read "And maybe fix it?". So now I saw this:

http://www.w3.org/TR/SVG/types.html#InterfaceSVGColor

Note: The SVGColor interface is deprecated, and may be dropped from future versions of the SVG specification.

[Rob Buis]

Slightly related:

https://bugzilla.mozilla.org/show_bug.cgi?id=500888

SVGPaint is also deprecated:

http://www.w3.org/TR/SVG/painting.html#InterfaceSVGPaint

Note: The SVGPaint interface is deprecated, and may be dropped from future versions of the SVG specification.

[Rob Buis]

Comment on attachment 135812 [details]
Patch

Clearing review flag.

[Rob Buis]

As discussed with Dirk, not much point in this bug anymore. Once SVG2 happens, we should remove SVGColor, or reconsider the implementation in this bug, depending on their decision.