1248338 - Support iterationStart in timing calculations

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

We currently allow the iterationStart parameters to be passed to be Element.animate but we don't actually do anything with it and getComputedTiming() always returns a value of 0 for iterationStart.

This bug is about incorporating the iterationStart in the timing calculations below and returning it from getComputedTiming().

  http://w3c.github.io/web-animations/#calculating-the-scaled-active-time
  http://w3c.github.io/web-animations/#calculating-the-iteration-time
  http://w3c.github.io/web-animations/#calculating-the-current-iteration

We will make this property writeable in bug 1244638.

Comment 1

[Brian Birtles (:birtles)]

I think we need at least the following tests:

* element.animate(... { iterationStart: ... } )
  -> animation.timing.iterationStart returns the same value
  -> animation.getComputedTiming() return the same value
* Test during delay phase with backwards fill
  -> animation.getComputedTiming().progress (e.g. iterationStart: 2.5 -> progress: 0.5)
  -> As above but with a timing function ('easing') applied to check that the iterationStart progress is scaled
  -> animation.getComputedTiming().currentIteration (e.g. iterationStart: 2.5 -> currentIteration: 2)
* Test animation.getComputedTiming().activeDuration is not affected
  e.g. iterations: 2, iterationStart: 0.5, duration: 1 -> activeDuration = 2 (NOT 1.5)
* Test at start of animation
  -> test progress
* Test at end of animation with forwards fill
  -> e.g. iterationStart: 2.5, iterations: 2 -> progress 0.5 during fill
  -> likewise currentIteration = 4
  -> Test for when we have a timing function ('easing') applied that we also apply it to the correct final value
  -> Test for special case when iterationStart + iterations is an integer
     e.g. iterationStart: 0.5, iterations: 1.5 -> progress 1.0, currentIteration = 1
* Test zero-duration animation
  -> Basically all the above tests but for an animation with zero duration

Mozilla-specific tests:

* Test compositor animation
  -> Test that we actually pass the animation to the compositor
     e.g. we could write a test similar to layout/style/test/test_animations_playbackrate.html
     Alternatively we could write a reftest for that

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: git diff as the patch

I attached the patch( 1242702.gitdiff ).
Please review this.

Comment 3

[Brian Birtles (:birtles)]

Comment on attachment 8721141 [details] [diff] [review]
git diff as the patch

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

This looks pretty good but the changes to KeyframeEffect.cpp need some work (including some spec work).

::: dom/animation/KeyframeEffect.cpp
@@ +234,5 @@
>    result.mIterations = IsNaN(aTiming.mIterations) || aTiming.mIterations < 0.0f ?
>                         1.0f :
>                         aTiming.mIterations;
> +
> +  result.mIterationStart = IsNaN(aTiming.mIterationStart) || aTiming.mIterationStart < 0.0f ?

Nit: Perhaps remove the black line before this so mIterations and mIterationStart are grouped together?

Also, please wrap to < 80 characters.

@@ +276,4 @@
>      activeTime = localTime - aTiming.mDelay;
>    }
>  
> +  // Calculating the scaled active time

Nit: Calculate

@@ +276,5 @@
>      activeTime = localTime - aTiming.mDelay;
>    }
>  
> +  // Calculating the scaled active time
> +  StickyTimeDuration startOffset = 

Nit: Trailing whitespace

@@ +277,5 @@
>    }
>  
> +  // Calculating the scaled active time
> +  StickyTimeDuration startOffset = 
> +                     result.mDuration.MultDouble(result.mIterationStart);

This doesn't handle the case when the result.mDuration is infinity and result.mIterationStart is zero. In that case MultDouble will assert.

We should add a test that gets that assertion to fail and then we should add a check such that if result.mIterationStart is zero, startOffset is zero.

e.g. StickyTimeDuration startOffset = result.mIterationStart == 0.0 ?
                                      StickyTimeDuration(0) :
                                      result.mDuration.MultDouble(
                                        result.mIterationStart);

Note that the spec calls this out as, "If the iteration start is zero, the start offset is zero."

