fix($animateCss): remove animation end event listeners on close

Previously the transition/animation end events were not removed when the
animation was closed. This normally didn't matter, because
the close function knows the animations are closed and won't do work
twice.
However, the listeners themselves do computation that could fail when
the event was missing some data, for example when the event was
triggered instead of natural.

Closes angular#10387

Author: Narretz

Diff for: src/ngAnimate/animateCss.js

@@ -474,6 +474,8 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
       var maxDelayTime;
       var maxDuration;
       var maxDurationTime;
+      var startTime;
+      var events = [];
 
       if (options.duration === 0 || (!$sniffer.animations && !$sniffer.transitions)) {
         return closeAndReturnNoopAnimator();
@@ -747,6 +749,11 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
           options.onDone();
         }
 
+        // Remove the transitionend / animationend listener(s)
+        if (events) {
+          element.off(events.join(' '), onAnimationProgress);
+        }
+
         // if the preparation function fails then the promise is not setup
         if (runner) {
           runner.complete(!rejected);
@@ -782,15 +789,37 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
         };
       }
 
+      function onAnimationProgress(event) {
+        event.stopPropagation();
+        var ev = event.originalEvent || event;
+        var timeStamp = ev.$manualTimeStamp || ev.timeStamp || Date.now();
+
+        /* Firefox (or possibly just Gecko) likes to not round values up
+         * when a ms measurement is used for the animation */
+        var elapsedTime = parseFloat(ev.elapsedTime.toFixed(ELAPSED_TIME_MAX_DECIMAL_PLACES));
+
+        /* $manualTimeStamp is a mocked timeStamp value which is set
+         * within browserTrigger(). This is only here so that tests can
+         * mock animations properly. Real events fallback to event.timeStamp,
+         * or, if they don't, then a timeStamp is automatically created for them.
+         * We're checking to see if the timeStamp surpasses the expected delay,
+         * but we're using elapsedTime instead of the timeStamp on the 2nd
+         * pre-condition since animationPauseds sometimes close off early */
+        if (Math.max(timeStamp - startTime, 0) >= maxDelayTime && elapsedTime >= maxDuration) {
+          // we set this flag to ensure that if the transition is paused then, when resumed,
+          // the animation will automatically close itself since transitions cannot be paused.
+          animationCompleted = true;
+          close();
+        }
+      }
+
       function start() {
         if (animationClosed) return;
         if (!node.parentNode) {
           close();
           return;
         }
 
-        var startTime, events = [];
-
         // even though we only pause keyframe animations here the pause flag
         // will still happen when transitions are used. Only the transition will
         // not be paused since that is not possible. If the animation ends when
@@ -953,30 +982,6 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
             element.removeData(ANIMATE_TIMER_KEY);
           }
         }
-
-        function onAnimationProgress(event) {
-          event.stopPropagation();
-          var ev = event.originalEvent || event;
-          var timeStamp = ev.$manualTimeStamp || ev.timeStamp || Date.now();
-
-          /* Firefox (or possibly just Gecko) likes to not round values up
-           * when a ms measurement is used for the animation */
-          var elapsedTime = parseFloat(ev.elapsedTime.toFixed(ELAPSED_TIME_MAX_DECIMAL_PLACES));
-
-          /* $manualTimeStamp is a mocked timeStamp value which is set
-           * within browserTrigger(). This is only here so that tests can
-           * mock animations properly. Real events fallback to event.timeStamp,
-           * or, if they don't, then a timeStamp is automatically created for them.
-           * We're checking to see if the timeStamp surpasses the expected delay,
-           * but we're using elapsedTime instead of the timeStamp on the 2nd
-           * pre-condition since animations sometimes close off early */
-          if (Math.max(timeStamp - startTime, 0) >= maxDelayTime && elapsedTime >= maxDuration) {
-            // we set this flag to ensure that if the transition is paused then, when resumed,
-            // the animation will automatically close itself since transitions cannot be paused.
-            animationCompleted = true;
-            close();
-          }
-        }
       }
     };
   }];

Diff for: test/ngAnimate/.jshintrc

@@ -8,6 +8,10 @@
     "applyAnimationStyles": false,
     "applyAnimationFromStyles": false,
     "applyAnimationToStyles": false,
-    "applyAnimationClassesFactory": false
+    "applyAnimationClassesFactory": false,
+    "TRANSITIONEND_EVENT": false,
+    "TRANSITION_PROP": false,
+    "ANIMATION_PROP": false,
+    "ANIMATIONEND_EVENT": false,
   }
 }

Diff for: test/ngAnimate/animateCssSpec.js

