1037316 - getAnimationPlayers should return the same object when an animation is updated

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Currently when we update animations we generate a new object and copy the start time across. This means that the identity of the objects returned by getAnimationPlayers changes even though intuitively it is the same object.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Return the same object when updating animations

Previously when updating animations we'd generate a new list of animation
objects then try to match up animations from the existing list and copy across
state such as start times and notification flags. However, this means that from
the API we end up returning different objects.

This patch makes us maintain the same object identity when updating an existing
animation. It does this by looking for matching animations in both lists. If it
finds a match it copies the necessary information from the *new* animation to
the *existing* animation (but preserving the start time, last notification
etc.). Then, finally, it puts the *existing* animation in the list of *new*
animations and removes the corresponding *new* animation. The existing
animation is also removed from the list of existing animations so that it only
matches once.

The method used for matching is probably not intuitive but this is addressed in
a subsequent patch in this series.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Return the same object when updating animations

Forgot one of the test files last time

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - When updating animations, match existing animations by beginning at the start of the list

This patch changes the order in which we look for matches when updating existing
animations. Previously we would start at the end of the list of existing
animations and search backwards. However, suppose we have:

  animation: anim 100s

and suppose later we make it

  animation: anim 100s, anim 100s

then again:

  animation: anim 100s, anim 100s, anim 100s

I think we expect the first animation returned by getAnimationPlayers() to be
the same object each time but if we match from the end of the list that won't be
the case.

This patch revises the matching of the animations to start looking from the
start of the list.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles) from comment #3)
> I think we expect the first animation returned by getAnimationPlayers() to be
> the same object each time but if we match from the end of the list that
> won't be
> the case.
> 
> This patch revises the matching of the animations to start looking from the
> start of the list.

Hmmm.  The reason I chose to match from the end of the list was (as the comment in nsAnimationManager::CheckAnimationRule says) that since later animations in the list override earlier ones, the last occurrence is most likely (though not certain, if it's not currently running/filling) to be the one that's actually influencing state.  I still think that's the more sensible behavior.

Note that the spec doesn't currently cover this behavior; all it says is:

	An animation applies to an element if its name appears as one of the identifiers in the computed value of the animation-name property and the animation uses a valid @keyframes rule. Once an animation has started it continues until it ends or the animation-name is removed. The values used for the keyframes and animation properties are snapshotted at the time the animation starts. Changing them during the execution of the animation has no effect. Note also that changing the value of animation-name does not necessarily restart an animation (e.g., if a list of animations are applied and one is removed from the list, only that animation will stop; The other animations will continue). In order to restart an animation, it must be removed then reapplied.

The spec probably should say what happens, though.

Do you know if this change moves us towards or away from matching other browsers, or if other browsers even follow the paragraph above?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8454267 [details] [diff] [review]
part 1 - Return the same object when updating animations

>+          nsRefPtr<ElementAnimation> oldAnim = nullptr;

No need to null-initialize a smart pointer; it's implicit.

>+          size_t oldIdx = collection->mAnimations.Length();
>+          while (oldIdx-- != 0) {

Still prefer for(), even with a blank increment-part.

>+          // FIXME: This is wrong, we should iterate from start to end to
>+          // provide intuitive results but we'll fix that in a subsequent
>+          // patch.

I'm not sure about this FIXME; see my comments above.

But the removal from the old array is definitely an improvement.

>+          newAnimations.ReplaceElementAt(newIdx, oldAnim);

So it's not clear to me why newAnim is an nsRefPtr rather than a
raw pointer, given that newAnimations owns it.  Perhaps newAnim
should be a raw pointer, and should be explicitly nulled out right
before this line?  Though I guess I'm ok with the current approach,
although it is extra refcount traffic.


It also might be a good idea for test_element-get-animation-players.html
to check appropriate characteristics of the player objects (i.e., that
they've changed).  Although I guess you test that for similar cases in
test_animations-dynamic-changes.html .  (It seems somewhat nice to test
both together, in case somebody working on another implementation tries
to fix one of the failures the wrong way, and triggers the other one.)

In test_animations-dynamic-changes.html:

>+  // Wait a moment so we can confirm the startTime doesn't change (and doesn't
>+  // simply reflect the current time).

Perhaps the test should test that the time has changed?

>+    window.getComputedStyle(div).animationDuration

semicolon, please.

(Does 'use strict' not require that?)

Similarly, maybe check the player object equality here?


r=dbaron with the above fixes

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #4)
> The spec probably should say what happens, though.
> 
> Do you know if this change moves us towards or away from matching other
> browsers, or if other browsers even follow the paragraph above?

I think there are a few questions here:

1) Do browsers preserve the start time of existing animations when adding other animations?

Test case:

  http://jsfiddle.net/Sjt5Y/

Results:

Chrome 36: Yes
IE 11: Yes
Safari iOS6: Yes
Gecko: Yes


2) Do browsers snapshot animation-* property values or allow dynamic updates?

(The spec currently says these are snapshotted but section 8 says there is a pending change to make them dynamically changeable)

Test case (tests animation-direction):

  http://jsfiddle.net/gFLfr/2/

Results:

Chrome 36: No update
IE 11: No update
Safari iOS6: Updates
Gecko: Updates (matches Safari)


3) When updating and multiple animation names match, which gets updated?

