1286476 - Unexpected fill mode applied when reversing an animation

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P2)

Description

[Brian Birtles (:birtles)]

See the JSBin from the bug URL.[1]

It defines an animation of 1s that plays backwards with no fill mode. Oddly, the animation never seems to finish (i.e. return to green) but I'm pretty sure it should.

On Chrome, however, the animation finishes.

[1] http://jsbin.com/likazu/edit?html,css,js,output

Comment 1

[Hiroyuki Ikezoe (:hiro)]

It might be a regression of recent changes.

Comment 2

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> It might be a regression of recent changes.

I wish it was, but I see the same result in current beta.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles; away 14-18 July, busy 19-22 July) from comment #2)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > It might be a regression of recent changes.
> 
> I wish it was, but I see the same result in current beta.

You are right.  I confirmed this issue has been there in the first place.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=23f22124b285c901e08e29b2d73fa830fc1c8b5c&tochange=f735dd2711d265c86322c485a22de7af43ac4f74

Comment 4

[Hiroyuki Ikezoe (:hiro)]

In this case, aLocalTime for GetComputedTimingAt() is zero, so these checks are not hit for the localtime.
http://hg.mozilla.org/mozilla-central/file/04821a70c739/dom/animation/KeyframeEffect.cpp#l274
http://hg.mozilla.org/mozilla-central/file/04821a70c739/dom/animation/KeyframeEffect.cpp#l285

Comment 5

[Brian Birtles (:birtles)]

I suspect this is a bug in the spec.

Comment 6

[Brian Birtles (:birtles)]

Actually, I think the behavior here might be correct. We're hitting finishing behavior--that is, when playing in reverse and the currentTime hits zero, it sticks there.

(That said, I think there *is* a spec bug in that if we seek to less than zero, on the next update the currentTime will be reset to zero. I'll file a separate bug for that, however.)

Comment 7

[Brian Birtles (:birtles)]

Spec updated: https://github.com/w3c/web-animations/commit/21de090dacc56efcc1ef7a7d04913126f96024ae

Comment 8

[Brian Birtles (:birtles)]

Attachment: Bug 1286476 part 1 - Add tests for calculating animation effect phases;

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

Comment 9

[Brian Birtles (:birtles)]

Attachment: Bug 1286476 part 2 - Respect the playback rate when calculating phase boundaries;

This implements the spec change in https://github.com/w3c/web-animations/commit/21de090dacc56efcc1ef7a7d04913126f96024ae

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

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8781413 [details]
Bug 1286476 part 1 - Add tests for calculating animation effect phases;

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

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8781414 [details]
Bug 1286476 part 2 - Respect the playback rate when calculating phase boundaries;

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8781413 [details]
Bug 1286476 part 1 - Add tests for calculating animation effect phases;

https://reviewboard.mozilla.org/r/71868/#review69650

This patch includes unintended changes against MANIFEST.json but I think it's OK since I've been suffering from it every time I rebase local tree. ;-)  A separete patch will be better I think.

I am wondering it's worth to add some test cases which have both of deley and endDelay?

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/phases-and-states.html:13
(Diff revision 2)
> +// --------------------------------------------------------------------
> +//
> +// Phases
> +//
> +// --------------------------------------------------------------------

