From a801df719ea8b5996676d4e7a88a26a5ece471e7 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Sat, 9 Jan 2016 13:32:57 +0100 Subject: [PATCH 1/2] fix(ngMock): ignore empty javascript animations in $animate.closeAndFlush() --- src/ngMock/angular-mocks.js | 5 ++++- test/ngMock/angular-mocksSpec.js | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 3910cca15baa..6a391832c638 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -797,7 +797,10 @@ angular.mock.animate = angular.module('ngAnimateMock', ['ng']) var animateJsConstructor = function() { var animator = $delegate.apply($delegate, arguments); - runners.push(animator); + // If no javascript animation is found, animator is undefined + if (animator) { + runners.push(animator); + } return animator; }; diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index 818d33e3003a..cdee629c6d40 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -2230,12 +2230,34 @@ describe('ngMockE2E', function() { expect(animationLog).toEqual(['start leave', 'end leave']); })); + it('should not throw when a regular animation has no javascript animation', + inject(function($animate, $$animation, $rootElement) { + + var element = jqLite('
'); + $rootElement.append(element); + + // Make sure the animation has valid $animateCss options + $$animation(element, null, { + from: { background: 'red' }, + to: { background: 'blue' }, + duration: 1, + transitionStyle: '1s linear all' + }); + + expect(function() { + $animate.closeAndFlush(); + }).not.toThrow(); + + dealoc(element); + })); + it('should throw an error if there are no animations to close and flush', inject(function($animate) { expect(function() { $animate.closeAndFlush(); }).toThrow('No pending animations ready to be closed or flushed'); + })); }); }); From d4fa3313088a03d15ccbf266583d6ecaa0d22241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 17 Dec 2015 16:53:07 -0800 Subject: [PATCH 2/2] 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 #13722 Closes #13578 --- src/ng/animateCss.js | 12 +++++--- src/ngAnimate/animateCss.js | 14 +++++---- test/ng/animateCssSpec.js | 22 +++++++++++++++ test/ngAnimate/animateCssSpec.js | 22 +++++++++++++++ test/ngAnimate/integrationSpec.js | 47 +++++++++++++++++++++++++++++++ test/ngMock/angular-mocksSpec.js | 4 ++- 6 files changed, 110 insertions(+), 11 deletions(-) diff --git a/src/ng/animateCss.js b/src/ng/animateCss.js index 552e51095ebf..2e133167a6c3 100644 --- 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 index 278814b1f744..b9757f27777c 100644 --- 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 index f0a027805cfa..dcc37b7fc9c9 100644 --- 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 index c8939d60252f..7de34ee58615 100644 --- a/test/ngAnimate/animateCssSpec.js +++ b/test/ngAnimate/animateCssSpec.js @@ -2036,6 +2036,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 index e72593d1cbb2..f3758250a00a 100644 --- 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('
'); + 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) { @@ -426,6 +447,32 @@ describe('ngAnimate integration tests', function() { expect(element).not.toHaveClass('hide'); })); + 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('
'); + + 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'); + }); + }); }); describe('JS animations', function() { diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index cdee629c6d40..b7d5fa9441b9 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -2233,6 +2233,8 @@ describe('ngMockE2E', function() { it('should not throw when a regular animation has no javascript animation', inject(function($animate, $$animation, $rootElement) { + if (!browserSupportsCssAnimations()) return; + var element = jqLite('
'); $rootElement.append(element); @@ -2241,7 +2243,7 @@ describe('ngMockE2E', function() { from: { background: 'red' }, to: { background: 'blue' }, duration: 1, - transitionStyle: '1s linear all' + transitionStyle: 'all 1s' }); expect(function() {