As proposed here: https://lists.w3.org/Archives/Public/public-fx/2015JulSep/0073.html Doing this properly likely requires refactoring the way animations are sampled. We should probably store a map per-document(?) that maps Animations to AnimationTimelines. Then we could make that map observe the refresh driver (not the AnimationTimeline). As a later step when we come to support script-defined timelines, we would like sample each timeline first, then pass the result to the corresponding animation. (Animations would need a way of getting the timeline time at other moments, either by storing the sample time on the animation--something that is quite complicated, see bug 1208385 comment 11--or by storing the returned timeline times on the map and having animations query the map each time.)

Actually, I think initially this doesn't even need to be a map. It could just be a hash of the animations exactly like we currently store on AnimationTimeline. We only need the map when we do things like: * Tick timelines as a separate step and cache their time * Provide a means of looking up animations by their timeline * etc.

This is mochitests what I believe to be correct behavior of Document.getAnimations(). Most of the tests are the same as tests in file_timeline-get-animations.html, some of tests are different because of lifetimes of animations in getAnimations(). The biggest difference is that 'Order of CSS Animations - free animations' test has been removed because freed animations are not appeared in getAnimations(). And the case for 'freed aniamtions are not appeared' is covered by a prior test. I will post the diff file between file_timeline-get-animations.html and file_document-get-animations.html for reference.

This is a WIP patch which makes most of tests in attachment 8677989 [details] [diff] [review] pass except a test case which replaces style animation by new animations like this: div.style.animation = 'anim1 10s, anim2 10s'; div.style.animation = 'newAnim1 10s, newAnim2 10s'; With the current WIP patch document.getAnimations() returns all of 4 animations in this case because the patch does not remove the previous animations at all in that case. I'm now thinking what is the right approach to solve this issue.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > The biggest difference is that 'Order of CSS Animations - free animations' > test has been removed because freed animations are not appeared in > getAnimations(). And the case for 'freed aniamtions are not appeared' is > covered by a prior test. Free animations do appear in getAnimations() if they are subsequently restarted. This test should remain since we need to test the order these restarted animations are returned in.

