Summary: | SVG doesn't support rgba colors | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, eric, kliegs, krit, simon.fraser, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | HasReduction | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Adam Roben (:aroben)
2007-11-29 01:20:02 PST
Created attachment 17589 [details]
testcase
There should be a green-stroked square in this SVG.
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. 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 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?
Not sure I understand your comments. Do you just want me to remove the source code comments and resubmit? (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? 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
(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. 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. 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
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.
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 on attachment 57874 [details]
Enable hsl, hsla and rgba color specifications in svg formats
r=me
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 (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' 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 on attachment 57901 [details]
Enable hsl, hsla and rgba color specifications in svg formats
r=me
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 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. :)
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 on attachment 57966 [details]
Enable hsl, hsla and rgba color specifications in svg formats
r=me
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> All reviewed patches have been landed. Closing bug. *** Bug 63912 has been marked as a duplicate of this bug. *** |