Bug 45482 – 'transparent' should be a valid color

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 67073 [details]
Proposed fix: Modify our known colors to include transparent. Fixed the logic to account for color non-fully opaque.

[Andreas Kling]

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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

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.

[Andreas Kling]

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

[Julien Chaffraix]

Created attachment 68780 [details]
Same as previously but updated to remove the work-around from bug 39168 (which we depend on)

[Darin Adler]

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.

[Julien Chaffraix]

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.

[Eric Seidel (no email)]

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.

[Julien Chaffraix]

Finally committed the 3rd patch (r+ by Andreas) with some tweak in r69937.

[Nikolas Zimmermann]

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

[Andreas Kling]

Created attachment 71438 [details]
Rework test to use SVG animation testing shizzle

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

(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.

[Andreas Kling]

Committed r70236: <http://trac.webkit.org/changeset/70236>