Closed
Bug 1032014
Opened 10 years ago
Closed 10 years ago
CSS Animations get added twice in BuildAnimations
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: birtles, Assigned: birtles)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Attachment #8447831 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•10 years ago
|
||
Comment 3•10 years ago
|
||
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•10 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)
Comment 6•10 years ago
|
||
(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•10 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 8•10 years ago
|
||
Assignee | ||
Comment 9•10 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.
Comment 10•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•