REGRESSION (r109610): Order of values in shorthand animation makes a difference

• Component: CSS
• Status: RESOLVED FIXED

[Lea Verou]

In WebKit build 536.9 (@114735) (current Chrome Canary and WebKit Nightlies), the animation shorthand is broken. It requires animation-name to be before animation-delay, although the spec mandates no such restriction: http://www.w3.org/TR/css3-animations/#animation-shorthand-property

Attached (WebKit-only) testcase is red when the bug is present and green when it isn't.

[Igor Trindade Oliveira]

http://trac.webkit.org/changeset/109610 is causing this bug. The spec also allows animation-name have different names. So, if we have an animation called "infinite" and we have the follow -webkit-animation: 
-webkit-animation: 1s test infinite 2; 
where the 2 is the animation-iteration-count,would be a little bit tricky to know what is the animation name.

(In reply to comment #0)
> In WebKit build 536.9 (@114735) (current Chrome Canary and WebKit Nightlies), the animation shorthand is broken. It requires animation-name to be before animation-delay, although the spec mandates no such restriction: http://www.w3.org/TR/css3-animations/#animation-shorthand-property
> 
> Attached (WebKit-only) testcase is red when the bug is present and green when it isn't.

[Radar WebKit Bug Importer]

<rdar://problem/11302643>

[Igor Trindade Oliveira]

The right fix for this bug needs to be done in the parser. When the animation shorthand is being parsed we should know what is the keyframe name, otherwise it will not possible to know what is the animation name. The changeset http://trac.webkit.org/changeset/109610 just did this bug more visible.

[Igor Trindade Oliveira]

Created attachment 138418 [details]
Test case2

Other test case.

[Igor Trindade Oliveira]

Created attachment 138419 [details]
Test case2

Oops now the right test case. This test case does not work before neither after r109610.,

[Igor Trindade Oliveira]

Created attachment 138682 [details]
Patch

Proposed patch.

[WebKit Review Bot]

Comment on attachment 138682 [details]
Patch

Attachment 138682 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12530522

New failing tests:
fast/css/transform-inline-style-remove.html

[WebKit Review Bot]

Created attachment 138766 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Igor Trindade Oliveira]

Created attachment 138848 [details]
Patch

Proposed patch.

[Alexis Menard (darktears)]

Comment on attachment 138848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138848&action=review

> Source/WebCore/css/CSSParser.cpp:2726
> +

I don't like to copy again the array back here. I was wondering if parseAnimationName could be patched to ignore value->id of others animations properties (e.g.infinite, ...)?

[Alexis Menard (darktears)]

(In reply to comment #10)
> (From update of attachment 138848 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138848&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:2726
> > +
> 
> I don't like to copy again the array back here. I was wondering if parseAnimationName could be patched to ignore value->id of others animations properties (e.g.infinite, ...)?

I also believe that calling a keyframe infinite or linear should not be supported later as animation-name. It conflicts with other properties.

[Dean Jackson]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 138848 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138848&action=review
> > 
> > > Source/WebCore/css/CSSParser.cpp:2726
> > > +
> > 
> > I don't like to copy again the array back here. I was wondering if parseAnimationName could be patched to ignore value->id of others animations properties (e.g.infinite, ...)?
> 
> I also believe that calling a keyframe infinite or linear should not be supported later as animation-name. It conflicts with other properties.

This makes sense, but it's acceptable to do this when not using shorthands, so I don't think we should disallow it. Shorthands are a convenience - that convenience comes with some restrictions.

[Dean Jackson]

Comment on attachment 138848 [details]
Patch

r- while we discuss the best way to fix this.

Alexis makes a good point that we could reject this in parseAnimationName. Or we could just change the order of the shorthand array.

[Dean Jackson]

I think we should ask on www-style. We can't reject animation-names without the spec saying so.

[Igor Trindade Oliveira]

Changing the order of shorthand array affects the webkitAnimation order(i did it in the first patch). And about the parseAnimationName, we will change how the content is handled and can break some sites.


(In reply to comment #13)
> (From update of attachment 138848 [details])
> r- while we discuss the best way to fix this.
> 
> Alexis makes a good point that we could reject this in parseAnimationName. Or we could just change the order of the shorthand array.

[Alexis Menard (darktears)]

(In reply to comment #15)
> Changing the order of shorthand array affects the webkitAnimation order(i did it in the first patch). And about the parseAnimationName, we will change how the content is handled and can break some sites.

Yes that's why I'd rather seek for clarification in the wg and fix the spec if needed. Would you mind sending a mail to the css-wg?

Other question : what is the order FF and Opera are returning, and more how the test case behave there? It's interesting input when writing the mail to the wg.

> 
> 
> (In reply to comment #13)
> > (From update of attachment 138848 [details] [details])
> > r- while we discuss the best way to fix this.
> > 
> > Alexis makes a good point that we could reject this in parseAnimationName. Or we could just change the order of the shorthand array.

[Alexis Menard (darktears)]

(In reply to comment #16)
> (In reply to comment #15)
> > Changing the order of shorthand array affects the webkitAnimation order(i did it in the first patch). And about the parseAnimationName, we will change how the content is handled and can break some sites.
> 
> Yes that's why I'd rather seek for clarification in the wg and fix the spec if needed. Would you mind sending a mail to the css-wg?
> 
> Other question : what is the order FF and Opera are returning, and more how the test case behave there? It's interesting input when writing the mail to the wg.

Well according to my quick test they suffer the same problem.

> 
> > 
> > 
> > (In reply to comment #13)
> > > (From update of attachment 138848 [details] [details] [details])
> > > r- while we discuss the best way to fix this.
> > > 
> > > Alexis makes a good point that we could reject this in parseAnimationName. Or we could just change the order of the shorthand array.

[Alexis Menard (darktears)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 138848 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=138848&action=review
> > > 
> > > > Source/WebCore/css/CSSParser.cpp:2726
> > > > +
> > > 
> > > I don't like to copy again the array back here. I was wondering if parseAnimationName could be patched to ignore value->id of others animations properties (e.g.infinite, ...)?
> > 
> > I also believe that calling a keyframe infinite or linear should not be supported later as animation-name. It conflicts with other properties.
> 
> This makes sense, but it's acceptable to do this when not using shorthands, so I don't think we should disallow it. Shorthands are a convenience - that convenience comes with some restrictions.

Yes, we could just restrict that in the shorthand, very valid point. We do have some restrictions also in some other shorthands.

[Igor Trindade Oliveira]

There is a W3C bug opened about this problem: https://www.w3.org/Bugs/Public/show_bug.cgi?id=14790

(In reply to comment #18)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > (From update of attachment 138848 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=138848&action=review
> > > > 
> > > > > Source/WebCore/css/CSSParser.cpp:2726
> > > > > +
> > > > 
> > > > I don't like to copy again the array back here. I was wondering if parseAnimationName could be patched to ignore value->id of others animations properties (e.g.infinite, ...)?
> > > 
> > > I also believe that calling a keyframe infinite or linear should not be supported later as animation-name. It conflicts with other properties.
> > 
> > This makes sense, but it's acceptable to do this when not using shorthands, so I don't think we should disallow it. Shorthands are a convenience - that convenience comes with some restrictions.
> 
> Yes, we could just restrict that in the shorthand, very valid point. We do have some restrictions also in some other shorthands.

[Simon Fraser (smfr)]

Bug 71623 caused a bug where animation-name had to be first in the shorthand, so we need a fix!

[Igor Trindade Oliveira]

Ok .. i will fix that ASAP.

(In reply to comment #20)
> Bug 71623 caused a bug where animation-name had to be first in the shorthand, so we need a fix!

[Dean Jackson]

Created attachment 151385 [details]
Patch

[Dean Jackson]

<rdar://problem/11831924>


The current patch will do as we described here - if you want an animation name that's "forwards", "both", "backwards", "ease", "infinite" etc then you'll almost definitely need to use the longhand properties, because the shorthand will try to match name last.

Comment on attachment 151385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151385&action=review

> Source/WebCore/css/CSSParser.cpp:3054
> +    // We want the computed style of the shorthand to display with
> +    // the animation-name property first, but when we parse it we
> +    // need to look for animation-name last because otherwise it might
> +    // match against the keywords for fill mode, timing functions and iteration.
> +    // This means that animation names that are the same as keywords
> +    // (e.g. 'forwards') won't always match in the shorthand. In that
> +    // case they should be using longhands (or reconsidering their
> +    // approach).

My reading of this comment is that getting the computed value of the property and setting it to that could break things. Is it true? If so, is it true for any other shorthand?

[Igor Trindade Oliveira]

Yes, Other css shorthand do the same thing.

(In reply to comment #24)
> (From update of attachment 151385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151385&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:3054
> > +    // We want the computed style of the shorthand to display with
> > +    // the animation-name property first, but when we parse it we
> > +    // need to look for animation-name last because otherwise it might
> > +    // match against the keywords for fill mode, timing functions and iteration.
> > +    // This means that animation names that are the same as keywords
> > +    // (e.g. 'forwards') won't always match in the shorthand. In that
> > +    // case they should be using longhands (or reconsidering their
> > +    // approach).
> 
> My reading of this comment is that getting the computed value of the property and setting it to that could break things. Is it true? If so, is it true for any other shorthand?

[Dean Jackson]

(In reply to comment #24)
> (From update of attachment 151385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151385&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:3054
> > +    // We want the computed style of the shorthand to display with
> > +    // the animation-name property first, but when we parse it we
> > +    // need to look for animation-name last because otherwise it might
> > +    // match against the keywords for fill mode, timing functions and iteration.
> > +    // This means that animation names that are the same as keywords
> > +    // (e.g. 'forwards') won't always match in the shorthand. In that
> > +    // case they should be using longhands (or reconsidering their
> > +    // approach).
> 
> My reading of this comment is that getting the computed value of the property and setting it to that could break things. Is it true? If so, is it true for any other shorthand?

Actually, we don't support getting the computed value of the animation shorthand yet. But yes, when we do it means that it won't roundtrip in some cases.

As I mentioned above, I think shorthands are a convenience that some with some restrictions. An alternative would be to ignore the whole rule if your animation name is one of the keywords, but that means that couldn't enhance the shorthand later (like we did when we added fill-mode with its three keywords).

[Dean Jackson]

(In reply to comment #26)
 
> Actually, we don't support getting the computed value of the animation shorthand yet. But yes, when we do it means that it won't roundtrip in some cases.
> 
> As I mentioned above, I think shorthands are a convenience that some with some restrictions. An alternative would be to ignore the whole rule if your animation name is one of the keywords, but that means that couldn't enhance the shorthand later (like we did when we added fill-mode with its three keywords).

Hmm, my comment doesn't make any sense.

1. When we implement the computed style for the shorthand, we should output animation name last.

2. Adding any new keywords to the list of properties in the shorthand may break things. I'm not sure what we can do about that, other than not add more properties!

[Simon Fraser (smfr)]

Which of the problems you describe need to be resolved at the spec level?

[Dean Jackson]

(In reply to comment #28)
> Which of the problems you describe need to be resolved at the spec level?

I think the spec should change its grammar for the shorthand to list animation name last. And also make it clear why we're doing this.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=17738

[Lea Verou]

(In reply to comment #29)
> (In reply to comment #28)
> > Which of the problems you describe need to be resolved at the spec level?
> 
> I think the spec should change its grammar for the shorthand to list animation name last. And also make it clear why we're doing this.
> 
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=17738

It makes much more sense to change the spec to explicitly forbid ambiguous animation names (similar to the reserved words concept in most programming languages) which only affects a tiny subset of cases, rather than making a change that affects every single use of the shorthand.

[Simon Fraser (smfr)]

Comment on attachment 151385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151385&action=review

>>>> Source/WebCore/css/CSSParser.cpp:3054
>>>> +    // approach).
>>> 
>>> My reading of this comment is that getting the computed value of the property and setting it to that could break things. Is it true? If so, is it true for any other shorthand?
>> 
>> 
> 
> Actually, we don't support getting the computed value of the animation shorthand yet. But yes, when we do it means that it won't roundtrip in some cases.
> 
> As I mentioned above, I think shorthands are a convenience that some with some restrictions. An alternative would be to ignore the whole rule if your animation name is one of the keywords, but that means that couldn't enhance the shorthand later (like we did when we added fill-mode with its three keywords).

If we don't intend to implement computed style for the 'animation' shorthand, then it's confusing if the comment refers to it. Ideally this comment would reference a paragraph in the spec.

> Source/WebCore/css/CSSParser.cpp:3073
>      bool parsedProperty[] = { false, false, false, false, false, false, false };

All these parallel arrays which have to be the same length as numProperties  are scarey.

> Source/WebCore/css/CSSParser.cpp:3109
> -    for (i = 0; i < numProperties; ++i) {
> +    for (i = 0; i < numProperties; i++) {

Why the ++i -> i++?

> LayoutTests/animations/animation-shorthand-name-order.html:17
> +@-webkit-keyframes test {
> +    from { left: 0px; }
> +    to { left: 100px; }
> +}

How about testing with keyframes called "alternate" or "backwards"?

[Dean Jackson]

Committed r122271: <http://trac.webkit.org/changeset/122271>

[Hayato Ito]

FYI.
I've removed 'true' from dumpAsText(true) from the test file.
http://trac.webkit.org/changeset/122300