Move getAnimations() from AnimationTimeline to Document

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: birtles, Assigned: hiro)

Tracking

(Blocks 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox46 fixed)

Details

Attachments

(4 attachments, 9 obsolete attachments)

3.72 KB, patch
heycam
: review+
smaug
: review+
Details | Diff | Splinter Review
3.68 KB, patch
birtles
: review+
Details | Diff | Splinter Review
7.58 KB, patch
hiro
: review+
Details | Diff | Splinter Review
19.44 KB, patch
hiro
: review+
Details | Diff | Splinter Review
Reporter

Description

4 years ago
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.)
Reporter

Updated

4 years ago
Reporter

Comment 1

4 years ago
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.
Assignee

Comment 2

4 years ago
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.
Assignee: nobody → hiikezoe
Assignee

Comment 4

4 years ago
Posted patch WIP patch (obsolete) — Splinter Review
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.
Reporter

Comment 5

4 years ago
(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.
Assignee

Comment 6

4 years ago
(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.
Reporter

Comment 7

4 years ago
(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
Assignee

Comment 8

4 years ago
(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.
Reporter

Comment 9

4 years ago
(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.
Assignee

Comment 10

4 years ago
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().
Assignee

Comment 11

4 years ago
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.
Reporter

Comment 12

4 years ago
(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.
Assignee

Updated

4 years ago
Blocks: 1234403
Assignee

Comment 13

4 years ago
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.
Attachment #8677989 - Attachment is obsolete: true
Attachment #8677991 - Attachment is obsolete: true
Attachment #8678003 - Attachment is obsolete: true
Attachment #8678722 - Attachment is obsolete: true
Attachment #8700901 - Flags: review?(cam)
Assignee

Comment 14

4 years ago
Just replaced timeline.getAnimations.
Attachment #8700903 - Flags: review?(cam)
Assignee

Comment 16

4 years ago
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).
Reporter

Comment 17

4 years ago
(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.
Assignee

Comment 18

4 years ago
Thanks Brian!

average = 0.11205000000005384
average = 0.11970000000004802
average = 0.11644999999996798
average = 0.11399999999975989
average = 0.11569999999996071

Little bit faster, and stable?
Attachment #8700922 - Flags: review?(cam)
Reporter

Comment 19

4 years ago
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.)
Assignee

Comment 20

4 years ago
(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!
Attachment #8700922 - Attachment is obsolete: true
Attachment #8700922 - Flags: review?(cam)
Attachment #8700924 - Flags: review?(cam)
Hiro, did you upload the right patch?  Part 1.5 looks identical to Part 1.
Flags: needinfo?(hiikezoe)
Assignee

Comment 22

4 years ago
I am sorry for the wrong patch.
Attachment #8700924 - Attachment is obsolete: true
Attachment #8700924 - Flags: review?(cam)
Flags: needinfo?(hiikezoe)
Attachment #8700933 - Flags: review?(cam)
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.
Attachment #8700901 - Flags: review?(cam) → review+
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.
Attachment #8700933 - Flags: review?(cam) → review+
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.
Attachment #8700903 - Flags: review?(cam) → review+
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?
Attachment #8700904 - Flags: review?(cam) → review+
BTW you'll need to get review from a DOM peer on the patches that touch .webidl files.
Reporter

Comment 28

4 years ago
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.
Assignee

Comment 29

4 years ago
Comment on attachment 8700901 [details] [diff] [review]
Part 1: Implement Document.getAnimations()

Olli, could you please look the part of webidl change?
Attachment #8700901 - Flags: review?(bugs)
Assignee

Comment 30

4 years ago
Comment on attachment 8700904 [details] [diff] [review]
Part 3:  Remove AnimationTimeline.getAnimations.

And this?
Attachment #8700904 - Flags: review?(bugs)
Assignee

Updated

4 years ago
Blocks: 1234481
Assignee

Comment 31

4 years ago
(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.
No longer blocks: 1234481
Assignee

Updated

4 years ago
Blocks: 1234481
Reporter

Comment 32

4 years ago
(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.
Assignee

Comment 33

4 years ago
(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.
Attachment #8700959 - Flags: review?(bbirtles)
Assignee

Updated

4 years ago
Duplicate of this bug: 1234481
Reporter

Updated

4 years ago
Attachment #8700959 - Flags: review?(bbirtles) → review+
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.
Attachment #8700901 - Flags: review?(bugs) → feedback?(hiikezoe)
Comment on attachment 8700904 [details] [diff] [review]
Part 3:  Remove AnimationTimeline.getAnimations.

Same applies to this patch.
Attachment #8700904 - Flags: review?(bugs)
Reporter

Comment 37

4 years ago
(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.
Assignee

Comment 38

4 years ago
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
Assignee

Comment 39

4 years ago
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,
Attachment #8700901 - Flags: feedback?(hiikezoe) → review?(bugs)
Assignee

Updated

4 years ago
Attachment #8700904 - Flags: review?(bugs)
Attachment #8700901 - Flags: review?(bugs) → review+
Attachment #8700904 - Flags: review?(bugs) → review+
Assignee

Comment 40

4 years ago
Attachment #8700901 - Attachment is obsolete: true
Attachment #8700933 - Attachment is obsolete: true
Attachment #8704813 - Flags: review+
Assignee

Comment 41

4 years ago
Attachment #8700903 - Attachment is obsolete: true
Attachment #8704814 - Flags: review+
Depends on: CVE-2017-5379
You need to log in before you can comment on or make changes to this bug.