1216846 - Pausing after finishing should not update the currentTime

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

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

This is possibly a spec issue but we currently have the following behavior:

  anim.play();
  anim.ready.then(() => {
    anim.finish(); // anim.currentTime === end of effect
    anim.pause();  // anim.currentTime === end of effect
    return anim.ready;
  }).then(() => {
    // anim.currentTime === end of effect plus a bit
  });

Comment 1

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

Attachment: Don't update hold time when completing a pause if it is already set

The Animation.pause() method operates asynchronously since, if the animation is
currently running on the compositor, we should wait for the animation to stop
on the compositor before establishing the pause time. Otherwise, if the
compositor is ahead of the main thread and we use the main thread's notion of
the current time to establish the pause time, the animation will jump backwards
when we take it off the compositor.

This pause time is represented using the "hold time".

However, when we have a finished animation, its current time is not advancing
but rather its current time is fixed to its end time. This too is represented
using the hold time. As a result, if we pause a finished animation we should
not update its hold time (by calculating the current time from the start time)
but just continue to use the existing hold time. This is true of any other
situation where we might have set the hold time before or during pausing.

Comment 2

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

If try doesn't reveal any surprises and there are no problems with the review, I'll update the spec as well.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cbfd8d47333

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2306162371

Comment 4

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

Updated the spec:
https://github.com/w3c/web-animations/commit/1665c727d81bf2f3a0dc6da588536bec255b75c4

Comment 5

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

When this landed this failure showed up:


  devtools/server/tests/browser/browser_animation_playerState.js | The 2nd animation's playState is correct - Got finished, expected running

It's shown up once or twice since too. I've no idea why. (Perhaps something in DevTools tests is expecting an animationchanged mutation record and no longer gets it since the bug is fixed?)

I've retriggered the job on try to see if I can reproduce it there:

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

Comment 6

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

(In reply to Brian Birtles (:birtles) from comment #5)
> I've retriggered the job on try to see if I can reproduce it there:
> 
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cbfd8d47333

That doesn't seem to fail at all so either:

* It's really intermittent,
* Something landed in between mid-last week (the revision that try run is based on) and the combination of whatever landed and this patch is producing an intermittent failure, or
* Something else landed recently that caused this problem and this patch was just unlucky to trip up on it when it landed

Triggering a few more runs to see if I can eliminate option 1.

Comment 7

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

Looking at that test, when it fails, it's because the animation is testing has already finished. But the animation under test is only 2s long! We normally make animations at least 100s long if we're going to assume they're still playing.

I think we just need to fix that test.

Comment 8

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

(In reply to Brian Birtles (:birtles) from comment #7)
> I think we just need to fix that test.

Filed bug 1220534 for the intermittent failure.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a2306162371

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4a2306162371