1004361 - CSS animations with short duration sometimes don't dispatch a start event

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles)]

Attachment: short-duration-events.html

In the attached test case is an animation with duration 0.001s. The test prints out the animation events that are dispatched. At least on my computer, only the end event gets dispatched.

Basically, if we have a sample that arrives before the animation starts then another sample that lands after it ends we only dispatch the end event.

This should be easy to fix once I land bug 999922 since it separates out the event queuing into a separate step.

Comment 1

[Robert Longson [:longsonr]]

This sounds the same as bug 733744

Comment 2

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #1)
> This sounds the same as bug 733744

This one actually turns out to be different. Basically in CSS animation event dispatch we only look at what the *last* event we should have dispatched should be. We never consider the case that two events might have fallen within that window of time.

For iteration events that's probably ok since trying to dispatch *all* the events in a time window can mean we need to dispatch thousands of events on a single sample when we have a really short animation that repeats infinitely. Both Gecko and Blink drop iteration events in this case. Trident doesn't but it limits the precision of times to milliseconds so you have a maximum number events that could possibly fall in a given window.

Bug 733744 is specifically due to the limited precision of times in SVG. That bug should ultimately be fixed when we switch SVG over to the same data structures as CSS animations which uses TimeStamp for times and that has higher precision.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Dispatch animationstart events as well when skipping entire animation intervals

Comment 4

[Brian Birtles (:birtles)]

Attachment: Dispatch animationstart events as well when skipping entire animation intervals

Comment 5

[Brian Birtles (:birtles)]

Attachment: Dispatch animationstart events as well when skipping entire animation intervals

Remove a comment about the bug

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8429073 [details] [diff] [review]
Dispatch animationstart events as well when skipping entire animation intervals

>@@ -257,16 +254,25 @@ ElementAnimations::GetEventsAt(TimeStamp
>       case ComputedTiming::AnimationPhase_After:
>+        // If we skipped the animation interval entirely, dispatch
>+        // 'animationstart' first
>+        if (anim->mLastNotification ==
>+            ElementAnimation::LAST_NOTIFICATION_NONE) {
>+          anim->mLastNotification = 0;

Maybe add a comment explaining that mLastNotification=0 setting, saying something along the lines of:
  // Notifying for start of 0th iteration

(This member-var is a bit tricky, since it takes *either* an enumerated value *or* an iteration index. Here, we're checking it against the former, and then setting it to the latter, which is a bit confusing & worth explaining.)

>diff --git a/layout/style/test/test_animations.html b/layout/style/test/test_animations.html

This file has a comment at the top saying "PLEASE KEEP THIS IN SYNC WITH test_animations_omta.html".

Should we be adding your new test code in the _omta test as well?

>--- a/layout/style/test/test_animations.html
>+++ b/layout/style/test/test_animations.html
>+/*
>+ * Bug 1004361 - CSS animations with short duration sometimes don't dispatch
>+ * a start event
>+ */
>+new_div("animation: anim2 1s 0.1s");
>+listen();
>+advance_clock(0); // Trigger animation
>+advance_clock(1200);

Add a comment saying "Skip past the animation's entire active duration" here.

(IIUC, that's what this is trying to do, to test this bug.)

r=me with the above.

Comment 7

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 8429073 [details] [diff] [review]
> Dispatch animationstart events as well when skipping entire animation
> intervals
> 
> >@@ -257,16 +254,25 @@ ElementAnimations::GetEventsAt(TimeStamp
> >       case ComputedTiming::AnimationPhase_After:
> >+        // If we skipped the animation interval entirely, dispatch
> >+        // 'animationstart' first
> >+        if (anim->mLastNotification ==
> >+            ElementAnimation::LAST_NOTIFICATION_NONE) {
> >+          anim->mLastNotification = 0;
> 
> Maybe add a comment explaining that mLastNotification=0 setting, saying
> something along the lines of:
>   // Notifying for start of 0th iteration

I added this as well as the following:

  // (This is overwritten below but we set it here to maintain
  // internal consistency.)

since setting this isn't strictly necessary (but if we later add code in between it would be good to ensure the internal state is consistent).

> This file has a comment at the top saying "PLEASE KEEP THIS IN SYNC WITH
> test_animations_omta.html".
> 
> Should we be adding your new test code in the _omta test as well?

My plan was, in bug 1004365--where I think we actually need to test the OMTA code path--to also add a comment saying why we're don't have OMTA tests for the other cases. Basically events are dispatched on the main thread so we don't really need to test the OMTA code path for these cases. But perhaps we should add one or two test cases in case we later tie the event-dispatch code to the throttling code.

Comment 8

[Brian Birtles (:birtles)]

Attachment: Dispatch animationstart events as well when skipping entire animation intervals;

Add OMTA version of test

Comment 9

[Brian Birtles (:birtles)]

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

Comment 10

[Carsten Book [:Tomcat]]

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