Currently 'transparent' is not considered a valid color but CSSParser::parseColor lies and does not report it to be wrong. As part of 39168, I have made a change to make CSSParser::parseColor not lie anymore as some caller needs to fall back to another methods. Thus this change is now testable.
Created attachment 67073 [details] Proposed fix: Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque.
Comment on attachment 67073 [details] Proposed fix: Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque. > + * svg/animations/svg-animate-color-transparent-expected.txt: Added. > + * svg/animations/svg-animate-color-transparent.svg: Added. These files are missing from the patch.
Created attachment 67284 [details] Proposed fix (with the test case): Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque.
Created attachment 67288 [details] Proposed fix (now with the right test case): Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque.
Comment on attachment 67288 [details] Proposed fix (now with the right test case): Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque. Interesting test. r=me
Created attachment 68780 [details] Same as previously but updated to remove the work-around from bug 39168 (which we depend on)
Comment on attachment 68780 [details] Same as previously but updated to remove the work-around from bug 39168 (which we depend on) Why not just land the already-reviewed fix from this bug first? I don’t understand why we are landing the fix to bug 39168 first.
Comment on attachment 68780 [details] Same as previously but updated to remove the work-around from bug 39168 (which we depend on) (In reply to comment #7) > (From update of attachment 68780 [details]) > Why not just land the already-reviewed fix from this bug first? I don’t understand why we are landing the fix to bug 39168 first. Looking back, this is feasible. The ordering was done to prove the progression (we make this test fail, then solve it). However it seems like we need to fix the transparent issue anyway as other tests needs it. I will go ahead and land both unmodified r+ patches in the order Darin suggested tomorrow.
Comment on attachment 67288 [details] Proposed fix (now with the right test case): Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque. Cleared Andreas Kling's review+ from obsolete attachment 67288 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Finally committed the 3rd patch (r+ by Andreas) with some tweak in r69937.
Hey guys, I've just seen that you've added a test to svg/animations, which is flawed. The svg/animations testcases should _only_ use the DRT API to snapshot SVG animations at a certain time, see the existing tests. Using setTimeout(..., 100) is flawed. It would be easy to rewrite the test using the snapshot framework, see the existing examples in script-tests/*. Only tests which test crashes/assertions are allowed to live in svg/animations, and don't need to use the SVGAnimationTestCase.js framework. Cheers, Niko
Created attachment 71438 [details] Rework test to use SVG animation testing shizzle
Comment on attachment 71438 [details] Rework test to use SVG animation testing shizzle View in context: https://bugs.webkit.org/attachment.cgi?id=71438&action=review Much better! Please fix some issues before landing: > LayoutTests/svg/animations/script-tests/animate-color-transparent.js:6 > +rect.setAttribute("id", "test"); This is not needed. > LayoutTests/svg/animations/script-tests/animate-color-transparent.js:12 > +animate.setAttribute("id", "animation"); Ditto. > LayoutTests/svg/animations/script-tests/animate-color-transparent.js:44 > +window.setTimeout("triggerUpdate(15, 30)", 0); Maybe use 50, 50 here, to indicate the middle of the rect, looks less obscure.
(In reply to comment #13) > (From update of attachment 71438 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71438&action=review > > Much better! Please fix some issues before landing: > > > LayoutTests/svg/animations/script-tests/animate-color-transparent.js:6 > > +rect.setAttribute("id", "test"); > > This is not needed. > > > LayoutTests/svg/animations/script-tests/animate-color-transparent.js:12 > > +animate.setAttribute("id", "animation"); > > Ditto. oops, forgot these ids are passed in runAnimationTest :-) Just ignore these two comments.
Committed r70236: <http://trac.webkit.org/changeset/70236>