Closed Bug 1063992 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + fixed
firefox35 --- fixed

People

(Reporter: lmcardle, Assigned: birtles)

References

Details

(Keywords: regression)

Attachments

(2 files)

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
Attached file CSS-only testcase
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.
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)
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: 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+
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
This is already fixed on trunk.
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+
(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.