Open Bug 1453568 Opened 6 years ago Updated 2 years ago

cancel event is not fired when setting null timeline

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
ASSIGNED

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(9 files)

An example
523 bytes, text/html
Details
Bug 1453568 - Rewrite async_test in oncancel.html with promise_test with async/await.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1453568 - Dispatch cancel event when setting null timeline.
59 bytes, text/x-review-board-request
Details
Bug 1453568 - Don't use arrow function for prototype function which uses `this` inside the function.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1453568 - Make sure that transitioncancel event happens, and neither transitionrun nor transitionstart happens when restarting cancelled transition by setting display none property.
59 bytes, text/x-review-board-request
Details
Bug 1453568 - Rewrite css-animations/test_event-dispatch.html with async/await.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1453568 - Rewrite css-transitions/test_event-dispatch.html with async/await.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1453568 - Make sure there is no new animation events as expected.
59 bytes, text/x-review-board-request
Details
Bug 1453568 - Add test case that animationcancel or transitioncancel event is not dispatched when setting null timeline in idle state.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Attached file An example — 
Per spec https://drafts.csswg.org/web-animations-1/#cancel-event;

> Queued whenever an animation enters the idle play state from another state. Creating a new animation that is initially idle does not generate a new cancel event.

And from https://drafts.csswg.org/web-animations-1/#pending-animation-event-queue

> Each Document maintains a pending animation event queue 

So, even if the animation is detached from timeline, the cancel event should be fired.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8968041 [details]
Bug 1453568 - Rewrite async_test in oncancel.html with promise_test with async/await.

https://reviewboard.mozilla.org/r/236730/#review242514
Attachment #8968041 - Flags: review?(bbirtles) → review+
Comment on attachment 8968042 [details]
Bug 1453568 - Dispatch cancel event when setting null timeline.

https://reviewboard.mozilla.org/r/236732/#review242516

::: dom/animation/Animation.cpp:248
(Diff revision 1)
>    if (!aTimeline) {
>      MaybeQueueCancelEvent(activeTime);
> +    DispatchPlaybackEvent(NS_LITERAL_STRING("cancel"));
>    }
>    UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);

A few questions:

a) Should we only do this if we are not already idle?

b) Should we check that we are actually idle first? (I think it's possible mHoldTime could be set if we were paused and then cleared the timeline?)

c) Should we do the other steps we normally do when canceling, e.g.
   - resetting pending tasks
   - rejecting the finished promise

d) Should we dispatch the playback event before the CSS one?

These are mostly questions for me -- I think the spec needs to be fixed -- but perhaps you have some ideas.
I need to think about this some more. Originally the cancel event was only going to be canceled when you call cancel() but the event description says that, "Queued whenever an animation enters the idle play state from another state."

If the author does:

anim.timeline = null;
anim.timeline = new DocumentTimeline(...);

I wonder if they should get a cancel event. Probably they should but I should look at the other issues regarding clearing a timeline.
(In reply to Brian Birtles (:birtles, travelling 7~18 April) from comment #5)
> d) Should we dispatch the playback event before the CSS one?

I thinks CSS animation/transition events should be dispatched prior to web animation events (finish/cancel) since all animation events (CSS ones and web animations') are sorted by composite order.  I already have a patch to reorder cancel event caused by cancel() (for bug 1354501 because we can't properly test the order without fixing that bug).

> a) Should we only do this if we are not already idle?

Oh, yes. A relevant thing I've noticed is that we shouldn't dispatch cancel event for already-finished animation. chrome doesn't dispatch cancel event at that time (animationcancel and transitioncancel don't do it either).

> b) Should we check that we are actually idle first? (I think it's possible
> mHoldTime could be set if we were paused and then cleared the timeline?)
> 
> c) Should we do the other steps we normally do when canceling, e.g.
>    - resetting pending tasks
>    - rejecting the finished promise

Probably yes, but I haven't deeply thought it.
Adding test cases that setting null timeline doesn't fire cancel event if the animation is already idle state. Also css-animations/test_event-dispatch.html and css-transitions/test_event-dispatch were rewritten.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22814ba894592a10f972d8a7d032793dba3bff2d
Thanks for doing this. Please let me look at this properly once I get back to the office. There is an outstanding spec bug to preserve the hold time when clearing a timeline which might mean we don't want to do the cancel actions most of the time.
Comment on attachment 8968379 [details]
Bug 1453568 - Make sure that transitioncancel event happens, and neither transitionrun nor transitionstart happens when restarting cancelled transition by setting display none property.

https://reviewboard.mozilla.org/r/237068/#review242898

::: commit-message-6722e:1
(Diff revision 1)
> +Bug 1453568 - Make sure that transitioncancel event happens, and neither transitionrun nor transitionstart happens when restarting cancelled transition by setting display none property. r?birtles

