Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Fix options copy #13722

Merged
merged 2 commits into from
Jan 11, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/ng/animateCss.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 8 additions & 6 deletions src/ngAnimate/animateCss.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
5 changes: 4 additions & 1 deletion src/ngMock/angular-mocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
22 changes: 22 additions & 0 deletions test/ng/animateCssSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
22 changes: 22 additions & 0 deletions test/ngAnimate/animateCssSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
47 changes: 47 additions & 0 deletions test/ngAnimate/integrationSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Expand Down Expand Up @@ -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('<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');
});
});
});

describe('JS animations', function() {
Expand Down
24 changes: 24 additions & 0 deletions test/ngMock/angular-mocksSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2230,12 +2230,36 @@ 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) {

if (!browserSupportsCssAnimations()) return;

var element = jqLite('<div></div>');
$rootElement.append(element);

// Make sure the animation has valid $animateCss options
$$animation(element, null, {
from: { background: 'red' },
to: { background: 'blue' },
duration: 1,
transitionStyle: 'all 1s'
});

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');

}));
});
});
Expand Down