Bug 54151 – Implement an API to play/pause/scrub animations

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Dean Jackson]

We need an API to allow control over running animations, including being able to pause them and scrub the current time.

[Simon Fraser (smfr)]

Can't wait.

[Dean Jackson]

Created attachment 81890 [details]
Work in progress. Not for review.

Work in progress. Creates the DOM objects for the animations, exposed as a WebKitAnimationList of WebKitAnimations. These are plumbed through to a KeyframeAnimation on the element, but only duration is hooked up right now.

DOMWindow has a method for getting the animations on a particular element.

[Dean Jackson]

BTW - I think if/when this lands, we can kill the pause API in DRT and simply do everything via the new API. This will allow us to go through multiple steps etc, and will handle a time before the delay, etc. So I've blocked the DRT bug on this.

[Dean Jackson]

<rdar://problem/8980852>

[Dean Jackson]

Created attachment 83423 [details]
Work in Progress. Not for review.

Not for review. This is against a fairly old checkout, but it implements the API for software animations. You can play/pause and scrub to any moment in the duration of an animation. The usage is something like this:

var animations = window.getAnimationsForElement(someElement);

for (var i = 0; i < animations.length; i++) {
  animations[i].pause();
  animations[i].elapsedTime = 10;
}

I need to get HW animations to work now. Step 1 will be to implement a way that HW animations can be instantly converted to software.

[Dean Jackson]

Created attachment 84828 [details]
Another WIP patch

Everything animationy works. There is a crasher in AnimationBase destructor. Once that's fixed, write test cases, make sure we're not killing transitions, and then split this out into smaller patches. There are some easy bug fixes in here.

[Dean Jackson]

Created attachment 85251 [details]
Part 1 Patch

This is the first patch for this bug. It adds the new IDLs and implementations, but read-only for now. You can select animations and query their properties. You cannot yet scrub or play/pause. New test case to exercise the API.

I'm willing to bet that I managed to screw up at least one of the non-Mac platform build files.

[Dean Jackson]

Comment on attachment 85251 [details]
Part 1 Patch

style fail!

[WebKit Review Bot]

Attachment 85251 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1

Source/WebCore/page/animation/WebKitAnimation.cpp:95:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/page/animation/WebKitAnimationList.cpp:60:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/page/animation/WebKitAnimationList.cpp:74:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/page/animation/WebKitAnimationList.cpp:78:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/page/animation/AnimationBase.h:175:  The parameter name "time" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/page/animation/WebKitAnimation.h:36:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/page/animation/WebKitAnimation.h:79:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 9 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 85255 [details]
Step 1 Patch

Same comments as last time. This just has style updates.

The style error in the v8 custom interface can be ignored. There is no header file for this.

[WebKit Review Bot]

Attachment 85255 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 85251 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8122043

[Simon Fraser (smfr)]

Comment on attachment 85255 [details]
Step 1 Patch

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

Generally looks good!

> LayoutTests/animations/animation-api-1.html:65
> +var a_anims = a.webkitGetAnimations();

a_anims is fugly.

> Source/WebCore/dom/Element.h:348
> +    PassRefPtr<WebKitAnimationList> webkitGetAnimations();

Method should be const. You don't need the prefix to propagate into the function names that are not exposed through bindings (but I guess this one does).

> Source/WebCore/page/animation/AnimationBase.cpp:811
> +    // Remove ourselves from the wait lists if we still have a renderer.

I think you should flip the comment around. When does it not have a renderer?

> Source/WebCore/page/animation/AnimationBase.h:192
> +    Animation* animation() { return m_animation.get(); }

const. Return const Animation*?

> Source/WebCore/page/animation/AnimationController.cpp:47
> +// FIXME: Why isn't this set to 60fps or something?

File a bug?

> Source/WebCore/rendering/RenderObject.h:754
> +    PassRefPtr<WebKitAnimationList> animations();

Not sure why you need to add this method to RenderObject. Why not just get the list via AnimationController?

[WebKit Review Bot]

Attachment 85255 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8118630

[Chris Marrin]

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

Overall looks good. Some comments we might want to talk over before committing...

> LayoutTests/ChangeLog:21
> +

Since you can now pause and set the animation to a particular time, seems like the perfect opportunity to do some pixel tests?

> Source/WebCore/page/animation/AnimationBase.cpp:1406
> +    // TODO

I think these are better as FIXME: something

> Source/WebCore/page/animation/AnimationBase.h:163
>      

It seems odd that this API is in AnimationBase since KeyframeAnimations are the only things you can use this API on. Although, if you could unify some of the API, maybe this can be applicable to the other "pause" uses?

> Source/WebCore/page/animation/AnimationBase.h:169
> +    void pause();

