User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170406030206 Steps to reproduce: Reproduction URL: https://www.scirra.com/labs/bugs/ffanimbug.html 1. Click the blue square 2. Observe what happens as it fades out Clicking the blue square uses element.animate() to fade the opacity from 1 to 0 over 1 second. In the onfinish handler it removes the element. Actual results: When the animation finishes, it briefly appears at 100% opacity again. Expected results: Since the element is removed as soon as the animation finishes, it should not appear at 100% again. It works correctly in Chrome.

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0 Hello and thanks for the report! Following the STR from comment 0, I was able to reproduce the issue using all Firefox channels, on Windows 10 x64, Ubuntu 16.04 LTS and MAC OS X.

I'm pretty sure that's the correct behavior. The animation doesn't specify `fill: 'forwards'` so it will complete and then paint at full opacity. At some time after that the onfinish event handler will run where the element is removed from the DOM. The spec doesn't make any requirements on when (as in what frame) the event handler runs. Leaving this open for now, however, since we may be able to arrange for these event handlers to be called within the same tick as the animation finishes which should avoid flicker (and this seems likely to become a common pattern).

It's at least an interop issue with Chrome, since the same page behaves differently between Chrome and Firefox. IMO the Chrome behavior is what is expected. Presumably Firefox does something like finish animation, paint, run onfinish handler, which doesn't seem to be a useful ordering.

I haven't forgotten this one but it's not at the top of my list either. It looks like Olli has made some progress on bug 1193394 which may affect this behavior.

(In reply to Brian Birtles (:birtles) from comment #4) > I haven't forgotten this one but it's not at the top of my list either. It > looks like Olli has made some progress on bug 1193394 which may affect this > behavior. (Sorry, this bug is about events so bug 1193394 has nothing to do this this. My mistake.)

Ok, I think I know what is happening here. There are two problems: 1) When we call UpdateTiming from Animation::Tick, we pass SyncNotifyFlag::Async so we end up posting a runnable to dispatch the event. There doesn't seem to be any need to use 'Async' there, and the spec doesn't require it (it doesn't say either way, but it probably should) so I think we should change that to SyncNotifyFlag::Sync and update the spec accordingly. That, alone, however, doesn't fix this. 2) When we do go to dispatch the event in Animation::DoFinishNotificationImmediately/DispatchPlaybackEvent, we use AsyncEventDispatcher which means we end up with the following sequence: * nsRefreshDriver::Tick * Flush style observers * Tick DocumentTimeline * Tick Animation * Notice that we're finished * Queue AsyncEventDispatcher for finish event * Update style and paint * nsRefreshDriver::Tick (different refresh driver) * Run AsyncEventDispatcher * onfinish handler runs I think I used AsyncEventDispatcher here just because I didn't want to call into script in the middle of ticking an animation but I didn't really think about when the event handler would end up running. Instead, I think we want to dispatch these events in nsRefreshDriver::DispatchAnimationEvents. That seems intuitive, lines up with the HTML spec's definition that says, "For each fully active Document in docs, run CSS animations and send events for that Document, passing in now as the timestamp"[1], i.e. the events for animations are dispatched immediately after ticking animations. It should also fix the flicker in this case. Doing that means we need somewhere else to store the pending events in the interim. For CSS animations and transitions we store them on a DelayedEventDispatcher object on the nsAnimationManager/nsTransitionManager associated with the nsPresContext. We *could* add a similar member to the EffectCompositor although it feels a little odd since the EffectCompositor is supposed to be only concerned with animation effects, not timing effects. Alternatively we could store the pending events in nsRefreshDriver::mPendingEvents and dispatch them in nsRefreshDriver::DispatchPendingEvents (called immediately after nsRefreshDriver::DispatchAnimationEvents which is probably close enough). If we do that, the ordering should be deterministic since DocumentTimeline::WillRefresh iterates over its animations using its mAnimationOrder linked-list when ticking them. However, they won't be sorted by time. For CSS animations and transition events we sort events by time before dispatching them and that's why we keep them in a separate list. For these "animation playback events" we don't currently do that. That's probably worth fixing at some point but maybe that can be a separate bug and possibly something to fix after fixing bug 1356533 which indicates that DispatchAnimationEvents is showing up in profiles. So, it might be worth just using mPendingEvents to fix the flicker in v55 and then do something more involved later where we sort the animation playback events by time. CC'ing Olli since I'll probably end up asking him for review when I get around to fixing this. [1] https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model

