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

Commit a2385bc

Browse files
matskomatsko
committed
fix($animate): ensure add/remove classes do not cancel each other out
1 parent fb0f923 commit a2385bc

File tree

8 files changed

+144
-103
lines changed

8 files changed

+144
-103
lines changed

‎src/AngularPublic.js

-2
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@
5959
$AnimateProvider,
6060
$$CoreAnimateQueueProvider,
6161
$$CoreAnimateRunnerProvider,
62-
$$AnimateClassResolverProvider,
6362
$BrowserProvider,
6463
$CacheFactoryProvider,
6564
$ControllerProvider,
@@ -222,7 +221,6 @@ function publishExternalAPI(angular) {
222221
$animate: $AnimateProvider,
223222
$$animateQueue: $$CoreAnimateQueueProvider,
224223
$$AnimateRunner: $$CoreAnimateRunnerProvider,
225-
$$animateClassResolver: $$AnimateClassResolverProvider,
226224
$browser: $BrowserProvider,
227225
$cacheFactory: $CacheFactoryProvider,
228226
$controller: $ControllerProvider,

‎src/ng/animate.js

+41-74
Original file line numberDiff line numberDiff line change
@@ -21,67 +21,21 @@ function extractElementNode(element) {
2121
}
2222
}
2323

24-
var $$AnimateClassResolverProvider = function() {
25-
var ADD_CLASS = 1;
26-
var REMOVE_CLASS = -1;
27-
28-
this.$get = function() {
29-
return function(existing, toAdd, toRemove) {
30-
var flags = {};
31-
existing = splitClasses(existing);
32-
33-
toAdd = splitClasses(toAdd);
34-
forEach(toAdd, function(value, key) {
35-
flags[key] = ADD_CLASS;
36-
});
37-
38-
toRemove = splitClasses(toRemove);
39-
forEach(toRemove, function(value, key) {
40-
flags[key] = flags[key] === ADD_CLASS ? null : REMOVE_CLASS;
41-
});
42-
43-
var classes = {
44-
addClass: '',
45-
removeClass: ''
46-
};
47-
48-
forEach(flags, function(val, klass) {
49-
var prop, allow;
50-
if (val === ADD_CLASS) {
51-
prop = 'addClass';
52-
allow = !existing[klass];
53-
} else if (val === REMOVE_CLASS) {
54-
prop = 'removeClass';
55-
allow = existing[klass];
56-
}
57-
if (allow) {
58-
if (classes[prop].length) {
59-
classes[prop] += ' ';
60-
}
61-
classes[prop] += klass;
62-
}
63-
});
64-
65-
return classes;
66-
};
67-
};
24+
function splitClasses(classes) {
25+
if (isString(classes)) {
26+
classes = classes.split(' ');
27+
}
6828

69-
function splitClasses(classes) {
70-
if (isString(classes)) {
71-
classes = classes.split(' ');
29+
var obj = {};
30+
forEach(classes, function(klass) {
31+
// sometimes the split leaves empty string values
32+
// incase extra spaces were applied to the options
33+
if (klass.length) {
34+
obj[klass] = true;
7235
}
73-
74-
var obj = {};
75-
forEach(classes, function(klass) {
76-
// sometimes the split leaves empty string values
77-
// incase extra spaces were applied to the options
78-
if (klass.length) {
79-
obj[klass] = true;
80-
}
81-
});
82-
return obj;
83-
}
84-
};
36+
});
37+
return obj;
38+
}
8539

8640
var $$CoreAnimateRunnerProvider = function() {
8741
this.$get = ['$q', '$$rAF', function($q, $$rAF) {
@@ -112,8 +66,8 @@ var $$CoreAnimateQueueProvider = function() {
11266
var postDigestQueue = new HashMap();
11367
var postDigestElements = [];
11468

115-
this.$get = ['$$AnimateRunner', '$rootScope', '$$animateClassResolver',
116-
function($$AnimateRunner, $rootScope, $$animateClassResolver) {
69+
this.$get = ['$$AnimateRunner', '$rootScope',
70+
function($$AnimateRunner, $rootScope) {
11771
return {
11872
enabled: noop,
11973
on: noop,
@@ -144,17 +98,19 @@ var $$CoreAnimateQueueProvider = function() {
14498
}
14599

146100
if (add) {
147-
classVal = add.split(' ');
148-
data.addClass = data.addClass
149-
? data.addClass.concat(classVal)
150-
: classVal;
101+
forEach(add.split(' '), function(className) {
102+
if (className) {
103+
data[className] = true;
104+
}
105+
});
151106
}
152107

153108
if (remove) {
154-
classVal = remove.split(' ');
155-
data.removeClass = data.removeClass
156-
? data.removeClass.concat(classVal)
157-
: classVal;
109+
forEach(remove.split(' '), function(className) {
110+
if (className) {
111+
data[className] = false;
112+
}
113+
});
158114
}
159115

160116
if (postDigestElements.length > 1) return;
@@ -163,12 +119,23 @@ var $$CoreAnimateQueueProvider = function() {
163119
forEach(postDigestElements, function(element) {
164120
var data = postDigestQueue.get(element);
165121
if (data) {
166-
var classes = $$animateClassResolver(element.attr('class'),
167-
data.addClass,
168-
data.removeClass);
122+
var existing = splitClasses(element.attr('class'));
123+
var toAdd = '';
124+
var toRemove = '';
125+
forEach(data, function(status, className) {
126+
var hasClass = !!existing[className];
127+
if (status !== hasClass) {
128+
if (status) {
129+
toAdd += (toAdd.length ? ' ' : '') + className;
130+
} else {
131+
toRemove += (toRemove.length ? ' ' : '') + className;
132+
}
133+
}
134+
});
135+
169136
forEach(element, function(elm) {
170-
classes.addClass && jqLiteAddClass(elm, classes.addClass);
171-
classes.removeClass && jqLiteRemoveClass(elm, classes.removeClass);
137+
toAdd && jqLiteAddClass(elm, toAdd);
138+
toRemove && jqLiteRemoveClass(elm, toRemove);
172139
});
173140
postDigestQueue.remove(element);
174141
}

‎src/ngAnimate/.jshintrc

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424

2525
"isPromiseLike": false,
2626
"mergeClasses": false,
27-
"mergeAnimationOptionsFactory": false,
27+
"mergeAnimationOptions": false,
2828
"prepareAnimationOptions": false,
2929
"applyAnimationStyles": false,
3030
"applyAnimationFromStyles": false,

‎src/ngAnimate/animateQueue.js

+2-3
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
5757
return currentAnimation.state === RUNNING_STATE && newAnimation.structural;
5858
});
5959

60-
this.$get = ['$$rAF', '$rootScope', '$rootElement', '$document', '$$HashMap', '$$animateClassResolver',
60+
this.$get = ['$$rAF', '$rootScope', '$rootElement', '$document', '$$HashMap',
6161
'$$animation', '$$AnimateRunner', '$templateRequest', '$$jqLite',
62-
function($$rAF, $rootScope, $rootElement, $document, $$HashMap, $$animateClassResolver,
62+
function($$rAF, $rootScope, $rootElement, $document, $$HashMap,
6363
$$animation, $$AnimateRunner, $templateRequest, $$jqLite) {
6464

6565
var activeAnimationsLookup = new $$HashMap();
@@ -109,7 +109,6 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {
109109
return classNameFilter.test(className);
110110
};
111111

112-
var mergeAnimationOptions = mergeAnimationOptionsFactory($$animateClassResolver);
113112
var applyAnimationClasses = applyAnimationClassesFactory($$jqLite);
114113

115114
function normalizeAnimationOptions(element, options) {

‎src/ngAnimate/shared.js

+74-20
Original file line numberDiff line numberDiff line change
@@ -149,28 +149,82 @@ function applyAnimationToStyles(element, options) {
149149
}
150150
}
151151

152-
function mergeAnimationOptionsFactory($$animateClassResolver) {
153-
return function(element, target, newOptions) {
154-
var toAdd = (target.addClass || '') + ' ' + (newOptions.addClass || '');
155-
var toRemove = (target.removeClass || '') + ' ' + (newOptions.removeClass || '');
156-
var classes = $$animateClassResolver(element.attr('class'), toAdd, toRemove);
157-
158-
extend(target, newOptions);
159-
160-
if (classes.addClass) {
161-
target.addClass = classes.addClass;
162-
} else {
163-
target.addClass = null;
152+
function mergeAnimationOptions(element, target, newOptions) {
153+
var toAdd = (target.addClass || '') + ' ' + (newOptions.addClass || '');
154+
var toRemove = (target.removeClass || '') + ' ' + (newOptions.removeClass || '');
155+
var classes = resolveElementClasses(element.attr('class'), toAdd, toRemove);
156+
157+
extend(target, newOptions);
158+
159+
if (classes.addClass) {
160+
target.addClass = classes.addClass;
161+
} else {
162+
target.addClass = null;
163+
}
164+
165+
if (classes.removeClass) {
166+
target.removeClass = classes.removeClass;
167+
} else {
168+
target.removeClass = null;
169+
}
170+
171+
return target;
172+
}
173+
174+
function resolveElementClasses(existing, toAdd, toRemove) {
175+
var ADD_CLASS = 1;
176+
var REMOVE_CLASS = -1;
177+
178+
var flags = {};
179+
existing = splitClassesToLookup(existing);
180+
181+
toAdd = splitClassesToLookup(toAdd);
182+
forEach(toAdd, function(value, key) {
183+
flags[key] = ADD_CLASS;
184+
});
185+
186+
toRemove = splitClassesToLookup(toRemove);
187+
forEach(toRemove, function(value, key) {
188+
flags[key] = flags[key] === ADD_CLASS ? null : REMOVE_CLASS;
189+
});
190+
191+
var classes = {
192+
addClass: '',
193+
removeClass: ''
194+
};
195+
196+
forEach(flags, function(val, klass) {
197+
var prop, allow;
198+
if (val === ADD_CLASS) {
199+
prop = 'addClass';
200+
allow = !existing[klass];
201+
} else if (val === REMOVE_CLASS) {
202+
prop = 'removeClass';
203+
allow = existing[klass];
164204
}
205+
if (allow) {
206+
if (classes[prop].length) {
207+
classes[prop] += ' ';
208+
}
209+
classes[prop] += klass;
210+
}
211+
});
165212

166-
if (classes.removeClass) {
167-
target.removeClass = classes.removeClass;
168-
} else {
169-
target.removeClass = null;
213+
function splitClassesToLookup(classes) {
214+
if (isString(classes)) {
215+
classes = classes.split(' ');
170216
}
171217

172-
return target;
173-
};
174-
}
218+
var obj = {};
219+
forEach(classes, function(klass) {
220+
// sometimes the split leaves empty string values
221+
// incase extra spaces were applied to the options
222+
if (klass.length) {
223+
obj[klass] = true;
224+
}
225+
});
226+
return obj;
227+
}
175228

176-
/* jshint ignore:end */
229+
return classes;
230+
}

‎test/ng/animateSpec.js

+24
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,30 @@ describe("$animate", function() {
177177
expect(style.color).toBe('green');
178178
expect(style.borderColor).toBe('purple');
179179
}));
180+
181+
it("should avoid cancelling out add/remove when the element already contains the class",
182+
inject(function($animate, $rootScope) {
183+
184+
var element = jqLite('<div class="ng-hide"></div>');
185+
186+
$animate.addClass(element, 'ng-hide');
187+
$animate.removeClass(element, 'ng-hide');
188+
$rootScope.$digest();
189+
190+
expect(element).not.toHaveClass('ng-hide');
191+
}));
192+
193+
it("should avoid cancelling out remove/add if the element does not contain the class",
194+
inject(function($animate, $rootScope) {
195+
196+
var element = jqLite('<div></div>');
197+
198+
$animate.removeClass(element, 'ng-hide');
199+
$animate.addClass(element, 'ng-hide');
200+
$rootScope.$digest();
201+
202+
expect(element).toHaveClass('ng-hide');
203+
}));
180204
});
181205

182206
describe('CSS class DOM manipulation', function() {

‎test/ngAnimate/.jshintrc

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"browser": true,
44
"newcap": false,
55
"globals": {
6-
"mergeAnimationOptionsFactory": false,
6+
"mergeAnimationOptions": false,
77
"prepareAnimationOptions": false,
88
"applyAnimationStyles": false,
99
"applyAnimationFromStyles": false,

‎test/ngAnimate/animationHelperFunctionsSpec.js

+1-2
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ describe("animation option helper functions", function() {
111111
});
112112

113113
describe('mergeAnimationOptions', function() {
114-
it('should merge in new options', inject(function($$animateClassResolver) {
114+
it('should merge in new options', inject(function() {
115115
element.attr('class', 'blue');
116116
var options = prepareAnimationOptions({
117117
name: 'matias',
@@ -120,7 +120,6 @@ describe("animation option helper functions", function() {
120120
removeClass: 'blue gold'
121121
});
122122

123-
var mergeAnimationOptions = mergeAnimationOptionsFactory($$animateClassResolver);
124123
mergeAnimationOptions(element, options, {
125124
age: 29,
126125
addClass: 'gold brown',

0 commit comments

Comments
 (0)