Bug 16183 - SVG doesn't support rgba colors
: SVG doesn't support rgba colors
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: HasReduction
:
:
  Show dependency treegraph
 
Reported: 2007-11-29 01:20 PST by
Modified: 2011-07-04 09:43 PST (History)


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 PST, Jonathan Kliegman
krit: review-
krit: commit‑queue-
Review Patch | 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 PST, Jonathan Kliegman
krit: review-
krit: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Enable hsl, hsla and rgba color specifications in svg formats (7.57 KB, patch)
2010-06-04 07:25 PST, Jonathan Kliegman
no flags Review Patch | Details | Formatted Diff | Diff
Enable hsl, hsla and rgba color specifications in svg formats (7.66 KB, patch)
2010-06-04 07:37 PST, Jonathan Kliegman
krit: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Enable hsl, hsla and rgba color specifications in svg formats (10.53 KB, patch)
2010-06-04 11:52 PST, Jonathan Kliegman
eric: review-
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Enable hsl, hsla and rgba color specifications in svg formats (10.56 KB, patch)
2010-06-05 06:41 PST, Jonathan Kliegman
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2007-11-29 01:20:33 PST -------
Created an attachment (id=17589) [details]
testcase

There should be a green-stroked square in this SVG.
------- Comment #2 From 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 From 2010-06-03 11:15:15 PST -------
Created an attachment (id=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 From 2010-06-03 11:29:31 PST -------
(From update of attachment 57792 [details])
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 From 2010-06-03 12:47:13 PST -------
Not sure I understand your comments.  Do you just want me to remove the source code comments and resubmit?
------- Comment #6 From 2010-06-03 13:02:02 PST -------
(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 From 2010-06-03 14:43:29 PST -------
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
------- Comment #8 From 2010-06-04 01:08:12 PST -------
(In reply to comment #7)
> Created an attachment (id=57815) [details] [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 From 2010-06-04 06:35:55 PST -------
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 From 2010-06-04 07:25:33 PST -------
Created an attachment (id=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 From 2010-06-04 07:29:51 PST -------
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 From 2010-06-04 07:37:44 PST -------
Created an attachment (id=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 From 2010-06-04 07:44:11 PST -------
(From update of attachment 57874 [details])
r=me
------- Comment #14 From 2010-06-04 09:29:48 PST -------
(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
------- Comment #15 From 2010-06-04 09:45:40 PST -------
(In reply to comment #14)
> (From update of attachment 57874 [details] [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 From 2010-06-04 11:52:22 PST -------
Created an attachment (id=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 From 2010-06-04 12:15:54 PST -------
(From update of attachment 57901 [details])
r=me
------- Comment #18 From 2010-06-04 21:28:28 PST -------
(From update of attachment 57901 [details])
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 From 2010-06-04 21:49:02 PST -------
(From update of attachment 57901 [details])
Please do not delete the "Reviewed by NOBODY (OOPS!)." line when using the automated toolchain. :)
------- Comment #20 From 2010-06-05 06:41:34 PST -------
Created an attachment (id=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 From 2010-06-05 07:13:35 PST -------
(From update of attachment 57966 [details])
r=me
------- Comment #22 From 2010-06-05 07:30:09 PST -------
(From update of attachment 57966 [details])
Clearing flags on attachment: 57966

Committed r60752: <http://trac.webkit.org/changeset/60752>
------- Comment #23 From 2010-06-05 07:30:17 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #24 From 2011-07-04 09:43:05 PST -------
*** Bug 63912 has been marked as a duplicate of this bug. ***