I've now defined general handling for all types of animation events: https://w3c.github.io/web-animations/#update-animations-and-send-events I suggest we expand the scope of this bug to implementing that behavior since it should fix the flicker at the same time. It's probably easiest to do this--or at least to write the tests for it--after the microtask changes have landed (bug 1193394). Also, it would be good to do this before we ship the Animation interface promises (bug 1398037).

This should definitely depend on bug 1415780. Actually I am working on the bug to be able to dispatch web animation events in nsRefreshDriver::DispatchAnimationEvents, actually the bug will not fix this, but will make this bug easier. Brian, I am mending this bug title since I lost this bug repeatedly, if it's not correct, would you mind correcting it?

Also I would appreciate it if someone (who try to fix this bug) writes web platform tests to check animation events order in the case where a bunch of CSS animations/transitions and web animation events are fired in the same tick.

Comment on attachment 8969505 [details] Bug 1354501 - Bail out if we know there is no need to dispatch any animation events in advance of checking which animation event we need. https://reviewboard.mozilla.org/r/238262/#review244006 ::: layout/style/nsAnimationManager.cpp (Diff revision 1) > - if (currentPhase == mPreviousPhase && > - currentIteration == mPreviousIteration) { > - return; > - } Oops. This will be less efficient. We should leave this as it is, and we should bail out from the above 'if (!mEffect)' block instead.

Comment on attachment 8969498 [details] Bug 1354501 - Rewrite promise_test in onfinish.html with async/await. https://reviewboard.mozilla.org/r/238248/#review244084

Comment on attachment 8969499 [details] Bug 1354501 - Rewrite async_test in onfinish.html with promise_test with async/await. https://reviewboard.mozilla.org/r/238250/#review244088 I agree this is a lot easier to read, but I think this test is specifically supposed to be testing the Animation.onfinish interface member, isn't it?

