In the attached test case there is a CSS animation with a positive duration and empty keyframes rule. It prints any dispatched start/end events---or it would if we actually dispatched them. The spec is pretty clear we should dispatch events in this case (although it's contradictory when the duration is zero). Chrome, I notice doesn't dispatch events however. Originally this was discussed in the thread starting here: http://lists.w3.org/Archives/Public/www-style/2012Dec/0268.html where the suggestion had been to not dispatch events when the keyframes rule was empty although I followed up with reasons why I thought we should. This was later revisited here: http://lists.w3.org/Archives/Public/www-style/2013Jan/0080.html where it was decided that we should dispatch events in this case. The spec is also pretty clear about this: > Any animation for which both a valid keyframe rule and a non-zero duration > are defined will run and generate events; this includes animations with empty > keyframe rules. (http://dev.w3.org/csswg/css-animations/#events) Bug 1004365 looks at the case where the duration is zero.

The test here seems a little off. Having to call div.clientTop to trigger style computation and hence event dispatch seems a little odd but I've confirmed that without the code changes the tests fail and with them they pass. Also, I checked that with the standalone test (attachment 8415764 [details]) the events are dispatched as expected.

Fix missing keyframes rule in OMTA test

Add tests for dynamic modifications making a rule empty and fixes

Sorry about continuing to change this. This is just a minor tweak so that the time-related checks are grouped together.

Comment on attachment 8436739 [details] [diff] [review] Dispatch events for CSS Animations with empty keyframes rules Review of attachment 8436739 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Just some nits. (I think my comments on test_animations.html all apply to similar code in the _omta version, too.) r=me with nits addressed. ::: layout/style/nsAnimationManager.cpp @@ +68,5 @@ > for (uint32_t animIdx = mAnimations.Length(); animIdx-- != 0; ) { > ElementAnimation* anim = mAnimations[animIdx]; > > if (anim->mProperties.IsEmpty()) { > + // Empty keyframes rule. Maybe add '@'? (so, "@keyframes rule") Looks like we tend to use the '@' in other documentation, but not a big deal. @@ +143,1 @@ > for (uint32_t propIdx = 0, propEnd = anim->mProperties.Length(); I don't think we need to bother with any empty-mProperties check here. All we're doing is bypassing a loop over the contents of mProperties, and that loop will already be a no-op if mProperties is empty. So, unless I'm missing something, let's drop the if-check/continue here. ::: layout/style/test/test_animations.html @@ +1922,5 @@ > + "end of animation with empty keyframes rule"); > +done_div(); > + > +// Test with a zero-duration animation > +new_div("margin-right: 200px; animation: empty 0s 1s both"); Add to this comment: "...and an empty @keyframes rule" (I think you have other tests that are labeled as testing "zero-duration animation", but the thing that distinguishes this one from those is the combination of that *with* empty @keyframes rule.) @@ +1946,5 @@ > +findKeyframesRule("nearlyempty").deleteRule("to"); > +is(cs.getPropertyValue("margin-left"), "0px", > + "margin-left for animation with (now) empty keyframes rule"); > +listen(); > +advance_clock(500); Might be worth moving the listen() to *before* the "deleteRule" call here, so you can make sure that we don't fire any events from that point up until your final result of that deletion (to check for bogus duplicate "start" or "end") So, I'm imagining that listen() would shift up, and we'd add a new line like... check_events([], "shouldn't be any triggered by keyframes-rule emptying"); ...right before your final advance_clock() call here. @@ +1970,5 @@ > +div.clientTop; // Trigger events > +check_events([{ type: 'animationstart', target: div, > + animationName: 'empty', elapsedTime: 0, > + pseudoElement: "" }], > + "events at end of animation updated to use " + I think this needs s/at end/at start/, right? ::: layout/style/test/test_animations_omta.html @@ +1937,5 @@ > + check_events([{ type: 'animationstart', target: gDiv, > + animationName: 'empty', elapsedTime: 0, > + pseudoElement: "" }], > + "events during middle of animation with empty keyframes rule"); > + advance_clock(3000); // Skip to end of animation Shouldn't this be 1000 instead of 3000? (We use 1000 here in the non-OMTA version.)

Comment on attachment 8436739 [details] [diff] [review] Dispatch events for CSS Animations with empty keyframes rules >To fix both these problems, this patch moves the check for an empty keyframes >rule until after mNeedsRefreshes is set. (Assuming you agree with my comment about dropping the empty-keyframes check / "continue", this chunk of the commit message will need updating. [since the check it's talking about will be dropped instead of moved])

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9ed7396f9a Pushed with review feedback addressed and commit message updated.