(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #5) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > > The biggest difference is that 'Order of CSS Animations - free animations' > > test has been removed because freed animations are not appeared in > > getAnimations(). And the case for 'freed aniamtions are not appeared' is > > covered by a prior test. > > Free animations do appear in getAnimations() if they are subsequently > restarted. This test should remain since we need to test the order these > restarted animations are returned in. I mean that 'freed animations' are animations which are cancelled by style. i.e. div.style.animation = ''. In these cases how can we restart these animations? I think 'restart' means that animations are newly created. i.e. div.style.animation = 'anim 10s'. Is there another way to free animations? One possible way I can think of is to remove animation element from the document. I did actually add this case as another patch.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > (In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #5) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > > > The biggest difference is that 'Order of CSS Animations - free animations' > > > test has been removed because freed animations are not appeared in > > > getAnimations(). And the case for 'freed aniamtions are not appeared' is > > > covered by a prior test. > > > > Free animations do appear in getAnimations() if they are subsequently > > restarted. This test should remain since we need to test the order these > > restarted animations are returned in. > > I mean that 'freed animations' are animations which are cancelled by style. > i.e. div.style.animation = ''. > In these cases how can we restart these animations? I think 'restart' means > that animations are newly created. i.e. div.style.animation = 'anim 10s'. Is > there another way to free animations? One possible way I can think of is to > remove animation element from the document. I did actually add this case as > another patch. In this context, a free animation is an animation that is not bound to markup. You can restart it like this: elem.style.animation = 'anim 10s'; var anim = elem.getAnimations()[0]; elem.style.animation = ''; // anim is now free (i.e. not bound to markup) anim.play(); // anim is free but restarted

(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #7) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > > (In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #5) > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > > > > The biggest difference is that 'Order of CSS Animations - free animations' > > > > test has been removed because freed animations are not appeared in > > > > getAnimations(). And the case for 'freed aniamtions are not appeared' is > > > > covered by a prior test. > > > > > > Free animations do appear in getAnimations() if they are subsequently > > > restarted. This test should remain since we need to test the order these > > > restarted animations are returned in. > > > > I mean that 'freed animations' are animations which are cancelled by style. > > i.e. div.style.animation = ''. > > In these cases how can we restart these animations? I think 'restart' means > > that animations are newly created. i.e. div.style.animation = 'anim 10s'. Is > > there another way to free animations? One possible way I can think of is to > > remove animation element from the document. I did actually add this case as > > another patch. > > In this context, a free animation is an animation that is not bound to > markup. You can restart it like this: > > elem.style.animation = 'anim 10s'; > var anim = elem.getAnimations()[0]; > elem.style.animation = ''; > // anim is now free (i.e. not bound to markup) > anim.play(); > // anim is free but restarted This example is very impressive to me. The animation play state after calling anim.play() is actually 'running' but there is no visual changes on the element. I agree it's worth testing this case but the restarting animation should be really returned in document.getAnimations() list? elem.getAnimations() returns also empty list after restarting. Is this bug? I am attaching an example for this case.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > This example is very impressive to me. The animation play state after > calling anim.play() is actually 'running' but there is no visual changes on > the element. That's because we don't support free animations yet. That's part of the refactoring we need to do to support script-generation animations (bug 1151731). > I agree it's worth testing this case but the restarting animation should be > really returned in document.getAnimations() list? Yes. > elem.getAnimations() > returns also empty list after restarting. Is this bug? Yes.

Note about current hash table owned by AnimationTimeline. The hash table contains temporary animations created in nsAnimationManager::BuildAnimations[1], even if the temporary animations are not necessary. e.g. changing animation duration property. [1] http://hg.mozilla.org/mozilla-central/file/1e700005a0dd/layout/style/nsAnimationManager.cpp#l670 The temporary animations in the hash will be surely removed in next tick, but we should avoid it. I am not sure we can fix this issue in case of document.getAnimations().

There are three cases what I am now concerned: a) div.style.animation = 'anim 1s'; var animations = div.getAnimations(); div.style.animation = ''; document.getAnimations(); // Should be an array consisting of one animation because |animations| holds the reference. b) div.style.animation = 'anim 1s'; div.style.animation = ''; document.getAnimations(); // Should be an empty array. c) div.style.animation = 'anim 1s'; var animations = div.getAnimations(); div.style.animation = ''; animations = null; document.getAnimations(); // Should be an empty array. To distinguish there cases I am now checking reference count of dom::Animation instance in Document::GetAnimations(). I think if the reference count equals to 1 we can ignore the animation. (KeyframeEffectReadOnly holds a reference.). But in case of b) and c), the reference count is 2. Even if the reference count is 2, the animation instance is disposed after a while. I guess it's done by cycle collector. I believe KeyframeEffectReadOnly holds one reference but I have yet no idea who the other one is held by.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11) > There are three cases what I am now concerned: > > a) > div.style.animation = 'anim 1s'; > var animations = div.getAnimations(); > div.style.animation = ''; > document.getAnimations(); // Should be an array consisting of one animation > because |animations| holds the reference. In this case document.getAnimations() should *not* return any animations since they have all been cancelled. i.e. div.style.animation = '' will call cancel() on all CSS animations associated with |div| which puts them in the idle state. Since they are idle, they are not current or in-effect so they won't be returned from getAnimations. > To distinguish there cases I am now checking reference count of > dom::Animation instance in Document::GetAnimations(). I think if the > reference count equals to 1 we can ignore the animation. We shouldn't need to do that. In all cases, we should not return the cancelled animations.

Now we have Element::GetAnimations using EffectSet, so we can use it from document. This patch calls Element::GetAnimations which involves sorting each time when it's called. I wondered it's slower than calling EffectSet::GetEffectSet directly to avoid the sorting. So I tried to measure average time of calling Document.getAnimations() 100 times. Measured on http://codepen.io/juliangarnier/pen/idhuG There are 26 animations in the document. average time (ms) Use Element::GetAnimations (this patch) average = 0.11280000000006112 average = 0.11474999999980355 average = 0.14084999999933642 average = 0.13170000000114668 average = 0.13854999999959547 Use EffectSet::GetEffectSet directly average = 0.14510000000023865 average = 0.1582500000006985 average = 0.12480000000083237 average = 0.12075000000011642 average = 0.15894999999960419 There is no signifficant differences. That's because, I guess, all of animating elements have only one animation. I think it's a common use case in the real world. Even if some elements have a couple of animations, I think the result will not be quite different.

Just replaced timeline.getAnimations.

I forgot to mention about animations on pseudo elements. Currently Document.getAnimations() does not return any animations on pseudo elements. Once we fix bug 1174575, we should return animations on pseudo elements (bug 1234403).

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13) > Created attachment 8700901 [details] [diff] [review] > Part 1: Implement Document.getAnimations() > > Now we have Element::GetAnimations using EffectSet, so we can use it from > document. > > This patch calls Element::GetAnimations which involves sorting each time > when it's called. I wondered it's slower than calling > EffectSet::GetEffectSet directly to avoid the sorting. I think we should only sort once at the end rather than re-sorting. If you want to re-use code from Element::GetAnimations you could factor out Element::GetAnimationsUnsorted and call that from both Element::GetAnimations and Document::GetAnimations. I think we should also avoid calling FlushPendingNotifications on every element.

