Bug 15012

Summary: SVGColor dom interface is not created for color property in SVG elements
Product: WebKit Reporter: Rob Buis <rwlbuis>
Component: SVGAssignee: Rob Buis <rwlbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: krit, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Bug Depends on: 46112    
Bug Blocks:    
Attachments:
Description Flags
Testcase
none
First attempt
none
Patch
none
Patch none

Description Rob Buis 2007-08-19 13:19:06 PDT
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.
Comment 1 Rob Buis 2007-08-19 13:23:36 PDT
Created attachment 16026 [details]
Testcase

Quick testcase showing the problem. Try in Opera to verify that it should report SVGColor, not CSSPrimitiveValue.
Comment 2 Rob Buis 2007-08-25 06:42:29 PDT
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.
Comment 3 Maciej Stachowiak 2007-08-29 02:05:12 PDT
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?
Comment 4 Eric Seidel (no email) 2007-09-29 12:49:44 PDT
Comment on attachment 16117 [details]
First attempt

I'm also not a fan of a separate parsing mode.  r-'ing after Maciej's comments.
Comment 5 Dirk Schulze 2012-03-30 22:09:13 PDT
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
Comment 6 Rob Buis 2012-03-31 05:20:31 PDT
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.
Comment 7 Rob Buis 2012-03-31 16:07:31 PDT
Created attachment 134981 [details]
Patch
Comment 8 Dirk Schulze 2012-03-31 17:31:30 PDT
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?
Comment 9 Rob Buis 2012-03-31 19:06:41 PDT
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.
Comment 10 Rob Buis 2012-04-04 14:59:23 PDT
The test also needs to check other color properties like flood-color etc. R- until the next patch...
Comment 11 Rob Buis 2012-04-05 05:56:54 PDT
Created attachment 135812 [details]
Patch
Comment 12 Rob Buis 2012-04-05 06:40:04 PDT
My latest patch now should test all color properties of SVG.
Comment 13 Rob Buis 2012-04-06 15:02:58 PDT
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.
Comment 14 Rob Buis 2012-04-06 15:20:30 PDT
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.
Comment 15 Rob Buis 2012-04-06 15:20:50 PDT
Comment on attachment 135812 [details]
Patch

Clearing review flag.
Comment 16 Rob Buis 2012-04-06 15:22:05 PDT
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.