Pausing after finishing should not update the currentTime

RESOLVED FIXED in Firefox 45, Firefox OS v2.5

Status

()

Core
DOM: Animation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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
  });
(Assignee)

Comment 1

3 years ago
Created attachment 8680325 [details] [diff] [review]
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.
Attachment #8680325 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
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
Attachment #8680325 - Flags: review?(cam) → review+
(Assignee)

Comment 5

3 years ago
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
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

3 years ago
(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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a2306162371
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.