@@ +286,5 @@
>    if (result.mDuration != zeroDuration) {
> +    iterationTime = result.mPhase == ComputedTiming::AnimationPhase::After && 
> +                 fmod(result.mIterations + result.mIterationStart, 1.0) == 0.0
> +                 ? result.mDuration
> +                 : scaledActiveTime % result.mDuration;

This doesn't seem right since it doesn't check if mIterations is zero or not. It's also not clear why it is different to the spec and the indentation is hard to read.

Perhaps we can do something like:

    // Although the spec makes the the first condition, "If the active time is
    // equal to the active duration..." we know that this is only ever true when
    // we are in the after phase. Furthermore, in the after phase the active
    // time is always equal to the active duration unless it is null, in which
    // case we will have already returned. Hence we just test here for whether
    // or not we are in the active phase.
    bool isEndOfFinalIteration =
      result.mPhase == ComputedTiming::AnimationPhase::After &&
      result.mIterations != 0.0 &&
      fmod(result.mIterations + result.mIterationStart, 1.0) == 0.0;
    iterationTime = isEndOfFinalIteration ?
                    result.mDuration :
                    scaledActiveTime % result.mDuration;

All that said, I'm not sure dropping isEndOfFinalIteration from the after phase branch above is much of an improvement.

@@ -284,5 @@
>    if (result.mDuration != zeroDuration) {
> -    iterationTime = isEndOfFinalIteration
> -                    ? result.mDuration
> -                    : activeTime % result.mDuration;
> -  } /* else, iterationTime is zero */

We should leave this comment here.

@@ +295,4 @@
>      // If the active time is zero we're either in the first iteration
>      // (including filling backwards) or we have finished an animation with an
>      // iteration duration of zero that is filling forwards (but we're not at
>      // the exact end of an iteration since we deal with that above).

This comment no longer makes sense since we dropped that condition.

@@ +302,5 @@
> +      ? static_cast<uint64_t>
> +        (result.mIterations + result.mIterationStart - 1)
> +      : static_cast<uint64_t>
> +        (result.mIterations + result.mIterationStart) // floor
> +      : static_cast<uint64_t> (result.mIterationStart);

So I guess the spec is wrong here since it fails to deal with the case when we're in the after phase.

Still, I think we can make this a lot more clear. Let me think about how to fix the spec then we can try to match it. Maybe something like the following?

  // Determine the 0-based index of the current iteration.
  if (isEndOfFinalIteration) {
    result.mCurrentIteration =
      IsInfinite(result.mIterations) ?
      UINT64_MAX : // In GetComputedTimingDictionary(), we will convert this
                   // into Infinity.
      result.mIterationStart + result.mIterations - 1;
  } else if (result.mPhase == ComputedTiming::AnimationPhase::After) {
    result.mCurrentIteration =
      IsInfinite(result.mIterations) ?
      UINT64_MAX : // In GetComputedTimingDictionary(), we will convert this
                   // into Infinity.
      ceil(result.mIterations + result.mIterationStart) - 1;
  } else if (result.mPhase == ComputedTiming::AnimationPhase::Before ||
             activeTime == zeroDuration) {
    result.mCurrentIteration = static_cast<uint64_t>(result.mIterationStart);
  } else {
    result.mCurrentIteration =
      result.mDuration == StickyTimeDuration::Forever()
      ? 0
      : static_cast<uint64_t>(scaledActiveTime / result.mDuration); // floor
  }

Anyway, let me think about it some more.

@@ +326,5 @@
>  
>    // Normalize the iteration time into a fraction of the iteration duration.
>    if (result.mPhase == ComputedTiming::AnimationPhase::Before) {
> +    double progress = fmod(result.mIterationStart, 1.0);
> +    result.mProgress.SetValue(progress);

Is this correct? I don't think this incorporates the timing function?

@@ +334,2 @@
>                        ? 1.0
> +                      : fmodValue;

Likewise, I don't think this incorporates the timing function?

::: layout/style/test/file_animations_iterationstart.html
@@ +37,5 @@
> +
> +addAsyncAnimTest(function *() {
> +  var [ div ] = new_div("test");
> +  var animation = div.animate(
> +    { transform:["translate(0px)", "translate(100px)"] },

Nit: space after 'transform:'

@@ +40,5 @@
> +  var animation = div.animate(
> +    { transform:["translate(0px)", "translate(100px)"] },
> +    { iterationStart: 0.5, duration: 10000, fill: "both"}
> +  );
> +  omta_is(div, "transform", { tx: 50 }, RunningOn.MainThread, "before paints");

This is fine but maybe it's better to wait for paints immediately and test that the animation is running on the compositor, e.g.

  var animation = div.animate(....);
 
  yield waitForPaints();
  omta_is(div, "transform", { tx: 50 }, RunningOn.Compositor,
          "Start of animation");

  advance_clock(4000);
  omta_is(div, "transform", { tx: 90 }, RunningOn.Compositor,
          "40% of animation");

  advance_clock(6000);
  yield waitForPaints();
  omta_is(div, "transform", { tx: 50 }, RunningOn.MainThread,
          "End of animation");

::: testing/web-platform/tests/web-animations/keyframe-effect/iterationStart.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>iterationStart tests</title>
> +<link rel="help" href="http://w3c.github.io/web-animations/#dom-animatable-animate">

https://w3c.github.io/web-animations/#iteration-start

@@ +9,5 @@
> +<link rel="stylesheet" href="/resources/testharness.css">
> +<body>
> +<div id="log"></div>
> +<iframe src="data:text/html;charset=utf-8," width="10" height="10"
> +  id="iframe"></iframe>

We don't need this iframe so we can remove it.

@@ +15,5 @@
> +'use strict';
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, {iterationStart: 0.5, duration: 1});

This line is over 80 characters long. We should wrap lines to be less than 80 characters.[1]

e.g.

  var anim = div.animate({ opacity: [ 0, 1 ] },
                         { iterationStart: 0.5, duration: 1 });

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Line_Length

Also, for Javascript objects, I think it is more clear if there is a space on the inside of the braces.[2]

[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects

@@ +18,5 @@
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, {iterationStart: 0.5, duration: 1});
> +  assert_equals(anim.effect.timing.iterationStart, 0.5);
> +  assert_equals(anim.effect.getComputedTiming().iterationStart, 0.5);
> +}, 'Test same value as a parameter be set in the object');

'iterationStart passed to constructor is reflected in specified and computed timing'
(But wrapped to fit in 80 characters.)

@@ +22,5 @@
> +}, 'Test same value as a parameter be set in the object');
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 

