Closed Bug 1007513 Opened 6 years ago Closed 6 years ago

AnimationEvent elapsedTimed reports wallclock time not animation time

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: birtles, Assigned: birtles)

References

Details

(Whiteboard: [parity-chrome])

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 1004361
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.)
(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.
See Also: → 1016757
Change cast to int64_t for now, will add comment / remove cast depending on outcome of bug 1016757.
Attachment #8429738 - Flags: review?(dholbert)
Attachment #8429087 - Attachment is obsolete: true
Attachment #8429087 - Flags: review?(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.
Attachment #8429738 - Flags: review?(dholbert) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/fb642eef5c91
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.