Oops.  I thought I left a comment here about incompleteness of this test case.
At this commit, this test doesn't check there is no new transition events other then transitioncancel event there.  In a subsequent patch the check will be introduced.  I will update this commit message.
For my own reference, the spec issue which I think I need to address first is this one:

  https://github.com/w3c/csswg-drafts/issues/2066

Specifically, the proposal there is that "if you set the timeline to null, we set the hold time". That would mean that we _don't_ produce a cancel event for a number of cases here.
Comment on attachment 8968380 [details]
Bug 1453568 - Rewrite css-animations/test_event-dispatch.html with async/await.

https://reviewboard.mozilla.org/r/237070/#review245614
Attachment #8968380 - Flags: review?(bbirtles) → review+
Comment on attachment 8968381 [details]
Bug 1453568 - Rewrite css-transitions/test_event-dispatch.html with async/await.

https://reviewboard.mozilla.org/r/237072/#review245626
Attachment #8968381 - Flags: review?(bbirtles) → review+
I'm really sorry for the delay on this. Unfortunately I suspect we want to update the spec and then changes these patches somewhat (the test fix-ups are great, however, and can be landed right away).

I've been working on bug 1456394 in an effort to bring our implementation in line with the current spec so I can make spec changes and test changes in sync again. So, this might take a few more weeks before I can review it, I guess.
Comment on attachment 8968378 [details]
Bug 1453568 - Don't use arrow function for prototype function which uses `this` inside the function.

https://reviewboard.mozilla.org/r/237066/#review253436

We should just rewrite this as an ES6 class I guess.
Attachment #8968378 - Flags: review?(bbirtles) → review+
Comment on attachment 8968382 [details]
Bug 1453568 - Make sure there is no new animation events as expected.

https://reviewboard.mozilla.org/r/237074/#review253438

It's quite confusing to manage both the handler and watcher. Can we rewrite this to use EventWatcher with { record: all }[1] instead?

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/testing/web-platform/tests/resources/testharness.js#654-676

::: dom/animation/test/css-animations/test_event-dispatch.html:49
(Diff revision 1)
> +                'animationstart event should not be receoved');
> +  assert_equals(animationEventHandler.animationiteration, undefined,
> +                'animationiteration event should not be receoved');
> +  assert_equals(animationEventHandler.animationend, undefined,
> +                'animationend event should not be receoved');
> +  assert_equals(animationEventHandler.animationcancel, undefined,
> +                'animationcancel event should not be receoved');

s/receoved/received/
Attachment #8968382 - Flags: review?(bbirtles)
Comment on attachment 8968379 [details]
Bug 1453568 - Make sure that transitioncancel event happens, and neither transitionrun nor transitionstart happens when restarting cancelled transition by setting display none property.

https://reviewboard.mozilla.org/r/237068/#review253440

This patch could use a comment explaining the change. Is this a behavior change? Was the test broken before?
Attachment #8968379 - Flags: review?(bbirtles)
Comment on attachment 8968383 [details]
Bug 1453568 - Add test case that animationcancel or transitioncancel event is not dispatched when setting null timeline in idle state.

https://reviewboard.mozilla.org/r/237076/#review253442

::: dom/animation/test/css-animations/test_event-dispatch.html:139
(Diff revision 1)
> +  const { animation, watcher, div, handler } = setupAnimation(t, 'anim 100s');
> +
> +  await watcher.wait_for('animationstart');
> +  animation.currentTime = 100.0;
> +  // Make idle
> +  animation.timeline = null;
> +  const event = await watcher.wait_for('animationcancel');
> +
> +  assert_time_equals_literal(event.elapsedTime, 0.1);
> +
> +  handler.clear();

As with the previous patch, let's rewrite this to no longer use the handler object.

::: dom/animation/test/css-transitions/test_event-dispatch.html:139
(Diff revision 1)
> +  const { transition, watcher, handler } =
> +    setupTransition(t, 'margin-left 100s 100s');
> +
> +  await Promise.all([ watcher.wait_for('transitionrun'), transition.ready ]);
> +  // Make idle
> +  transition.timeline = null;
> +
> +  const event = await watcher.wait_for('transitioncancel');
> +  assert_equals(event.elapsedTime, 0.0);
> +
> +  handler.clear();

Here too.
Attachment #8968383 - Flags: review?(bbirtles) → review+
Comment on attachment 8968042 [details]
Bug 1453568 - Dispatch cancel event when setting null timeline.

https://reviewboard.mozilla.org/r/236732/#review253444

The spec doesn't say to do this, and I don't think it's likely to either so I don't think we want this change.
Attachment #8968042 - Flags: review?(bbirtles)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: