CSS Animations get added twice in BuildAnimations

RESOLVED FIXED in mozilla33

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

({regression})

Trunk
mozilla33
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8447831 [details] [diff] [review]
Remove extra call to AppendElement when generating animations
Attachment #8447831 - Flags: review?(dbaron)
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.)
Attachment #8447831 - Flags: review?(dbaron) → review+
(Assignee)

Comment 4

4 years ago
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.)
Flags: needinfo?(dbaron)
No; I hadn't gotten to looking at that yet.
Flags: needinfo?(dbaron)
(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.)
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
(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.
See Also: → bug 1033881
https://hg.mozilla.org/mozilla-central/rev/31edc9350b84
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.