There is an extra space at the end of this line. Can you adjust your editor to highlight trailing spaces?

@@ +23,5 @@
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 2.5, delay:1000, fill:'backwards', duration: 1});

We should line these parameters up more neatly, e.g.

  var anim = div.animate({ opacity: [ 0, 1 ] },
                         { iterationStart: 2.5, delay: 1000,
                           fill: 'backwards', duration: 1 });

@@ +32,5 @@
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 2.5, delay:1000, 
> +              fill:'backwards', easing:'ease-in', duration: 1});

Similarly here, please remove trailing whitespace and line up the arguments more neatly.

@@ +33,5 @@
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 2.5, delay:1000, 
> +              fill:'backwards', easing:'ease-in', duration: 1});
> +  assert_not_equals(anim.effect.getComputedTiming().progress, 0.5);

Instead of assert_not_equals I think it would be better to check that the correct easing was applied. That's probably easier to do with a step timing function (and an iterationStart that is, e.g. 2.75 or 2.25 instead of 2.5).

We should also add a test for direction: 'reverse'.

@@ +41,5 @@
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 0.5, iterations:2, duration: 1});
> +  assert_equals(anim.effect.getComputedTiming().activeDuration, 2);

On an implementation that only stores times to millisecond precision, this test might pass accidentally (since the active duration might be mistakenly calculated as 1.5 and then rounded up to the correct answer). Can we make the duration a bit longer, e.g. 100?

@@ +48,5 @@
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 0.5, iterations:2, duration:1});
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.5);

Please add a test for currentIteration here.

@@ +50,5 @@
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 0.5, iterations:2, duration:1});
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.5);
> +  anim.finished.then(t.step_func(function() {
> +    assert_equals(anim.effect.getComputedTiming().progress, null);    

The second part of this test is not needed. It seems like it will pass regardless of how we handle iterationStart.

@@ +71,5 @@
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 0.5, iterations:2, fill:'forwards', easing:'ease-in', duration: 1});
> +  anim.finished.then(t.step_func(function() {
> +    assert_not_equals(anim.effect.getComputedTiming().progress, 0.5);

As before, it would be better to use a timing function here whose value we can test (e.g. a step timing function).

@@ +89,5 @@
> +  }));
> +}, 'Test special case when iterationStart + iterations is an integer');
> +
> +//
> +//Test zero-duration animation

Nit: Space before 'Test'

@@ +96,5 @@
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, {iterationStart: 0.5, duration:0});
> +  assert_equals(anim.effect.timing.iterationStart, 0.5);
> +  assert_equals(anim.effect.getComputedTiming().iterationStart, 0.5);
> +}, 'Test zero-duration: same value as a parameter be set in the object');

I don't think we need this test.

@@ +111,5 @@
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 2.5, delay:1000, 
> +              fill:'backwards', easing:'ease-in', duration:0});
> +  assert_not_equals(anim.effect.getComputedTiming().progress, 0.5);

Again, we should use a different timing function and assert_equals.

@@ +119,5 @@
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 0.5, iterations:2, duration:0});
> +  assert_equals(anim.effect.getComputedTiming().progress, null);

Please test currentIteration too.

