Bug 58531 - Invalid color handling is broken for SVG
Summary: Invalid color handling is broken for SVG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Oliver Varga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-14 05:23 PDT by Oliver Varga
Modified: 2011-06-10 06:43 PDT (History)
8 users (show)

See Also:


Attachments
Draft patch (1.42 KB, patch)
2011-04-14 05:39 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Draft patch second edition (1.87 KB, patch)
2011-04-15 04:11 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
a test SVG file for testing the invalid URI (1.12 KB, image/svg+xml)
2011-04-20 06:38 PDT, Oliver Varga
no flags Details
Draft patch third edition (3.58 KB, patch)
2011-04-20 06:43 PDT, Oliver Varga
zimmermann: review-
Details | Formatted Diff | Diff
updated patch (9.13 KB, patch)
2011-05-23 06:56 PDT, Oliver Varga
zimmermann: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.32 MB, application/zip)
2011-05-23 07:16 PDT, WebKit Review Bot
no flags Details
Added mac png-s and expected files. (198.23 KB, patch)
2011-05-24 07:05 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
Added mac png-s and expected files. (198.32 KB, patch)
2011-05-24 07:11 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff
updated patch (198.30 KB, patch)
2011-05-25 03:02 PDT, Oliver Varga
zimmermann: review-
zimmermann: commit-queue-
Details | Formatted Diff | Diff
updated patch (197.56 KB, patch)
2011-05-25 03:27 PDT, Oliver Varga
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.30 MB, application/zip)
2011-05-25 03:47 PDT, WebKit Review Bot
no flags Details
updated patch (210.79 KB, patch)
2011-05-25 23:59 PDT, Oliver Varga
zimmermann: review-
zimmermann: commit-queue-
Details | Formatted Diff | Diff
updated patch (243.06 KB, patch)
2011-05-31 01:15 PDT, Oliver Varga
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Varga 2011-04-14 05:23:40 PDT
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".
Comment 1 Oliver Varga 2011-04-14 05:39:16 PDT
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.
Comment 2 Dirk Schulze 2011-04-14 06:24:10 PDT
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?
Comment 3 Oliver Varga 2011-04-15 01:37:10 PDT
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.
Comment 4 Oliver Varga 2011-04-15 04:11:27 PDT
Created attachment 89765 [details]
Draft patch second edition

Handling invalid colors in SVG. Fixed color inheritance (fill and stroke too) .
Comment 5 Nikolas Zimmermann 2011-04-15 04:20:16 PDT
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!
Comment 6 Oliver Varga 2011-04-20 06:38:08 PDT
Created attachment 90332 [details]
a test SVG file for testing the invalid URI
Comment 7 Oliver Varga 2011-04-20 06:43:07 PDT
Created attachment 90334 [details]
Draft patch third edition
Comment 8 Robert Longson 2011-04-24 00:54:48 PDT
(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
Comment 9 Dirk Schulze 2011-04-24 21:32:35 PDT
(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?
Comment 10 Oliver Varga 2011-04-28 06:07:20 PDT
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"
Comment 11 Robert Longson 2011-04-28 06:25:57 PDT
(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.
Comment 12 Robert Longson 2011-04-28 06:49:23 PDT
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.
Comment 13 Andras Becsi 2011-05-19 09:16:47 PDT
Oliver, could you please update the patch and put it up for review?
Comment 14 Nikolas Zimmermann 2011-05-23 05:56:39 PDT
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.
Comment 15 Oliver Varga 2011-05-23 06:56:32 PDT
Created attachment 94419 [details]
updated patch

Addressed Niko's comments.
Comment 16 WebKit Review Bot 2011-05-23 07:16:02 PDT
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
Comment 17 WebKit Review Bot 2011-05-23 07:16:07 PDT
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
Comment 18 Nikolas Zimmermann 2011-05-23 07:34:02 PDT
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..
Comment 19 Oliver Varga 2011-05-24 07:05:12 PDT
Created attachment 94606 [details]
Added mac png-s and expected files.

Added mac png-s and expected files.
Comment 20 Oliver Varga 2011-05-24 07:11:28 PDT
Created attachment 94607 [details]
Added mac png-s and expected files.

Added mac png-s and expected files.
Comment 21 Oliver Varga 2011-05-25 03:02:58 PDT
Created attachment 94760 [details]
updated patch
Comment 22 WebKit Review Bot 2011-05-25 03:05:03 PDT
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.
Comment 23 Nikolas Zimmermann 2011-05-25 03:09:56 PDT
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
> +2011-05-25  Oliver Varga  <Varga.Oliver@stud.u-szeged.hu>
> +
> +        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.
Comment 24 Oliver Varga 2011-05-25 03:27:53 PDT
Created attachment 94764 [details]
updated patch
Comment 25 WebKit Review Bot 2011-05-25 03:47:36 PDT
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
Comment 26 WebKit Review Bot 2011-05-25 03:47:44 PDT
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
Comment 27 Oliver Varga 2011-05-25 23:59:41 PDT
Created attachment 94926 [details]
updated patch
Comment 28 Nikolas Zimmermann 2011-05-26 01:47:23 PDT
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.
Comment 29 Oliver Varga 2011-05-31 01:15:04 PDT
Created attachment 95403 [details]
updated patch
Comment 30 Nikolas Zimmermann 2011-05-31 01:41:51 PDT
Comment on attachment 95403 [details]
updated patch

Looks good, r=me.
Comment 31 WebKit Commit Bot 2011-05-31 03:46:40 PDT
Comment on attachment 95403 [details]
updated patch

Clearing flags on attachment: 95403

Committed r87721: <http://trac.webkit.org/changeset/87721>
Comment 32 WebKit Commit Bot 2011-05-31 03:46:55 PDT
All reviewed patches have been landed.  Closing bug.