fix($animateCss): don't unregister events when empty #13514
Conversation
Narretz
force-pushed
the
fix-animate-endevent-10387
branch
from
f79f435 to
6d32907
Compare
Narretz
force-pushed
the
fix-animate-endevent-10387
branch
from
6d32907 to
6e876cd
Compare
Previously, when an animation was closed because no animation styles where found, it would call .off() with an empty string as the argument. For both jquery/jqlite this is the same as calling .off() without any argument, which deregisters all event listeners on an element. Closes angular#13514
Narretz
force-pushed
the
fix-animate-endevent-10387
branch
from
6e876cd to
2292497
Compare
Previously, when an animation was closed because no animation styles where found, it would call .off() with an empty string as the argument. For both jquery/jqlite this is the same as calling .off() without any argument, which deregisters all event listeners on an element. Closes angular#13514
Narretz
force-pushed
the
fix-animate-endevent-10387
branch
from
2292497 to
dc88816
Compare
Previously, when an animation was closed because no animation styles where found, it would call .off() with an empty string as the argument. For both jquery/jqlite this is the same as calling .off() without any argument, which deregisters all event listeners on an element. Closes angular#13514
| @@ -750,7 +750,7 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { | |||
| } | |||
|
|
|||
| // Remove the transitionend / animationend listener(s) | |||
| if (events) { | |||
| if (events && events.length) { | |||
There was a problem hiding this comment.
Nitpick: events will always be truthy, since it is initialized to [].
There was a problem hiding this comment.
Not a nitpick, good catch!
|
We need to make sure that we will not call |
|
Hmm. I think we should only test behavior. The unit tests should not be concerned with the events variable or if it can be empty or not. I realize that keeping implementation out of unit tests isn't always easy. For example, with $animateCss, you have to base the tests on the assumption that an animation will only actually start after a requestAnimationFrame. I will have another look at the code and see if there are other cases where the animation process gets aborted before the events are registered. |
|
I probably wasn't clear enough. By "check" I didn't mean in unit tests, I meant in the actual code that sets up the listeners. I.e. change this line: // Before:
element.on(events.join(' '), onAnimationProgress);
// ^-- If events is empty, a listener will be registered for '' (and never deregistered)
// After:
if (events.length) element.on(events.join(' '), onAnimationProgress); |
Narretz
force-pushed
the
fix-animate-endevent-10387
branch
from
dc88816 to
7ed2336
Compare
…en added Previously, when an animation was closed because no animation styles where found, it would call .off() with an empty string as the argument. For both jquery/jqlite this is the same as calling .off() without any argument, which deregisters all event listeners on an element. Closes angular#13514
|
Ok, I see what you mean. I don't think events can be empty when listeners are added, but the |
…dded Previously, when an animation was closed because no animation styles where found, it would call .off() with an empty string as the argument. For both jquery/jqlite this is the same as calling .off() without any argument, which deregisters all event listeners on an element. Closes angular#13514
Narretz
force-pushed
the
fix-animate-endevent-10387
branch
from
7ed2336 to
11357cc
Compare
| @@ -749,8 +749,8 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { | |||
| options.onDone(); | |||
| } | |||
|
|
|||
| // Remove the transitionend / animationend listener(s) | |||
| if (events) { | |||
| if (events && events.length) { | |||
There was a problem hiding this comment.
It's actually possible that events is undefined, because if no node is detected, the animation is closed before a bunch of variables are even declared.
|
LGTM |
No description provided.