@@ +121,5 @@
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 0.5, iterations:2, duration:0});
> +  assert_equals(anim.effect.getComputedTiming().progress, null);
> +  anim.finished.then(t.step_func(function() {
> +    assert_equals(anim.effect.getComputedTiming().progress, null);    

I don't think we need this part.

@@ +143,5 @@
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 
> +             {iterationStart: 0.5, iterations:2, 
> +              fill:'forwards', easing:'ease-in', duration:0});
> +  anim.finished.then(t.step_func(function() {
> +    assert_not_equals(anim.effect.getComputedTiming().progress, 0.5);

Again, assert_equals and a step timing function

Comment 4

[Brian Birtles (:birtles)]

I pushed this to try server too:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b31694425fb

Comment 5

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #3)
> Still, I think we can make this a lot more clear. Let me think about how to
> fix the spec then we can try to match it. Maybe something like the following?
> 
>   // Determine the 0-based index of the current iteration.
>   if (isEndOfFinalIteration) {
>     result.mCurrentIteration =
>       IsInfinite(result.mIterations) ?
>       UINT64_MAX : // In GetComputedTimingDictionary(), we will convert this
>                    // into Infinity.
>       result.mIterationStart + result.mIterations - 1;
>   } else if (result.mPhase == ComputedTiming::AnimationPhase::After) {
>     result.mCurrentIteration =
>       IsInfinite(result.mIterations) ?
>       UINT64_MAX : // In GetComputedTimingDictionary(), we will convert this
>                    // into Infinity.
>       ceil(result.mIterations + result.mIterationStart) - 1;
>   } else if (result.mPhase == ComputedTiming::AnimationPhase::Before ||
>              activeTime == zeroDuration) {
>     result.mCurrentIteration = static_cast<uint64_t>(result.mIterationStart);
>   } else {
>     result.mCurrentIteration =
>       result.mDuration == StickyTimeDuration::Forever()
>       ? 0
>       : static_cast<uint64_t>(scaledActiveTime / result.mDuration); // floor
>   }

It seems like this could even be just:

  if (result.mPhase == ComputedTiming::AnimationPhase::After) {
    result.mCurrentIteration =
      IsInfinite(result.mIterations) ?
      UINT64_MAX : // In GetComputedTimingDictionary(), we will convert this
                   // into Infinity.
      ceil(result.mIterations + result.mIterationStart) - 1;
  } else if (result.mPhase == ComputedTiming::AnimationPhase::Before ||
             activeTime == zeroDuration) {
    result.mCurrentIteration = static_cast<uint64_t>(result.mIterationStart);
  } else {
    result.mCurrentIteration =
      result.mDuration == StickyTimeDuration::Forever()
      ? 0
      : static_cast<uint64_t>(scaledActiveTime / result.mDuration); // floor
  }

I'm going to update the spec accordingly.

Comment 6

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #5)
> I'm going to update the spec accordingly.

Updated: https://github.com/w3c/web-animations/commit/145e50126276e581f0305b5424d15822e43cd7df

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: 1248338_v2.patch

Fixed the bugs that Brian pointed.

Comment 8

[Brian Birtles (:birtles)]

I'll look into this tomorrow, but for now I've pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=312e8020b829

Comment 9

[Hiroyuki Ikezoe (:hiro)]

With order fix in mochitest.ini.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f18f50bf353

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I did not realize that some test files were missed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2fc76da4ea6

Comment 11

[Daisuke Akatsuka (:daisuke)]

Attachment: 1248338_v3.patch

Made the patch without --no-prefix of git.
Also, fixed the bug of 'Test zero duration with zero iteration count' in layout/style/test/test_animations.html

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8722840 [details] [diff] [review]
1248338_v3.patch

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

Getting closer. I haven't reviewed the tests yet, however.

::: dom/animation/AnimationEffectTimingReadOnly.h
@@ +51,4 @@
>    dom::OwningUnrestrictedDoubleOrString mDuration;
>    TimeDuration mDelay;      // Initializes to zero
>    double mIterations = 1.0; // Can be NaN, negative, +/-Infinity
> +  double mIterationStart = 0.0; // greater than or equal to zero

I think we should drop this comment since I don't think we actually clamp it when it is set, right? Maybe we should.

::: dom/animation/KeyframeEffect.cpp
@@ +247,5 @@
>                         aTiming.mIterations;
> +  result.mIterationStart = IsNaN(aTiming.mIterationStart) ||
> +                           aTiming.mIterationStart < 0.0f ?
> +                           0.0f :
> +                           aTiming.mIterationStart;

I don't think iteration start can be NaN since it is declared in WebIDL to be 'double' not 'unrestricted double'.

Also, I've been getting this wrong, apparently the style for ternary operators is:

  a = b
      ? c
      : d;

According to: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

So I think this should be:

  result.mIterationStart = aTiming.mIterationStart < 0.0f
                           ? 0.0f
                           : aTiming.mIterationStart;

In which case we could just do:

  result.mIterationStart = std::max(aTiming.mIterationStart, 0.0);

@@ +286,5 @@
> +    // case we will have already returned. Hence we just test here for whether
> +    // or not we are in the active phase.
> +    isEndOfFinalIteration = result.mIterations != 0.0
> +                            && fmod(result.mIterations +
> +                                   result.mIterationStart, 1.0) == 0.0;

This comment no longer matches the code.

Also, I don't think this works when result.mIterations = infinity and result.mIterationsStart = 0.0?

Apparently, for fmod:

  "If x is ±∞ and y is not NaN, NaN is returned and FE_INVALID is raised"

@@ +306,5 @@
> +  // Calculate the scaled active time
> +  StickyTimeDuration startOffset = result.mIterationStart == 0.0
> +                                   ? StickyTimeDuration(0)
> +                                   : result.mDuration.MultDouble(
> +                                                      result.mIterationStart);

Nit: I'm not sure the final line needs to be so far indented.

Maybe just:

  StickyTimeDuration startOffset = result.mIterationStart == 0.0
                                   ? StickyTimeDuration(0)
                                   : result.mDuration.MultDouble(
                                       result.mIterationStart);

Or better still:

  StickyTimeDuration startOffset =
    result.mIterationStart == 0.0
    ? StickyTimeDuration(0)
    : result.mDuration.MultDouble(result.mIterationStart);

@@ +319,5 @@
>    } /* else, iterationTime is zero */
>  
>    // Determine the 0-based index of the current iteration.
> +  if (result.mPhase == ComputedTiming::AnimationPhase::Before ||
> +             result.mIterations == 0) {

Indentation is off here:

  if (result.mPhase == ComputedTiming::AnimationPhase::Before ||
      result.mIterations == 0) {

@@ +332,5 @@
>    } else {
>      result.mCurrentIteration =
> +      result.mDuration == StickyTimeDuration::Forever()
> +      ? 0
> +      : static_cast<uint64_t>(scaledActiveTime / result.mDuration); // floor

I think the following will work?

  result.mDuration =
    static_cast<uint64_t>(scaledActiveTime / result.mDuration); // floor

Since I think the following code should handle this case for us:

  https://dxr.mozilla.org/mozilla-central/rev/a9e33d8c48b5ca93ca1937eba4220f681a0f05ec/xpcom/ds/StickyTimeDuration.h#127

But we should make sure we have a test that covers this.

@@ +337,5 @@
>    }
>  
>    // Normalize the iteration time into a fraction of the iteration duration.
> +  if (result.mPhase == ComputedTiming::AnimationPhase::Before ||
> +             result.mIterations == 0) {

Indentation is off.

@@ +348,1 @@
>                        ? 1.0

Is this right? If we have iterationStart = 0.5, iterations = Infinity, duration = 0 shouldn't the progress be 0.5?

Comment 13

[Daisuke Akatsuka (:daisuke)]

Attachment: 1248338_v4.patch

Fixed following bugs:
* Remove extra comments
* Indentation
* Revise logic for result.mIterationStart and isEndOfFinalIteration
* Revise logic for progress at After phase
* Add infinity duration logic for currentIteration at Active phase
* Add two tests for infinity duration

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8723351 [details] [diff] [review]
1248338_v4.patch

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

::: dom/animation/KeyframeEffect.cpp
@@ +276,5 @@
>        return result;
>      }
>      activeTime = result.mActiveDuration;
> +    isEndOfFinalIteration = result.mIterations != 0.0
> +                            && !IsInfinite(result.mIterations)

So apparently && and || go at the end of the line, but other operators go at the start:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

More importantly, though, I'm not sure this is right. If you have an animation that animates some property from 100 to 200, with duration: 0 and iterations: Infinity, then after that animation is finished, I think you expect the result to be 200. i.e. I think you expect isEndOfFinalIteration to be true in that case.

Also, if you have duration: 0, iterations: Infinity, iterationStart: 0.5, then I think you expect the animation to finish half-way through the interval. So I think the spec is wrong here.

I think we need to add tests for both of the above cases.

I think what we want is something like:

  result.mIterations != 0.0 &&
  fmod((IsInfinite(result.mIterations) ? 0.0 : result.mIterations)
       + mIterationStart), 1.0) == 0.0

But that's really clumsy. So, perhaps we can write something like:

  double roughProgress =
    (IsInfinite(result.mIterations) ? 0.0 : result.mIterations)
    + result.mIterationStart;
  isEndOfFinalIteration = result.mIterations != 0.0 &&
                          fmod(roughProgress, 1.0) == 0;

@@ +278,5 @@
>      activeTime = result.mActiveDuration;
> +    isEndOfFinalIteration = result.mIterations != 0.0
> +                            && !IsInfinite(result.mIterations)
> +                            && fmod(result.mIterations +
> +                                   result.mIterationStart, 1.0) == 0.0;

Indentation here is off.

@@ +320,4 @@
>        ? UINT64_MAX // In GetComputedTimingDictionary(), we will convert this
>                     // into Infinity.
> +      : static_cast<uint64_t>(ceil(result.mIterations +
> +                                 result.mIterationStart)) - 1;

Indentation is off here.

@@ +324,3 @@
>    } else {
> +    result.mCurrentIteration = result.mDuration == StickyTimeDuration::Forever()
> +      ? static_cast<uint64_t>(ceil(result.mIterationStart)) - 1

This doesn't seem right. If result.mIterationStart is 0.0, then we'll end up calculating:

  static_cast<uint64_t>(ceil(result.mIterationStart)) - 1
  = static_cast<uint64_t>(ceil(0.0)) - 1
  = static_cast<uint64_t>(0.0) - 1
  = 0 - 1
  = -1
  = MAX_UINT64

Shouldn't this just be floor(result.mIterationStart)?

Or even just static_cast<uint64_t>(result.mIterationStart)?

@@ +339,5 @@
> +      ? 1.0
> +      : fmod(result.mIterationStart, 1.0)
> +      : fmod(result.mIterations + result.mIterationStart, 1.0) == 0.0
> +      ? 1.0
> +      : fmod(result.mIterations + result.mIterationStart, 1.0);

This is very hard to read and needs to be rewritten. Don't be afraid to use extra variables or branches in order to make the code more clear.

e.g. Maybe something like

double progress;
if (isEndOfFinalIteration) {
  progress = 1.0;
} else if (IsInfinite(result.mIterations)) {
  progress = fmod(result.mIterationStart, 1.0);
} else {
  progress = fmod(result.mIterations + result.mIterationStart, 1.0);
}

::: testing/web-platform/tests/web-animations/keyframe-effect/iterationStart.html
@@ +194,5 @@
> +    assert_equals(anim.effect.getComputedTiming().progress, 0.5);
> +    t.done();
> +  }));
> +}, 'Test zero-duration: ' +
> +   'iterations is infinity and iterationStart is not zero');

