Closed Bug 1422248 Opened 3 years ago Closed 3 years ago

cancel() should not dispatch cancel events if we are already idle

Categories

(Core :: DOM: Animation, enhancement, P3)

Product:
Core
Component:
DOM: Animation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: birtles, Assigned: birtles)

Details

Attachments

(4 files)

Bug 1422248 - Add tests that canceling an idle animation does not reject promises or fire events;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1422248 - Don't reject promises of dispatch cancel events when canceling idle animations;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1422248 - Drop redundant and contradictory test from web-animations/interfaces/finished.html;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1422248 - Use US spelling of canceling/canceled in finished.html;
59 bytes, text/x-review-board-request
hiro
: review+
Details 
See:
https://codepen.io/birtles/pen/zPyPLJ

i.e.

const animation = div.animate({}, 10000);
animation.oncancel = () => {
  div.textContent += 'oncancel ';
};
animation.cancel();
animation.cancel();

According to the spec[1] we only dispatch events if we are not already idle so the above should only print 'oncancel' once. However, it prints it twice in Firefox (but only once in Chrome).

[1] https://w3c.github.io/web-animations/#cancel-an-animation
Priority: -- → P3
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Comment on attachment 8936962 [details]
Bug 1422248 - Add tests that canceling an idle animation does not reject promises or fire events;

https://reviewboard.mozilla.org/r/207694/#review213578

::: testing/web-platform/tests/web-animations/timing-model/animations/canceling-an-animation.html:69
(Diff revision 1)
> +    null
> +  );
> +  assert_equals(animation.playState, 'idle',
> +                'The animation should be initially idle');
> +
> +  animation.finished.then(t.step_func(() => {

Do we need step_func() here in promise_test?

::: testing/web-platform/tests/web-animations/timing-model/animations/canceling-an-animation.html:89
(Diff revision 1)
> +    null
> +  );
> +  assert_equals(animation.playState, 'idle',
> +                'The animation should be initially idle');
> +
> +  animation.oncancel = t.step_func(() => {

Likewise here.
Attachment #8936962 - Flags: review?(hikezoe) → review+
Comment on attachment 8936963 [details]
Bug 1422248 - Don't reject promises of dispatch cancel events when canceling idle animations;

https://reviewboard.mozilla.org/r/207696/#review213580
Attachment #8936963 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8936962 [details]
> Bug 1422248 - Add tests that canceling an idle animation does not reject
> promises or fire events;
> 
> https://reviewboard.mozilla.org/r/207694/#review213578
> 
> :::
> testing/web-platform/tests/web-animations/timing-model/animations/canceling-
> an-animation.html:69
> (Diff revision 1)
> > +    null
> > +  );
> > +  assert_equals(animation.playState, 'idle',
> > +                'The animation should be initially idle');
> > +
> > +  animation.finished.then(t.step_func(() => {
> 
> Do we need step_func() here in promise_test?

Yeah we do. I tried without it but it often didn't report the error or it reported it at the top instead of alongside the test. That's why I added it.

I think we can only skip it when the Promise function is part of the regular test flow.

e.g. return promiseSomething.then(() => {
  ...
})

But in this case, it's not part of the regular test flow so it seems to be necessary.

> :::
> testing/web-platform/tests/web-animations/timing-model/animations/canceling-
> an-animation.html:89
> (Diff revision 1)
> > +    null
> > +  );
> > +  assert_equals(animation.playState, 'idle',
> > +                'The animation should be initially idle');
> > +
> > +  animation.oncancel = t.step_func(() => {
> 
> Likewise here.

This too I added because it turned out to be necessary.
(In reply to Brian Birtles (:birtles) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > Comment on attachment 8936962 [details]
> > Bug 1422248 - Add tests that canceling an idle animation does not reject
> > promises or fire events;
> > 
> > https://reviewboard.mozilla.org/r/207694/#review213578
> > 
> > :::
> > testing/web-platform/tests/web-animations/timing-model/animations/canceling-
> > an-animation.html:69
> > (Diff revision 1)
> > > +    null
> > > +  );
> > > +  assert_equals(animation.playState, 'idle',
> > > +                'The animation should be initially idle');
> > > +
> > > +  animation.finished.then(t.step_func(() => {
> > 
> > Do we need step_func() here in promise_test?
> 
> Yeah we do. I tried without it but it often didn't report the error or it
> reported it at the top instead of alongside the test. That's why I added it.
> 
> I think we can only skip it when the Promise function is part of the regular
> test flow.

Ah, that makes sense.  I thought it works as well even outside Promise because I found another place doing the same thing.

https://hg.mozilla.org/mozilla-central/file/04433cdc9395/testing/web-platform/tests/web-animations/interfaces/Animation/onfinish.html#l94

We should fix all of such usages at some point.
I went to land this but it appears that web-animations/interfaces/Animation/finished.html is testing that we *do* create a new Promise when doing a redundant cancel(). I'm not even sure now how this should work.
Comment on attachment 8937071 [details]
Bug 1422248 - Use US spelling of canceling/canceled in finished.html;

https://reviewboard.mozilla.org/r/207778/#review213662
Attachment #8937071 - Flags: review?(hikezoe) → review+
Comment on attachment 8937070 [details]
Bug 1422248 - Drop redundant and contradictory test from web-animations/interfaces/finished.html;

https://reviewboard.mozilla.org/r/207776/#review213664
Attachment #8937070 - Flags: review?(hikezoe) → review+
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad80f0331d84
Add tests that canceling an idle animation does not reject promises or fire events; r=hiro
https://hg.mozilla.org/integration/autoland/rev/3465cdfb74b8
Drop redundant and contradictory test from web-animations/interfaces/finished.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/884067088663
Use US spelling of canceling/canceled in finished.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/03045ec2b4f6
Don't reject promises of dispatch cancel events when canceling idle animations; r=hiro
You need to log in before you can comment on or make changes to this bug.