Test case:

  http://jsfiddle.net/pkD5e/

None of the browsers seem to update when we change from "animation-name: a" to "animation-name: a, a". i.e. none of them restart the animation. Gecko doesn't either prior to this bug.  


Regarding the desired behavior here, as I understand from your comment, "But the removal from the old array is definitely an improvement", simply applying the start time from the last matching item in the old list of animations to all animations in the new list with the same name is not the desired behavior. That leaves at least two options:

i) Match animations one-by-one beginning at the start of the list
ii) Match animations one-by-one beginning at the end of the list

Here is my understanding of the different behavior this produces for the above test case:

> Initially (t=0):

  Set animation-name: a

Call the identity of the object returned from getAnimationPlayers "A".


> Step 1 (t=2):

  Set animation-name: a, a
  
In both (i) and (ii) we end up with the following animations:
  { start t=0 }, { start t=2 }

As a result the animation restarts since the last animation wins.

Note that this differs from the behavior before this bug where we would apply the original start time to *both* animations and end up with { start t=0 }, { start t=0 }, hence the animation would *not* restart.

In both (i) and (ii) getAnimationPlayers returns "A, B" where B is the new animation


> Step 2 (t=4):

  Set animation-direction: reverse, normal

i) Gives { start t=0, direction: reverse }, { start t=2, direction: normal }
As a result nothing changes.
getAnimationPlayers returns A, B

ii) Gives { start t=2, direction: reverse }, { start t=0, direction: normal }
As a result the animation jumps forward because now we applied the earlier start time to the last animation
getAnimationPlayers returns B, A