These tests are good but I think we should test in each phase as well, i.e. before and after in this case.

Comment 15

[Brian Birtles (:birtles)]

Pushed a couple of fixes to the spec:
https://github.com/w3c/web-animations/commit/f8ad57db4d564b5e71905543b90afa1402b9d8b3
https://github.com/w3c/web-animations/commit/e2c00e1383d28e5d039f41e34b81eb2b598d8732

Comment 16

[Daisuke Akatsuka (:daisuke)]

Attachment: 1248338_v5.patch

Revised following things:
* coding style
* currentIteration logic
* progress logic
* Add tests for currentIteration as getComputedTiming-currentIteration.html
* Add tests for progress as getComputedTiming-progress.html
* Remove tests iterationStart.html

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8724545 [details] [diff] [review]
1248338_v5.patch

>--- a/dom/animation/KeyframeEffect.cpp
>+++ b/dom/animation/KeyframeEffect.cpp
>@@ -272,10 +276,11 @@ KeyframeEffectReadOnly::GetComputedTimingAt(
>       return result;
>     }
>     activeTime = result.mActiveDuration;
>-    // Note that infinity == floor(infinity) so this will also be true when we
>-    // have finished an infinitely repeating animation of zero duration.
>+    double roughProgress =
>+      (IsInfinite(result.mIterations) ? 0.0 : result.mIterations)
>+      + result.mIterationStart;
>     isEndOfFinalIteration = result.mIterations != 0.0 &&
>-                            result.mIterations == floor(result.mIterations);
>+                            fmod(roughProgress, 1.0) == 0;