I think this 'Phases' comment implies we are going to write test cases for 'States' in this test file. Right?

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/phases-and-states.html:43
(Diff revision 2)
> +                  + ' (progress is null with \'none\' fill mode)');
> +
> +    animation.effect.timing.fill = phase === 'before'
> +                                   ? 'backwards'
> +                                   : 'forwards';
> +    assert_not_equals(animation.effect.getComputedTiming().progress !== null,

This should be;
 assert_not_equals(progress, null, ...);

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/phases-and-states.html:88
(Diff revision 2)
> +test(function(t) {
> +  var animation = createDiv(t).animate(null, { duration: 1, endDelay: 1 });
> +
> +  [ { currentTime: -1, phase: 'before' },
> +    { currentTime:  0, phase: 'active' },
> +    { currentTime:  1, phase: 'after' },

Nit: one more space is needed before the last brace.

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/phases-and-states.html:89
(Diff revision 2)
> +  var animation = createDiv(t).animate(null, { duration: 1, endDelay: 1 });
> +
> +  [ { currentTime: -1, phase: 'before' },
> +    { currentTime:  0, phase: 'active' },
> +    { currentTime:  1, phase: 'after' },
> +    { currentTime:  2, phase: 'after'  } ]

I just noticed now that assert_phase_at_time can't check after phase reliably.  I guess this after check for currentTime:2 will pass even if endDelay is 0. No?

I don't think it is a problem for now but I am not sure in future.

Comment 13

[Brian Birtles (:birtles)]

Thanks for the quick review!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> This patch includes unintended changes against MANIFEST.json but I think
> it's OK since I've been suffering from it every time I rebase local tree.
> ;-)  A separete patch will be better I think.

Yes, that was deliberate. It seems like some people aren't using ./mach web-platform-tests --manifest-update. I think we should fix this file whenever we touch it or else it causes trouble for everyone. (That said, I had to remove some of the changes created by --manifest-update since it picked up /XMLHttpRequest/event-error.html which is in the tree but times out.) I'll split the unrelated changes into a separate patch when landing.

> I am wondering it's worth to add some test cases which have both of deley
> and endDelay?

Yes, I started doing that, but I couldn't imagine a situation where an implementation could pass the previous tests and fail that test. I'll add a test for that anyway.

> :::
> testing/web-platform/tests/web-animations/timing-model/animation-effects/
> phases-and-states.html:13
> (Diff revision 2)
> > +// --------------------------------------------------------------------
> > +//
> > +// Phases
> > +//
> > +// --------------------------------------------------------------------
> 
> I think this 'Phases' comment implies we are going to write test cases for
> 'States' in this test file. Right?

Yes, some day I hope someone will! I'm not sure yet what the best way would be to test that, however.

> :::
> testing/web-platform/tests/web-animations/timing-model/animation-effects/
> phases-and-states.html:43
> (Diff revision 2)
> > +                  + ' (progress is null with \'none\' fill mode)');
> > +
> > +    animation.effect.timing.fill = phase === 'before'
> > +                                   ? 'backwards'
> > +                                   : 'forwards';
> > +    assert_not_equals(animation.effect.getComputedTiming().progress !== null,
> 
> This should be;
>  assert_not_equals(progress, null, ...);

Oh, good catch!

> :::
> testing/web-platform/tests/web-animations/timing-model/animation-effects/
> phases-and-states.html:89
> (Diff revision 2)
> > +  var animation = createDiv(t).animate(null, { duration: 1, endDelay: 1 });
> > +
> > +  [ { currentTime: -1, phase: 'before' },
> > +    { currentTime:  0, phase: 'active' },
> > +    { currentTime:  1, phase: 'after' },
> > +    { currentTime:  2, phase: 'after'  } ]
> 
> I just noticed now that assert_phase_at_time can't check after phase
> reliably.  I guess this after check for currentTime:2 will pass even if
> endDelay is 0. No?

I think it's ok. It's still testing the phase calculation correctly -- it's just that a positive end delay has no effect on the phase boundaries.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8781414 [details]
Bug 1286476 part 2 - Respect the playback rate when calculating phase boundaries;

https://reviewboard.mozilla.org/r/71870/#review69656

::: dom/animation/KeyframeEffect.cpp:278
(Diff revision 2)
> +  StickyTimeDuration beforePhaseBoundary =
> +    std::min(StickyTimeDuration(aTiming.mDelay), result.mEndTime);
> +  StickyTimeDuration afterPhaseBoundary =

I love these variable names, but don't we need to match the names in the spec?  I.e., beforeActiveBoundary and activeAfterBoundary?

Comment 15

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> ::: dom/animation/KeyframeEffect.cpp:278
> (Diff revision 2)
> > +  StickyTimeDuration beforePhaseBoundary =
> > +    std::min(StickyTimeDuration(aTiming.mDelay), result.mEndTime);
> > +  StickyTimeDuration afterPhaseBoundary =
> 
> I love these variable names, but don't we need to match the names in the
> spec?  I.e., beforeActiveBoundary and activeAfterBoundary?

Yes, good idea. Will do.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd8c37eb695
part 0 - Tidy up MANIFEST.json; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/3947ab570883
part 1 - Add tests for calculating animation effect phases; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/53bbfa02d45d
part 2 - Respect the playback rate when calculating phase boundaries; r=hiro

Comment 17

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #6)
> (That said, I think there *is* a spec bug in that if we seek to less than
> zero, on the next update the currentTime will be reset to zero. I'll file a
> separate bug for that, however.)

Filed bug 1295842.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for failures in wpt: https://treeherder.mozilla.org/logviewer.html#?job_id=33994150&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/73a588493cbe

Comment 19

[Brian Birtles (:birtles)]

Looks like a precision issue for those tests that use fractional time values. I've got some updated patches on try now and will push again once everything looks ok.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eae9f661764
part 0 - Tidy up MANIFEST.json; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/0efdbade2f18
part 1 - Add tests for calculating animation effect phases; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b53e804d0aab
part 2 - Respect the playback rate when calculating phase boundaries; r=hiro

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5eae9f661764
https://hg.mozilla.org/mozilla-central/rev/0efdbade2f18
https://hg.mozilla.org/mozilla-central/rev/b53e804d0aab