I think for this case anyway (i) is preferable but perhaps there's some other behavior worth considering?

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #5)
> Comment on attachment 8454267 [details] [diff] [review]
> part 1 - Return the same object when updating animations
...
> It also might be a good idea for test_element-get-animation-players.html
> to check appropriate characteristics of the player objects (i.e., that
> they've changed).

I agree but we don't yet expose the changed properties through the API (there is bug 1037321 for the pause state though so we should be able to test that soon). I'll update this test when that API is available. For now I've added a couple of FIXME comments for now.

> In test_animations-dynamic-changes.html:
> 
> >+  // Wait a moment so we can confirm the startTime doesn't change (and doesn't
> >+  // simply reflect the current time).
> 
> Perhaps the test should test that the time has changed?

Added.

> >+    window.getComputedStyle(div).animationDuration
> 
> semicolon, please.

Fixed.

> (Does 'use strict' not require that?)

Apparently not (I just looked it up).

> Similarly, maybe check the player object equality here?

I think I've confused these two test files a bit. The original distinction I was trying to make was:

test_element-get-animation-players.html:
  tests primarily about the number and identity of objects returned by Element.getAnimationPlayers
test_animations-dynamic-changes.html:
  tests primarily concerned with checking that changes made to CSS markup are correctly reflected in the returned objects

However, in part 2 I added a bunch of tests to test_animations-dynamic-changes.html that check object identity when we make dynamic changes. Perhaps these should go in test_element-get-animation-players.html and we can leave object identity testing out of dynamic-changes?

For now I've added the object identity test as suggested.


I've made all the other fixes as suggested.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Return the same object when updating animations

Address review feedback

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, I now realize I was misunderstanding the state we were in after part 1.  I was thinking that the old code was iterating both lists from end-to-start, but it wasn't, and didn't need to, because there was no removal.  Once you introduce the removal aspect, it's important that both lists be iterated in the same direction, so a part 2 is definitely needed.  But I'd still prefer that direction be end-to-start.  This will avoid the animation restart you describe in Step 1 of comment 6.

Does that make sense?

(It's probably worth double-checking that nothing else depends on the order of iteration of newAnimations, though I don't see anything obvious.)

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #9)
> OK, I now realize I was misunderstanding the state we were in after part 1. 
> I was thinking that the old code was iterating both lists from end-to-start,
> but it wasn't, and didn't need to, because there was no removal.  Once you
> introduce the removal aspect, it's important that both lists be iterated in
> the same direction, so a part 2 is definitely needed.  But I'd still prefer
> that direction be end-to-start.  This will avoid the animation restart you
> describe in Step 1 of comment 6.
> 
> Does that make sense?

It makes sense, and it's definitely better than (ii) from comment 6. However, it seems surprising to me.

That means that if we initially have:

  animation-name: a

then we set:

  animation-name: a, a

The "new" animation gets added to the front of the list. That's observable in the fact that we don't restart the animation. I'd expect the new one to be appended to the end of the list--but that's about the best argument I can come up with. If you think the prepending behavior is fine then I'll go ahead and adjust the patch to do that (and adjust the FIXME comment in the underlying patch).


(With regards to the return value of getAnimationPlayers we are supposed to return objects in the order they are created from oldest to newest which would mean we'd return the "new" animation as the second in the list. However, the purpose of that behavior is that it's meant to correspond to the order in which animations are composited so we should probably add wording to the spec that defines how CSS maps to these objects that makes this order match the order within animation-name.)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Until the CSS spec says otherwise, I'd prefer to stick with the preserving-last behavior.  But we should try to get the spec to say what happens.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8454269 [details] [diff] [review]
part 2 - When updating animations, match existing animations by beginning at the start of the list

re-request if you disagree with comment 11

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - When updating animations, match existing animations

Updated to match animations from the end of the list

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #11)
> Until the CSS spec says otherwise, I'd prefer to stick with the
> preserving-last behavior.  But we should try to get the spec to say what
> happens.

I still think matching from the start of the list produces more intuitive behavior. Although matching from the end prevents an initial restart, unless the fill mode is forwards/both, there will be a jump later when the original animation ends and the new animation that was previously overridden starts taking effect. I think that subsequent jump is more surprising than a jump that occurs in immediate response to a style change. I also think that appending is the more intuitive result when replacing "a" with "a, a".

For now, however, I'm happy to keep the behavior as-is. The tests in this patch are intended to be imported into web-platform-tests so we'll at least have a chance to revisit this when other browsers come to implement this API.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8459951 [details] [diff] [review]
part 2 - When updating animations, match existing animations

r=dbaron.  Thanks for changing it.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47348c9d0f01
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec7f2af2c4d6

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47348c9d0f01
https://hg.mozilla.org/mozilla-central/rev/ec7f2af2c4d6