Bug 19028 – Implement CSS Animation (with keyframes, events, interfaces, etc)

• Component: CSS
• Status: RESOLVED WONTFIX

[Dean Jackson]

Implement the CSS Advanced Visual Effects specifications from http://www.webkit.org/specs/CSSVisualEffects

The current TOT webkit implements CSS Transitions. The goal is to bring the transition implementation up to date with a new Animation Controller, add support for CSS Animations with keyframes (and therefore the @-webkit-keyframes rule), add events for animation start/end/iteration, interfaces for accessing the keyframes and enhancing the CSS Transforms to provide a basis for 3d.

[Dean Jackson]

Created attachment 21107 [details]
in progress patch - not ready for review

This patch is included here to get initial feedback. Some parts are stubbed out and there are a couple of known bugs, but it gives an idea of the general approach. A patch ready for review will be attached within a day.

[Dean Jackson]

Attached a patch that does most things but:

- does not implement events yet
- does not do computed style for animations
- does not do the JS interfaces for keyframes or matrices
- has a known bug where style is applied (possibly) too often in animation loops
- has unknown behaviour if 3d transforms are specified
- does not parse perspective (not sure if it should, since it is ignored anyway)

A complete patch should appear within 24 hours.

[Dean Jackson]

Created attachment 21108 [details]
Simple keyframe animation with infinite repeat and easing.

[Dave Hyatt]

Don't ever do

if (layer())

That makes a virtual function call.  We have 

if (hasLayer())

that checks a bit just to avoid that.

I'm a bit confused as to why a bunch of animation functions got added to RenderObject.  That doesn't seem appropriate, especially given that they're just calling through to a layer...

[Dave Hyatt]

Comment on attachment 21107 [details]
in progress patch - not ready for review

This is just comments from a cursory reading, not a real review yet:

I see that the RenderLayer functions are just stubs.  I'd just remove them for now then.

The class declaration of KeyframeList is indented using very odd style.  Please pull the inline function definitions back next to the function names (instead of doing odd spacing to try to line up the braces).

+        // make sure there are no duplicate properties. This is an O(n^2) algorithm
+        // but the lists tend to be very short, so it is probably ok

Why are we removing duplicate properties here?  I don't like O(n^2) algorithms even when the lists are short. :)

+    // tell the animation controller that the style is all setup and it can start animations

"setup" should be two words.  We usually capitalize the first words of comment sentences, e.g., "// Tell", not "// tell".

I'm still not convinced keyframes are implemented in a way that really works.

[Simon Fraser (smfr)]

> Why are we removing duplicate properties here?

It's possible to end up with two "cAnimateAll" transtisions, e.g. for properties like:
-webkit-transition-property: "color", "left";

Note the quotes, which result in parsing failure. It's hard to throw those away at parsing time, because the comma-separated values have to be kept in sync with transition-duration etc.

[Dean Jackson]

<rdar://problem/5704655>

[Dean Jackson]

Created attachment 21146 [details]
Completed patch (to be split)

This is pretty much everything. Adds Animation Events, IDL interfaces, computed style. Addresses the comments by Hyatt above.

[Dean Jackson]

Created attachment 21179 [details]
Update support for CSS Transforms

This is an extraction of the transforms code from the complete patch.

[Dean Jackson]

Created attachment 21180 [details]
Patch for using updated animation controller and keyframe parsing

Extracted patch for software animations and keyframe parsing.

[Dean Jackson]

Created attachment 21181 [details]
Patch for animation/transition events and other dom interfaces

This is the extraction of the IDL/Events from the complete patch.

[Dave Hyatt]

Comment on attachment 21179 [details]
Update support for CSS Transforms

In Transform3D.cpp:

+Transform3D &Transform3D::function() {}

Move the & to be with the return value, so:

Transform3D& Transform3D::function() {}

In CodeGeneratorObjC.pm:

I don't understand this:

-    return "CSSValue" if $parent eq "SVGColor";
+    return "CSSValue" if $parent eq "SVGColor" or $parent eq "CSSValueList";

In DerivedSources.make:

     VoidCallback \
+	WebKitCSSTransformValue \
     WheelEvent \

