Invalid color handling is broken for SVG

• Component: SVG
• Status: RESOLVED FIXED

[Oliver Varga]

According to the other browsers ( e.g. Firefox, Opera ) the invalid colors of svg object should be substitued with the inherited one.

Let's see the following example:
svg/custom/invalid-fill-hex.svg

Its result is an orange rectangle in Firefox and Opera. In spite of this webkit has a red one, because it considered the color as "none".

[Oliver Varga]

Created attachment 89563 [details]
Draft patch

It is just a draft, so it doesn't contain test updates. I just would like to ask your opinion.

[Dirk Schulze]

Comment on attachment 89563 [details]
Draft patch

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

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:92
> +            color = object->parent()->style()->svgStyle()->fillPaintColor();

You take the fill-color - always. But what if the color in the stroke attribute is invalid? Don't we want to fallback to strokePaintColor() in this case? Also, do you have a link to the SVG specification where the behavior on invalid colors is described?

[Oliver Varga]

Yes, you're right. I really have to distinguish the two cases.
INSTEAD OF this:
> Source/WebCore/rendering/svg/RenderSVGResource.cpp:92
> +            color = object->parent()->style()->svgStyle()->fillPaintColor();

The new code:
> Source/WebCore/rendering/svg/RenderSVGResource.cpp:92
> + const SVGRenderStyle* tempSvgStyle = object->parent()->style()->svgStyle();
>   color = applyToFill ? tempSvgStyle->fillPaintColor() : tempSvgStyle->strokePaintColor();

As I know in this part of the code just fillColor and strokeColor are handled.

Secondly:
Unfortunatelly I still haven't find any rule in the spec what to do if we have an invalid color. I just followed the behaviour of the other browsers (Opera, Firefox).

But I try to find something what refers to this.

[Oliver Varga]

Created attachment 89765 [details]
Draft patch second edition

Handling invalid colors in SVG. Fixed color inheritance (fill and stroke too) .

[Nikolas Zimmermann]

Comment on attachment 89765 [details]
Draft patch second edition

Hm, this only affects handling of invalid color paint servers, what about URIs? We want a similar mechanism there, no?
This needs much more tests!

[Oliver Varga]

Created attachment 90332 [details]
a test SVG file for testing the invalid URI

[Oliver Varga]

Created attachment 90334 [details]
Draft patch third edition

[Robert Longson]