Sorry, I know I suggested roughProgress, but I think finiteProgress would
be a better name here.

>@@ -291,49 +296,61 @@ KeyframeEffectReadOnly::GetComputedTimingAt(
>     activeTime = localTime - aTiming.mDelay;
>   }
> 
>+  // Calculate the scaled active time

We should add an extra explanation here:

  // Calculate the scaled active time
  // (We handle the case where the iterationStart is zero separately in case
  // the duration is infinity, since 0 * Infinity is undefined.)

>   } else {
>-    result.mCurrentIteration =
>-      static_cast<uint64_t>(activeTime / result.mDuration); // floor
>+    result.mCurrentIteration = result.mDuration == StickyTimeDuration::Forever()
>+      ? static_cast<uint64_t>(result.mIterationStart)
>+      : static_cast<uint64_t>(scaledActiveTime / result.mDuration); // floor
>   }

This is fine, but since we are already in if-else block, it would be more clear
to just add more conditions to that block, rather than introducing a ternary
operator here. Also, it would match the spec better.

So instead of:

  } else {
    result.mCurrentIteration = result.mDuration == StickyTimeDuration::Forever()
      ? static_cast<uint64_t>(result.mIterationStart)
      : static_cast<uint64_t>(scaledActiveTime / result.mDuration); // floor
  }

