1151731 - Refactor animation code to support script-generated animations

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

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

There are a number of limitations with our current animation architecture that we need to address. I'm interested in tackling this as part of bug 1150810 but it will also be needed in order to implement Element.animate which would be good to ship this year.

Some specific goals:

1. Efficiently getting all animations from document.timeline.getAnimations() [Bug 1150810] -- I'm not sure the use cases for this API are that strong. For example, I think DevTools are more interested in getting all the animations in a subtree. We *might* be able to drop this from the spec, but I think in light of (2) it won't make a difference.

2. Allowing global (i.e. timeline-based) playback control, e.g. getting the document timeline and speeding it up or pausing it. This is not specced (we didn't want people to pause the document timeline and then get surprised that their transitions are no longer working). However, Chrome have implemented it and DevTools have expressed interest in it so it seems likely to happen even if it's a chrome-privileges API.

3. Supporting animations that don't have a target element. The API allows this, and these animations should still have behavior with regards to *time* such as resolving promises at the appropriate moments and firing events.

4. Supporting other sorts of timelines like scroll-based timelines. This probably doesn't necessitate significant architectural changes unless we open it right up to script-based timelines in which case we will want to sample those timelines at a safe point in time (e.g. not in the middle of layout).

Some other problems to address:

5. There's still a fair bit of duplicate code between nsTransitionManager and nsAnimationManager and the two often have subtle differences that aren't well understood. I'd really rather not add a third nsJavascriptAnimationManager which is different again.

6. Currently we don't have any guarantee that an Animation(Player) only gets ticked once. In fact, it seems like that's often the case. There have been a few bugs where it would have been more simple if we could have assumed that AnimationPlayer::Tick gets called exactly once per refresh driver tick.

Comment 1

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

My initial idea for how to approach is basically to separate updating time from updating style.

Breaking it down, it might look something like this:

* Part 1: Sample all timelines

- Timelines (NOT nsAnimationManager / nsTransitionManager) are refresh driver observers.
- Animations observe timelines [I'm referring to animations here in the generic sense. The concrete class is question is called AnimationPlayer but we plan to rename that to Animation in the next few weeks.]
- When timelines tick, all the animations observing them tick and update their timing information.
- Animations post a restyle for their target element, if necessary. They don't actually update style rules at this point [this is the part I'm least clear about]
- Events are queued at this point since events are really a timing property. We might need some helper object to keep track of the queued events so we can dispatch them later.
(By queuing events as part of timing, we hopefully avoid bugs like we had in bug 1117603 comment 25 where we'd sometimes update style rules and stop observing the refresh driver without dispatching events.)
  - Existing event queuing code moves to the CSSAnimation and CSSTransition classes (current CSSAnimationPlayer and CSSTransitionPlayer), out of nsTransitionManager and nsAnimationManager.
- Timelines need to stop observing the refresh driver when they have no more current animations. To support that we might need to deviate from the observer pattern somewhat and, while ticking each animation, check if there are any still needing future ticks.
- In order to implement document.timeline.getAnimations(), we can re-use the observer list and simply filter out any animations that are not current or in effect.
- In order to implement playback control on timelines we can reuse the same observer mechanism to update all the animations watching that timeline.
- If we later support script-based timelines this is the point in the cycle where we would sample those timelines. It occurs just before we do requestAnimationFrame callbacks and before processing restyles.
- Using weak references from the timeline to the animations might ease housekeeping since we will keep a strong reference to them below.

* Part 2: Composite animations

- In response to the restyles posted in part 1, we update the style for each animated element
- Factor out a new AnimationCompositor that includes most of the common functionality from AnimationCommon/nsTransitionManager/nsAnimationManager. Specifically, for a given element:
 - Working out which animation wins in the cascade
 - Composing the animation style rule
 - Working out which animations to send to the compositor (GetAnimationsForCompositor)
 (- In future: adding together animations once we support that)
- nsTransitionManager and nsAnimationManager are simply responsible for creating/destroying/updating the appropriate animation objects based on the provided style information (they continue to implement the nsIStyleRuleProcessor methods). That also includes correctly reversing transitions etc.
- Animations and Transition continue to be stored in separate lists on their target element (if we ever implement timing groups, that might need to change, but for now it's ok). The main reason is to keep updating transitions or animations efficient.

One issue here is that the interaction between animations/transitions/API animations is not really speced. I wanted to treat CSS Animations and script-generated animations alike but I think the rules we ended up with for reconciling multiple uses of the same animation-name in bug 1037316 comment 10 prohibit that (at the same time, I've yet to persuade Google of those rules so maybe we'll need to change that behavior anyway).

I think we'll probably end up specifying something along the lines of different animation "sources" which are sorted independently with different rules and then there's a priority between different sources. There are a lot of edge cases (like some sources targeting different layers in the cascade) but I think it would work.

I think for script-generated animations it would be ok to have yet another list on the target element. I think SVG animations could eventually use the same list (and if we redo SVG animation in JS they almost certainly will). I don't think we'd need an additional manager for that list either since AnimationCompositor should do everything needed for composing script-generated animations and simply call out to nsTransitionManager/nsAnimationManager for CSS-specific behavior.

For animations without a target element we might need a generic parking lot that simply keeps the objects alive so they keep getting ticks from their timeline (assuming the timeline only keeps a weak reference to the objects observing it).

David, what do you think about all this?

Comment 2

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

+CC: Patrick in case he has any comments on the relevance of items 1 and 2 from comment 0.

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #0)
About items 1 and 2 from comment 0:
Right now devtools only cares about the animations on the currently selected node (in the inspector) and on the nodes in its subtree.
However, one of longer term goals we have is to create a more advanced animation tool (in its own devtools tab) that would allow editing and creating animations and I think that tool will need to be able to show all defined animations in the document, so document.timeline.getAnimations() could be handy then (but we could also just walk the DOM to get the same information).
I don't think having playback control at document.timeline level is a big requirement from devtools. When a user needs to slow an animation down, or set its current time precisely, it's usually because the user is tweaking one given animation (or a small number of animations, on a few elements, like a popup that appears by sliding down from the top, while a background overlay fades in). So, as long as can retrieve the Animation(Player) objects for these few nodes, then we're good, we don't really need to pause or slow-down at document level.

Comment 4

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

Attachment: WIP patch v1

I made a prototype of the approach outlined in comment 1.

It covers most of the functionality except for introducing a compositor object (thoughts about that outlined below). It's a pretty massive patch so the challenge is to carve off pieces that can be landed and bake independently.

I think we can approach this from two sides as follows:

From the timeline side:
* Add the list of animations to Animation and implement getAnimations() there (bug 1150810)

From the manager side:
* Move more and more code out of FlushAnimations / FlushTransitions and into Animation::Tick (but still call Animation::Tick from FlushAnimations / FlushTransitions for the time being) as follows:
  a) Make event queueing a separate step performed by Animation::Tick and dispatched in nsRefreshDriver::Tick
  b) Move throttling control to a generic AnimationCollection::RequestRestyle method and call it from Animation::Tick (this is definitely the hardest part but the WIP patch outlines the general approach)
  c) Move mutation observer batching to happen when we compose style, *not* when we tick
