"animation-play-state: paused;" doesn't apply immediately

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: leo, Assigned: birtles)

Tracking

({regression})

Trunk
mozilla35
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34+ fixed, firefox35 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
When using "animation-play-state: paused;", the pausing doesn't seem to kick in immediately, allowing the animation to start before pausing.

Testcase showing bug: http://jsfiddle.net/op039zhy/2/

Expected result: http://i.imgur.com/9VEI6Hi.png
Actual result: http://i.imgur.com/ymBu36A.png

Results from mozregression:
Last good revision: eed9fe35a00d (2014-08-30)
First bad revision: 1db35d2c9a2f (2014-08-31)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eed9fe35a00d&tochange=1db35d2c9a2f
Last good revision: 6a6b450605c5
First bad revision: 1db35d2c9a2f
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6a6b450605c5&tochange=1db35d2c9a2f

Comment 1

4 years ago
Created attachment 8485463 [details]
CSS-only testcase

Updated

4 years ago
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Flags: needinfo?(dzbarsky)
Flags: needinfo?(birtles)
[Tracking Requested - why for this release]: Regression in web-facing functionality.
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
(Assignee)

Comment 3

4 years ago
I'm pretty sure that in changeset http://hg.mozilla.org/mozilla-central/rev/9db3e43c19c1 the following lines:

-    dest->mStartTime = aTimeline->GetCurrentTimeDuration();
+    dest->mStartTime = now;
     dest->mPlayState = src.GetPlayState();
     if (dest->IsPaused()) {
       dest->mHoldTime = now;
-    } else {
-      dest->mHoldTime = TimeStamp();
     }

should actually set the hold time to zero when dest->IsPaused(). i.e.

      dest->mHoldTime.SetValue(TimeDuration(0));

Patch coming up.
Flags: needinfo?(dzbarsky)
Flags: needinfo?(birtles)
(Assignee)

Comment 4

4 years ago
Created attachment 8485600 [details] [diff] [review]
Set the animation player hold time to zero when it is initially paused

This patch fixes a regression from bug 1033114, m-c changeset 9db3e43c19c1.

That changeset changed the meaning of mHoldTime (despite the commit message
which erroneously refers to mStartTime) to make it an offset from the start time
rather than a timestamp. However, it failed to update the case when we have an
initially-paused player. In that case the offset should be zero but the existing
code set it to the same value as the start time (which is, itself, an offset
from the beginning of the timeline) and the above changeset failed to update
that.
Attachment #8485600 - Flags: review?(dzbarsky)
(Assignee)

Updated

4 years ago
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Comment on attachment 8485600 [details] [diff] [review]
Set the animation player hold time to zero when it is initially paused

Review of attachment 8485600 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch.  It's a little worrisome that we didn't have a test for this...
Attachment #8485600 - Flags: review?(dzbarsky) → review+
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ef4e254bd15

I'll request aurora approval after a few days if this looks ok on trunk.
https://hg.mozilla.org/mozilla-central/rev/8ef4e254bd15
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → affected
tracking-firefox34: ? → +
tracking-firefox35: ? → +
(Assignee)

Comment 9

4 years ago
This is already fixed on trunk.
status-firefox35: affected → fixed
tracking-firefox35: + → ---
(Assignee)

Comment 10

4 years ago
Comment on attachment 8485600 [details] [diff] [review]
Set the animation player hold time to zero when it is initially paused

Approval Request Comment
[Feature/regressing bug #]: bug 1033114
[User impact if declined]: Some animations will be incorrect when initially paused
[Describe test coverage new/current, TBPL]: Patch includes test, has had about a week of bake time on m-c.
[Risks and why]: Minimal, one line change.
[String/UUID change made/needed]: None.
Attachment #8485600 - Flags: approval-mozilla-aurora?
Comment on attachment 8485600 [details] [diff] [review]
Set the animation player hold time to zero when it is initially paused

Aurora+

FYI, in terms of tracking flags, we'll often leave bugs that have been fixed marked with tracking+ so that we are alerted in case they are reopened. (I don't think we need to do so in this case just so you know for future.)
Attachment #8485600 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 13

4 years ago
(In reply to Lawrence Mandel [:lmandel] from comment #11)
> FYI, in terms of tracking flags, we'll often leave bugs that have been fixed
> marked with tracking+ so that we are alerted in case they are reopened. (I
> don't think we need to do so in this case just so you know for future.)

Ok thanks for the explanation, I didn't know that.
layout/style/test/test_animations.html
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.