fix(ngAnimate): only copy over the animation options once

A bug in material has exposed that ngAnimate makes a copy of the provided animation options twice. By making two copies, the same DOM operations are performed during and at the end of the animation. If the CSS classes being added/ removed contain existing transition code, then this will lead to rendering issues. Closes angular#13722 Closes angular#13578
Narretz · Jan 11, 2016 · e08fb08 · e08fb08
1 parent ee22b57
commit e08fb08
Show file tree

Hide file tree

Showing 6 changed files with 143 additions and 45 deletions.
diff --git a/src/ng/animateCss.js b/src/ng/animateCss.js
@@ -15,10 +15,14 @@ var $CoreAnimateCssProvider = function() {
   this.$get = ['$$rAF', '$q', '$$AnimateRunner', function($$rAF, $q, $$AnimateRunner) {
 
     return function(element, initialOptions) {
-      // we always make a copy of the options since
-      // there should never be any side effects on
-      // the input data when running `$animateCss`.
-      var options = copy(initialOptions);
+      // all of the animation functions should create
+      // a copy of the options data, however, if a
+      // parent service has already created a copy then
+      // we should stick to using that
+      var options = initialOptions || {};
+      if (!options.$$prepared) {
+        options = copy(options);
+      }
 
       // there is no point in applying the styles since
       // there is no animation that goes on at all in

diff --git a/src/ngAnimate/animateCss.js b/src/ngAnimate/animateCss.js
@@ -447,10 +447,14 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
     }
 
     return function init(element, initialOptions) {
-      // we always make a copy of the options since
-      // there should never be any side effects on
-      // the input data when running `$animateCss`.
-      var options = copy(initialOptions);
+      // all of the animation functions should create
+      // a copy of the options data, however, if a
+      // parent service has already created a copy then
+      // we should stick to using that
+      var options = initialOptions || {};
+      if (!options.$$prepared) {
+        options = prepareAnimationOptions(copy(options));
+      }
 
       var restoreStyles = {};
       var node = getDomNode(element);
@@ -460,8 +464,6 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
         return closeAndReturnNoopAnimator();
       }
 
-      options = prepareAnimationOptions(options);
-
       var temporaryStyles = [];
       var classes = element.attr('class');
       var styles = packageStyles(options);

diff --git a/test/ng/animateCssSpec.js b/test/ng/animateCssSpec.js
@@ -31,6 +31,28 @@ describe("$animateCss", function() {
       expect(copiedOptions).toEqual(initialOptions);
     }));
 
+    it("should not create a copy of the provided options if they have already been prepared earlier",
+      inject(function($animateCss, $$rAF) {
+
+      var options = {
+        from: { height: '50px' },
+        to: { width: '50px' },
+        addClass: 'one',
+        removeClass: 'two'
+      };
+
+      options.$$prepared = true;
+      var runner = $animateCss(element, options).start();
+      runner.end();
+
+      $$rAF.flush();
+
+      expect(options.addClass).toBeFalsy();
+      expect(options.removeClass).toBeFalsy();
+      expect(options.to).toBeFalsy();
+      expect(options.from).toBeFalsy();
+    }));
+
     it("should apply the provided [from] CSS to the element", inject(function($animateCss) {
       $animateCss(element, { from: { height: '50px' }}).start();
       expect(element.css('height')).toBe('50px');

diff --git a/test/ngAnimate/animateCssSpec.js b/test/ngAnimate/animateCssSpec.js
@@ -2028,6 +2028,28 @@ describe("ngAnimate $animateCss", function() {
         expect(copiedOptions).toEqual(initialOptions);
       }));
 
+      it("should not create a copy of the provided options if they have already been prepared earlier",
+        inject(function($animate, $animateCss) {
+
+        var options = {
+          from: { height: '50px' },
+          to: { width: '50px' },
+          addClass: 'one',
+          removeClass: 'two'
+        };
+
+        options.$$prepared = true;
+        var runner = $animateCss(element, options).start();
+        runner.end();
+
+        $animate.flush();
+
+        expect(options.addClass).toBeFalsy();
+        expect(options.removeClass).toBeFalsy();
+        expect(options.to).toBeFalsy();
+        expect(options.from).toBeFalsy();
+      }));
+
       describe("[$$skipPreparationClasses]", function() {
         it('should not apply and remove the preparation classes to the element when true',
           inject(function($animateCss) {

diff --git a/test/ngAnimate/integrationSpec.js b/test/ngAnimate/integrationSpec.js
@@ -28,6 +28,27 @@ describe('ngAnimate integration tests', function() {
   describe('CSS animations', function() {
     if (!browserSupportsCssAnimations()) return;
 
+    it("should only create a single copy of the provided animation options",
+      inject(function($rootScope, $rootElement, $animate) {
+
+      ss.addRule('.animate-me', 'transition:2s linear all;');
+
+      var element = jqLite('<div class="animate-me"></div>');
+      html(element);
+
+      var myOptions = {to: { 'color': 'red' }};
+
+      var spy = spyOn(window, 'copy');
+      expect(spy).not.toHaveBeenCalled();
+
+      var animation = $animate.leave(element, myOptions);
+      $rootScope.$digest();
+      $animate.flush();
+
+      expect(spy).toHaveBeenCalledOnce();
+      dealoc(element);
+    }));
+
     they('should render an $prop animation',
       ['enter', 'leave', 'move', 'addClass', 'removeClass', 'setClass'], function(event) {
 
@@ -637,50 +658,77 @@ describe('ngAnimate integration tests', function() {
         }
       });
     });
+  });
 
-    it("should not alter the provided options values in anyway throughout the animation", function() {
-      var animationSpy = jasmine.createSpy();
-      module(function($animateProvider) {
-        $animateProvider.register('.this-animation', function() {
-          return {
-            enter: function(element, done) {
-              animationSpy();
-              done();
-            }
-          };
-        });
+  it("should not alter the provided options values in anyway throughout the animation", function() {
+    var animationSpy = jasmine.createSpy();
+    module(function($animateProvider) {
+      $animateProvider.register('.this-animation', function() {
+        return {
+          enter: function(element, done) {
+            animationSpy();
+            done();
+          }
+        };
       });
+    });
 
-      inject(function($animate, $rootScope, $compile) {
-        element = jqLite('<div class="parent-man"></div>');
-        var child = jqLite('<div class="child-man one"></div>');
+    inject(function($animate, $rootScope, $compile) {
+      element = jqLite('<div class="parent-man"></div>');
+      var child = jqLite('<div class="child-man one"></div>');
 
-        var initialOptions = {
-          from: { height: '50px' },
-          to: { width: '100px' },
-          addClass: 'one',
-          removeClass: 'two'
-        };
+      var initialOptions = {
+        from: { height: '50px' },
+        to: { width: '100px' },
+        addClass: 'one',
+        removeClass: 'two'
+      };
 
-        var copiedOptions = copy(initialOptions);
-        expect(copiedOptions).toEqual(initialOptions);
+      var copiedOptions = copy(initialOptions);
+      expect(copiedOptions).toEqual(initialOptions);
 
-        html(element);
-        $compile(element)($rootScope);
+      html(element);
+      $compile(element)($rootScope);
 
-        $animate.enter(child, element, null, copiedOptions);
-        $rootScope.$digest();
-        expect(copiedOptions).toEqual(initialOptions);
+      $animate.enter(child, element, null, copiedOptions);
+      $rootScope.$digest();
+      expect(copiedOptions).toEqual(initialOptions);
 
-        $animate.flush();
-        expect(copiedOptions).toEqual(initialOptions);
+      $animate.flush();
+      expect(copiedOptions).toEqual(initialOptions);
 
-        expect(child).toHaveClass('one');
-        expect(child).not.toHaveClass('two');
+      expect(child).toHaveClass('one');
+      expect(child).not.toHaveClass('two');
 
-        expect(child.attr('style')).toContain('100px');
-        expect(child.attr('style')).toContain('50px');
-      });
+      expect(child.attr('style')).toContain('100px');
+      expect(child.attr('style')).toContain('50px');
+    });
+  });
+
+  it('should handle ng-if & ng-class with a class that is removed before its add animation has concluded', function() {
+    inject(function($animate, $rootScope, $compile, $timeout, $$rAF) {
+
+      ss.addRule('.animate-me', 'transition: all 0.5s;');
+
+      element = jqLite('<section><div ng-if="true" class="animate-me" ng-class="{' +
+        'red: red,' +
+        'blue: blue' +
+        '}"></div></section>');
+
+      html(element);
+      $rootScope.blue = true;
+      $rootScope.red = true;
+      $compile(element)($rootScope);
+      $rootScope.$digest();
+
+      var child = element.find('div');
+
+      // Trigger class removal before the add animation has been concluded
+      $rootScope.blue = false;
+      $animate.closeAndFlush();
+
+      expect(child).toHaveClass('red');
+      expect(child).not.toHaveClass('blue');
     });
   });
 });
diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js
@@ -2159,7 +2159,7 @@ describe('ngMockE2E', function() {
           from: { background: 'red' },
           to: { background: 'blue' },
           duration: 1,
-          transitionStyle: '1s linear all'
+          transitionStyle: 'all 1s'
         });
 
         expect(function() {