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

fix(ngAnimate): defer DOM operations for changing classes to postDigest #9283

Closed
wants to merge 9 commits into from
Closed

fix(ngAnimate): defer DOM operations for changing classes to postDigest #9283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c3ce5d4
test(matchers.js): make toHaveClass matcher work better for SVG+jQuery
caitp Sep 30, 2014
6233f0c
fix(ngAnimate): defer DOM operations for changing classes to postDigest
caitp Sep 25, 2014
43dcb36
WIP: Fixuppin
caitp Oct 1, 2014
a6ee572
WIP --- fixups
caitp Oct 8, 2014
3e2ad02
refactor($compile): use labaled variables to represent nodeType values
caitp Oct 8, 2014
bf84fd1
chore(ngAnimate): add TODO messages indicating desire to remove hack
caitp Oct 8, 2014
e954d22
perf($animate): don't join classes before it's necessary in resolveEl…
caitp Oct 8, 2014
cbf7d92
perf($animate): access DOM less in resolveElementClasses
caitp Oct 8, 2014
6949e5d
chore(AngularPublic): remove `$$hasClass` from angular exports
caitp Oct 8, 2014
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
7 changes: 7 additions & 0 deletions src/.jshintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@

"skipDestroyOnNextJQueryCleanData": true,

"NODE_TYPE_ELEMENT": false,
"NODE_TYPE_TEXT": false,
"NODE_TYPE_COMMENT": false,
"NODE_TYPE_COMMENT": false,
"NODE_TYPE_DOCUMENT": false,
"NODE_TYPE_DOCUMENT_FRAGMENT": false,

/* filters.js */
"getFirstThursdayOfYear": false,

Expand Down
18 changes: 14 additions & 4 deletions src/Angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@
getBlockNodes: true,
hasOwnProperty: true,
createMap: true,

NODE_TYPE_ELEMENT: true,
NODE_TYPE_TEXT: true,
NODE_TYPE_COMMENT: true,
NODE_TYPE_DOCUMENT: true,
NODE_TYPE_DOCUMENT_FRAGMENT: true,
*/

