Bug 16183 – SVG doesn't support rgba colors

• Component: SVG
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

SVG doesn't support rgba colors. I'd expect them to be supported at least for things like stroke and fill.

[Adam Roben (:aroben)]

Created attachment 17589 [details]
testcase

There should be a green-stroked square in this SVG.

[Eric Seidel (no email)]

I think we currently have some ASSERTs in debug mode to catch when a color value has an alpha component, since we don't currently add any alpha from the color into our calculations.

To fix this, we would have to re-enable rgba parsing for the SVG color parsing path, and then get rid of those asserts. It might "just work" with the code as is, since GraphicsContext knows how to handle color values in RGBA.

SVG 1.1 only talks about handling RGB values, since opacity and stroke-opacity/fill-opacity are generally the ways to handle alpha.  However I can see that those coming from the HTML/CSS world would expect this type of functionality, so we should add it.

[Jonathan Kliegman]

Created attachment 57792 [details]
Patch removing the blocks on hsl, hsla and rgba color specifications from SVG parsing

This removes the checks to prevent processing of hsl, hsla and rgba color specifications in SVG.   The SVG spec calls for CSS2 compliance and hsl, hsla and rgba are part of CSS3 so not technically supported by the standard.

Discussion on webkit-dev sees no harm in enabling this capability as other browsers have done so and its what web developers and users will expect (archive here - https://lists.webkit.org/pipermail/webkit-dev/2010-June/013027.html)

[Dirk Schulze]

Comment on attachment 57792 [details]
Patch removing the blocks on hsl, hsla and rgba color specifications from SVG parsing

The comments are irrelevant. Also remove the comment // hsl, hsla and rgba are not in the SVG spec.
The test just check rgba, but not hsl and hsla.
Did something went wrong with the pixel test result?

[Jonathan Kliegman]

Not sure I understand your comments.  Do you just want me to remove the source code comments and resubmit?

[Dirk Schulze]

(In reply to comment #5)
> Not sure I understand your comments.  Do you just want me to remove the source code comments and resubmit?

Yes, but more important: more tests to check the behavior for hsl and hsla, since your patch also activate this values for SVG. Also, the pretty diff is talking about /dev/null for the pixel test result. Can you take a look at it?

[Jonathan Kliegman]

Created attachment 57815 [details]
Patch removing the blocks on hsl, hsla and rgba color specifications from SVG parsing

Updated patch with hsl and hsla test added.  Extraneous comments describing the change removed from the code

[Dirk Schulze]

(In reply to comment #7)
> Created an attachment (id=57815) [details]
> Patch removing the blocks on hsl, hsla and rgba color specifications from SVG parsing
> 
> Updated patch with hsl and hsla test added.  Extraneous comments describing the change removed from the code

Not sure why you changed the style of the ChangeLog's?!? Can you please update the patch and follow the prevered style of a ChangeLog? That means original bug report title, link to the bug, description if your changes and added Tests at the end? You can also use WebKitTools/Scripts/prepare-ChangeLog --bug 16183. That helps to create a reasonable ChangeLog.
If you're on it, can you explain the context a bit more? Say that the Spec normally uses CSS2, but that we also provide colors in the way of CSS3, that we also follow other browser vendors with this change and so on. I already wrote it in the last review, but can you also delete the comment in SVGColor?: // hsl, hsla and rgba are not in the SVG spec.

To the tests: Why do you test stroke? Wouldn't it be more sensible to test fill? The problem is, that with stroke the tolerance level on pixel tests may not get crossed on wrong code changes. Pixeltest ignore changes that are lower than 0.1%, this can cause regressions that noone recognize.

There is still something wrong with your binarys. The pixel test results are still not in the patch. Can you take a look at it again?

Patch still looks great! I hope you take the suggestions into account and upload a new patch.

[Jonathan Kliegman]

Thanks for your patience Dirk.  

Sorry about the changelog.  I'd entered into a local commit many of the same comments so after running the prepare-Changelog script the changelog had duplicate lines and I must have overcleaned.  

I'll remove the other comments and change the test to use fill - I was using stroke as the initial test case I saw used stroke so was working off of that.

I'll check the prepared patch more closely this time and make sure the binary files are properly included as well.

[Jonathan Kliegman]

Created attachment 57872 [details]
Enable hsl, hsla and rgba color specifications in svg formats

Enabled processing of rgba, hsl and hsla color specifications for SVG files.  SVG spec calls for CSS2 but common usage and other browsers support CSS3 colors being used in SVG files

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 9 files


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

[Jonathan Kliegman]

Created attachment 57874 [details]
Enable hsl, hsla and rgba color specifications in svg formats

Enabled processing of rgba, hsl and hsla color specifications for SVG files.  SVG spec calls for CSS2 but common usage and other browsers support CSS3 colors being used in SVG files

[Dirk Schulze]

Comment on attachment 57874 [details]
Enable hsl, hsla and rgba color specifications in svg formats

r=me

[WebKit Commit Bot]

Comment on attachment 57874 [details]
Enable hsl, hsla and rgba color specifications in svg formats

Rejecting patch 57874 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
R 0310
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/CSSParser.o /Users/eseidel/Projects/CommitQueue/WebCore/css/CSSParser.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/3070043

[Dirk Schulze]

(In reply to comment #14)
> (From update of attachment 57874 [details])
> Rejecting patch 57874 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
> Last 500 characters of output:
> R 0310
>     setenv YACC /Developer/usr/bin/yacc
>     /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
> ** BUILD FAILED **
> 
> The following build commands failed:
> WebCore:
>     Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/CSSParser.o /Users/eseidel/Projects/CommitQueue/WebCore/css/CSSParser.cpp normal i386 c++ com.apple.compilers.gcc.4_2
> (1 failure)
> 
> 
> Full output: http://webkit-commit-queue.appspot.com/results/3070043

the problem is:
/Users/eseidel/Projects/CommitQueue/WebCore/css/CSSParser.cpp:3797: warning: unused parameter 'svg'

[Jonathan Kliegman]

Created attachment 57901 [details]
Enable hsl, hsla and rgba color specifications in svg formats

Enabled processing of rgba, hsl and hsla color specifications for SVG files.  SVG spec calls for CSS2 but common usage and other browsers support CSS3 colors being used in SVG files

Removed unused svg parameter from CSSParser::parseColorFromValue and callers (parameter made obsolete by this change)

[Dirk Schulze]

Comment on attachment 57901 [details]
Enable hsl, hsla and rgba color specifications in svg formats

r=me

[WebKit Commit Bot]

Comment on attachment 57901 [details]
Enable hsl, hsla and rgba color specifications in svg formats

Rejecting patch 57901 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 57901, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=57901&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=16183&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 57901 from bug 16183.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Eric Seidel (no email)]

Comment on attachment 57901 [details]
Enable hsl, hsla and rgba color specifications in svg formats

Please do not delete the "Reviewed by NOBODY (OOPS!)." line when using the automated toolchain. :)

[Jonathan Kliegman]

Created attachment 57966 [details]
Enable hsl, hsla and rgba color specifications in svg formats

Enabled processing of rgba, hsl and hsla color specifications for SVG files.  SVG spec calls for CSS2 but common usage and other browsers support CSS3 colors being used in SVG files

Removed unused svg parameter from CSSParser::parseColorFromValue and callers (parameter made obsolete by this change)

[Dirk Schulze]

Comment on attachment 57966 [details]
Enable hsl, hsla and rgba color specifications in svg formats

r=me

[WebKit Commit Bot]

Comment on attachment 57966 [details]
Enable hsl, hsla and rgba color specifications in svg formats

Clearing flags on attachment: 57966

Committed r60752: <http://trac.webkit.org/changeset/60752>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

*** Bug 63912 has been marked as a duplicate of this bug. ***