Double-check if tabs are actually used in that file.

I see "KeyframeList" in this patch.  I think that was unintentional.  Can you remove it?

This is very close to being able to land.  I think I'll be able to r+ it with one more revision.

[Dave Hyatt]

Should we maybe split this up into separate bugs?  The attachment list is going to get long and confusing as we iterate over each patch. :)

[Dave Hyatt]

Also, make sure you are adding files to all projects (Qt, wx, Windows, OS X, GTK).

[Dean Jackson]

Transforms moved to http://bugs.webkit.org/show_bug.cgi?id=19091

[Dean Jackson]

How do I add files to all projects?

[Dean Jackson]

CSS Animations now in http://bugs.webkit.org/show_bug.cgi?id=19092

[Dean Jackson]

DOM stuff now in http://bugs.webkit.org/show_bug.cgi?id=19091

[Maciej Stachowiak]

Since this bug has been broken up, should the patches on this bug be unflagged and obsoleted?

[Dean Jackson]

I'll be deprecating this bug soon.
See 
http://bugs.webkit.org/show_bug.cgi?id=19938

[Eric Seidel (no email)]

Comment on attachment 21181 [details]
Patch for animation/transition events and other dom interfaces

In general this patch looks fine.

Tabs vs. spaces here:
39     VoidCallback \
 340 	WebKitAnimationEvent \
 341 	WebKitCSSKeyframeRule \
 342 	WebKitCSSKeyframesRule \
 343 	WebKitCSSTransformValue \
340344     WheelEvent \

Single line ifs don't get { } (yes, I know the rest of that function has them wrong already:
+    } else if (attr->name() == onwebkittransitionendAttr) {
+        setHTMLEventListener(webkitTransitionEndEvent, attr);        
     }

Can't we use CamelCase for these?
+onwebkitanimationend
+onwebkitanimationiteration
+onwebkitanimationstart
+onwebkittransitionend

Seems it would make them more readable.  Yes, i know the other event attributes are all lower-case.  I'm not sure they should be though.  I guess the case matters for XHTML though, so I guess they probably have to be all lower case...

This seems wrong:
+    if (eventType == "WebKitAnimationEvent")
+        return new WebKitAnimationEvent;    
+    if (eventType == "WebKitTransitionEvent")
+        return new WebKitTransitionEvent;  

I guess Events haven't been converted to use the new Foo::create() syntax yet?  They still start with a RefCount of 0 instead of 1?

These could probably go in the header just fine:
+bool Event::isWebKitTransitionEvent() const
+{
+    return false;
+}   


Again, no create() syntax?

+    RefPtr<WebKitAnimationEvent> animEvent = new WebKitAnimationEvent(eventType, animationName, elapsedTime);


I would think we would want to use the newer PassRefPtr<WebKitAnimationEvent> create(); syntax for all new classes.  If the parent class hasn't yet been converted to start with a RefCount of 1, this one could easily ASSERT that new WebKitAnimationEvent() comes back with a RefCount of O (to make things crash if someone changes the base class and forgets to change this one) and then still return it from create().  When someone fixes Event to start with a ref count of 1, then they'll have to change the "new" in your create() method to be adoptRef(new Foo) instead.

I'm surprised this ASSERT isn't in dispatchEvent!?
+    ASSERT(!eventDispatchForbidden());

Why should each caller need to ASSERT that?

This does not need to call the constructor for the String, but it *does* need to init the double to some sane value:
+    WebKitAnimationEvent::WebKitAnimationEvent()
+    : m_animationName()
+    {

And again:
+    WebKitTransitionEvent::WebKitTransitionEvent()
+    : m_propertyName()
+    {

Seems these two classes are just copy/paste of one another... maybe they should just be the same class?

This is not needed in the idl files:
+    // Introduced in DOM Level ?:


Otherwise this looks fine.  Maybe you still want Hyatt to look at it for content.

r- though for the various small nits above.

[Dean Jackson]

Oops. Thanks for the comments Eric, but this bug is stale. I'll copy them into the relevant new bugs.

[Eric Seidel (no email)]

Comment on attachment 21180 [details]
Patch for using updated animation controller and keyframe parsing

stale patch.