1203009 - Update composite order of animations to match latest spec behavior

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

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

After some discussion, the sort order for animations has changed as described here:

  https://github.com/w3c/web-animations/issues/99

In summary:
* Animations should be ordered based on creation order.
* Instead of referring to sequence numbers, we now refer to a global animation list.[1]
* CSS animations / transitions have their own ordering. Once they are disassociated from markup their sort order is undefined until they next transition out of the idle state (which effectively acts as their creation time).[2]

I'd like to fix this at the same time as updating our sorting so that it is always defined.

In bug 1183461 I'm occasionally running into trouble because we use the animation sort order as part of sorting events but by the point we sort events an animation might have become idle which currently triggers an assertion.

We could probably find a way to avoid hitting this condition for now but when we go to implement animationcancel events we will need to know how to sort events from idle animations so we should just make this sorting always defined.

[1] http://w3c.github.io/web-animations/#global-animation-list
[2] https://rawgit.com/shans/csswg-drafts/animations-2/css-animations-2/Overview.html#animation-composite-order. Equivalent spec text for transitions has yet to be written.

Comment 1

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

Attachment: part 1 - Rename sequence number to animation index

The Web Animations specification has replaced the term "sequence number" with
references to a global animation list. This patch applies similar naming
to our animation structures.

Comment 2

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

Attachment: part 2 - Remove {CSSAnimation,CSSTransition}::OwningElement() getter

This doesn't really save us much, but we don't need this method so we may
as well drop it.

Comment 3

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

Attachment: part 3 - Add mNeedsNewAnimationIndexWhenRun flag to CSSAnimation and CSSTransition

In the new composite order arrangement CSSAnimations and CSSTransition have the
following life-cycle:

1. Animation created by markup
   => composite order determined by markup
      (e.g. CSS animations use tree order and animation-name order;
            CSS transitions use transition trigger order)

2. Animation cancelled by changing markup
   => composite order is undefined

3. Animation is played again using the API
   => composite order is defined by when the animation is first played.
      Another way of saying this is that, at the point when the animation is
      played, it is appended to the "global animation list".

4. Animation is subsequently cancelled / played => no change

We need a way to know when we are going from 2 to 3. It would seem like we
could do that by setting mAnimationIndex to some sentinel value while it is
in 2. However, even when in 2, although the spec doesn't define the composite
order animations at this point (from an API point of view you can't access these
objects and they don't contribute to style so it doesn't need to be defined), we
sometimes will need to establish an order.

This can happen, for example, when an animation queues events and then is later
cancelled before the events are dispatched. Because we sort events based on
their associated animation at the time of dispatch (for performance reasons) we
need a deterministic order for these idle animations.

We do that (in a subsequent patch in this series) by setting mAnimationIndex
when we transition from 1 to 2. That is, these idle animations are effectively
ordered by when they became idle (which always happens in a deterministic
fashion).

Comment 4

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

Attachment: part 4 - Implement new composite ordering

Comment 5

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

Attachment: part 5 - Remove IsUsingCustomCompositeOrder

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd91ee3349cb

Comment 7

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

Hi David, I thought you might want to check these patches to see if you agree with this behaviour. I'm sure heycam or dholbert would be suitable reviewers, however, if you're busy.

Comment 8

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

I'm ok with the behavior, and you should switch reviewers to one of heycam or dholbert.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8659745 [details] [diff] [review]
part 1 - Rename sequence number to animation index

Review of attachment 8659745 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/Animation.h
@@ +411,4 @@
>  
> +  // The position of this animation within the global animation list.
> +  // This is kNoIndex while the animation is in the idle state and is updated
> +  // each time the animation transitions out of the idle state.

The "animation index" isn't an index into a hypothetical array of animations, unless you're allowed to have holes in the array, since new indexes are always greater than previous ones.  Would it make sense to call it the "relative position of this animation"?

Should you mention something here about how transitions can have the same animation index?  (That wasn't something I realised before I saw the code in CSSTransition::HasLowerCompositeOrderThan.)

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8659749 [details] [diff] [review]
part 5 - Remove IsUsingCustomCompositeOrder

Review of attachment 8659749 [details] [diff] [review]:
-----------------------------------------------------------------

That's fine, though do we have a term for Animation objects for CSS Transitions/Animations that haven't been disconnected from them?  It might make sense to still have a method named after such a term to make the code clearer in HasLowerCompositeOrderThan.  Either way.

Comment 11

[Cameron McCormack (:heycam)]

Can we test these changes?

Comment 12

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

Attachment: part 6 - Add tests for new composite order

(In reply to Cameron McCormack (:heycam) from comment #11)
> Can we test these changes?

Yes!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b2ef0b4946
https://hg.mozilla.org/integration/mozilla-inbound/rev/5196bf0855ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7af8129961f
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b6deedaab6
https://hg.mozilla.org/integration/mozilla-inbound/rev/44462cfe3134
https://hg.mozilla.org/integration/mozilla-inbound/rev/1444325d4840

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f9b2ef0b4946
https://hg.mozilla.org/mozilla-central/rev/5196bf0855ad
https://hg.mozilla.org/mozilla-central/rev/b7af8129961f
https://hg.mozilla.org/mozilla-central/rev/37b6deedaab6
https://hg.mozilla.org/mozilla-central/rev/44462cfe3134
https://hg.mozilla.org/mozilla-central/rev/1444325d4840