Bug 45482

Summary: 'transparent' should be a valid color
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: WebCore Misc.Assignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: kling, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 39168    
Bug Blocks:    
Attachments:
Description Flags
Proposed fix: Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque.
kling: review-
Proposed fix (with the test case): Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque.
none
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.
none
Same as previously but updated to remove the work-around from bug 39168 (which we depend on)
none
Rework test to use SVG animation testing shizzle none

Description Julien Chaffraix 2010-09-09 11:43:57 PDT
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.
Comment 1 Julien Chaffraix 2010-09-09 11:47:44 PDT
Created attachment 67073 [details]
Proposed fix: Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque.
Comment 2 Andreas Kling 2010-09-10 01:05:30 PDT
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.
Comment 3 Julien Chaffraix 2010-09-10 18:57:52 PDT
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.
Comment 4 Julien Chaffraix 2010-09-10 19:24:05 PDT
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 5 Andreas Kling 2010-09-11 07:16:28 PDT
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
Comment 6 Julien Chaffraix 2010-09-24 16:09:10 PDT
Created attachment 68780 [details]
Same as previously but updated to remove the work-around from bug 39168 (which we depend on)
Comment 7 Darin Adler 2010-09-24 18:07:10 PDT
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 8 Julien Chaffraix 2010-09-27 20:46:00 PDT
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 9 Eric Seidel (no email) 2010-09-28 03:19:10 PDT
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.
Comment 10 Julien Chaffraix 2010-10-17 18:28:29 PDT
Finally committed the 3rd patch (r+ by Andreas) with some tweak in r69937.
Comment 11 Nikolas Zimmermann 2010-10-17 23:49:43 PDT
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
Comment 12 Andreas Kling 2010-10-21 08:05:08 PDT
Created attachment 71438 [details]
Rework test to use SVG animation testing shizzle
Comment 13 Nikolas Zimmermann 2010-10-21 08:12:52 PDT
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.
Comment 14 Nikolas Zimmermann 2010-10-21 08:20:30 PDT
(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.
Comment 15 Andreas Kling 2010-10-21 08:34:29 PDT
Committed r70236: <http://trac.webkit.org/changeset/70236>