1004365 - CSS animations with zero duration don't dispatch events, don't run

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: zero-duration-animation.html

In the attached test case there is a CSS animation with zero duration. Events that are received are printed on the page.

No events are received and the div does not move despite the forwards fill mode. The spec indicates events should be dispatched and the fill mode should be applied:

> If the <time> is 0s, like the initial value, the keyframes of the animation
> have no effect, but the animation itself still occurs instantaneously. That
> is, animation-fill-mode applies as normal, filling backwards or forwards as
> appropriate, and animation events still fire.
(http://dev.w3.org/csswg/css-animations/#animation-duration)

This should be somewhat easier to fix once I land bug 999922 since it simplifies this logic somewhat.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #0)
> > If the <time> is 0s, like the initial value, the keyframes of the animation
> > have no effect, but the animation itself still occurs instantaneously. That
> > is, animation-fill-mode applies as normal, filling backwards or forwards as
> > appropriate, and animation events still fire.
> (http://dev.w3.org/csswg/css-animations/#animation-duration)

I notice that later in the spec it says:
> Any animation for which both a valid keyframe rule and a non-zero duration
> are defined will run and generate events; this includes animations with empty
> keyframe rules.
> 
> Issue: This contradicts the animation-delay section, which says that a 0s
> duration animation still fires events.
(http://dev.w3.org/csswg/css-animations/#events)

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Make active duration calculation handle zero-duration animations

This patch also makes ElementAnimation::ActiveDuration a static method that
takes timing parameters as an argument. This is so this method can be used
within ElementAnimations::GetComputedTimingAt (a static method) in a future
patch.

We could also make ActiveDuration() a method of AnimationTiming. I suspect
this logic belongs together in ElementAnimation however.

In a future patch we could also add the active duration to the ComputedTiming
struct which would simplify the only other place this is currently used
which is ElementAnimations::GetEventsAt.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Add TimeDuration != operator

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3b.1 - Add % operator to TimeDuration

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3b.2 - Make ElementAnimation::GetComputedTimingAt handle zero-duration animations

This patch adjusts GetComputedTimingAt to set the time fraction and current
iteration fields of the output computed timing correctly for animations with
zero iteration duration. Care must be taken to handle cases such as animations
that have zero duration but repeat infinitely.

The code is significantly re-arranged to more closely align with the naming and
algorithms defined in Web Animations.

A couple of tests in test_animations.html have been tweaked to account for
floating-point error. This is not because the new code is less precise but
actually the opposite. These tests fall on the transition point of step-timing
functions. The new code uses the closest possible floating-point representation
of these times which happens to cause them to fall on the opposite side of the
transition point.

For example, in evaluating a point 3s into a reversed interval the old code
would give us an intermediate time fraction of:

   0.29999999999999982

When we reverse that by subtracting from 1.0 we get: 0.70000000000000018

With the code in this patch we get an intermediate time fraction of:

   0.29999999999999999

When we reverse that by subtracting from 1.0 we get: 0.69999999999999996

Hence we fall on the opposite side of the transition boundary.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Make nsAnimationManager.cpp no longer skip zero-duration animations

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Add tests for zero-duration animations

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3a - Alternative approach to making ElementAnimations::GetComputedTimingAt handle zero-duration animations

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

A little explanation about the patches. There's a bit of "choose your own adventure" here. (Was that a thing in the US?)

I've taken two different approaches to the core part: rewriting GetComputedTimingAt.

* part 3a is basically "the simplest thing that could possibly work". It's less code. Basically one big switch.

* part 3b.1 + part 3b.2 is the "staged approach". It's more code but in a sense it's simpler because it's broken up into more of less independent stages that could be easily split off into separate methods. It lines up with the naming and algorithms in Web Animations closely.

The second approach happens to provide slightly greater accuracy in some cases but basically both approaches produce identical results (at least to about a dozen decimal points). I ran them both side by side and compared the results for all the tests in test_animations.html to make sure they were the same.

I would prefer to land 3b.1 and 3b.2 since I think that's the direction we'll end up going in the future as we implement more of Web Animations. However, I can easily be convinced to go with the shorter approach for now and just keep these patches up my sleeve for later.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8431405 [details] [diff] [review]
part 1 - Make active duration calculation handle zero-duration animations

>+++ b/layout/style/AnimationCommon.h
>-  mozilla::TimeDuration ActiveDuration() const {
>+  // Return the duration of the active interval for the given timing parameters.
>+  static mozilla::TimeDuration
>+  ActiveDuration(const AnimationTiming& aTiming) {

Nit: The new function-signature will still (barely) fit on one line, in 80 characters. (It's 1 character shorter than the comment above it.)

Probably collapse it, for coding-style consistency.

>+    if (aTiming.mIterationCount == NS_IEEEPositiveInfinity()) {
>+      // An animation that repeats forever has an infinite active duration
>+      // unless its iteration duration is zero, in which case it has a zero
>+      // active duration.
>+      return aTiming.mIterationDuration == TimeDuration()
>+             ? TimeDuration()
>+             : mozilla::TimeDuration::Forever();
>+    }
>+    return aTiming.mIterationDuration.MultDouble(aTiming.mIterationCount);

Two points on this:
(1) Maybe it's just me, but I don't immediately think "zero" when I see "TimeDuration()", which makes this hard to understand. To make this more readable (and save one of your two TimeDuration() constructor-calls), I'd suggest expressing this with a local-var for "zero", e.g.:

  const TimeDuration zeroDuration;
  return aTiming.mIterationDuration == zeroDuration ?
    zeroDuration :
    mozilla::TimeDuration::Forever();

(2) Optional: seems to me like the logic would be cleaner if we allowed MultDouble to handle infinity (as it does in the current code), and only return early if we have 0, to save us from the 0-times-infinity multiplication.

e.g.:
  TimeDuration zeroDuration;
  if (aTiming.mIterationDuration == zeroDuration) {
    return zeroDuration;
  }
  return aTiming.mIterationDuration.MultDouble(aTiming.mIterationCount);

Then we don't need to bother with NS_IEEEPositiveInfinity() or Forever() here, and the logic is simpler (just 1 branch).

But if you have a reason for preferring your formulation, I'm OK with that too.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8431411 [details] [diff] [review]
part 4 - Make nsAnimationManager.cpp no longer skip zero-duration animations

>@@ -108,18 +107,17 @@ ElementAnimations::EnsureStyleRuleFor(Ti
> 
>-      if (anim->mProperties.IsEmpty() ||
>-          anim->mTiming.mIterationDuration.ToMilliseconds() <= 0.0) {
>+      if (anim->mProperties.IsEmpty()) {
>         // The animation isn't active or filling at this time.
>         continue;

Is the "active or filling at this time" comment here correct anymore? (It looks like it's not correct after this patch, and I'm not sure it's correct before this patch is applied, either.) 

>@@ -213,23 +211,18 @@ ElementAnimations::EnsureStyleRuleFor(Ti
>-    // We should *not* skip animations with zero duration (bug 1004365) or
>-    // those with no keyframes (bug 1004377).
>-    // We will fix this separately but for now this is necessary since
>-    // ElementAnimation::GetComputedTimingAt does not yet handle
>-    // zero-duration iterations.
>-    if (anim->mProperties.IsEmpty() ||
>-        anim->mTiming.mIterationDuration.ToMilliseconds() <= 0.0) {
>+    // We should *not* skip animations with no keyframes (bug 1004377).
>+    if (anim->mProperties.IsEmpty()) {
>       // The animation isn't active or filling at this time.
>       continue;

Same here.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (2) Optional: seems to me like the logic would be cleaner if we allowed
> MultDouble to handle infinity (as it does in the current code)

Er, never mind about this part -- when I wrote this, I was thinking that TimeDuration used a float under the hood, but it doesn't, so this wouldn't work like I was thinking it would.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Daniel Holbert [:dholbert] from comment #10 and comment #11)

Thanks for the feedback Daniel! I've fixed these issues locally.
 
> (2) Optional: seems to me like the logic would be cleaner if we allowed
> MultDouble to handle infinity (as it does in the current code), and only
> return early if we have 0, to save us from the 0-times-infinity
> multiplication.

As you pointed out in comment 12, TimeDuration doesn't use a float under the hood so this doesn't fall out quite so naturally as it otherwise might. But that's not to say we couldn't do it. We already have TimeDuration::Forever which acts somewhat like an infinity value (or at least that's how I've been using it).

We *could* make MultDouble check for infinity and return TimeDuration::Forever. It would also probably want to check for negative infinity and return TimeDuration::NegativeForever? (MinusForever?) in that case.

(I also notice that although we only have NS_IEEEPositiveInfinity(), mfbt/FloatingPoint.h gives us both mozilla::PositiveInfinity<double> and mozilla::NegativeInfinity<double>. I probably should be using mozilla::PositiveInfinity<double> where I've been using NS_IEEEPositiveInfinity().)

I could do this as a follow-up if you think it's worth it. I'm currently still working out if we will be able to continue using TimeDuration to represent animation times. If we can then this is probably worth doing.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #13)
> We *could* make MultDouble check for infinity and return
> TimeDuration::Forever. It would also probably want to check for negative
> infinity and return TimeDuration::NegativeForever? (MinusForever?) in that
> case.

Yeah... It's probably not worth it, though. There are probably other TimeDuration users who never deal with infinity (not intentionally at least), and we probably shouldn't slow down all of their MultDouble calls with this additional check.

> (I also notice that although we only have NS_IEEEPositiveInfinity(),
> mfbt/FloatingPoint.h gives us both mozilla::PositiveInfinity<double> and
> mozilla::NegativeInfinity<double>. I probably should be using
> mozilla::PositiveInfinity<double> where I've been using
> NS_IEEEPositiveInfinity().)

Ah -- that probably does make sense, yes. It looks like most of the (few) remaining usages of NS_IEEEPositiveInfinity are in animation code, and they probably should be using PositiveInfinity<double>.

> I could do this as a follow-up if you think it's worth it.

The s/NS_IEEEPositiveInfinity/PositiveInfinity/ switch is probably worth it, but I'd lean against the MultDouble change, I think.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8431409 [details] [diff] [review]
part 3b.1 - Add % operator to TimeDuration

>+  TimeDuration operator%(const TimeDuration& aOther) const {
>+    MOZ_ASSERT(aOther.mValue != 0, "Division by zero");
>+    if (aOther.mValue == INT64_MAX) {
>+      // Division by "forever" leaves no remainder
>+      return TimeDuration();
>+    }
>+    // If the sign of mValue and aOther.mValue differ, the sign of the
>+    // result is implementation-defined. That's probably not desirable.
>+    MOZ_ASSERT((mValue >= 0) ^ (aOther.mValue < 0), "Sign of operands differs");
>+    return TimeDuration::FromTicks(mValue % aOther.mValue);
>+  }

Per IRC, I don't think we want the INT64_MAX special-case here.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8431410 [details] [diff] [review]
part 3b.2 - Make ElementAnimation::GetComputedTimingAt handle zero-duration animations

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

::: layout/style/AnimationCommon.cpp
@@ +403,5 @@
> +
> +  // When we finish exactly at the end of an iteration we need to report
> +  // the end of the final iteration and not the start of the next iteration
> +  // so we set up a flag for that case.
> +  bool endOfLastIteration = false;

Maybe s/Last/Final/ in the variable-name?  (since "last" can also mean "previous"... stupid English.)

And while you're at it, could you add an "is" prefix, to make it clearer that this is a bool and not a timestamp?

So, that would be e.g. s/endOfLastIteration/isEndOfFinalIteration/

@@ +441,5 @@
> +  if (aTiming.mIterationDuration != TimeDuration()) {
> +    iterationTime = endOfLastIteration
> +                    ? aTiming.mIterationDuration
> +                    : activeTime % aTiming.mIterationDuration;
> +  } /* else, iteration time is zero */

s/iteration time/iterationTime/ in the comment

@@ +452,5 @@
> +                   // to make sure it ends up being infinity.
> +      : static_cast<uint64_t>(aTiming.mIterationCount) - 1;
> +  } else if (activeTime == TimeDuration()) {
> +    // If the active time is zero we're either in the first iteration or
> +    // we have finished an animation with an iteration duration of zero that

RE this comment at the end here -- I think activeTime==0 could also mean we're *before* the first iteration (and filling backwards).  At least, that's what it looks like from the clause above where we set "AnimationPhase_Before", with the " // activeTime is zero" comment.

(I suppose that backward-fill still counts at the 0th iteration, so I think the code is correct here. Might be worth clarifying the comment to more clearly acknowledge this "before" case, though.)

@@ +473,5 @@
> +                         ? 1.0
> +                         : fmod(aTiming.mIterationCount, 1.0f);
> +  } else {
> +    // We are in the active phase so the iteration duration can't be zero.
> +    result.mTimeFraction =

Assert that aTiming.mIterationDuration != TimeDuration(), to sanity-check the comment here.

::: layout/style/test/test_animations.html
@@ +700,5 @@
>            "keyframe timing functions test at 31s");
>  advance_clock(1000);
>  is_approx(px_to_num(cs.paddingBottom), 120 - 100 * gTF.linear(0.2), 0.01,
>            "keyframe timing functions test at 32s");
> +advance_clock(990); // avoid floating-point error

For places like this where you're changing from e.g. advancing 1000 to now advancing 990 or 1010 (to be on the correct side of a transition-boundary, per your extended commit message), could we instead use 999 / 1001? (i.e. only giving ourselves 1ms of fudge factor, instead of 10.)

Or does that produce the wrong results?

A 10ms fudge-factor is smallish, but still seems like a decent amount of allowable-error. I'd rather clamp down the allowable-error here to the smallest value where we still pass, which I'm hoping might be 1ms, but maybe I'm being optimistic. :)

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8431411 [details] [diff] [review]
part 4 - Make nsAnimationManager.cpp no longer skip zero-duration animations

r=me on part 4, modulo comment 11.

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> ::: layout/style/test/test_animations.html
> @@ +700,5 @@
> >            "keyframe timing functions test at 31s");
> >  advance_clock(1000);
> >  is_approx(px_to_num(cs.paddingBottom), 120 - 100 * gTF.linear(0.2), 0.01,
> >            "keyframe timing functions test at 32s");
> > +advance_clock(990); // avoid floating-point error
> 
> For places like this where you're changing from e.g. advancing 1000 to now
> advancing 990 or 1010 (to be on the correct side of a transition-boundary,
> per your extended commit message), could we instead use 999 / 1001? (i.e.
> only giving ourselves 1ms of fudge factor, instead of 10.)
> 
> Or does that produce the wrong results?
> 
> A 10ms fudge-factor is smallish, but still seems like a decent amount of
> allowable-error. I'd rather clamp down the allowable-error here to the
> smallest value where we still pass, which I'm hoping might be 1ms, but maybe
> I'm being optimistic. :)

Yes, I tend to agree. However, there was already an existing case in this file where the author had changed the sample time by 10ms to account for floating-point error so I simply followed suit. I try changing all three cases to 1ms and push to try to see if it is enough for all platforms.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8431412 [details] [diff] [review]
part 5 - Add tests for zero-duration animations

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

r=me with comments addressed.

::: layout/style/test/test_animations.html
@@ +1615,5 @@
> +advance_clock(0);
> +// Seek to exactly the point where the animation starts and stops
> +advance_clock(1000);
> +is(cs.getPropertyValue("margin-right"), "100px",
> +   "margin-right at exact end of zero-duration animation");

Probably worth checking that *right before* we hit the animation start/end point, we have the backwards-fill applied still? (Not sure that's checked in any other tests)  That better-demonstrates that this is a hard threshold.

So, instead of the current advance_clock(1000): I'm imagining you'd do advance_clock(999), check for 0px, and then advance_clock(1) (to step over the threshold), and check that we've got 100px.  Or something like that.

@@ +1689,5 @@
> +is(cs.getPropertyValue("margin-right"), "100px",
> +   "margin-right during forwards fill of infinitely repeating " +
> +   "zero-duration animation");
> +// Check we don't get infinite iteration events :)
> +check_events([{ type: 'animationstart', target: div,

Heh. :) That reminds me -- I don't think I see any tests that count how many events we send for e.g. a 0-duration animation that has 2 repeats (or N repeats, for N != infinity).  (You have tests for the *computed value* with various iteration counts, but not for the events that get sent.) 

Maybe add a test for that?

ALSO: Could you add a test w/ a duration of 0 *and* an animation-iteration-count of 0? That seems like an edge-case that'd be worth testing, and I don't see any tests for it currently.  (The spec says, for animation-iteration-count=0: "similar to an animation-duration of 0s, causes the animation to occur instantaneously".)

@@ +1743,5 @@
> +             "with negative delay");
> +done_div();
> +
> +/*
> + * Bug 1004365 - zero-duration animations

It looks like everything below this point (this second instance of the comment) is identical to the section above it.  I think you accidentally copypasted two copies of this new section here...? :)

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3b.1 - Add % operator to TimeDuration

Remove unnecessary branch as per comment 15.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8437487 [details] [diff] [review]
part 3b.1 - Add % operator to TimeDuration

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

r=me with the change below.

::: xpcom/ds/TimeStamp.h
@@ +132,5 @@
> +  TimeDuration operator%(const TimeDuration& aOther) const {
> +    MOZ_ASSERT(aOther.mValue != 0, "Division by zero");
> +    // If the sign of mValue and aOther.mValue differ, the sign of the
> +    // result is implementation-defined. That's probably not desirable.
> +    MOZ_ASSERT((mValue >= 0) ^ (aOther.mValue < 0), "Sign of operands differs");

Older C++ standards were even stricter: only if both operands are non-negative is the sign of the result defined.

Since we are assuming C++11 now, which specifies the sign of the result, I think we can dispense with this check.

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb50fd44b52a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bda7f4d2b98b
https://hg.mozilla.org/integration/mozilla-inbound/rev/25494b32627f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e097537a5589
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e75b68e61f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b35a6797354b

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb50fd44b52a
https://hg.mozilla.org/mozilla-central/rev/bda7f4d2b98b
https://hg.mozilla.org/mozilla-central/rev/25494b32627f
https://hg.mozilla.org/mozilla-central/rev/e097537a5589
https://hg.mozilla.org/mozilla-central/rev/7e75b68e61f3
https://hg.mozilla.org/mozilla-central/rev/b35a6797354b