Thanks Brian! average = 0.11205000000005384 average = 0.11970000000004802 average = 0.11644999999996798 average = 0.11399999999975989 average = 0.11569999999996071 Little bit faster, and stable?

Comment on attachment 8700922 [details] [diff] [review] Part 1.5: Add Element::GetAnimationsUnsorted and use it. >Bug 1212720 - Part 1.5: Add Element::GetAnimationsUnsorted and use it. r?heycam >@@ -3138,17 +3138,17 @@ nsDocument::GetAnimations(nsTArray<RefPt > for (nsIContent* node = nsINode::GetFirstChild(); > node; > node = node->GetNextNode(this)) { > if (!node->IsElement()) { > continue; > } > > nsTArray<RefPtr<Animation>> array; >- node->AsElement()->GetAnimations(array); >+ node->AsElement()->GetAnimationsUnsorted(array); > if (array.IsEmpty()) { > continue; > } > > aAnimations.AppendElements(array); Why don't you just pass aArray into GetAnimationsUnsorted? (Rather than allocating a new array, only to copy and discard it.)

(In reply to Brian Birtles (:birtles) from comment #19) > > nsTArray<RefPtr<Animation>> array; > >- node->AsElement()->GetAnimations(array); > >+ node->AsElement()->GetAnimationsUnsorted(array); > > if (array.IsEmpty()) { > > continue; > > } > > > > aAnimations.AppendElements(array); > > Why don't you just pass aArray into GetAnimationsUnsorted? (Rather than > allocating a new array, only to copy and discard it.) Thanks again!

Hiro, did you upload the right patch? Part 1.5 looks identical to Part 1.

I am sorry for the wrong patch.

Comment on attachment 8700901 [details] [diff] [review] Part 1: Implement Document.getAnimations() Review of attachment 8700901 [details] [diff] [review]: ----------------------------------------------------------------- r=me but I would fold in Part 1.5 into this patch when landing.

Comment on attachment 8700933 [details] [diff] [review] Part 1.5: Add Element::GetAnimationsUnsorted and use it. v2 Review of attachment 8700933 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Element.h @@ +821,5 @@ > { > } > > void GetAnimations(nsTArray<RefPtr<Animation>>& aAnimations); > + void GetAnimationsUnsorted(nsTArray<RefPtr<Animation>>& aAnimations); Please add a comment in here pointing out that GetAnimations will flush style while GetAnimationsUnsorted won't.

Comment on attachment 8700903 [details] [diff] [review] Part 2: Tests for document.getAnimations. Review of attachment 8700903 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/test/mochitest.ini @@ +66,5 @@ > [css-transitions/test_element-get-animations.html] > skip-if = buildapp == 'mulet' > support-files = css-transitions/file_element-get-animations.html > +[css-transitions/test_document-get-animations.html] > +support-files = css-transitions/file_document-get-animations.html It looks like the tests in this file are listed in sorted order, so please move this renamed test file into its correct sorted position.

Comment on attachment 8700904 [details] [diff] [review] Part 3: Remove AnimationTimeline.getAnimations. Review of attachment 8700904 [details] [diff] [review]: ----------------------------------------------------------------- I wonder if it's worth preserving the comment about not supporting pseudo-elements, somewhere in nsDocument::GetAnimations?

BTW you'll need to get review from a DOM peer on the patches that touch .webidl files.

Also, I wonder what we should do about this: https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/dom/animation/test/testcommon.js#32 This function specifically handles animations that run on elements that aren't part of the document tree.

Comment on attachment 8700901 [details] [diff] [review] Part 1: Implement Document.getAnimations() Olli, could you please look the part of webidl change?

Comment on attachment 8700904 [details] [diff] [review] Part 3: Remove AnimationTimeline.getAnimations. And this?

(In reply to Brian Birtles (:birtles) from comment #28) > Also, I wonder what we should do about this: > > > https://dxr.mozilla.org/mozilla-central/rev/ > 388bdc46ba51ee31da8b8abe977e0ca38d117434/dom/animation/test/testcommon.js#32 > > This function specifically handles animations that run on elements that > aren't part of the document tree. Yeah, I left this part as is because I am not confident that the case (animations still exist in the tree) is still valid or not when we use EffectSet, and I have no idea when the case happens/happened. test_timeline_get_animations covers the case?, or bug 1232829 was related to? I just filed bug 1234481 for that.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #31) > (In reply to Brian Birtles (:birtles) from comment #28) > > Also, I wonder what we should do about this: > > > > > > https://dxr.mozilla.org/mozilla-central/rev/ > > 388bdc46ba51ee31da8b8abe977e0ca38d117434/dom/animation/test/testcommon.js#32 > > > > This function specifically handles animations that run on elements that > > aren't part of the document tree. > > Yeah, I left this part as is because I am not confident that the case > (animations still exist in the tree) is still valid or not when we use > EffectSet, and I have no idea when the case happens/happened. > test_timeline_get_animations covers the case?, or bug 1232829 was related to? I don't think we need that any more. It was most likely needed because the number of animations returned by document.timeline.getAnimations() was sometimes wrong due to animations that were running on elements no longer bound to the tree. That's no longer going to happen with this new implementation of getAnimations(). > I just filed bug 1234481 for that. I think you should fix it here rather than check in code that calls functions that no longer exist.

(In reply to Brian Birtles (:birtles) from comment #32) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #31) > > (In reply to Brian Birtles (:birtles) from comment #28) > > > Also, I wonder what we should do about this: > > > > > > > > > https://dxr.mozilla.org/mozilla-central/rev/ > > > 388bdc46ba51ee31da8b8abe977e0ca38d117434/dom/animation/test/testcommon.js#32 > > > > > > This function specifically handles animations that run on elements that > > > aren't part of the document tree. > > > > Yeah, I left this part as is because I am not confident that the case > > (animations still exist in the tree) is still valid or not when we use > > EffectSet, and I have no idea when the case happens/happened. > > test_timeline_get_animations covers the case?, or bug 1232829 was related to? > > I don't think we need that any more. It was most likely needed because the > number of animations returned by document.timeline.getAnimations() was > sometimes wrong due to animations that were running on elements no longer > bound to the tree. That's no longer going to happen with this new > implementation of getAnimations(). > > > I just filed bug 1234481 for that. > > I think you should fix it here rather than check in code that calls > functions that no longer exist. OK. This patch removes it.

Comment on attachment 8700901 [details] [diff] [review] Part 1: Implement Document.getAnimations() So the spec still doesn't have getAnimations() in Document interface, even though https://lists.w3.org/Archives/Public/public-fx/2015JulSep/0073.html was sent months ago. I assume it is still the plan to change the spec, right? What kinds of interfaces does blink use here? At least their dev builds have getAnimations() in Element. What about release versions? It is always harder or less-comfortable to r+ webidl changes for something not defined in any spec. hiikezoe, could you please check what blink ships and where, and whether Element.getAnimations will be in release builds soon. Because if so, we may not be able to do the change. And then re-ask review if they aren't going to ship what is in the spec, but what was proposed in the email.

Comment on attachment 8700904 [details] [diff] [review] Part 3: Remove AnimationTimeline.getAnimations. Same applies to this patch.

(In reply to Olli Pettay [:smaug] from comment #35) > Comment on attachment 8700901 [details] [diff] [review] > Part 1: Implement Document.getAnimations() > > So the spec still doesn't have getAnimations() in Document interface, even > though https://lists.w3.org/Archives/Public/public-fx/2015JulSep/0073.html > was sent months ago. I assume it is still the plan to change the spec, right? I've updated the spec now: http://w3c.github.io/web-animations/#extensions-to-the-document-interface (Changeset: https://github.com/w3c/web-animations/commit/2e7e1485b414255fece69840621314fe1e293c96) > What kinds of interfaces does blink use here? At least their dev builds have > getAnimations() in Element. What about release versions? Right, but they don't expose that on release versions and there is no intent to ship that to release yet (same with us). I'll be meeting with Google early next month to discuss plans here.

Thank you, Brian, for updating the spec. (In reply to Olli Pettay [:smaug] from comment #35) > hiikezoe, could you please check what blink ships and where, and whether > Element.getAnimations will be in release builds soon. Because if so, we may > not be able to do the change. > And then re-ask review if they aren't going to ship what is in the spec, but > what was proposed in the email. In their document for their intents [1], there is no entry for Element.getAnimations(). (Actually it has been already implemented as Brian noted.) [1] https://docs.google.com/spreadsheets/d/1pvXEMD5pRioognaqEzglS-4ZBSQ_YmzL8Fiz7yt4Bb4/edit#gid=0

Comment on attachment 8700901 [details] [diff] [review] Part 1: Implement Document.getAnimations() I noticed that getAnimations() in webidl should be moved right below 'DocumentTimeline timeline' entry. I will fix it once this patch gets review+. Thank you,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af1ff2d054ff Thank you, all!

https://hg.mozilla.org/integration/mozilla-inbound/rev/832653b10478 https://hg.mozilla.org/integration/mozilla-inbound/rev/26974dc2a13d https://hg.mozilla.org/integration/mozilla-inbound/rev/bf320c73ab1a https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fefee9f84e

https://hg.mozilla.org/mozilla-central/rev/832653b10478 https://hg.mozilla.org/mozilla-central/rev/26974dc2a13d https://hg.mozilla.org/mozilla-central/rev/bf320c73ab1a https://hg.mozilla.org/mozilla-central/rev/f2fefee9f84e