Bug 15012

Created attachment 16026 [details] Testcase Quick testcase showing the problem. Try in Opera to verify that it should report SVGColor, not CSSPrimitiveValue.

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.

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 on attachment 16117 [details] First attempt I'm also not a fan of a separate parsing mode. r-'ing after Maciej's comments.

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

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.

Created attachment 134981 [details] Patch

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?

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.

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

Created attachment 135812 [details] Patch

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

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.

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 on attachment 135812 [details] Patch Clearing review flag.

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.