fix(ngAnimate): callback detection should only use RAF when necessary

matsko · matsko · commit 8b27c3f064b3 · 2015-09-22T11:01:33.000-07:00
Callbacks are detected within the internals of ngAnimate whenever an
animation starts and ends. In order to allow the user to set callbacks
the callback detection needs to happen during the next tick. Prior to
this fix we used $$rAF to do the tick detection, however, with this
patch we intelligently use $$postDigest to do that for us and then
only issue a call to `$$rAF` if necessary.
diff --git a/src/ngAnimate/animateQueue.js b/src/ngAnimate/animateQueue.js
@@ -75,6 +75,24 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
     var disabledElementsLookup = new $$HashMap();
     var animationsEnabled = null;
 
+    function postDigestTaskFactory() {
+      var postDigestCalled = false;
+      return function(fn) {
+        // we only issue a call to postDigest before
+        // it has first passed. This prevents any callbacks
+        // from not firing once the animation has completed
+        // since it will be out of the digest cycle.
+        if (postDigestCalled) {
+          fn();
+        } else {
+          $rootScope.$$postDigest(function() {
+            postDigestCalled = true;
+            fn();
+          });
+        }
+      };
+    }
+
     // Wait until all directive and route-related templates are downloaded and
     // compiled. The $templateRequest.totalPendingRequests variable keeps track of
     // all of the remote templates being currently downloaded. If there are no
@@ -137,14 +155,6 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
       return matches;
     }
 
-    function triggerCallback(event, element, phase, data) {
-      $$rAF(function() {
-        forEach(findCallbacks(element, event), function(callback) {
-          callback(element, phase, data);
-        });
-      });
-    }
-
     return {
       on: function(event, container, callback) {
         var node = extractElementNode(container);
@@ -239,6 +249,9 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
       // These methods will become available after the digest has passed
       var runner = new $$AnimateRunner();
 
+      // this is used to trigger callbacks in postDigest mode
+      var runInNextPostDigestOrNow = postDigestTaskFactory();
+
       if (isArray(options.addClass)) {
         options.addClass = options.addClass.join(' ');
       }
@@ -459,7 +472,20 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
       return runner;
 
       function notifyProgress(runner, event, phase, data) {
-        triggerCallback(event, element, phase, data);
+        runInNextPostDigestOrNow(function() {
+          var callbacks = findCallbacks(element, event);
+          if (callbacks.length) {
+            // do not optimize this call here to RAF because
+            // we don't know how heavy the callback code here will
+            // be and if this code is buffered then this can
+            // lead to a performance regression.
+            $$rAF(function() {
+              forEach(callbacks, function(callback) {
+                callback(element, phase, data);
+              });
+            });
+          }
+        });
         runner.progress(event, phase, data);
       }
 
diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js
@@ -890,7 +890,6 @@ describe("animations", function() {
         });
 
         $rootScope.$digest();
-        $animate.flush();
         expect(capturedAnimation).toBeTruthy();
         expect(runner1done).toBeFalsy();
 
@@ -910,7 +909,6 @@ describe("animations", function() {
         });
 
         $rootScope.$digest();
-        $animate.flush();
         expect(capturedAnimation).toBeTruthy();
         expect(runner2done).toBeFalsy();
 
@@ -990,8 +988,6 @@ describe("animations", function() {
         var doneHandler = jasmine.createSpy('addClass done');
         runner.done(doneHandler);
 
-        $animate.flush();
-
         expect(doneHandler).not.toHaveBeenCalled();
 
         $animate.removeClass(element, 'active-class');
@@ -1011,8 +1007,6 @@ describe("animations", function() {
         var doneHandler = jasmine.createSpy('addClass done');
         runner.done(doneHandler);
 
-        $animate.flush();
-
         expect(doneHandler).not.toHaveBeenCalled();
 
         $animate.addClass(element, 'active-class');
@@ -1519,7 +1513,6 @@ describe("animations", function() {
       element = jqLite('<div></div>');
       $animate.enter(element, $rootElement);
       $rootScope.$digest();
-      $animate.flush();
 
       expect(callbackTriggered).toBe(false);
     }));
@@ -1707,6 +1700,69 @@ describe("animations", function() {
       expect(count).toBe(1);
     }));
 
+    it('should always detect registered callbacks after one postDigest has fired',
+      inject(function($animate, $rootScope, $rootElement) {
+
+      element = jqLite('<div></div>');
+
+      var spy = jasmine.createSpy();
+      registerCallback();
+
+      var runner = $animate.enter(element, $rootElement);
+      registerCallback();
+
+      $rootScope.$digest();
+      registerCallback();
+
+      expect(spy.callCount).toBe(0);
+      $animate.flush();
+
+      // this is not 3 since the 3rd callback
+      // was added after the first callback
+      // was fired
+      expect(spy.callCount).toBe(2);
+
+      spy.reset();
+      runner.end();
+
+      $animate.flush();
+
+      // now we expect all three callbacks
+      // to fire when the animation ends since
+      // the callback detection happens again
+      expect(spy.callCount).toBe(3);
+
+      function registerCallback() {
+        $animate.on('enter', element, spy);
+      }
+    }));
+
+    it('should use RAF if there are detected callbacks within the hierachy of the element being animated',
+      inject(function($animate, $rootScope, $rootElement, $$rAF) {
+
+      var runner;
+
+      element = jqLite('<div></div>');
+      runner = $animate.enter(element, $rootElement);
+      $rootScope.$digest();
+      runner.end();
+
+      assertRAFsUsed(false);
+
+      var spy = jasmine.createSpy();
+      $animate.on('leave', element, spy);
+
+      runner = $animate.leave(element, $rootElement);
+      $rootScope.$digest();
+      runner.end();
+
+      assertRAFsUsed(true);
+
+      function assertRAFsUsed(bool) {
+        expect($$rAF.queue.length)[bool ? 'toBeGreaterThan' : 'toBe'](0);
+      }
+    }));
+
     it('leave: should remove the element even if another animation is called after',
       inject(function($animate, $rootScope, $rootElement) {
 
diff --git a/test/ngAnimate/integrationSpec.js b/test/ngAnimate/integrationSpec.js
@@ -319,6 +319,38 @@ describe('ngAnimate integration tests', function() {
         }
       });
     });
+
+    it('should trigger callbacks at the start and end of an animation',
+      inject(function($rootScope, $rootElement, $animate, $compile) {
+
+      ss.addRule('.animate-me', 'transition:2s linear all;');
+
+      var parent = jqLite('<div><div ng-if="exp" class="animate-me"></div></div>');
+      element = parent.find('div');
+      html(parent);
+
+      $compile(parent)($rootScope);
+      $rootScope.$digest();
+
+      var spy = jasmine.createSpy();
+      $animate.on('enter', parent, spy);
+
+      $rootScope.exp = true;
+      $rootScope.$digest();
+
+      element = parent.find('div');
+
+      $animate.flush();
+
+      expect(spy.callCount).toBe(1);
+
+      browserTrigger(element, 'transitionend', { timeStamp: Date.now(), elapsedTime: 2 });
+      $animate.flush();
+
+      expect(spy.callCount).toBe(2);
+
+      dealoc(element);
+    }));
   });
 
   describe('JS animations', function() {
