Bug 40880 – SVG properties fill and stroke do not accept system colors

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

The problem is clearly demonstrated by the given url

[Rob Buis]

Created attachment 59188 [details]
Patch

[Rob Buis]

Created attachment 59620 [details]
Test all css system colors

[Dirk Schulze]

I would put all system colors into an array and create the elements on the fly by  with it's property in a loop, that would make the test much more readable. Something like:

svgElement = document.getElementsByTagName("svg")[0];
systemColors = new Array("Window", "WindowFrame", ...)
for(int i=0;i<systemColors.length;++i){
...


Every PASS message has the same result.  Maybe you can add the current system Color name and the real RGB numbers. This makes it easier to analyse the test on regression that may occure later and should be easy to implement with the mentioned loop of above.

What do you think?

[Rob Buis]

Hello Dirk,
(In reply to comment #3)
> I would put all system colors into an array and create the elements on the fly by  with it's property in a loop, that would make the test much more readable. Something like:
> 
> svgElement = document.getElementsByTagName("svg")[0];
> systemColors = new Array("Window", "WindowFrame", ...)
> for(int i=0;i<systemColors.length;++i){
> ...
> 
> 
> Every PASS message has the same result.  Maybe you can add the current system Color name and the real RGB numbers. This makes it easier to analyse the test on regression that may occure later and should be easy to implement with the mentioned loop of above.
> 
> What do you think?

You are right of course! I already felt when reediting the text that it was a bit "exhaustive" and verbose. I like the suggestions and will create an improved patch, after which we can hopefully wrap this one up :)
Cheers,

Rob.

PS: even though I discovered "Menu" system color needed to be supported compared to the old patch(done with this patch), it doesnt solve "text on background with similar color" problem. For the moment I assume this is just because unfortunate system colors were supported, or maybe some not at all with a default to black?

[Dirk Schulze]

(In reply to comment #4)
> You are right of course! I already felt when reediting the text that it was a bit "exhaustive" and verbose. I like the suggestions and will create an improved patch, after which we can hopefully wrap this one up :)
> Cheers,
> 
> Rob.
> 
> PS: even though I discovered "Menu" system color needed to be supported compared to the old patch(done with this patch), it doesnt solve "text on background with similar color" problem. For the moment I assume this is just because unfortunate system colors were supported, or maybe some not at all with a default to black?

Hi Rob,

Menu and others have a default color. Menu black, others can have different colors. This is allowed according to the Spec:
"For systems that do not have a corresponding value, the specified value should be mapped to the nearest system value, or to a default color."
At the long term even the default vlaues schould move to the platform code. But this is not our problem at the moment and should be solved in RenderTheme instead of SVG.

PS: I was wrong, system colors are part of CSS2 and therefor have to be supported by SVG :-)

[Rob Buis]

Created attachment 59689 [details]
Incorporate Dirks suggestions into test

[Dirk Schulze]

Comment on attachment 59689 [details]
Incorporate Dirks suggestions into test

Great Patch! r=me

[WebKit Review Bot]

http://trac.webkit.org/changeset/61837 might have broken Qt Linux Release

[Rob Buis]

Fixed by r61837.