How are these different from pauseAnimation/startAnimation and all that glop? Can the be unified?

> Source/WebCore/page/animation/AnimationController.cpp:47
> +// FIXME: Why isn't this set to 60fps or something?

Did you open a bug to deal with this at some point?

> Source/WebCore/page/animation/CompositeAnimation.cpp:581
> +

Is this possibly expensive operation in any fast paths? Or is it just used when the author asks for animations?

> Source/WebCore/page/animation/WebKitAnimation.cpp:1
> +/*

This is a confusing name. How is it different from AnimationBase, ImplicitAnimation, CompositeAnimation, etc.? Yes, I know many of the names are confusing, but let's not add to it. Maybe it would be better for this object to be in dom or html or something?

> Source/WebCore/page/animation/WebKitAnimationList.cpp:1
> +/*

Same comment about where to put this file. It's really a DOM node, so maybe it belongs somewhere else?

[James Robinson]

Comment on attachment 85255 [details]
Step 1 Patch

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

>> Source/WebCore/page/animation/AnimationController.cpp:47
>> +// FIXME: Why isn't this set to 60fps or something?
> 
> File a bug?

I asked hyatt why this was set to 25ms and he said he just picked a number that seemed good at the time.  Since you're here, why not just bump it to 60fps?  I tried that locally and no tests failed, but it didn't seem to make any huge difference in animation quality either (at least to my eyes).

[James Robinson]

Hey and since you are poking at animations, do any of y'all wanna look at the patch on bug 51952? :P

[Dean Jackson]

(In reply to comment #15)
> 
> > LayoutTests/ChangeLog:21
> > +
> 
> Since you can now pause and set the animation to a particular time, seems like the perfect opportunity to do some pixel tests?

Nope - I can't pause yet. That's step/patch 2. 

> 
> > Source/WebCore/page/animation/AnimationBase.cpp:1406
> > +    // TODO
> 
> I think these are better as FIXME: something

OK

> 
> > Source/WebCore/page/animation/AnimationBase.h:163
> >      
> 
> It seems odd that this API is in AnimationBase since KeyframeAnimations are the only things you can use this API on. Although, if you could unify some of the API, maybe this can be applicable to the other "pause" uses?

You might be able to pause transitions (eventually). The reason they are on AnimationBase is that Step 2 dives deep into the heart of the animation state machine, and for that it needs class access.

> 
> > Source/WebCore/page/animation/AnimationBase.h:169
> > +    void pause();
> 
> How are these different from pauseAnimation/startAnimation and all that glop? Can the be unified?

This pause() comes externally (from JS). I think it will be more clear in Patch 2.

> 
> > Source/WebCore/page/animation/AnimationController.cpp:47
> > +// FIXME: Why isn't this set to 60fps or something?
> 
> Did you open a bug to deal with this at some point?

Nope, but I will.

> 
> > Source/WebCore/page/animation/CompositeAnimation.cpp:581
> > +
> 
> Is this possibly expensive operation in any fast paths? Or is it just used when the author asks for animations?

I'm not sure what operation you're talking about, but probably the one where you get the list of animations. I don't think it will ever be called outside the JS API. It is a bit expensive to create a list each time, but given that animations can disappear we can't really have it live. 

My feeling in general is that once you start using this API you're going to suffer in performance. Hardware animations will jump to software. But that's ok because you're probably an authoring tool of some sort, or have a good reason to start manipulating things.

> 
> > Source/WebCore/page/animation/WebKitAnimation.cpp:1
> > +/*
> 
> This is a confusing name. How is it different from AnimationBase, ImplicitAnimation, CompositeAnimation, etc.? 

It's got this name because that is what makes sense externally. I can't think of a better name to expose to the bindings.

> Yes, I know many of the names are confusing, but let's not add to it. Maybe it would be better for this object to be in dom or html or something?

Good point. The implementation probably could live in page.

> 
> > Source/WebCore/page/animation/WebKitAnimationList.cpp:1
> > +/*
> 
> Same comment about where to put this file. It's really a DOM node, so maybe it belongs somewhere else?

page? It isn't strictly a DOM node - it doesn't exist in the document.

Thanks!

[Dean Jackson]

(In reply to comment #16)

> I asked hyatt why this was set to 25ms and he said he just picked a number that seemed good at the time.  Since you're here, why not just bump it to 60fps?  I tried that locally and no tests failed, but it didn't seem to make any huge difference in animation quality either (at least to my eyes).

I'll do that, thanks!

James, while you're watching, I can't work out why Chromium builds are not generating V8WebKitAnimation.h. Do you have any idea? 

I added a new section for page/animation/%.idl in Android.derived.v8bindings.mk

[James Robinson]

(In reply to comment #19)
> (In reply to comment #16)
> 
> > I asked hyatt why this was set to 25ms and he said he just picked a number that seemed good at the time.  Since you're here, why not just bump it to 60fps?  I tried that locally and no tests failed, but it didn't seem to make any huge difference in animation quality either (at least to my eyes).
> 
> I'll do that, thanks!
> 
> James, while you're watching, I can't work out why Chromium builds are not generating V8WebKitAnimation.h. Do you have any idea? 
> 
> I added a new section for page/animation/%.idl in Android.derived.v8bindings.mk

Chromium doesn't use Android.derived.v8bindings.mk (confusing, isn't it)

I think you need to add the new IDLs to the webcore_bindings_idl_files list in http://trac.webkit.org/export/80677/trunk/Source/WebCore/WebCore.gypi

[Simon Fraser (smfr)]

(In reply to comment #16)

> I asked hyatt why this was set to 25ms and he said he just picked a number that seemed good at the time.  Since you're here, why not just bump it to 60fps?  I tried that locally and no tests failed, but it didn't seem to make any huge difference in animation quality either (at least to my eyes).

Probably because of bug 47447.

[Chris Marrin]

(In reply to comment #18)
> > > 
> > > Source/WebCore/page/animation/AnimationBase.h:163
> > >      
> > 
> > It seems odd that this API is in AnimationBase since KeyframeAnimations are the only things you can use this API on. Although, if you could unify some of the API, maybe this can be applicable to the other "pause" uses?
> 
> You might be able to pause transitions (eventually). The reason they are on AnimationBase is that Step 2 dives deep into the heart of the animation state machine, and for that it needs class access.

Right. That makes sense.

> 
> > 
> > > Source/WebCore/page/animation/AnimationBase.h:169
> > > +    void pause();
> > 
> > How are these different from pauseAnimation/startAnimation and all that glop? Can the be unified?
> 
> This pause() comes externally (from JS). I think it will be more clear in Patch 2.

But right now we have suspend/resume to stop all animations/transitions, the pauseAPI (which your patch will hopefully allow us to rip out), the play-state property, and your new API. Seems like it could all be rolled into one as far as AnimationBase is concerned. That would make the state machine simpler. Then the higher level logic (CompositeAnimation and AnimationController) can use that single API as needed.

But maybe that's a big scary change at this point. So maybe we just defer it until later (meaning "never" :-)

> > 
> > > Source/WebCore/page/animation/CompositeAnimation.cpp:581
> > > +
> > 
> > Is this possibly expensive operation in any fast paths? Or is it just used when the author asks for animations?
> 
> I'm not sure what operation you're talking about, but probably the one where you get the list of animations. I don't think it will ever be called outside the JS API. It is a bit expensive to create a list each time, but given that animations can disappear we can't really have it live. 
> 
> My feeling in general is that once you start using this API you're going to suffer in performance. Hardware animations will jump to software. But that's ok because you're probably an authoring tool of some sort, or have a good reason to start manipulating things.

Yeah, that's fine. As long as this never gets called from any style change logic. If it's just for your new API, it can be slow as hell and the author will never notice.

> 
> > 
> > > Source/WebCore/page/animation/WebKitAnimation.cpp:1
> > > +/*
> > 
> > This is a confusing name. How is it different from AnimationBase, ImplicitAnimation, CompositeAnimation, etc.? 
> 
> It's got this name because that is what makes sense externally. I can't think of a better name to expose to the bindings.
> 
> > Yes, I know many of the names are confusing, but let's not add to it. Maybe it would be better for this object to be in dom or html or something?
> 
> Good point. The implementation probably could live in page.

My only push back is that the AnimationList is returned from a call on Element, so having it in 'dom' might make sense. But moving it out to page with all the other node'y things with idl's is probably fine.

[Build Bot]

Attachment 85255 [details] did not build on win:
Build output: http://queues.webkit.org/results/8114888

[Dean Jackson]

Created attachment 85410 [details]
Review changes

Changes from review. Hoping that I've fixed build errors.

[Dean Jackson]

Comment on attachment 85410 [details]
Review changes

Setting to r? I assume that's what kicks off the bots. The patch already has an r+.

[WebKit Review Bot]

Attachment 85410 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 85410 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8141093

[Build Bot]

Attachment 85410 [details] did not build on win:
Build output: http://queues.webkit.org/results/8131105

[Dean Jackson]

Created attachment 85435 [details]
Build fixes for Windows and Cr-Linux

Some moronic errors were breaking the build.

[Dean Jackson]

Comment on attachment 85435 [details]
Build fixes for Windows and Cr-Linux

No review needed

[WebKit Review Bot]

Attachment 85435 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 85435 [details] did not build on win:
Build output: http://queues.webkit.org/results/8139146

[Dean Jackson]

Created attachment 85459 [details]
Updated patch

No review needed.

- It turns out the style error that I thought was a mistake was telling me I'd made a typo (Webkit -> WebKit) in the v8 bindings :)
- Use numeric_limit<> rather than INFINITY
- Make sure the animations are ordered as they are returned (test was occasionally failing)
- Test also checks for null now.

[WebKit Review Bot]

Attachment 85459 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Part 1 landed.

http://trac.webkit.org/changeset/80846

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/animations/animation-api-1-expected.txt
	A	LayoutTests/animations/animation-api-1.html
	M	LayoutTests/fast/dom/Window/window-properties-expected.txt
	M	LayoutTests/fast/dom/Window/window-property-descriptors-expected.txt
	M	LayoutTests/fast/dom/prototype-inheritance-2-expected.txt
	M	LayoutTests/fast/dom/prototype-inheritance-expected.txt
	M	LayoutTests/fast/js/global-constructors-expected.txt
	M	Source/WebCore/Android.derived.jscbindings.mk
	M	Source/WebCore/Android.derived.v8bindings.mk
	M	Source/WebCore/Android.mk
	M	Source/WebCore/Android.v8bindings.mk
	M	Source/WebCore/CMakeLists.txt
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/CodeGenerators.pri
	M	Source/WebCore/DerivedSources.cpp
	M	Source/WebCore/DerivedSources.make
	M	Source/WebCore/GNUmakefile.am
	M	Source/WebCore/WebCore.gypi
	M	Source/WebCore/WebCore.pro
	M	Source/WebCore/WebCore.vcproj/WebCore.vcproj
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
	M	Source/WebCore/bindings/js/JSBindingsAllInOne.cpp
	A	Source/WebCore/bindings/js/JSWebKitAnimationCustom.cpp
	A	Source/WebCore/bindings/js/JSWebKitAnimationListCustom.cpp
	A	Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp
	M	Source/WebCore/dom/Element.cpp
	M	Source/WebCore/dom/Element.h
	M	Source/WebCore/dom/Element.idl
	M	Source/WebCore/page/DOMWindow.idl
	A	Source/WebCore/page/WebKitAnimation.cpp
	A	Source/WebCore/page/WebKitAnimation.h
	A	Source/WebCore/page/WebKitAnimation.idl
	A	Source/WebCore/page/WebKitAnimationList.cpp
	A	Source/WebCore/page/WebKitAnimationList.h
	A	Source/WebCore/page/WebKitAnimationList.idl
	M	Source/WebCore/page/animation/AnimationBase.cpp
	M	Source/WebCore/page/animation/AnimationBase.h
	M	Source/WebCore/page/animation/AnimationController.cpp
	M	Source/WebCore/page/animation/AnimationController.h
	M	Source/WebCore/page/animation/AnimationControllerPrivate.h
	M	Source/WebCore/page/animation/CompositeAnimation.cpp
	M	Source/WebCore/page/animation/CompositeAnimation.h
Committed r80846

[WebKit Review Bot]

http://trac.webkit.org/changeset/80846 might have broken Leopard Intel Release (Build)

[Tony Gentilcore]

It looks like this might be causing bug 56325.

[Eric Seidel (no email)]

Comment on attachment 85255 [details]
Step 1 Patch

Cleared Simon Fraser's review+ from obsolete attachment 85255 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Dean Jackson]

*** Bug 26850 has been marked as a duplicate of this bug. ***

[Paul Bakaus]

How much work still needs to be done to finish this feature? The web community is eargerly awaiting it (read: me :P).

Thanks,
Paul

[Dean Jackson]

(In reply to comment #40)
> How much work still needs to be done to finish this feature? The web community is eargerly awaiting it (read: me :P).

It's stalled on me making a proposal to public-fx so that other vendors can comment :( I've been busy with other things.

If someone wants to start that ball rolling, I'd be most appreciative.

[Paul Bakaus]

(In reply to comment #41)
> (In reply to comment #40)
> > How much work still needs to be done to finish this feature? The web community is eargerly awaiting it (read: me :P).
> 
> It's stalled on me making a proposal to public-fx so that other vendors can comment :( I've been busy with other things.
> 
> If someone wants to start that ball rolling, I'd be most appreciative.

Hey Dean, thanks for the update. I'm actually happy to help if you can get me a gist of the API. Just sent you an email, let's connect and make it happen.

[Rafael Brandao]

How is this going? Is it done on some other bug or it has been declined?

[Dean Jackson]

There has been work on an Animation API in W3C. I've stalled on this bug until things are more clear there.

[Antoine Quint]

We now support Web Animations and each Animation object can have its currentTime property manipulated, including CSS Animations and CSS Transitions.