Bug 16183 - SVG doesn't support rgba colors
Summary: SVG doesn't support rgba colors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
: 63912 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-11-29 01:20 PST by Adam Roben (:aroben)
Modified: 2011-07-04 09:43 PDT (History)
6 users (show)

See Also:


Attachments
testcase (201 bytes, image/svg+xml)
2007-11-29 01:20 PST, Adam Roben (:aroben)
no flags Details
Patch removing the blocks on hsl, hsla and rgba color specifications from SVG parsing (5.37 KB, patch)
2010-06-03 11:15 PDT, Jonathan Kliegman
krit: review-
krit: commit-queue-
Details | Formatted Diff | Diff
Patch removing the blocks on hsl, hsla and rgba color specifications from SVG parsing (6.99 KB, patch)
2010-06-03 14:43 PDT, Jonathan Kliegman
krit: review-
krit: commit-queue-
Details | Formatted Diff | Diff
Enable hsl, hsla and rgba color specifications in svg formats (7.57 KB, patch)
2010-06-04 07:25 PDT, Jonathan Kliegman
no flags Details | Formatted Diff | Diff
Enable hsl, hsla and rgba color specifications in svg formats (7.66 KB, patch)
2010-06-04 07:37 PDT, Jonathan Kliegman
krit: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Enable hsl, hsla and rgba color specifications in svg formats (10.53 KB, patch)
2010-06-04 11:52 PDT, Jonathan Kliegman
eric: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Enable hsl, hsla and rgba color specifications in svg formats (10.56 KB, patch)
2010-06-05 06:41 PDT, Jonathan Kliegman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2007-11-29 01:20:02 PST
SVG doesn't support rgba colors. I'd expect them to be supported at least for things like stroke and fill.
Comment 1 Adam Roben (:aroben) 2007-11-29 01:20:33 PST
Created attachment 17589 [details]
testcase

There should be a green-stroked square in this SVG.
Comment 2 Eric Seidel (no email) 2007-11-29 01:29:16 PST
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.
Comment 3 Jonathan Kliegman 2010-06-03 11:15:15 PDT
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)
Comment 4 Dirk Schulze 2010-06-03 11:29:31 PDT
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?
Comment 5 Jonathan Kliegman 2010-06-03 12:47:13 PDT
Not sure I understand your comments.  Do you just want me to remove the source code comments and resubmit?
Comment 6 Dirk Schulze 2010-06-03 13:02:02 PDT
(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?
Comment 7 Jonathan Kliegman 2010-06-03 14:43:29 PDT
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
Comment 8 Dirk Schulze 2010-06-04 01:08:12 PDT
(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.
Comment 9 Jonathan Kliegman 2010-06-04 06:35:55 PDT
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.
Comment 10 Jonathan Kliegman 2010-06-04 07:25:33 PDT
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
Comment 11 WebKit Review Bot 2010-06-04 07:29:51 PDT
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.
Comment 12 Jonathan Kliegman 2010-06-04 07:37:44 PDT
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
Comment 13 Dirk Schulze 2010-06-04 07:44:11 PDT
Comment on attachment 57874 [details]
Enable hsl, hsla and rgba color specifications in svg formats

r=me
Comment 14 WebKit Commit Bot 2010-06-04 09:29:48 PDT
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
Comment 15 Dirk Schulze 2010-06-04 09:45:40 PDT
(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'
Comment 16 Jonathan Kliegman 2010-06-04 11:52:22 PDT
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)
Comment 17 Dirk Schulze 2010-06-04 12:15:54 PDT
Comment on attachment 57901 [details]
Enable hsl, hsla and rgba color specifications in svg formats

r=me
Comment 18 WebKit Commit Bot 2010-06-04 21:28:28 PDT
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 eseidel@chromium.org...
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).
Comment 19 Eric Seidel (no email) 2010-06-04 21:49:02 PDT
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. :)
Comment 20 Jonathan Kliegman 2010-06-05 06:41:34 PDT
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)
Comment 21 Dirk Schulze 2010-06-05 07:13:35 PDT
Comment on attachment 57966 [details]
Enable hsl, hsla and rgba color specifications in svg formats

r=me
Comment 22 WebKit Commit Bot 2010-06-05 07:30:09 PDT
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>
Comment 23 WebKit Commit Bot 2010-06-05 07:30:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Simon Fraser (smfr) 2011-07-04 09:43:05 PDT
*** Bug 63912 has been marked as a duplicate of this bug. ***