1283387 - Update end delay handling in after phase to match recent changes to the spec

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

Specifically these changes:

  https://github.com/w3c/web-animations/commit/a9ba51338ed09170d16c47317f8e4e2eef122a82

This is based on issues reported on Chrome when they updated to match the spec.

Comment 1

[Brian Birtles (:birtles)]

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

Comment 2

[Brian Birtles (:birtles)]

Attachment: Bug 1283387 part 1 - Add tests for updated end delay handling

Based on the following changes to the specification:

  https://github.com/w3c/web-animations/commit/a9ba51338ed09170d16c47317f8e4e2eef122a82

Review commit: https://reviewboard.mozilla.org/r/61714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61714/

Comment 3

[Brian Birtles (:birtles)]

Attachment: Bug 1283387 part 2 - Implement updated calculation of active time in after phase with negative end delay

This implements the changes to the specified behavior from the following
changeset:

  https://github.com/w3c/web-animations/commit/a9ba51338ed09170d16c47317f8e4e2eef122a82

It also updates test expectations based on the tests updated in part 1 of this
patch series.

Review commit: https://reviewboard.mozilla.org/r/61716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61716/

Comment 4

[Brian Birtles (:birtles)]

Attachment: Bug 1283387 part 3 - Tidy-up tests for simple iteration progress and current iteration

Review commit: https://reviewboard.mozilla.org/r/61718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61718/

Comment 5

[Brian Birtles (:birtles)]

Attachment: Bug 1283387 part 4 - Add further tests for simple iteration progress and current iteration when using an end delay

Review commit: https://reviewboard.mozilla.org/r/61720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61720/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8766997 [details]
Bug 1283387 part 1 - Add tests for updated end delay handling

https://reviewboard.mozilla.org/r/61714/#review58516

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/active-time.html:65
(Diff revision 1)
> +test(function(t) {
> +  var anim = createDiv(t).animate(null, { duration: 0,
> +                                          iterations: Infinity,
> +                                          fill: 'forwards' });
> +  anim.finish();
> +  assert_equals(anim.effect.getComputedTiming().progress, 1);
> +}, 'Active time in after phase with forwards fill, zero-duration, and '
> +   + ' infinite iteration count is the active duration');

Can't we check iteration count as well as other test cases do?  All of these kind of test check *active time* not just only progress.  So, I was bit confused when I read this test description.

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/active-time.html:133
(Diff revision 1)
> +test(function(t) {
> +  // Create an effect with a non-zero duration so we ensure we're not just
> +  // testing the after-phase behavior.
> +  var effect = new KeyframeEffect(null, null, 1);
> +  assert_equals(effect.getComputedTiming().progress, null);
> +}, 'Active time when the local time is unresolved, is unresolved');

Nice!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8766998 [details]
Bug 1283387 part 2 - Implement updated calculation of active time in after phase with negative end delay

https://reviewboard.mozilla.org/r/61716/#review58530

::: dom/animation/KeyframeEffect.cpp:332
(Diff revision 1)
> +  // when the animation has been effectively made into a zero-duration animation
> +  // using a negative end-delay, however.
>    if (result.mPhase == ComputedTiming::AnimationPhase::After &&
>        progress == 0.0 &&
> -      result.mIterations != 0.0) {
> +      result.mIterations != 0.0 &&
> +      (activeTime != zeroDuration || result.mDuration == zeroDuration)) {

I think the latter condition 'result.mDuration == zeroDuration' is what exactly the below test case which added in part 1, but it did not mark as FAIL.  Does that mean 'progress == 0' cover the case of 'result.mDuration == zeroDuration'?  If not, we need a test case for this.

test(function(t) {
  var anim = createDiv(t).animate(null, { duration: 0,
                                          iterations: Infinity,
                                          fill: 'forwards' });
  anim.finish();
  assert_equals(anim.effect.getComputedTiming().progress, 1);
}, 'Active time in after phase with forwards fill, zero-duration, and '
   + ' infinite iteration count is the active duration');

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8766999 [details]
Bug 1283387 part 3 - Tidy-up tests for simple iteration progress and current iteration

https://reviewboard.mozilla.org/r/61718/#review58520

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/current-iteration.html:32
(Diff revision 1)
>        if (typeof currentTest.after !== 'undefined') {
>          anim.finish();
>          assert_equals(anim.effect.getComputedTiming().currentIteration,
>                        currentTest.after);
>        }
> -    }, description + testParams);
> +    }, description + ':' + testParams);

