Update composite order of animations to match latest spec behavior

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

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

Comment 1

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

Updated

4 years ago
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee

Comment 2

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

Comment 3

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

Updated

4 years ago
Summary: Update sorting of animations to match latest spec behavior → Update composite order of animations to match latest spec behavior
Assignee

Comment 7

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

Updated

4 years ago
Attachment #8659745 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8659746 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8659747 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8659748 - Flags: review?(dbaron)
Assignee

Updated

4 years ago
Attachment #8659749 - Flags: review?(dbaron)
I'm ok with the behavior, and you should switch reviewers to one of heycam or dholbert.
Flags: needinfo?(bbirtles)
Assignee

Updated

4 years ago
Flags: needinfo?(bbirtles)
Attachment #8659745 - Flags: review?(dbaron) → review?(cam)
Assignee

Updated

4 years ago
Attachment #8659746 - Flags: review?(dbaron) → review?(cam)
Assignee

Updated

4 years ago
Attachment #8659747 - Flags: review?(dbaron) → review?(cam)
Assignee

Updated

4 years ago
Attachment #8659748 - Flags: review?(dbaron) → review?(cam)
Assignee

Updated

4 years ago
Attachment #8659749 - Flags: review?(dbaron) → review?(cam)
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.)
Attachment #8659745 - Flags: review?(cam) → review+
Attachment #8659746 - Flags: review?(cam) → review+
Attachment #8659747 - Flags: review?(cam) → review+
Attachment #8659748 - Flags: review?(cam) → review+
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.
Attachment #8659749 - Flags: review?(cam) → review+
Can we test these changes?
Assignee

Comment 12

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

Yes!
Attachment #8661060 - Flags: review?(cam)
Attachment #8661060 - Flags: review?(cam) → review+
You need to log in before you can comment on or make changes to this bug.