we should just write:

  } else if (result.mDuration == StickyTimeDuration::Forever()) {
    result.mCurrentIteration = static_cast<uint64_t>(result.mIterationStart);
  } else {
    result.mCurrentIteration =
      static_cast<uint64_t>(scaledActiveTime / result.mDuration); // floor
  }

>+    double progress;
>+    if (isEndOfFinalIteration) {
>+      progress = 1.0;
>+    } else {
>+      double fmodResult =
>+        IsInfinite(result.mIterations)
>+        ? fmod(result.mIterationStart, 1.0)
>+        : fmod(result.mIterations + result.mIterationStart, 1.0);
>+      progress = fmodResult == 0 ? 1.0 : fmodResult;
>+    }

fmodResult is a little odd. I still think the following is easier to read:

  double progress;
  if (isEndOfFinalIteration) {
    progress = 1.0;
  } else if (IsInfinite(result.mIterations)) {
    progress = fmod(result.mIterationStart, 1.0);
  } else {
    progress = fmod(result.mIterations + result.mIterationStart, 1.0);
  }

>--- /dev/null
>+++ b/testing/web-platform/tests/web-animations/keyframe-effect/getComputedTiming-currentIteration.html
>+<script>
>+'use strict';
>+
>+var NO_TEST = "No test";

Instead of NO_TEST, why don't we just leave 'after' undefined when we don't
expect to test it?

