1032014 - CSS Animations get added twice in BuildAnimations

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

In debugging bug 1031319 I noticed we often have more ElementAnimation objects generated than we ought to. It turns out in nsAnimationManager::BuildAnimations we add the newly created animation to the list twice.

Once here:

  http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/layout/style/nsAnimationManager.cpp#l683

And then we add the same object again here:

  http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/layout/style/nsAnimationManager.cpp#l854

This appears to have been introduced by me here:

  http://hg.mozilla.org/mozilla-central/rev/b7b9f80931b8

I think in an earlier version of the patch I created the ElementAnimation object independently of the list of animations and later added it but in a subsequent version I decided to add the object to the list immediately and never removed the second AppendElement call.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Remove extra call to AppendElement when generating animations

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=168ee7c45aab

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8447831 [details] [diff] [review]
Remove extra call to AppendElement when generating animations

r=dbaron

(I suspect there's a difference in behavior for the no-@keyframes-rule and no-keyframes-in-the-@keyframes-rule cases, in terms of when the animation of that name is considered to have started, which could matter if the situation changes so there is a @keyframes rule with keyframes.  I'm pretty confident this is the right thing for a @keyframes rule with no keyframes; slightly less confident for no-@keyframes.  I didn't look at the spec, though; it ought to be clear in the spec, and if it's not we should probably raise an issue on the spec.)

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks for the review David!

Was the following comment meant to apply to bug 1031319 however? And if so, is the r+ also for that bug?

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #3)
> (I suspect there's a difference in behavior for the no-@keyframes-rule and
> no-keyframes-in-the-@keyframes-rule cases, in terms of when the animation of
> that name is considered to have started, which could matter if the situation
> changes so there is a @keyframes rule with keyframes.  I'm pretty confident
> this is the right thing for a @keyframes rule with no keyframes; slightly
> less confident for no-@keyframes.  I didn't look at the spec, though; it
> ought to be clear in the spec, and if it's not we should probably raise an
> issue on the spec.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No; I hadn't gotten to looking at that yet.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(This was about the behavior of the existing |continue|s in the loop rather than the one you're adding there, and whether the AppendElement should come before or after them.)

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #6)
> (This was about the behavior of the existing |continue|s in the loop rather
> than the one you're adding there, and whether the AppendElement should come
> before or after them.)

Ok, I understand now. I've checked the spec and found:

  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

The non-zero duration part is contradicted elsewhere in the spec and I'll post to www-style shortly asking this be fixed but the interesting part is the requirement for a valid keyframe rule. So we really shouldn't be dispatching events for "animation-name: non-existent-none".

So we really should adjust this method to do the AppendElement after checking for a keyframe rule. I'll file a follow-up bug for that.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31edc9350b84

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #7)
> So we really should adjust this method to do the AppendElement after
> checking for a keyframe rule. I'll file a follow-up bug for that.

Filed bug 1033881 for that.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31edc9350b84