Bug 19028

Summary: Implement CSS Animation (with keyframes, events, interfaces, etc)
Product: WebKit Reporter: Dean Jackson <dino>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: bdakin, dino, mitz, sekundes, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 19861, 19863, 19864, 19938, 20068, 20088, 20119    
Bug Blocks:    
Attachments:
Description Flags
in progress patch - not ready for review
none
Simple keyframe animation with infinite repeat and easing.
none
Completed patch (to be split)
none
Update support for CSS Transforms
none
Patch for using updated animation controller and keyframe parsing
none
Patch for animation/transition events and other dom interfaces none

Description Dean Jackson 2008-05-13 05:02:30 PDT
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.
Comment 1 Dean Jackson 2008-05-13 05:06:12 PDT
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.
Comment 2 Dean Jackson 2008-05-13 05:10:07 PDT
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.
Comment 3 Dean Jackson 2008-05-13 05:11:17 PDT
Created attachment 21108 [details]
Simple keyframe animation with infinite repeat and easing.
Comment 4 Dave Hyatt 2008-05-13 10:44:32 PDT
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...
Comment 5 Dave Hyatt 2008-05-13 10:53:00 PDT
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.
Comment 6 Simon Fraser (smfr) 2008-05-13 12:17:41 PDT
> 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.
Comment 7 Dean Jackson 2008-05-13 15:44:28 PDT
<rdar://problem/5704655>
Comment 8 Dean Jackson 2008-05-14 18:27:04 PDT
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.
Comment 9 Dean Jackson 2008-05-15 14:17:01 PDT
Created attachment 21179 [details]
Update support for CSS Transforms

This is an extraction of the transforms code from the complete patch.
Comment 10 Dean Jackson 2008-05-15 14:40:22 PDT
Created attachment 21180 [details]
Patch for using updated animation controller and keyframe parsing

Extracted patch for software animations and keyframe parsing.
Comment 11 Dean Jackson 2008-05-15 14:51:55 PDT
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.
Comment 12 Dave Hyatt 2008-05-15 15:11:57 PDT
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.
Comment 13 Dave Hyatt 2008-05-15 15:12:46 PDT
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. :)

Comment 14 Dave Hyatt 2008-05-15 15:20:34 PDT
Also, make sure you are adding files to all projects (Qt, wx, Windows, OS X, GTK).

Comment 15 Dean Jackson 2008-05-15 15:21:55 PDT
Transforms moved to http://bugs.webkit.org/show_bug.cgi?id=19091
Comment 16 Dean Jackson 2008-05-15 15:22:37 PDT
How do I add files to all projects?
Comment 17 Dean Jackson 2008-05-15 15:35:48 PDT
CSS Animations now in http://bugs.webkit.org/show_bug.cgi?id=19092

Comment 18 Dean Jackson 2008-05-15 15:47:00 PDT
DOM stuff now in http://bugs.webkit.org/show_bug.cgi?id=19091
Comment 19 Maciej Stachowiak 2008-05-24 23:39:49 PDT
Since this bug has been broken up, should the patches on this bug be unflagged and obsoleted?
Comment 20 Dean Jackson 2008-07-22 17:57:14 PDT
I'll be deprecating this bug soon.
See 
http://bugs.webkit.org/show_bug.cgi?id=19938
Comment 21 Eric Seidel (no email) 2008-08-04 01:54:27 PDT
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.
Comment 22 Dean Jackson 2008-08-04 10:59:13 PDT
Oops. Thanks for the comments Eric, but this bug is stale. I'll copy them into the relevant new bugs.

Comment 23 Eric Seidel (no email) 2008-08-04 11:34:14 PDT
Comment on attachment 21180 [details]
Patch for using updated animation controller and keyframe parsing

stale patch.