////////////////////////////////////
Expand Down Expand Up @@ -192,7 +198,7 @@ function isArrayLike(obj) {

var length = obj.length;

if (obj.nodeType === 1 && length) {
if (obj.nodeType === NODE_TYPE_ELEMENT && length) {
return true;
}

Expand Down Expand Up @@ -1028,11 +1034,9 @@ function startingTag(element) {
// are not allowed to have children. So we just ignore it.
element.empty();
} catch(e) {}
// As Per DOM Standards
var TEXT_NODE = 3;
var elemHtml = jqLite('<div>').append(element).html();
try {
return element[0].nodeType === TEXT_NODE ? lowercase(elemHtml) :
return element[0].nodeType === NODE_TYPE_TEXT ? lowercase(elemHtml) :
elemHtml.
match(/^(<[^>]+>)/)[1].
replace(/^<([\w\-]+)/, function(match, nodeName) { return '<' + lowercase(nodeName); });
Expand Down Expand Up @@ -1610,3 +1614,9 @@ function getBlockNodes(nodes) {
function createMap() {
return Object.create(null);
}

var NODE_TYPE_ELEMENT = 1;
var NODE_TYPE_TEXT = 3;
var NODE_TYPE_COMMENT = 8;
var NODE_TYPE_DOCUMENT = 9;
var NODE_TYPE_DOCUMENT_FRAGMENT = 11;
3 changes: 1 addition & 2 deletions src/AngularPublic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,7 @@ function publishExternalAPI(angular){
'getTestability': getTestability,
'$$minErr': minErr,
'$$csp': csp,
'reloadWithDebugInfo': reloadWithDebugInfo,
'$$hasClass': jqLiteHasClass
'reloadWithDebugInfo': reloadWithDebugInfo
});

angularModule = setupModuleLoader(window);
Expand Down
16 changes: 8 additions & 8 deletions src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ function jqLiteAcceptsData(node) {
// The window object can accept data but has no nodeType
// Otherwise we are only interested in elements (1) and documents (9)
var nodeType = node.nodeType;
return nodeType === 1 || !nodeType || nodeType === 9;
return nodeType === NODE_TYPE_ELEMENT || !nodeType || nodeType === NODE_TYPE_DOCUMENT;
}

function jqLiteBuildFragment(html, context) {
Expand Down Expand Up @@ -419,7 +419,7 @@ function jqLiteController(element, name) {
function jqLiteInheritedData(element, name, value) {
// if element is the document object work with the html element instead
// this makes $(document).scope() possible
if(element.nodeType == 9) {
if(element.nodeType == NODE_TYPE_DOCUMENT) {
element = element.documentElement;
}
var names = isArray(name) ? name : [name];
Expand All @@ -432,7 +432,7 @@ function jqLiteInheritedData(element, name, value) {
// If dealing with a document fragment node with a host element, and no parent, use the host
// element as the parent. This enables directives within a Shadow DOM or polyfilled Shadow DOM
// to lookup parent controllers.
element = element.parentNode || (element.nodeType === 11 && element.host);
element = element.parentNode || (element.nodeType === NODE_TYPE_DOCUMENT_FRAGMENT && element.host);
}
}

Expand Down Expand Up @@ -610,7 +610,7 @@ forEach({
function getText(element, value) {
if (isUndefined(value)) {
var nodeType = element.nodeType;
return (nodeType === 1 || nodeType === 3) ? element.textContent : '';
return (nodeType === NODE_TYPE_ELEMENT || nodeType === NODE_TYPE_TEXT) ? element.textContent : '';
}
element.textContent = value;
}
Expand Down Expand Up @@ -832,7 +832,7 @@ forEach({
children: function(element) {
var children = [];
forEach(element.childNodes, function(element){
if (element.nodeType === 1)
if (element.nodeType === NODE_TYPE_ELEMENT)
children.push(element);
});
return children;
Expand All @@ -844,7 +844,7 @@ forEach({

append: function(element, node) {
var nodeType = element.nodeType;
if (nodeType !== 1 && nodeType !== 11) return;
if (nodeType !== NODE_TYPE_ELEMENT && nodeType !== NODE_TYPE_DOCUMENT_FRAGMENT) return;

node = new JQLite(node);

Expand All @@ -855,7 +855,7 @@ forEach({
},

prepend: function(element, node) {
if (element.nodeType === 1) {
if (element.nodeType === NODE_TYPE_ELEMENT) {
var index = element.firstChild;
forEach(new JQLite(node), function(child){
element.insertBefore(child, index);
Expand Down Expand Up @@ -906,7 +906,7 @@ forEach({

parent: function(element) {
var parent = element.parentNode;
return parent && parent.nodeType !== 11 ? parent : null;
return parent && parent.nodeType !== NODE_TYPE_DOCUMENT_FRAGMENT ? parent : null;
},

next: function(element) {
Expand Down
112 changes: 106 additions & 6 deletions src/ng/animate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,57 @@ var $AnimateProvider = ['$provide', function($provide) {
return this.$$classNameFilter;
};

this.$get = ['$$q', '$$asyncCallback', function($$q, $$asyncCallback) {
this.$get = ['$$q', '$$asyncCallback', '$rootScope', function($$q, $$asyncCallback, $rootScope) {

var currentDefer;

function runAnimationPostDigest(fn) {
var cancelFn, defer = $$q.defer();
defer.promise.$$cancelFn = function ngAnimateMaybeCancel() {
cancelFn && cancelFn();
};

$rootScope.$$postDigest(function ngAnimatePostDigest() {
cancelFn = fn(function ngAnimateNotifyComplete() {
defer.resolve();
});
});

return defer.promise;
}

function resolveElementClasses(element, cache) {
var toAdd = [], toRemove = [];

var hasClasses = {};
forEach((element.attr('class') || '').replace(/\s+/g, ' ').split(' '), function(className) {
hasClasses[className] = true;
});

forEach(cache.classes, function(status, className) {
var hasClass = hasClasses[className] === true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=== true so that classes like toString or constructor don't cause problems

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use createMap instead and then you don't need this


// If the most recent class manipulation (via $animate) was to remove the class, and the
// element currently has the class, the class is scheduled for removal. Otherwise, if
// the most recent class manipulation (via $animate) was to add the class, and the
// element does not currently have the class, the class is scheduled to be added.
if (status === false && hasClass) {
toRemove.push(className);
} else if (status === true && !hasClass) {
toAdd.push(className);
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correclty, the class will end u in the toAdd/toRemove arrays, based on the number of times a class was requested to be added/removed.
But shouldn't the order play a more important role here ?

E.g. if in the same $digest the following calls are made:

addClass(someElem, 'class1');
addClass(someElem, 'class1');
removeClass(someElem, 'class1');

I would expect that the class is not added (removed if present).
Yet, with the current implementation, the above calls will end up with a cache like this:

{
    add: ['class1', 'class1'],
    remove: ['class1']
}

which will lead to a map like this: {class1: 1} (thus adding/not removing the class).

Is this the intended behaviour @caitp ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

during digest, that won't happen (it would not appear in either add or remove)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caitp: Sorry, but I didn't get it. What would not happen ? Having classes in both add and remove arrays ? Or having the same class multiple times in the same array ?
Couldn't there be multiple directives calling addClass/removeClass during the same $digest ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolveElementClasses() discards items which don't really change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caitp: It discards them, if the are "added" and "removed" the same number of times (i.e. if they appear in the add/remove arrays the same number of times).

My question is basically this: "Shouldn't order matter ?"
E.g. if something says "add that class" and then (later, but in the same $digest) something else says "remove that class", shouldn't we remove it ?
The point is that the order of calling addClass/removeClass is not "respected". Do you think order should matter or is it fine as it is ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is #8946 which changes the behavior in ngAnimate to exactly what @gkalpak proposed. @matsko is implementing it for rc5. so we should change this PR to do the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also add tests for this scenario that @gkalpak described

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reading #8946 and I'm not seeing how it's different to what is already implemented in ngAnimate, and what this PR implements --- resolveElementClasses() eliminates the no-op situations, is that not what we want? I implemented an alternative way of eliminating the no-ops previously, but decided to go with this one since it's already in ngAnimate

I am happy to add the extra tests though, although it's basically an alternative version of the tests which are added here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FWIW, since the current behaviour does not care about ordering, the test case proposed in that issue does not help much here.

I think it would be good to follow up on this and make sure resolveElementClasses() can respect ordering, but for the time being it doesn't.

I think fixing that is sort of out of scope for this PR, but I will look at taking that on once this is merged

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I posted a code snippet above with what I think this should look like. do you now see the diff?


return (toAdd.length + toRemove.length) > 0 && [toAdd.length && toAdd, toRemove.length && toRemove];
}

function cachedClassManipulation(cache, classes, op) {
for (var i=0, ii = classes.length; i < ii; ++i) {
var className = classes[i];
cache[className] = op;
}
}

function asyncPromise() {
// only serve one instance of a promise in order to save CPU cycles
if (!currentDefer) {
Expand Down Expand Up @@ -187,13 +235,17 @@ var $AnimateProvider = ['$provide', function($provide) {
* @return {Promise} the animation callback promise
*/
addClass : function(element, className) {
return this.setClass(element, className, []);
},

$$addClassImmediately : function addClassImmediately(element, className) {
element = jqLite(element);
className = !isString(className)
? (isArray(className) ? className.join(' ') : '')
: className;
forEach(element, function (element) {
jqLiteAddClass(element, className);
});
return asyncPromise();
},

/**
Expand All @@ -209,6 +261,11 @@ var $AnimateProvider = ['$provide', function($provide) {
* @return {Promise} the animation callback promise
*/
removeClass : function(element, className) {
return this.setClass(element, [], className);
},

$$removeClassImmediately : function removeClassImmediately(element, className) {
element = jqLite(element);
className = !isString(className)
? (isArray(className) ? className.join(' ') : '')
: className;
Expand All @@ -231,10 +288,53 @@ var $AnimateProvider = ['$provide', function($provide) {
* @param {string} remove the CSS class which will be removed from the element
* @return {Promise} the animation callback promise
*/
setClass : function(element, add, remove) {
this.addClass(element, add);
this.removeClass(element, remove);
return asyncPromise();
setClass : function(element, add, remove, runSynchronously) {
var self = this;
var STORAGE_KEY = '$$animateClasses';
var createdCache = false;
element = jqLite(element);

if (runSynchronously) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, ngClass has a test which fails if core ngAnimate doesn't work synchronously. I spent some time trying to fix the test, but I couldn't really decide how it should work.

This is bad because it's a definite difference in behaviour for one special case, so I think we'll want to fix this --- but maybe it can wait a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obviously, runSynchronously is not documented since I think we should get rid of it within a week or two, but I don't think it should block landing the CL

// TODO(@caitp/@matsko): Remove undocumented `runSynchronously` parameter, and always
// perform DOM manipulation asynchronously or in postDigest.
self.$$addClassImmediately(element, add);
self.$$removeClassImmediately(element, remove);
return asyncPromise();
}

var cache = element.data(STORAGE_KEY);
if (!cache) {
cache = {
classes: {}
};
createdCache = true;
}

var classes = cache.classes;

add = isArray(add) ? add : add.split(' ');
remove = isArray(remove) ? remove : remove.split(' ');
cachedClassManipulation(classes, add, true);
cachedClassManipulation(classes, remove, false);

if (createdCache) {
cache.promise = runAnimationPostDigest(function(done) {
var cache = element.data(STORAGE_KEY);
element.removeData(STORAGE_KEY);

var classes = cache && resolveElementClasses(element, cache);

if (classes) {
if (classes[0]) self.$$addClassImmediately(element, classes[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that you need this if. do you?

if (classes[1]) self.$$removeClassImmediately(element, classes[1]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it saves us a bit of work and it's nice for the tests because it shows that the spies don't get called when they aren't needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh. so you are testing if the string is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes --- now that we aren't joining strings in resolveElementClasses, it's checking that the length of the array is > 0 instead

}

done();
});
element.data(STORAGE_KEY, cache);
}

return cache.promise;
},

enabled : noop,
Expand Down
16 changes: 8 additions & 8 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1141,7 +1141,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
// We can not compile top level text elements since text nodes can be merged and we will
// not be able to attach scope data to them, so we will wrap them in <span>
forEach($compileNodes, function(node, index){
if (node.nodeType == 3 /* text node */ && node.nodeValue.match(/\S+/) /* non-empty */ ) {
if (node.nodeType == NODE_TYPE_TEXT && node.nodeValue.match(/\S+/) /* non-empty */ ) {
$compileNodes[index] = jqLite(node).wrap('<span></span>').parent()[0];
}
});
Expand Down Expand Up @@ -1344,7 +1344,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
className;

switch(nodeType) {
case 1: /* Element */
case NODE_TYPE_ELEMENT: /* Element */
// use the node name: <directive>
addDirective(directives,
directiveNormalize(nodeName_(node)), 'E', maxPriority, ignoreDirective);
Expand Down Expand Up @@ -1399,10 +1399,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
}
break;
case 3: /* Text Node */
case NODE_TYPE_TEXT: /* Text Node */
addTextInterpolateDirective(directives, node.nodeValue);
break;
case 8: /* Comment */
case NODE_TYPE_COMMENT: /* Comment */
try {
match = COMMENT_DIRECTIVE_REGEXP.exec(node.nodeValue);
if (match) {
Expand Down Expand Up @@ -1442,7 +1442,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
"Unterminated attribute, found '{0}' but no matching '{1}' found.",
attrStart, attrEnd);
}
if (node.nodeType == 1 /** Element **/) {
if (node.nodeType == NODE_TYPE_ELEMENT) {
if (node.hasAttribute(attrStart)) depth++;
if (node.hasAttribute(attrEnd)) depth--;
}
Expand Down Expand Up @@ -1625,7 +1625,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
compileNode = $template[0];

if ($template.length != 1 || compileNode.nodeType !== 1) {
if ($template.length != 1 || compileNode.nodeType !== NODE_TYPE_ELEMENT) {
throw $compileMinErr('tplrt',
"Template for directive '{0}' must have exactly one root element. {1}",
directiveName, '');
Expand Down Expand Up @@ -2107,7 +2107,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
compileNode = $template[0];

if ($template.length != 1 || compileNode.nodeType !== 1) {
if ($template.length != 1 || compileNode.nodeType !== NODE_TYPE_ELEMENT) {
throw $compileMinErr('tplrt',
"Template for directive '{0}' must have exactly one root element. {1}",
origAsyncDirective.name, templateUrl);
Expand Down Expand Up @@ -2533,7 +2533,7 @@ function removeComments(jqNodes) {

while (i--) {
var node = jqNodes[i];
if (node.nodeType === 8) {
if (node.nodeType === NODE_TYPE_COMMENT) {
splice.call(jqNodes, i, 1);
}
}
Expand Down
15 changes: 12 additions & 3 deletions src/ngAnimate/animate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,9 +477,14 @@ angular.module('ngAnimate', ['ng'])
});
});

var hasClasses = {};
forEach((element.attr('class') || '').replace(/\s+/g, ' ').split(' '), function(className) {
hasClasses[className] = true;
});

var toAdd = [], toRemove = [];
forEach(cache.classes, function(status, className) {
var hasClass = angular.$$hasClass(element[0], className);
var hasClass = hasClasses[className] === true;
var matchingAnimation = lookup[className] || {};

// When addClass and removeClass is called then $animate will check to
Expand Down Expand Up @@ -979,7 +984,10 @@ angular.module('ngAnimate', ['ng'])
element = stripCommentsFromElement(element);

if (classBasedAnimationsBlocked(element)) {
return $delegate.setClass(element, add, remove);
// TODO(@caitp/@matsko): Don't use private/undocumented API here --- we should not be
// changing the DOM synchronously in this case. The `true` parameter must eventually be
// removed.
return $delegate.setClass(element, add, remove, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we use jqLite here instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, we need to return the promise which complicates things

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do, when jQuery is used we'll die on SVG elements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do want to fix this up so that it sucks less though (but I think matsko will need to help fix broken tests that fail when we don't do this synchronously)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. let's keep it as is for now. can we add a comment that we call it this way via undocumented api.

}

// we're using a combined array for both the add and remove
Expand Down Expand Up @@ -1033,7 +1041,8 @@ angular.module('ngAnimate', ['ng'])
return !classes
? done()
: performAnimation('setClass', classes, element, parentElement, null, function() {
$delegate.setClass(element, classes[0], classes[1]);
if (classes[0]) $delegate.$$addClassImmediately(element, classes[0]);
if (classes[1]) $delegate.$$removeClassImmediately(element, classes[1]);
}, done);
});
},
Expand Down
Loading