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

Commit 7a81e6f

Browse files
matskoNarretz
authored andcommittedDec 2, 2015
fix(ngAnimate): do not alter the provided options data
Prior to this fix the provided options object would be altered as the animation kicks off due to the underlying mechanics of ngAnimate. This patch ensures that a copy of the provided options is used instead. This patch also works for when `$animateCss` is used by itself. Fixes #13040 Closes #13175
1 parent 9c49eb1 commit 7a81e6f

10 files changed

+163
-7
lines changed
 

‎src/ng/animateCss.js

+7-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,13 @@
1313
*/
1414
var $CoreAnimateCssProvider = function() {
1515
this.$get = ['$$rAF', '$q', '$$AnimateRunner', function($$rAF, $q, $$AnimateRunner) {
16-
return function(element, options) {
16+
17+
return function(element, initialOptions) {
18+
// we always make a copy of the options since
19+
// there should never be any side effects on
20+
// the input data when running `$animateCss`.
21+
var options = copy(initialOptions);
22+
1723
// there is no point in applying the styles since
1824
// there is no animation that goes on at all in
1925
// this version of $animateCss.

‎src/ngAnimate/.jshintrc

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
"angular": false,
88
"noop": false,
99

10+
"copy": false,
1011
"forEach": false,
1112
"extend": false,
1213
"jqLite": false,

‎src/ngAnimate/animateCss.js

+6-1
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,12 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
446446
return timings;
447447
}
448448

449-
return function init(element, options) {
449+
return function init(element, initialOptions) {
450+
// we always make a copy of the options since
451+
// there should never be any side effects on
452+
// the input data when running `$animateCss`.
453+
var options = copy(initialOptions);
454+
450455
var restoreStyles = {};
451456
var node = getDomNode(element);
452457
if (!node

‎src/ngAnimate/animateQueue.js

+6-1
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,12 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
238238
}
239239
};
240240

241-
function queueAnimation(element, event, options) {
241+
function queueAnimation(element, event, initialOptions) {
242+
// we always make a copy of the options since
243+
// there should never be any side effects on
244+
// the input data when running `$animateCss`.
245+
var options = copy(initialOptions);
246+
242247
var node, parent;
243248
element = stripCommentsFromElement(element);
244249
if (element) {

‎src/ngAnimate/shared.js

+1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
/* jshint ignore:start */
44
var noop = angular.noop;
5+
var copy = angular.copy;
56
var extend = angular.extend;
67
var jqLite = angular.element;
78
var forEach = angular.forEach;

‎test/ng/animateCssSpec.js

+15
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,21 @@ describe("$animateCss", function() {
1616

1717
describe("without animation", function() {
1818

19+
it("should not alter the provided options input in any way", inject(function($animateCss) {
20+
var initialOptions = {
21+
from: { height: '50px' },
22+
to: { width: '50px' },
23+
addClass: 'one',
24+
removeClass: 'two'
25+
};
26+
27+
var copiedOptions = copy(initialOptions);
28+
29+
expect(copiedOptions).toEqual(initialOptions);
30+
$animateCss(element, copiedOptions).start();
31+
expect(copiedOptions).toEqual(initialOptions);
32+
}));
33+
1934
it("should apply the provided [from] CSS to the element", inject(function($animateCss) {
2035
$animateCss(element, { from: { height: '50px' }}).start();
2136
expect(element.css('height')).toBe('50px');

‎test/ng/animateSpec.js

+21
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,27 @@ describe("$animate", function() {
378378
});
379379
});
380380

381+
it("should not alter the provided options input in any way throughout the animation", inject(function($animate, $rootElement, $rootScope) {
382+
var element = jqLite('<div></div>');
383+
var parent = $rootElement;
384+
385+
var initialOptions = {
386+
from: { height: '50px' },
387+
to: { width: '50px' },
388+
addClass: 'one',
389+
removeClass: 'two'
390+
};
391+
392+
var copiedOptions = copy(initialOptions);
393+
expect(copiedOptions).toEqual(initialOptions);
394+
395+
var runner = $animate.enter(element, parent, null, copiedOptions);
396+
expect(copiedOptions).toEqual(initialOptions);
397+
398+
$rootScope.$digest();
399+
expect(copiedOptions).toEqual(initialOptions);
400+
}));
401+
381402
describe('CSS class DOM manipulation', function() {
382403
var element;
383404
var addClass;

‎test/ngAnimate/animateCssSpec.js

+31
Original file line numberDiff line numberDiff line change
@@ -1865,6 +1865,37 @@ describe("ngAnimate $animateCss", function() {
18651865
};
18661866
}));
18671867

1868+
it("should not alter the provided options input in any way throughout the animation", inject(function($animateCss) {
1869+
var initialOptions = {
1870+
from: { height: '50px' },
1871+
to: { width: '50px' },
1872+
addClass: 'one',
1873+
removeClass: 'two',
1874+
duration: 10,
1875+
delay: 10,
1876+
structural: true,
1877+
keyframeStyle: '1s rotate',
1878+
transitionStyle: '1s linear',
1879+
stagger: 0.5,
1880+
staggerIndex: 3
1881+
};
1882+
1883+
var copiedOptions = copy(initialOptions);
1884+
expect(copiedOptions).toEqual(initialOptions);
1885+
1886+
var animator = $animateCss(element, copiedOptions);
1887+
expect(copiedOptions).toEqual(initialOptions);
1888+
1889+
var runner = animator.start();
1890+
expect(copiedOptions).toEqual(initialOptions);
1891+
1892+
triggerAnimationStartFrame();
1893+
expect(copiedOptions).toEqual(initialOptions);
1894+
1895+
runner.end();
1896+
expect(copiedOptions).toEqual(initialOptions);
1897+
}));
1898+
18681899
describe("[$$skipPreparationClasses]", function() {
18691900
it('should not apply and remove the preparation classes to the element when true',
18701901
inject(function($animateCss) {

‎test/ngAnimate/animateSpec.js

+30-4
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,24 @@ describe("animations", function() {
130130
};
131131
}));
132132

133+
it("should not alter the provided options input in any way throughout the animation", inject(function($animate, $rootScope) {
134+
var initialOptions = {
135+
from: { height: '50px' },
136+
to: { width: '50px' },
137+
addClass: 'one',
138+
removeClass: 'two'
139+
};
140+
141+
var copiedOptions = copy(initialOptions);
142+
expect(copiedOptions).toEqual(initialOptions);
143+
144+
var runner = $animate.enter(element, parent, null, copiedOptions);
145+
expect(copiedOptions).toEqual(initialOptions);
146+
147+
$rootScope.$digest();
148+
expect(copiedOptions).toEqual(initialOptions);
149+
}));
150+
133151
it('should animate only the specified CSS className matched within $animateProvider.classNameFilter', function() {
134152
module(function($animateProvider) {
135153
$animateProvider.classNameFilter(/only-allow-this-animation/);
@@ -149,32 +167,40 @@ describe("animations", function() {
149167
});
150168
});
151169

152-
they('should nullify both options.$prop when passed into an animation if it is not a string or an array', ['addClass', 'removeClass'], function(prop) {
170+
they('should not apply the provided options.$prop value unless it\'s a string or string-based array', ['addClass', 'removeClass'], function(prop) {
153171
inject(function($animate, $rootScope) {
172+
var startingCssClasses = element.attr('class') || '';
173+
154174
var options1 = {};
155175
options1[prop] = function() {};
156176
$animate.enter(element, parent, null, options1);
157177

158-
expect(options1[prop]).toBeFalsy();
178+
expect(element.attr('class')).toEqual(startingCssClasses);
179+
159180
$rootScope.$digest();
160181

161182
var options2 = {};
162183
options2[prop] = true;
163184
$animate.leave(element, options2);
164185

165-
expect(options2[prop]).toBeFalsy();
186+
expect(element.attr('class')).toEqual(startingCssClasses);
187+
166188
$rootScope.$digest();
167189

168190
capturedAnimation = null;
169191

170192
var options3 = {};
171193
if (prop === 'removeClass') {
172194
element.addClass('fatias');
195+
startingCssClasses = element.attr('class');
173196
}
174197

175198
options3[prop] = ['fatias'];
176199
$animate.enter(element, parent, null, options3);
177-
expect(options3[prop]).toBe('fatias');
200+
201+
$rootScope.$digest();
202+
203+
expect(element.attr('class')).not.toEqual(startingCssClasses);
178204
});
179205
});
180206

‎test/ngAnimate/integrationSpec.js

+45
Original file line numberDiff line numberDiff line change
@@ -562,5 +562,50 @@ describe('ngAnimate integration tests', function() {
562562
}
563563
});
564564
});
565+
566+
it("should not alter the provided options values in anyway throughout the animation", function() {
567+
var animationSpy = jasmine.createSpy();
568+
module(function($animateProvider) {
569+
$animateProvider.register('.this-animation', function() {
570+
return {
571+
enter: function(element, done) {
572+
animationSpy();
573+
done();
574+
}
575+
};
576+
});
577+
});
578+
579+
inject(function($animate, $rootScope, $compile) {
580+
element = jqLite('<div class="parent-man"></div>');
581+
var child = jqLite('<div class="child-man one"></div>');
582+
583+
var initialOptions = {
584+
from: { height: '50px' },
585+
to: { width: '100px' },
586+
addClass: 'one',
587+
removeClass: 'two'
588+
};
589+
590+
var copiedOptions = copy(initialOptions);
591+
expect(copiedOptions).toEqual(initialOptions);
592+
593+
html(element);
594+
$compile(element)($rootScope);
595+
596+
$animate.enter(child, element, null, copiedOptions);
597+
$rootScope.$digest();
598+
expect(copiedOptions).toEqual(initialOptions);
599+
600+
$animate.flush();
601+
expect(copiedOptions).toEqual(initialOptions);
602+
603+
expect(child).toHaveClass('one');
604+
expect(child).not.toHaveClass('two');
605+
606+
expect(child.attr('style')).toContain('100px');
607+
expect(child.attr('style')).toContain('50px');
608+
});
609+
});
565610
});
566611
});

0 commit comments

Comments
 (0)
This repository has been archived.