Nit: I'd prefer a space after colon.

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/simple-iteration-progress.html:33
(Diff revision 1)
>        if (typeof currentTest.after !== 'undefined') {
>          anim.finish();
>          assert_equals(anim.effect.getComputedTiming().progress,
>                        currentTest.after);
>        }
> -    }, description + testParams);
> +    }, description + ':' + testParams);

Nit: Here as well.

Comment 9

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Comment on attachment 8766998 [details]
> Bug 1283387 part 2 - Implement updated calculation of active time in after
> phase with negative end delay
> 
> https://reviewboard.mozilla.org/r/61716/#review58530
> 
> ::: dom/animation/KeyframeEffect.cpp:332
> (Diff revision 1)
> > +  // when the animation has been effectively made into a zero-duration animation
> > +  // using a negative end-delay, however.
> >    if (result.mPhase == ComputedTiming::AnimationPhase::After &&
> >        progress == 0.0 &&
> > -      result.mIterations != 0.0) {
> > +      result.mIterations != 0.0 &&
> > +      (activeTime != zeroDuration || result.mDuration == zeroDuration)) {
> 
> I think the latter condition 'result.mDuration == zeroDuration' is what
> exactly the below test case which added in part 1, but it did not mark as
> FAIL.  Does that mean 'progress == 0' cover the case of 'result.mDuration ==
> zeroDuration'?  If not, we need a test case for this.

It didn't fail before part 2 because before part 2 we didn't compare activeTime with zeroDuration.

If we add *only* activeTime != zeroDuration then this test will fail. So the test was added to check that we preserve the existing behavior for this case.

Comment 10

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > -    }, description + testParams);
> > +    }, description + ':' + testParams);
> 
> Nit: I'd prefer a space after colon.

testParams includes a leading space.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8767000 [details]
Bug 1283387 part 4 - Add further tests for simple iteration progress and current iteration when using an end delay

https://reviewboard.mozilla.org/r/61720/#review58518

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8766997 [details]
Bug 1283387 part 1 - Add tests for updated end delay handling

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61714/diff/1-2/

Comment 13

[Brian Birtles (:birtles)]

Comment on attachment 8766998 [details]
Bug 1283387 part 2 - Implement updated calculation of active time in after phase with negative end delay

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61716/diff/1-2/

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8766999 [details]
Bug 1283387 part 3 - Tidy-up tests for simple iteration progress and current iteration

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61718/diff/1-2/

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8767000 [details]
Bug 1283387 part 4 - Add further tests for simple iteration progress and current iteration when using an end delay

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61720/diff/1-2/

Comment 16

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Comment on attachment 8766997 [details]
> Bug 1283387 part 1 - Add tests for updated end delay handling
> 
> https://reviewboard.mozilla.org/r/61714/#review58516
> 
> :::
> testing/web-platform/tests/web-animations/timing-model/animation-effects/
> active-time.html:65
> (Diff revision 1)
> > +test(function(t) {
> > +  var anim = createDiv(t).animate(null, { duration: 0,
> > +                                          iterations: Infinity,
> > +                                          fill: 'forwards' });
> > +  anim.finish();
> > +  assert_equals(anim.effect.getComputedTiming().progress, 1);
> > +}, 'Active time in after phase with forwards fill, zero-duration, and '
> > +   + ' infinite iteration count is the active duration');
> 
> Can't we check iteration count as well as other test cases do?  All of these
> kind of test check *active time* not just only progress.  So, I was bit
> confused when I read this test description.

Good point. I'm not sure why that was missing. I've added it now.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f6ea9a678af
part 1 - Add tests for updated end delay handling r=hiro
https://hg.mozilla.org/integration/autoland/rev/822dd776bcf0
part 2 - Implement updated calculation of active time in after phase with negative end delay r=hiro
https://hg.mozilla.org/integration/autoland/rev/47ab48dbf206
part 3 - Tidy-up tests for simple iteration progress and current iteration r=hiro
https://hg.mozilla.org/integration/autoland/rev/5716ad44520d
part 4 - Add further tests for simple iteration progress and current iteration when using an end delay r=hiro

Comment 18

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/3f6ea9a678af
https://hg.mozilla.org/mozilla-central/rev/822dd776bcf0
https://hg.mozilla.org/mozilla-central/rev/47ab48dbf206
https://hg.mozilla.org/mozilla-central/rev/5716ad44520d