>+function executeTests(tests, description) {
>+  tests.forEach(function(stest) {
>+    var desc = "";

Having both desc and description is odd. Perhaps call this paramList?
testParams?

>+    for (var attr in stest.input) {
>+      desc += " " + attr + ":" + stest.input[attr];
>+    }
>+    async_test(function(t) {
>+      var div = createDiv(t);
>+      var anim = div.animate({ opacity: [ 0, 1 ] },
>+                               stest.input);

Nit: Indentation is off, but we may as well put this on one line.

>+      assert_equals(anim.effect.getComputedTiming().currentIteration, stest.before);

Nit: Line length here > 80 chars

>+      if (stest.after != NO_TEST) {
>+        anim.finished.then(t.step_func(function() {
>+          assert_equals(anim.effect.getComputedTiming().currentIteration, stest.after);

Nit: Line length

>+          t.done();
>+        }));
>+      } else {
>+        t.done();
>+      }

Then this would become something like:

  if (typeof stest.after !== "undefined") {
    ....
  } else {
    t.done();
  }

>+async_test(function(t) {
>+  var div = createDiv(t);
>+  var anim = div.animate({ opacity: [ 0, 1 ] },
>+                         { delay:100 });

Nit: Space after 'delay'

>+  assert_equals(anim.effect.getComputedTiming().currentIteration, null);
>+  anim.finished.then(t.step_func(function() {
>+    assert_equals(anim.effect.getComputedTiming().currentIteration, null);
>+    t.done();
>+  }));
>+}, 'fill:none');

s/fill:none/Test currentIteration during before and after phase when fill is
none/


There's a bigger issue here, however. Using a delay of 100 means that this test
and all the other ones will take a minimum of 100 milliseconds.

There are something like ~80 tests in this file that use this pattern which
means this test will take a minimum of 8 seconds. Then some tests use a duration
of 100 as well, and then there is a similar pattern in the progress tests. So
we're adding probably > 30 seconds to every try run that includes these tests
and every build from now on.

For the delay, I think a delay of 1 will work just as well. However, better
still, why don't we just seek the time?

I think these tests would be easier to read and write if we tested the before,
active, and after phases all at once or all separately. Currently when we test
the active and after phases we still use the terms 'before' and 'after' which is
confusing.

How about we use 'before', 'active', and 'after' and test like:

  assert_equals(anim.effect.getComputedTiming().currentIteration,
                stest.before);
  anim.currentTime = stest.input.delay || 0;
  assert_equals(anim.effect.getComputedTiming().currentIteration,
                stest.active);
  if (typeof stest.after !== 'undefined') {
    anim.finish();
    assert_equals(anim.effect.getComputedTiming().currentIteration,
                  stest.after);
  }
  t.done();

Also, one more nit, can we rename stest to 'currentTest' or something more
descriptive?


The code changes are good after those few comments are addressed. The testing
is fantastic but I'd like to look at the tests once more after re-arranging
them to take less time and to test the phases all together.

Comment 18

[Daisuke Akatsuka (:daisuke)]

Attachment: 1248338_v6.patch

Revised following things:
* variable name
* add comment for scaled active time
* not use some ternary operators for readability
* coding style
* unify before, active and after phase tests.
* no need to test > undefined after variable

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8724631 [details] [diff] [review]
1248338_v6.patch

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

Those tests are excellent! Thank you!

r=birtles with comments addressed.

::: testing/web-platform/tests/web-animations/keyframe-effect/getComputedTiming-currentIteration.html
@@ +28,5 @@
> +                    currentTest.active);
> +      if (typeof currentTest.after !== 'undefined') {
> +        anim.finished.then(t.step_func(function() {
> +          assert_equals(anim.effect.getComputedTiming().currentIteration,
> +                    currentTest.after);

Is there any reason to use finished rather than finish()?

If we use finish() we don't need to use async_test and the code becomes a lot simpler.

i.e. instead of

      assert_equals(anim.effect.getComputedTiming().currentIteration,
                    currentTest.before);
      anim.currentTime = currentTest.input.delay || 0;
      assert_equals(anim.effect.getComputedTiming().currentIteration,
                    currentTest.active);
      if (typeof currentTest.after !== 'undefined') {
        anim.finished.then(t.step_func(function() {
          assert_equals(anim.effect.getComputedTiming().currentIteration,
                        currentTest.after);
          t.done();
        }));
      } else {
        t.done();
      }

we would have:

      assert_equals(anim.effect.getComputedTiming().currentIteration,
                    currentTest.before);
      anim.currentTime = currentTest.input.delay || 0;
      assert_equals(anim.effect.getComputedTiming().currentIteration,
                    currentTest.active);
      if (typeof currentTest.after !== 'undefined') {
        anim.finish();
        assert_equals(anim.effect.getComputedTiming().currentIteration,
                      currentTest.after);
      }

@@ +196,5 @@
> +
> +  {
> +    input:    { iterations: 3,
> +                iterationStart: 2.5,
> +                duration: 100,

If we use 'finished' above, then I think this will take 100ms to complete so using 'finish()' seems better.

@@ +247,5 @@
> +    active: 3
> +  }
> +];
> +
> +var gTests_dicimal_iterations = [

s/dicimal/fractional/

::: testing/web-platform/tests/web-animations/keyframe-effect/getComputedTiming-progress.html
@@ +26,5 @@
> +      anim.currentTime = currentTest.input.delay || 0;
> +      assert_equals(anim.effect.getComputedTiming().progress,
> +                    currentTest.active);
> +      if (typeof currentTest.after !== 'undefined') {
> +        anim.finished.then(t.step_func(function() {

As with the currentIteration test, I think we should just use finish() here and make this no longer async.

@@ +247,5 @@
> +    active: 0
> +  }
> +];
> +
> +var gTests_dicimal_iterations = [

s/dicimal/fractional/

Comment 20

[Daisuke Akatsuka (:daisuke)]

Attachment: 1248338_v7.patch

Revised following points at tests:
* Use finish() method instead of promise of finished to reduce test time
* Change method name decimal to fractional

Comment 21

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d316c83004da

Comment 22

[Brian Birtles (:birtles)]

Regarding the test failures on try, this appears to be a spec issue. If we have an infinite duration we will have:

  start offset = iteration start × iteration duration
               = Infinity

  scaled active time = active time + start offset
                     = Infinity

  iteration time = scaled active time % iteration duration
                 = Infinity % Infinity
                 = NaN

Now, x % Infinity = x, i.e. is well-defined. However, Infinity % x is undefined.

I went through other code paths in the spec and I can't see a case where we will ever encounter Infinity % x where x != Infinity. So, we only need to decide what happens when we have Infinity % Infinity.

In this case I think the correct result is Infinity. I need to update the spec to add this step and also add similar handling for the directed time and progress.

Comment 23

[Daisuke Akatsuka (:daisuke)]

Attachment: 1248338_v8.patch

Revised a logic that calculates iteration time by infinity duration.

Comment 24

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50724fff0991

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12dfe2b87a79

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12dfe2b87a79