* Eventually we should be able to make a common FlushAnimations method on CommonAnimationManager (which would be a significant milestone in terms of aligning parts of nsAnimationManager and nsTransitionManager that ought to be common)
* Move calling Animation(Collection)::Tick to just *Manager::WillRefresh and leave FlushAnimations as simply posting restyles. If this works, then we should be pretty confident that calling tick on the appropriate objects from the timeline instead will work.

Bringing the two together:
* Make the DocumentTimeline observe the refresh driver and call tick on its animations. There's complexity here but I think the WIP patch is probably not too bad in this area.
* Remove all refresh driver watching from the managers.

After that, we need to make the animation compositor. This is largely so that we have a place to store and apply script-generated animations without needing a whole new manager. For this I think for we can reuse AnimationCollection. We'd just rename it to AnimationCompositor and make it store *both* the list of animations and transitions. It would keep the lists separate so we can quickly get the set of, e.g. animations only, so we can efficiently process changes to animation-name.

By managing both the transitions and animations, however, the compositor is a more natural place to perform correct cascading between the two and, in future, addition and layering. Also, the compositor can manage the complexity of having two lists (and later three--for script-generated animations) so call sites don't have to query both lists themselves (e.g. when getting animations for the compositor). Furthermore, we should store these lists as their concrete types (CSSTransition / CSSAnimation) and hopefully remove some casting.

Comment 5

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

I've been thinking more about the compositor object, and especially about what will be needed to support CSSAnimation and CSSTransition objects that have their effect replaced with an effect that targets a completely different element.

I wonder if we might end up with something like:

class AnimationCollection {
  // Fills in mCompositor as needed
  AnimationCompositor& Compositor();

  // Animations we own that we're keeping alive
  // Keep these as separate arrays for fast updating (and so we can preserve their concrete type)
  nsTArray<nsRefPtr<CSSTransition>> mCSSTransitions;
  nsTArray<nsRefPtr<CSSAnimation>> mCSSAnimations;
  // It's not 100% clear if the target of a script-generated animation needs to keep it alive but I suspect it does
  nsTArray<nsRefPtr<Animation>> mAnimations;

  // Animations we don't need to keep alive but which might be targetting this element
  // (we could clean up this list every time we generate mCompositor)
  // Alternatively we could use a hashtable with strong references
  nsTArray<nsWeakPtr<Animation>> mForeignAnimations;

  // Created lazily and discarded regularly (see notes below)
  nsRefPtr<AnimationCompositor> mCompositor;
};

class AnimationCompositor {
  nsTArray<nsRefPtr<KeyframeEffectReadonly>> mEffects;
};

* The compositor is populated with all the effects that target the given element in priority order
* We blow it away and create a new one anytime anything might change the set/order of effects
  - This would include things like animations being added/removed/reordered and possibly even animations finishing if we decide to only include effects that are in-effect/relevant. As a first pass we could blow it away eagerly and then gradually make it more conservative.
* We'd build the compositor lazily whenever we go to composite style or when we get a call to getAnimations.
  - This implies it's possible to get from an effect to its animation which currently isn't the case (that's by-design: we wanted to reduce the number of two-way relationships and make effects independent of the animations driving them)
* In the common case of only having local transitions/animations we could build up effects without having to sort. It's only when we have foreign transitions/animations or script-generated animations that we might need to sort.
* In generating the AnimationCompositor we'd update the cascade information too
* For updating style we just walk the array of effects and apply them in turn

I *think* this kind of structure--separating the list of effects from animations--would actually work for implementing groups too although it's unclear where we would store the animation referring to a group so that it stays alive even when it has no timeline.

Comment 6

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

I think we can close this now. There's still some more clean up work to do in bug 1239945 but nothing that blocks this.