@@ -12,6 +12,16 @@ describe("ngAnimate $animateCss", function() {
         : expect(className).not.toMatch(regex);
   }
 
+  function keyframeProgress(element, duration, delay) {
+    browserTrigger(element, 'animationend',
+      { timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration });
+  }
+
+  function transitionProgress(element, duration, delay) {
+    browserTrigger(element, 'transitionend',
+      { timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration });
+  }
+
   var fakeStyle = {
     color: 'blue'
   };
@@ -355,16 +365,6 @@ describe("ngAnimate $animateCss", function() {
           assert.toHaveClass('ng-enter-active');
         }
 
-        function keyframeProgress(element, duration, delay) {
-          browserTrigger(element, 'animationend',
-            { timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration });
-        }
-
-        function transitionProgress(element, duration, delay) {
-          browserTrigger(element, 'transitionend',
-            { timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration });
-        }
-
         beforeEach(inject(function($rootElement, $document) {
           element = jqLite('<div></div>');
           $rootElement.append(element);
@@ -1404,6 +1404,125 @@ describe("ngAnimate $animateCss", function() {
           expect(count.stagger).toBe(2);
         }));
       });
+
+      describe('transitionend/animationend event listeners', function() {
+        var element, callLog, proxyListener, origListener, type, progress;
+
+        beforeEach(inject(function($rootElement, $document) {
+
+          element = jqLite('<div></div>');
+          $rootElement.append(element);
+          jqLite($document[0].body).append($rootElement);
+
+          callLog = {};
+
+          var origOn = element.on,
+          origOff = element.off;
+
+          proxyListener = function() {
+            callLog[type] = callLog[type]++ || 1;
+            origListener.apply(this, arguments);
+          };
+
+          // Overwrite the .on function to log calls to the event listeners
+          element.on = function() {
+            origListener = arguments[1];
+            type = arguments[0];
+            origOn.call(this, arguments[0], proxyListener);
+          };
+
+          // Make sure the .off function removes the proxyListener
+          element.off = function() {
+            origOff.call(this, arguments[0], proxyListener);
+          };
+
+        }));
+
+        they('should remove the $prop event listeners on cancel',
+          [TRANSITIONEND_EVENT, ANIMATIONEND_EVENT],
+          function(event) {inject(function($animateCss) {
+
+            switch (event) {
+              case TRANSITIONEND_EVENT:
+                progress = bind(this, transitionProgress, element, 10);
+                break;
+              case ANIMATIONEND_EVENT:
+                progress = bind(this, keyframeProgress, element, 10);
+                break;
+            }
+
+            var animator = $animateCss(element, {
+              duration: 10,
+              to: { 'background': 'red' }
+            });
+
+            var runner = animator.start();
+            triggerAnimationStartFrame();
+
+            runner.cancel();
+
+            // Trigger an end event
+            progress();
+
+            expect(callLog[event]).toBeFalsy();
+          });
+        });
+
+        they("should remove the $prop event listener when the animation is closed",
+          [TRANSITIONEND_EVENT, ANIMATIONEND_EVENT], function(event) {
+          inject(function($animateCss) {
+
+            switch (event) {
+              case TRANSITIONEND_EVENT:
+                ss.addRule('.ng-enter', TRANSITION_PROP + '1s linear all;' +
+                                        TRANSITION_PROP + '-duration:10s;');
+
+                progress = bind(this, transitionProgress, element, 10);
+                break;
+              case ANIMATIONEND_EVENT:
+                ss.addRule('.ng-enter', ANIMATION_PROP + ':animation 10s;');
+                progress = bind(this, keyframeProgress, element, 10);
+                break;
+            }
+
+            var animator = $animateCss(element, {
+              event: 'enter',
+              structural: true
+            });
+
+            var runner = animator.start();
+            triggerAnimationStartFrame();
+
+            progress();
+            expect(element).not.toHaveClass('ng-enter ng-enter-active')
+
+            // Trigger another end event
+            progress();
+
+            expect(callLog[event]).toBe(1);
+          });
+        });
+
+        it("should remove the transitionend event listener when the closing timeout occurs",
+          inject(function($animateCss, $document, $rootElement, $timeout) {
+
+            ss.addRule('.ng-enter', 'transition:10s linear all;');
+
+            var animator = $animateCss(element, {
+              event: 'enter',
+              structural: true
+            });
+
+            animator.start();
+            triggerAnimationStartFrame();
+            $timeout.flush(15000);
+
+            // Force an transitionend event
+            transitionProgress(element, 10);
+
+            expect(callLog[TRANSITIONEND_EVENT]).toBeFalsy();
+        }));
+      });
     });
 
     it('should avoid applying the same cache to an element a follow-up animation is run on the same element',