(In reply to Brian Birtles (:birtles) from comment #24) > Comment on attachment 8969499 [details] > Bug 1354501 - Rewrite async_test in onfinish.html with promise_test with > async/await. > > https://reviewboard.mozilla.org/r/238250/#review244088 > > I agree this is a lot easier to read, but I think this test is specifically > supposed to be testing the Animation.onfinish interface member, isn't it? Actually I think those tests should mostly be moved to the appropriate places in timing-model and then we should add some very superficial test here that onfinish also correctly registers an event listener.

(In reply to Brian Birtles (:birtles) from comment #24) > Comment on attachment 8969499 [details] > Bug 1354501 - Rewrite async_test in onfinish.html with promise_test with > async/await. > > https://reviewboard.mozilla.org/r/238250/#review244088 > > I agree this is a lot easier to read, but I think this test is specifically > supposed to be testing the Animation.onfinish interface member, isn't it? Oh right, indeed. We should move them into timing-model/animation-events/finish.html, or something like that?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #26) > (In reply to Brian Birtles (:birtles) from comment #24) > > Comment on attachment 8969499 [details] > > Bug 1354501 - Rewrite async_test in onfinish.html with promise_test with > > async/await. > > > > https://reviewboard.mozilla.org/r/238250/#review244088 > > > > I agree this is a lot easier to read, but I think this test is specifically > > supposed to be testing the Animation.onfinish interface member, isn't it? > > Oh right, indeed. We should move them into > timing-model/animation-events/finish.html, or something like that? Looking at the content of those tests, I think they mostly belong in: timing-model/animations/updating-the-finished-state.html Then, as per comment 25, I think we should leave 1~2 tests in onfinish.html that checks that it allows registering an event listener (or whatever the actual required behavior is).

Comment on attachment 8969500 [details] Bug 1354501 - Add test cases that animation event is fired for the animation on an orphaned element. https://reviewboard.mozilla.org/r/238252/#review244102 This too look great, but I suspect they belong somewhere in timing-model. Sorry about this situation. It's quite frustrating since every time I touch the web-animations WPT I feel like I need to move tests to the correct location. Hopefully we're getting better though!

Comment on attachment 8969501 [details] Bug 1354501 - Factor out calculations for interval start/end times. https://reviewboard.mozilla.org/r/238254/#review244108 ::: dom/animation/Animation.h:488 (Diff revision 1) > + StickyTimeDuration > + IntervalStartTime(const StickyTimeDuration& aActiveDuration) const Can we add a comment here, even just something like: // Earlier side of the elapsed time range reported in CSS Animations and CSS Transitions events. // // https://drafts.csswg.org/css-animations-2/#interval-start // https://drafts.csswg.org/css-transitions-2/#interval-start would be good. ::: dom/animation/Animation.h:499 (Diff revision 1) > + StickyTimeDuration > + IntervalEndTime(const StickyTimeDuration& aActiveDuration) const Likewise here too.

This is looking really good. I'm afraid I'm not likely to finish reviewing this this week. I think I need to concentrate carefully on the second last patch in particular. So far, however, it looks good.

Comment on attachment 8969502 [details] Bug 1354501 - Change event target variable type to EventTarget and rename it to mTarget. https://reviewboard.mozilla.org/r/238256/#review244498

Comment on attachment 8969503 [details] Bug 1354501 - Introduce a new function to queue a single animation event. https://reviewboard.mozilla.org/r/238258/#review244500 ::: dom/animation/AnimationEventDispatcher.cpp:43 (Diff revision 1) > void > -AnimationEventDispatcher::QueueEvents(nsTArray<AnimationEventInfo>&& aEvents) > +AnimationEventDispatcher::ScheduleDispatch() > { > - MOZ_ASSERT(mPresContext, > + MOZ_ASSERT(mPresContext, "The pres context should be valid"); > - "The pres context should be valid"); > - > - mPendingEvents.AppendElements(Move(aEvents)); > - mIsSorted = false; > if (!mIsObserving) { > mPresContext->RefreshDriver()->ScheduleAnimationEventDispatch(this); > mIsObserving = true; > } > } Perhaps move this last in the .cpp file to match the order in the .h file?

Comment on attachment 8969504 [details] Bug 1354501 - Dispatch web animation events at the same time when CSS animations/transitions events are dispatched. https://reviewboard.mozilla.org/r/238260/#review244504 This looks really good but I'm clearing the review for now based on the comments below about finish time handling. ::: dom/animation/Animation.h:447 (Diff revision 1) > void MaybeResolveFinishedPromise(); > void DoFinishNotification(SyncNotifyFlag aSyncNotifyFlag); > friend class AsyncFinishNotification; > void DoFinishNotificationImmediately(MicroTaskRunnable* aAsync = nullptr); > - void DispatchPlaybackEvent(const nsAString& aName); > + void QueuePlaybackEvent(const nsAString& aName, > + const StickyTimeDuration& aElapsedTime); I'm not sure about the factoring here. "Elapsed time" is not really a Web Animations concept -- just something we use in CSS Animations / CSS Transitions. Could we make the parameter here a TimeStamp and call it aScheduledEventTime (to match the spec)? (See comments later on the calculation of this.) ::: dom/animation/Animation.h:500 (Diff revision 1) > StickyTimeDuration > IntervalEndTime(const StickyTimeDuration& aActiveDuration) const > { > - MOZ_ASSERT(AsCSSTransition() || AsCSSAnimation(), > - "Should be called for CSS animations or transitions"); > - > static constexpr StickyTimeDuration zeroDuration = StickyTimeDuration(); > return std::max( > std::min((EffectEnd() - mEffect->SpecifiedTiming().Delay()), > aActiveDuration), > zeroDuration); > } Is this really equivalent? Here we have: max(min(EffectEnd() - <delay>), <activeDuration>), 0) In QueuePlaybackEvent we call ElapsedTimeToTimeStamp which adds the delay, so: max(min(EffectEnd() - <delay>), <activeDuration>), 0) + <delay> But the spec says that for the finish event we should use the "target effect end", i.e. EffectEnd() It's not clear to me, at least, that these are equivalent (and if they are we should clearly document that). ::: dom/animation/Animation.cpp:880 (Diff revision 1) > if (mFinished) { > mFinished->MaybeReject(NS_ERROR_DOM_ABORT_ERR); > } > ResetFinishedPromise(); > > - DispatchPlaybackEvent(NS_LITERAL_STRING("cancel")); > + QueuePlaybackEvent(NS_LITERAL_STRING("cancel"), activeTime); Assuming we stick with my suggestion to pass a timestamp here, I think the spec[1] just says this should be something like: mTimeline ? mTimeline->ToTimeStamp(mTimeline->GetCurrentTime()) : TimeStamp(); (Although there's probably a simpler way of writing that.) [1] https://drafts.csswg.org/web-animations-1/#cancel-an-animation ::: testing/web-platform/tests/web-animations/timing-model/timelines/dispatching-events.html:75 (Diff revision 1) > + div.animate(null, 100 * MS_PER_SEC); > + const animations = div.getAnimations(); > + > + let receivedEvents = []; > + animations.forEach(anim => { > + anim.onfinish = (event) => { Nit: no () around event ::: testing/web-platform/tests/web-animations/timing-model/timelines/dispatching-events.html:93 (Diff revision 1) > + 'finish, animationend and transitionend events should be sorted ' + > + 'by composite order'); I'm not sure about the mention of animationend and transitionend? This test doesn't cover them. ::: testing/web-platform/tests/web-animations/timing-model/timelines/dispatching-events.html:106 (Diff revision 1) > + div.onanimationcancel = (event) => { > + receiveEvent(event.type, event.timeStamp); > + }; > + div.ontransitioncancel = (event) => { Nit: Unneeded () around event ::: testing/web-platform/tests/web-animations/timing-model/timelines/dispatching-events.html:144 (Diff revision 1) > + // XXX: It's not clear whether CSS cancel event is prior to web > + // animation cancel event. Within a web-anim/css pair, the Web Animations one should be fired first. This is implied by the way the corresponding spec sections are written but it could definitely be more clear. Let's just make this comment: "This ordering needs more clarification in the spec, but the intention is that the cancel playback event fires before the equivalent CSS cancel event in each case."

Comment on attachment 8969505 [details] Bug 1354501 - Bail out if we know there is no need to dispatch any animation events in advance of checking which animation event we need. https://reviewboard.mozilla.org/r/238262/#review244510 ::: layout/style/nsTransitionManager.cpp:213 (Diff revision 2) > + if (currentPhase == mPreviousTransitionPhase) { > + return; > + } Won't this early return stop us from updating mPreviousTransitionPhase from idle to pending? And also stop us from dispatching a transitionrun event in that case?

(In reply to Brian Birtles (:birtles) from comment #34) > Comment on attachment 8969505 [details] > Bug 1354501 - Bail out if we know there is no need to dispatch any animation > events in advance of checking which animation event we need. > > https://reviewboard.mozilla.org/r/238262/#review244510 > > ::: layout/style/nsTransitionManager.cpp:213 > (Diff revision 2) > > + if (currentPhase == mPreviousTransitionPhase) { > > + return; > > + } > > Won't this early return stop us from updating mPreviousTransitionPhase from > idle to pending? And also stop us from dispatching a transitionrun event in > that case? Thanks. I missed the case. Updating currentPhase to Pending had to be moved early as well. Anyway, we have to write a test case for that since try runs didn't catch this fault.

(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #33) > ::: dom/animation/Animation.cpp:880 > (Diff revision 1) > > if (mFinished) { > > mFinished->MaybeReject(NS_ERROR_DOM_ABORT_ERR); > > } > > ResetFinishedPromise(); > > > > - DispatchPlaybackEvent(NS_LITERAL_STRING("cancel")); > > + QueuePlaybackEvent(NS_LITERAL_STRING("cancel"), activeTime); > > Assuming we stick with my suggestion to pass a timestamp here, I think the > spec[1] just says this should be something like: > > mTimeline ? mTimeline->ToTimeStamp(mTimeline->GetCurrentTime()) : > TimeStamp(); The timestamp value is just used for sorting events, it's different from the time in event object. So I believe the original one is correct. But, > testing/web-platform/tests/web-animations/timing-model/timelines/dispatching- > events.html:144 > (Diff revision 1) > > + // XXX: It's not clear whether CSS cancel event is prior to web > > + // animation cancel event. > > Within a web-anim/css pair, the Web Animations one should be fired first. > This is implied by the way the corresponding spec sections are written but > it could definitely be more clear. > > Let's just make this comment: > > "This ordering needs more clarification in the spec, but the intention is > that the cancel playback event fires before the equivalent CSS cancel event > in each case." now I tried to order this, and noticed that the timestamps for sorting of animationcancel and transitionscancel are null! That's because mStartTime is nullified just before calling MaybeQueueCancelEvent(). So, should the timestamp for play back events be null, or should the time stamp for CSS events be a valid?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36) > now I tried to order this, and noticed that the timestamps for sorting of > animationcancel and transitionscancel are null! That's because mStartTime > is nullified just before calling MaybeQueueCancelEvent(). So, should the > timestamp for play back events be null, or should the time stamp for CSS > events be a valid? I am going to use null timestamp for cancel event here in this bug. (Making the time tamp for CSS cancel events requires more work there). If cancel events should be fired after other events, I will open a new bug for that.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #35) > (In reply to Brian Birtles (:birtles) from comment #34) > > Comment on attachment 8969505 [details] > > Bug 1354501 - Bail out if we know there is no need to dispatch any animation > > events in advance of checking which animation event we need. > > > > https://reviewboard.mozilla.org/r/238262/#review244510 > > > > ::: layout/style/nsTransitionManager.cpp:213 > > (Diff revision 2) > > > + if (currentPhase == mPreviousTransitionPhase) { > > > + return; > > > + } > > > > Won't this early return stop us from updating mPreviousTransitionPhase from > > idle to pending? And also stop us from dispatching a transitionrun event in > > that case? > > Thanks. I missed the case. Updating currentPhase to Pending had to be > moved early as well. Anyway, we have to write a test case for that since > try runs didn't catch this fault. It seems impossible to write the test case, i.e. generating pending state in the middle of queuing transition events because FinishPendingAt is called in Tick().

Comment on attachment 8969500 [details] Bug 1354501 - Add test cases that animation event is fired for the animation on an orphaned element. https://reviewboard.mozilla.org/r/238252/#review260888

Comment on attachment 8969504 [details] Bug 1354501 - Dispatch web animation events at the same time when CSS animations/transitions events are dispatched. https://reviewboard.mozilla.org/r/238260/#review260894 This is looking really good. Thanks for doing this. I don't have a lot of time this week for reviews so I'm just passing on my initial feedback. Hopefully I can look at the tests more thoroughly tomorrow. ::: dom/animation/Animation.cpp:877 (Diff revision 2) > if (mFinished) { > mFinished->MaybeReject(NS_ERROR_DOM_ABORT_ERR); > } > ResetFinishedPromise(); > > - DispatchPlaybackEvent(NS_LITERAL_STRING("cancel")); > + QueuePlaybackEvent(NS_LITERAL_STRING("cancel"), TimeStamp()); (I still need to look into whether this works or not.) ::: dom/animation/Animation.cpp:1627 (Diff revision 2) > - DispatchPlaybackEvent(NS_LITERAL_STRING("finish")); > + QueuePlaybackEvent(NS_LITERAL_STRING("finish"), > + ElapsedTimeToTimeStamp(EffectEnd())); This doesn't seem right. ElapsedTimeToTimeStamp adds the delay but EffectEnd() already includes the delay. Shouldn't this just use AnimationTimeToTimeStamp? ::: dom/animation/Animation.cpp:1635 (Diff revision 2) > + // We have to use GetDocumentIfCurrent() instead of GetComposedDoc() since > + // we have to fire animation events even if the target element is not > + // associated with any documents. > + nsIDocument* doc = GetDocumentIfCurrent(); > + if (!doc) { > + return; > + } The spec says we should use the animation's "document for timing" here: https://drafts.csswg.org/web-animations-1/#document-for-timing ::: dom/animation/AnimationEventDispatcher.h:230 (Diff revision 2) > + if (a.IsWebAnimationEvent() != b.IsWebAnimationEvent() && > + (a.IsWebAnimationEvent() || b.IsWebAnimationEvent())) { I don't think we need the part after && ... If the first condition: a.IsWebAnimationEvent() != b.IsWebAnimationEvent() is true then either |a| or |b| _must_ be a web animation event, right? ::: testing/web-platform/meta/MANIFEST.json:618770 (Diff revision 2) > ], > "web-animations/timing-model/time-transformations/transformed-progress.html": [ > "2e55f43def584a67eeb313f050154cd146002938", > "testharness" > ], > + "web-animations/timing-model/timelines/dispatching-events.html": [ I was just wondering: should this be called update-animations-and-send-events.html? Maybe it's fine as-is.

Comment on attachment 8969504 [details] Bug 1354501 - Dispatch web animation events at the same time when CSS animations/transitions events are dispatched. https://reviewboard.mozilla.org/r/238260/#review260894 This is looking really good. Thanks for doing this. I don't have a lot of time this week for reviews so I'm just passing on my initial feedback. Hopefully I can look at the tests more thoroughly tomorrow. ::: dom/animation/Animation.cpp:877 (Diff revision 2) > if (mFinished) { > mFinished->MaybeReject(NS_ERROR_DOM_ABORT_ERR); > } > ResetFinishedPromise(); > > - DispatchPlaybackEvent(NS_LITERAL_STRING("cancel")); > + QueuePlaybackEvent(NS_LITERAL_STRING("cancel"), TimeStamp()); (I still need to look into whether this works or not.) ::: dom/animation/Animation.cpp:1627 (Diff revision 2) > - DispatchPlaybackEvent(NS_LITERAL_STRING("finish")); > + QueuePlaybackEvent(NS_LITERAL_STRING("finish"), > + ElapsedTimeToTimeStamp(EffectEnd())); This doesn't seem right. ElapsedTimeToTimeStamp adds the delay but EffectEnd() already includes the delay. Shouldn't this just use AnimationTimeToTimeStamp? ::: dom/animation/Animation.cpp:1635 (Diff revision 2) > + // We have to use GetDocumentIfCurrent() instead of GetComposedDoc() since > + // we have to fire animation events even if the target element is not > + // associated with any documents. > + nsIDocument* doc = GetDocumentIfCurrent(); > + if (!doc) { > + return; > + } The spec says we should use the animation's "document for timing" here: https://drafts.csswg.org/web-animations-1/#document-for-timing ::: dom/animation/AnimationEventDispatcher.h:230 (Diff revision 2) > + if (a.IsWebAnimationEvent() != b.IsWebAnimationEvent() && > + (a.IsWebAnimationEvent() || b.IsWebAnimationEvent())) { I don't think we need the part after && ... If the first condition: a.IsWebAnimationEvent() != b.IsWebAnimationEvent() is true then either |a| or |b| _must_ be a web animation event, right? ::: testing/web-platform/meta/MANIFEST.json:618770 (Diff revision 2) > ], > "web-animations/timing-model/time-transformations/transformed-progress.html": [ > "2e55f43def584a67eeb313f050154cd146002938", > "testharness" > ], > + "web-animations/timing-model/timelines/dispatching-events.html": [ I was just wondering: should this be called update-animations-and-send-events.html? Maybe it's fine as-is.

Sorry for the double posting of the review comments. MozReview wasn't working very well over hotel wifi.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #38) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #35) > > Thanks. I missed the case. Updating currentPhase to Pending had to be > > moved early as well. Anyway, we have to write a test case for that since > > try runs didn't catch this fault. > > It seems impossible to write the test case, i.e. generating pending state in > the middle of queuing transition events because FinishPendingAt is called in > Tick(). Can we do it if the timeline is null?

Comment on attachment 8969505 [details] Bug 1354501 - Bail out if we know there is no need to dispatch any animation events in advance of checking which animation event we need. https://reviewboard.mozilla.org/r/238262/#review260914 This looks good but obviously if we can write a test it would be nice. I wonder if we can keep it in the pending play state by setting a null timeline before we play it?

(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #51) > Comment on attachment 8969505 [details] > Bug 1354501 - Bail out if we know there is no need to dispatch any animation > events in advance of checking which animation event we need. > > https://reviewboard.mozilla.org/r/238262/#review260914 > > This looks good but obviously if we can write a test it would be nice. I > wonder if we can keep it in the pending play state by setting a null > timeline before we play it? I did try to call pause() & timeline = null but it didn't work. I will debug it what's going on there.

Yeah, I wonder if canceling the animation, setting the timeline to null, then calling play() might work.

(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #53) > Yeah, I wonder if canceling the animation, setting the timeline to null, > then calling play() might work. Tricky... but it doesn't work either. As far as I can tell, after the play() call on null timeline, we will never call Tick() after that, thus we will never call QueueEvents() either. Is this a bug? Note that no transition event fires on nightly either.

Comment on attachment 8969504 [details] Bug 1354501 - Dispatch web animation events at the same time when CSS animations/transitions events are dispatched. https://reviewboard.mozilla.org/r/238260/#review260894 > The spec says we should use the animation's "document for timing" here: > > https://drafts.csswg.org/web-animations-1/#document-for-timing The document returned by GetDocumentIfCurrent() is slightly different from the document for timing. The document here is the document that the animations blongs to. I did amend the comment. > I don't think we need the part after && ... > > If the first condition: > > a.IsWebAnimationEvent() != b.IsWebAnimationEvent() > > is true then either |a| or |b| _must_ be a web animation event, right? thanks. I did forget to drop them after I noticed that the order mismatch was caused by null timeline for CSS cancel events.

Oh dear MozReview. Please fix the remaining issue count on https://reviewboard.mozilla.org/r/238260/diff/3#index_header :/

(In reply to Hiroyuki Ikezoe (:hiro) from comment #54) > (In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from > comment #53) > > Yeah, I wonder if canceling the animation, setting the timeline to null, > > then calling play() might work. > > Tricky... but it doesn't work either. As far as I can tell, after the > play() call on null timeline, we will never call Tick() after that, thus we > will never call QueueEvents() either. Is this a bug? Note that no > transition event fires on nightly either. In that case, don't worry about the test.

Comment on attachment 8969504 [details] Bug 1354501 - Dispatch web animation events at the same time when CSS animations/transitions events are dispatched. https://reviewboard.mozilla.org/r/238260/#review261174 ::: dom/animation/Animation.cpp:877 (Diff revision 3) > if (mFinished) { > mFinished->MaybeReject(NS_ERROR_DOM_ABORT_ERR); > } > ResetFinishedPromise(); > > - DispatchPlaybackEvent(NS_LITERAL_STRING("cancel")); > + QueuePlaybackEvent(NS_LITERAL_STRING("cancel"), TimeStamp()); The spec says we should use the timestamp of the associated timeline at the point when the event is canceled: https://drafts.csswg.org/web-animations-1/#canceling-an-animation-section Please either fix this or file a bug to fix it. (And I'd like to fix this sooner rather than later.) Thanks! ::: dom/animation/Animation.cpp:1635 (Diff revision 3) > + // We have to use GetDocumentIfCurrent() instead of GetComposedDoc() since > + // we have to fire animation events on the document associated with this > + // animation object instead of the document associated with the target > + // element. > + nsIDocument* doc = GetDocumentIfCurrent(); > + if (!doc) { > + return; > + } Can we make this use the "document for timing", i.e. lookup the timeline, add something to AnimationTimeline liked GetDocument() and use that? Or should the spec change to say the document associated with the event target? Or add a comment explaining that for all intents and purposes these are the same? ::: testing/web-platform/tests/web-animations/timing-model/timelines/dispatching-events.html:12 (Diff revision 3) > +promise_test(async t => { > + const div = createDiv(t); > + const animation = div.animate(null, 100 * MS_PER_SEC); > + > + await animation.ready; > + > + let rAFReceived = false; > + requestAnimationFrame(() => { > + rAFReceived = true; > + }); > + > + const eventWatcher = new EventWatcher(t, animation, 'cancel'); > + animation.cancel(); > + > + await eventWatcher.wait_for('cancel'); > + > + assert_false(rAFReceived, > + 'cancel event should be fired before requestAnimationFrame'); > +}, 'Fires cancel event before requestAnimationFrame'); This is something that needs to be specified more clearly in the spec, but basically, as I understand it, this is testing that the ready promise is resolved as part of step (1) in "update animations and send events", the callback is run in step (2), and then the events are dispatched subsequently. Given that, I really think this test is covering the whole "Update and send events" procedure and we should name this file "update-and-send-events.html". It might also be good to add a comment before the `await animation.ready` line saying: // The ready promise should be resolved as part of the micro-task checkpoint after updating the current time of all timelines in the procedure to "update animations and send events". ::: testing/web-platform/tests/web-animations/timing-model/timelines/dispatching-events.html:36 (Diff revision 3) > + > +promise_test(async t => { > + const div = createDiv(t); > + const animation = div.animate(null, 100 * MS_PER_SEC); > + > + await animation.ready; Likewise, a comment might be nice here. ::: testing/web-platform/tests/web-animations/timing-model/timelines/dispatching-events.html:136 (Diff revision 3) > + // cancel() queues cancel event synchronously so that cancel event can be > + // received in the same frame. > + await waitForAnimationFrames(1); Perhaps this comment should be: // Note: waitForAnimationFrames(1) here only waits until the next requestAnimationFrame callback which is actually the _same_ frame since we are currently operating in the `ready` callback of the animations which happens as part of the "Update animations and send events" procedure _before_ we run animation frame callbacks. That's ok because cancel events are queued synchronously and will be dispatched as part of the "Update animations and send events" procedure before requestAnimationFrame callbacks are run.

Thanks for a bunch of reviews! (In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #60) > > ::: dom/animation/Animation.cpp:1635 > (Diff revision 3) > > + // We have to use GetDocumentIfCurrent() instead of GetComposedDoc() since > > + // we have to fire animation events on the document associated with this > > + // animation object instead of the document associated with the target > > + // element. > > + nsIDocument* doc = GetDocumentIfCurrent(); > > + if (!doc) { > > + return; > > + } > > Can we make this use the "document for timing", i.e. lookup the timeline, > add something to AnimationTimeline liked GetDocument() and use that? > > Or should the spec change to say the document associated with the event > target? > > Or add a comment explaining that for all intents and purposes these are the > same? I think the spec should explain somehow this situation. It's somewhat related to bug 1244848. We haven't aligned animation events in subframes at all. My intention here was that the animation event is aligned with other animations in the same document that the animation object belongs to. But hmm, I don't know which is better for web developers. Anyway I will change the document to the timeline one since CSS animations/transitions currently do so.

A question arose here. If we do use the document for the timeline there in QueuePlaybackEvent, we will not meet the condition that the timeline is null, that makes me think that we need an AnimationEventDispatcher independent from the document? Anyway we can handle it in a later bug though.

I think the spec says you just queue a task to dispatch the event (without using the timeline's queue) in that case: https://drafts.csswg.org/web-animations-1/#finish-notification-steps

Comment on attachment 8969504 [details] Bug 1354501 - Dispatch web animation events at the same time when CSS animations/transitions events are dispatched. https://reviewboard.mozilla.org/r/238260/#review261174 > The spec says we should use the timestamp of the associated timeline at the point when the event is canceled: > > https://drafts.csswg.org/web-animations-1/#canceling-an-animation-section > > Please either fix this or file a bug to fix it. > > (And I'd like to fix this sooner rather than later.) > > Thanks! Filed bug 1472900 for that.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6979ef3af921 Rewrite promise_test in onfinish.html with async/await. r=birtles https://hg.mozilla.org/integration/autoland/rev/5aa0a52055ce Add test cases that animation event is fired for the animation on an orphaned element. r=birtles https://hg.mozilla.org/integration/autoland/rev/1c53b4e00456 Factor out calculations for interval start/end times. r=birtles https://hg.mozilla.org/integration/autoland/rev/19f47d750767 Change event target variable type to EventTarget and rename it to mTarget. r=birtles https://hg.mozilla.org/integration/autoland/rev/44f00022ed62 Introduce a new function to queue a single animation event. r=birtles https://hg.mozilla.org/integration/autoland/rev/8beb2cbc6b25 Dispatch web animation events at the same time when CSS animations/transitions events are dispatched. r=birtles https://hg.mozilla.org/integration/autoland/rev/f6e1ff7b57e6 Bail out if we know there is no need to dispatch any animation events in advance of checking which animation event we need. r=birtles

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11762 for changes under testing/web-platform/tests

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

https://hg.mozilla.org/mozilla-central/rev/6979ef3af921 https://hg.mozilla.org/mozilla-central/rev/5aa0a52055ce https://hg.mozilla.org/mozilla-central/rev/1c53b4e00456 https://hg.mozilla.org/mozilla-central/rev/19f47d750767 https://hg.mozilla.org/mozilla-central/rev/44f00022ed62 https://hg.mozilla.org/mozilla-central/rev/8beb2cbc6b25 https://hg.mozilla.org/mozilla-central/rev/f6e1ff7b57e6

Thanks for the fix. Verified our PWA works fine with Web Animations in Firefox Nightly now!

Thanks for checking!

Upstream PR merged