Bug 84533 - REGRESSION (r109610): Order of values in shorthand animation makes a difference
Summary: REGRESSION (r109610): Order of values in shorthand animation makes a difference
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Dean Jackson
URL: http://result.dabblet.com/gist/2437311
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-04-21 09:01 PDT by Lea Verou
Modified: 2012-07-10 23:22 PDT (History)
15 users (show)

See Also:


Attachments
Test case2 (382 bytes, text/html)
2012-04-23 13:50 PDT, Igor Trindade Oliveira
no flags Details
Test case2 (380 bytes, text/html)
2012-04-23 13:57 PDT, Igor Trindade Oliveira
no flags Details
Patch (7.43 KB, patch)
2012-04-24 16:33 PDT, Igor Trindade Oliveira
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.32 MB, application/zip)
2012-04-25 02:28 PDT, WebKit Review Bot
no flags Details
Patch (7.28 KB, patch)
2012-04-25 11:24 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (14.31 KB, patch)
2012-07-09 19:03 PDT, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lea Verou 2012-04-21 09:01:50 PDT
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.
Comment 1 Igor Trindade Oliveira 2012-04-21 09:13:52 PDT
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.
Comment 2 Radar WebKit Bug Importer 2012-04-23 13:39:09 PDT
<rdar://problem/11302643>
Comment 3 Igor Trindade Oliveira 2012-04-23 13:49:51 PDT
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.
Comment 4 Igor Trindade Oliveira 2012-04-23 13:50:35 PDT
Created attachment 138418 [details]
Test case2

Other test case.
Comment 5 Igor Trindade Oliveira 2012-04-23 13:57:07 PDT
Created attachment 138419 [details]
Test case2

Oops now the right test case. This test case does not work before neither after r109610.,
Comment 6 Igor Trindade Oliveira 2012-04-24 16:33:43 PDT
Created attachment 138682 [details]
Patch

Proposed patch.
Comment 7 WebKit Review Bot 2012-04-25 02:27:58 PDT
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
Comment 8 WebKit Review Bot 2012-04-25 02:28:05 PDT
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
Comment 9 Igor Trindade Oliveira 2012-04-25 11:24:51 PDT
Created attachment 138848 [details]
Patch

Proposed patch.
Comment 10 Alexis Menard (darktears) 2012-04-25 12:03:06 PDT
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, ...)?
Comment 11 Alexis Menard (darktears) 2012-04-25 12:12:48 PDT
(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.
Comment 12 Dean Jackson 2012-04-25 17:00:29 PDT
(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.
Comment 13 Dean Jackson 2012-04-25 17:10:14 PDT
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.
Comment 14 Dean Jackson 2012-04-25 17:12:33 PDT
I think we should ask on www-style. We can't reject animation-names without the spec saying so.
Comment 15 Igor Trindade Oliveira 2012-04-25 17:16:02 PDT
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.
Comment 16 Alexis Menard (darktears) 2012-04-26 05:22:59 PDT
(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.
Comment 17 Alexis Menard (darktears) 2012-04-26 05:25:00 PDT
(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.
Comment 18 Alexis Menard (darktears) 2012-04-26 05:27:04 PDT
(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.
Comment 19 Igor Trindade Oliveira 2012-05-05 21:07:34 PDT
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.
Comment 20 Simon Fraser (smfr) 2012-07-09 12:47:42 PDT
Bug 71623 caused a bug where animation-name had to be first in the shorthand, so we need a fix!
Comment 21 Igor Trindade Oliveira 2012-07-09 12:52:09 PDT
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!
Comment 22 Dean Jackson 2012-07-09 19:03:17 PDT
Created attachment 151385 [details]
Patch
Comment 23 Dean Jackson 2012-07-09 19:07:30 PDT
<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 24 mitz 2012-07-09 19:19:04 PDT
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?
Comment 25 Igor Trindade Oliveira 2012-07-09 19:23:17 PDT
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?
Comment 26 Dean Jackson 2012-07-09 19:27:51 PDT
(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).
Comment 27 Dean Jackson 2012-07-09 19:32:29 PDT
(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!
Comment 28 Simon Fraser (smfr) 2012-07-09 20:06:57 PDT
Which of the problems you describe need to be resolved at the spec level?
Comment 29 Dean Jackson 2012-07-10 12:36:46 PDT
(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
Comment 30 Lea Verou 2012-07-10 12:39:13 PDT
(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.
Comment 31 Simon Fraser (smfr) 2012-07-10 13:07:06 PDT
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"?
Comment 32 Dean Jackson 2012-07-10 15:46:35 PDT
Committed r122271: <http://trac.webkit.org/changeset/122271>
Comment 33 Hayato Ito 2012-07-10 23:22:24 PDT
FYI.
I've removed 'true' from dumpAsText(true) from the test file.
http://trac.webkit.org/changeset/122300