1134379 - old AnimationPlayers for redirected transitions should be cancelled

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 1

[Cameron McCormack (:heycam)]

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

Comment 2

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f74fcc98ea52

Comment 3

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

I wonder if the call to Cancel should be conditioned on !IsFinishedTransition().  (Is it important to be consistent about that?  Is it observable?)

Comment 4

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

(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #3)
> I wonder if the call to Cancel should be conditioned on
> !IsFinishedTransition().  (Is it important to be consistent about that?  Is
> it observable?)

Yes, it is observable. If script held onto an AnimationPlayer object corresponding to a transition and then another transition on the same property was triggered, script would see the startTime of the player go to null on the (now cancelled) transition.

I think that's ok? Alternatively we could define transitions such that we call cancel on them as soon as they finish. It's probably an issue to discuss when we spec transitions in terms of Web Animations.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f74fcc98ea52

Comment 6

[u279076]

Does this need tests of it's own or is it sufficiently covered by pre-existing tests? Are there any areas of risk QA should be on the lookout for?

Comment 7

[Cameron McCormack (:heycam)]

It should have been covered by test_animation_observers.html, but it looks like it isn't.  We should add another sub-test in that file that is similar to single_transition_noninterpolable, but which sets the property to an interpolable value, and then checks that the original AnimationPlayer is removed and a new one is added at the same time.  Do you want to add the test Anthony?

Comment 8

[u279076]

I'd be happy to add a test provided some mentorship. Where should I begin?

Comment 9

[Cameron McCormack (:heycam)]

Great!  I'm not sure how familiar you are with the Web Animations API: http://w3c.github.io/web-animations/

To summarise the relevant parts of it: for every running CSS animation (and transition), there is an AnimationPlayer object associated with the element that the animation is targetting.  The AnimationPlayer object exposes to script dynamic information about the animation, such as its current time within the animation, what time the animation started, whether it's paused, and also has controls for starting/stopping/etc. the animation.  You can get the AnimationPlayer objects associated with a given element by calling getAnimationPlayers() on the element.

(There have been some recent names in the spec, AnimationPlayer -> Animation, getAnimationPlayers -> getAnimations, and others, but we still implement the old names.)

The tests in test_animation_observers.html are for the chrome-only animation observation API, which got added in bug 1123523.  It's basically an extension to MutationObserver that generates records with the list of AnimationPlayer objects that were added, changed and removed from an element.  So you could use it like this:

  <style>
    div { background-color: blue; transition: background-color 1s; }
    div:hover { background-color: lime; }
  </style>
  <div>hello</div>
  <script>
    var div = document.querySelector("div");
    var obs = new MutationObserver(function(records) {
      for (var i = 0; i < records.length; i++) {
        var record = records[i];
        if (record.addedAnimations.length)
          console.log("added " + record.addedAnimations.length + " animations");
        if (record.changedAnimations.length)
          console.log("changed " + record.addedAnimations.length + " animations");
        if (record.removedAnimations.length)
          console.log("removed " + record.addedAnimations.length + " animations");
      }
    });
    obs.observe(div, { animations: true });
  </script>

and then hovering over the div will generate a record with an AnimationPlayer object in the addedAnimations array, and when the transition finishes, it will generate a record with that same object in the removedAnimations array.  The generation of these two records is what's tested by the single_transition sub-test in test_animation_observers.html.

Transitions get a new AnimationPlayer for each new animations between two values it performs.  So for example, if you moved the mouse away from the div in the middle of the transition to background-color:lime, then you would get a new record that had a removal of the existing AnimationPlayer and the addition of a new AnimationPlayer representing the animation from the current between-blue-and-lime colour back to blue.

Before this bug was fixed, when you redirected a transition from one end-value to a new end-value (and specifically, to a new end-value that is not the original start-value, i.e. we're not reversing the transition), the original AnimationPlayer was not cancelled.  The new AnimationPlayer object was created and overrode the old one, but the old one stuck around.  This was observable using the animation observation API.

So I think we want a test that starts a transition, updates the style of the transitioning element mid-way through the transition to a new end-value, and then verifies that we get the notification records:

  just after the initial transition starts:
    { added: [firstPlayer] }

  just after the style is updated to transition to a different value:
    { added: [secondPlayer], removed: [firstPlayer] }

  just after the (second) transition finishes:
    { removed: [secondPlayer] }

single_transition_cancelled_noninterpolable is probably the closest subtest to this one; that subtest is testing the above but where the new end-value is a non-interpolable value, but that's meant to cancel the AnimationPlayer without adding a new one like this new one will.

Comment 10

[u279076]

(In reply to Cameron McCormack (:heycam) from comment #9)
> Great!  I'm not sure how familiar you are with the Web Animations API:
> http://w3c.github.io/web-animations/

I'm sorry but I'm going to have to drop this for now. I switched over to the gfx team last quarter and just don't have the time to continue working on this. My apologies but I'd rather be honest and drop this than let it linger indefinitely.

Comment 11

[Cameron McCormack (:heycam)]

No problem, Anthony.