(In reply to comment #3)
> 
> Secondly:
> Unfortunatelly I still haven't find any rule in the spec what to do if we have an invalid color. I just followed the behaviour of the other browsers (Opera, Firefox).
> 

Invalid colours are ignored. This is not in the SVG spec but comes from CSS1 and SVG combined really. fill and stroke are mapped to CSS in SVG full and CSS1 says that if you can't parse anything it's ignored. Therefore you just treat the element as not having a fill or stroke attribute. For fill the lacuna value is black, for stroke it's none.

There's a w3c test here: http://dev.w3.org/SVG/profiles/1.1F2/test/svg/styling-pres-01-t.svg

[Dirk Schulze]

(In reply to comment #8)
> (In reply to comment #3)
> > 
> > Secondly:
> > Unfortunatelly I still haven't find any rule in the spec what to do if we have an invalid color. I just followed the behaviour of the other browsers (Opera, Firefox).
> > 
> 
> Invalid colours are ignored. This is not in the SVG spec but comes from CSS1 and SVG combined really. fill and stroke are mapped to CSS in SVG full and CSS1 says that if you can't parse anything it's ignored. Therefore you just treat the element as not having a fill or stroke attribute. For fill the lacuna value is black, for stroke it's none.
> 
> There's a w3c test here: http://dev.w3.org/SVG/profiles/1.1F2/test/svg/styling-pres-01-t.svg

I thought we discussed this on the www-svg mailing list as well. But I was unsure about that. Seems the current behavior of WebKit is correct and we can mark this bug as invalid?

[Oliver Varga]

I saw the behavior of the CSS, in my opinion it is relevant. I would illustrated:
<html>
	<style type="text/css">
		body {
			background-color:red;
		}
	</style>
	<body style="background-color:green">
		
	</body>
</html>

Then the background-color is green, but if the color is invalid, it is like I wouldn't set the color.
It follows if I wouldn't set the color, or anything, then the default behavior -> it should inherit those property from its parent.

<html>
	<style type="text/css">
		body {
			background-color:red;
		}
	</style>
	<body style="background-color:INVALIDCOLOR">
		
	</body>
</html>

The behavior of SVG sholud be the same.
What dou you think of it?

ps: ---> http://www.xanthir.com/blog/b4AD0  "Invalid or Undefined Variables"

[Robert Longson]

(In reply to comment #10)
> I saw the behavior of the CSS, in my opinion it is relevant. I would illustrated:
> <html>
>     <style type="text/css">
>         body {
>             background-color:red;
>         }
>     </style>
>     <body style="background-color:green">
> 
>     </body>
> </html>
> 

> What dou you think of it?

I think background-color is a bad case to pick as the initial value is transparent (http://www.w3.org/TR/CSS2/colors.html#propdef-background-color). On top of that you don't have any inheritance in your example above as you've got body both times. Let's say you had this though...

<html>
    <style type="text/css">
        body {
            background-color:red;
        }
    </style>
    <body>
        <div style="width:100%;height:100%;background-color:INVALIDCOLOR">

    </body>
</html>

The div has an invalid background-color so that becomes background-color: transparent and then the body shows through. So whether you had background-color inherit or use the lacuna value, you'd get the same visual result.

[Robert Longson]

This seems to be the rule to follow I think... http://www.w3.org/TR/CSS21/cascade.html#specified-value In the invalid case step 1 is skipped so you get step 2.

[Andras Becsi]

Oliver, could you please update the patch and put it up for review?

[Nikolas Zimmermann]

Comment on attachment 90334 [details]
Draft patch third edition

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

Almost there, though you did not add a new test or upload existing test changes, this looks suspicious, so r-.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:92
> +            const SVGRenderStyle* parentSvgStyle = object->parent()->style()->svgStyle();

To be on the safe side:
if (!object->parent() || !object->parent()->stye())
    return 0;
const SVGRenderStyle* parentSVGStyle = object->parent()->stye()->svgStye();
...

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:105
> +            const SVGRenderStyle* parentSvgStyle = object->parent()->style()->svgStyle();

Same safety guard should be added here.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:118
> +            const SVGRenderStyle* parentSvgStyle = object->parent()->style()->svgStyle();

Ditto.

And please name it parentSVGStyle everywhere.

[Oliver Varga]

Created attachment 94419 [details]
updated patch

Addressed Niko's comments.

[WebKit Review Bot]

Comment on attachment 94419 [details]
updated patch

Attachment 94419 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8729205

New failing tests:
svg/custom/invalid-fill.svg
svg/custom/invalid-fill-hex.svg

[WebKit Review Bot]

Created attachment 94421 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 94419 [details]
updated patch

Great Oliver, almost there. Though you still have to update the invalid-expected.txt/png files, and add new results for your new tests. You need to do this with a mac..

[Oliver Varga]

Created attachment 94606 [details]
Added mac png-s and expected files.

Added mac png-s and expected files.

[Oliver Varga]

Created attachment 94607 [details]
Added mac png-s and expected files.

Added mac png-s and expected files.

[Oliver Varga]

Created attachment 94760 [details]
updated patch

[WebKit Review Bot]

Attachment 94760 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 94760 [details]
updated patch

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

Almost there, please fix the tabs and the duplicated ChangeLog entry, then I'll r+/cq+.

> Source/WebCore/ChangeLog:27
> [phone]  Oliver Varga  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Handling invalid colors in SVG. Fixed color inheritance (fill, stroke, uri).
> +        https://bugs.webkit.org/show_bug.cgi?id=58531
> +
> +        Tests: svg/custom/invalid-stroke-hex.svg
> +               svg/custom/invalid-uri-stroke.svg
> +               svg/custom/invalid-uri.svg
> +
> +        * rendering/svg/RenderSVGResource.cpp: Fix fallback.
> +        (WebCore::requestPaintingResource):

Double entry.

[Oliver Varga]

Created attachment 94764 [details]
updated patch

[WebKit Review Bot]

Comment on attachment 94764 [details]
updated patch

Attachment 94764 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8729820

New failing tests:
svg/custom/invalid-fill.svg
svg/custom/invalid-uri-stroke.svg
svg/custom/invalid-stroke-hex.svg
svg/custom/invalid-fill-hex.svg
svg/custom/invalid-uri.svg

[WebKit Review Bot]

Created attachment 94767 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Oliver Varga]

Created attachment 94926 [details]
updated patch

[Nikolas Zimmermann]

Comment on attachment 94926 [details]
updated patch

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

Okay, it turns out I have even more comments, we're discussing them on IRC soon :-)

> LayoutTests/ChangeLog:8
> +        Handling invalid colors in SVG. Fixed color inheritance (fill, stroke, uri) .
> +        Added png-s and expected files.
> +        https://bugs.webkit.org/show_bug.cgi?id=58531
> +

Oops, I overlooked this probably, the style rules dictates:
<Bug title>
<Bug url>
\n
Your description.

It should read like:
Invalid color handling is broken for SVG
https://bugs.webkit.org/show_bug.cgi?id=58531

Update baselines for the tests covering invalid color handling.

> Source/WebCore/ChangeLog:7
> +        Handling invalid colors in SVG. Fixed color inheritance (fill, stroke, uri) .
> +        Added png-s and expected files.
> +        https://bugs.webkit.org/show_bug.cgi?id=58531

Invalid color handling is broken for SVG
https://bugs.webkit.org/show_bug.cgi?id=58531

Fix invalid color fallback handling. If the fill/stroke attributes computed value leads to a an invalid color, inherit the desired color from the parent style instead. Matches Opera/FF and SVG 1.1 Second Edition (Can you try to look up a link to the spec here, and include it?)

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:88
>      RenderSVGResourceSolidColor* colorResource = RenderSVGResource::sharedSolidPaintingResource();

Please introduce a helper function above this function:
static inline bool inheritColorFromParentStyleIfNeeded(RenderObject* object, bool applyToFill, Color& color)
{
    if (color.isValid())
        return true;
    if (!object->parent() || !object->parent()->style())
        return false;
    const SVGRenderStyle* parentSVGStyle = object->parent()->style()->svgStyle();
    color = applyToFill ? parentSVGStyle->fillPaintColor() : parentSVGStyle->strokePaintColor();
}

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:89
>      if (paintType < SVGPaint::SVG_PAINTTYPE_URI_NONE) {

Comment is invalid, and can be avoided at all, as the "inheritColorFromParentStyleIfNeeded" function is descriptive now...

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:-92
> -        if (!color.isValid())
> -            return 0;

Instead if:
if (!color.isValid())
    return 0;
use
if (!inheritColorFromParentStyleIfNeeded(object, applyToFill, color))
    return 0;

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:105
>          // If a paint server is specified, and no or an invalid fallback color is given, default to fill/stroke="black".

The comment is invalid now, remove it.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:111
> -        if (!color.isValid())
> -            color = Color::black;
> +        if (!color.isValid()) {
> +            if (!object->parent() || !object->parent()->style())
> +                return 0;
> +           const SVGRenderStyle* parentSVGStyle = object->parent()->style()->svgStyle();
> +           color = applyToFill ? parentSVGStyle->fillPaintColor() : parentSVGStyle->strokePaintColor();
> +        }

Use the new helper function.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:120
>          // If a paint server is specified, and no or an invalid fallback color is given, default to fill/stroke="black".

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:126
> +        if (!color.isValid()) {
> +            if (!object->parent() || !object->parent()->style())
> +                return 0;
> +           const SVGRenderStyle* parentSVGStyle = object->parent()->style()->svgStyle();
> +           color = applyToFill ? parentSVGStyle->fillPaintColor() : parentSVGStyle->strokePaintColor();
> +        }

Ditto.

[Oliver Varga]

Created attachment 95403 [details]
updated patch

[Nikolas Zimmermann]

Comment on attachment 95403 [details]
updated patch

Looks good, r=me.

[WebKit Commit Bot]

Comment on attachment 95403 [details]
updated patch

Clearing flags on attachment: 95403

Committed r87721: <http://trac.webkit.org/changeset/87721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.