1007513 - AnimationEvent elapsedTimed reports wallclock time not animation time

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles)]

The AnimationEvent.elapsedTime member has the following description,

"The amount of time the animation has been running, in seconds, when this event fired, excluding any time the animation was paused."
(http://dev.w3.org/csswg/css-animations/#interface-animationevent-attributes)

The important feature here is that it reports the amount of time the animation has been running. So, if we dispatch an animationend event for a 2s animation, 10s after it finishes, it has still only been animating for 2s so the elapsedTime should be 2, not 10.

Using the following fiddle I notice that Blink will give us the length of the animation but Gecko gives us the amount of time from the start of the animation until the time when the event is queued.

  http://jsfiddle.net/g878t/

For start times, the spec is quite clear:

"For an animationstart event, the elapsedTime is zero unless there was a negative value for animation-delay, in which case the event will be fired with an elapsedTime of (-1 * delay)."

Basically, the elapsedTime should report the time when the event would occur given infinite sampling frequency, not when it was actually queued.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Make AnimationEvent.elapsedTime report the actual time the animation has been running

This patch implements the behavior proposed in:

  http://lists.w3.org/Archives/Public/www-style/2014May/0356.html

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8429087 [details] [diff] [review]
Make AnimationEvent.elapsedTime report the actual time the animation has been running

Haven't gotten through this yet, but here's the first thing I noticed:

>diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp
>+          TimeDuration iterationStart =
>+            anim->mTiming.mIterationDuration *
>+            static_cast<uint32_t>(computedTiming.mCurrentIteration);

Looks like this static_cast is because mCurrentIteration is uint64_t, and TimeDuration doesn't have an operator*() method for uint64_t. Yes?

If we have to cast, I'd prefer we cast to int64_t (instead of uint32_t), to make this less lossy. Though that's still a bit sketchy.

I'd *really* prefer that we just add a new operator* method for TimeDuration that takes a uint64_t.  (It presumably will have odd results (wrapping or something) for large uint64_t values, but that's to be expected, and is already the case for the operator*() for int64_t if you pass in large values.) (That probably should happen in a helper-bug & should get review from someone with ownership over TimeDuration.)

Comment 3

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> I'd *really* prefer that we just add a new operator* method for TimeDuration
> that takes a uint64_t.  (It presumably will have odd results (wrapping or
> something) for large uint64_t values, but that's to be expected, and is
> already the case for the operator*() for int64_t if you pass in large
> values.) (That probably should happen in a helper-bug & should get review
> from someone with ownership over TimeDuration.)

Filed as bug 1016757.

Comment 4

[Brian Birtles (:birtles)]

Attachment: Make AnimationEvent.elapsedTime report the actual time the animation has been running

Change cast to int64_t for now, will add comment / remove cast depending on outcome of bug 1016757.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8429738 [details] [diff] [review]
Make AnimationEvent.elapsedTime report the actual time the animation has been running

(In reply to Brian Birtles (:birtles) from comment #4)
> Change cast to int64_t for now, will add comment / remove cast depending on
> outcome of bug 1016757.

Thanks! r+ with one or the other of those, yeah.

Comment 6

[Brian Birtles (:birtles)]

Thanks Daniel!

(In reply to Brian Birtles (:birtles) from comment #1)
> This patch implements the behavior proposed in:
> 
>   http://lists.w3.org/Archives/Public/www-style/2014May/0356.html

Also, Tab replied to this message agreeing with the proposed behavior, so from a spec-compliance point of view this patch should be ok.

Comment 7

[Brian Birtles (:birtles)]

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

Comment 8

[Carsten Book [:Tomcat]]

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