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)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
Details
Attachments
(4 files)
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
Assignee | ||
Updated•3 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Comment 3•3 years ago
|
||
mozreview-review |
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 4•3 years ago
|
||
mozreview-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+
Assignee | ||
Comment 5•3 years ago
|
||
(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.
Comment 6•3 years ago
|
||
(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.
Assignee | ||
Comment 7•3 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•3 years ago
|
||
mozreview-review |
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 16•3 years ago
|
||
mozreview-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+
Comment 17•3 years ago
|
||
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
Comment 18•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad80f0331d84 https://hg.mozilla.org/mozilla-central/rev/3465cdfb74b8 https://hg.mozilla.org/mozilla-central/rev/884067088663 https://hg.mozilla.org/mozilla-central/rev/03045ec2b4f6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•