Closed Bug 1286476 Opened 3 years ago Closed 3 years ago

Unexpected fill mode applied when reversing an animation

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

()

Details

Attachments

(2 files)

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
It might be a regression of recent changes.
(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.
(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
Flags: needinfo?(hiikezoe)
I suspect this is a bug in the spec.
Priority: -- → P2
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.)
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
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.
Attachment #8781413 - Flags: review?(hiikezoe) → review+
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 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?
Attachment #8781414 - Flags: review?(hiikezoe) → review+
(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.
Pushed by bbirtles@mozilla.com:
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
(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.
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.
Flags: needinfo?(bbirtles)
Pushed by bbirtles@mozilla.com:
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
Depends on: 1291489
You need to log in before you can comment on or make changes to this bug.