From c3ce5d4a59dba43342c3f239992cb6fb3b75c3e5 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Tue, 30 Sep 2014 13:42:51 -0400 Subject: [PATCH 1/9] test(matchers.js): make toHaveClass matcher work better for SVG+jQuery --- test/helpers/matchers.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/helpers/matchers.js b/test/helpers/matchers.js index 1c3b17c39212..43dc34d97514 100644 --- a/test/helpers/matchers.js +++ b/test/helpers/matchers.js @@ -167,9 +167,13 @@ beforeEach(function() { this.message = function() { return "Expected '" + angular.mock.dump(this.actual) + "' to have class '" + clazz + "'."; }; - return this.actual.hasClass ? - this.actual.hasClass(clazz) : - angular.element(this.actual).hasClass(clazz); + var classes = clazz.trim().split(/\s+/); + for (var i=0; i Date: Wed, 24 Sep 2014 22:04:06 -0400 Subject: [PATCH 2/9] fix(ngAnimate): defer DOM operations for changing classes to postDigest When ngAnimate is used, it will defer changes to classes until postDigest. Previously, AngularJS (when ngAnimate is not loaded) would always immediately perform these DOM operations. Now, even when the ngAnimate module is not used, if $rootScope is in the midst of a digest, class manipulation is deferred. This helps reduce jank in browsers such as IE11. Closes #8234 Closes #9263 --- src/ng/animate.js | 108 ++++++++++++++- src/ngAnimate/animate.js | 9 +- test/ng/animateSpec.js | 239 +++++++++++++++++++++++++++++++++ test/ng/directive/inputSpec.js | 39 ++++++ 4 files changed, 388 insertions(+), 7 deletions(-) diff --git a/src/ng/animate.js b/src/ng/animate.js index ababe39f96e9..209247fecb69 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -81,9 +81,69 @@ 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; + var ELEMENT_NODE = 1; + + function extractElementNodes(element) { + var elements = new Array(element.length); + var count = 0; + for(var i = 0; i < element.length; i++) { + var elm = element[i]; + if (elm.nodeType == ELEMENT_NODE) { + elements[count++] = elm; + } + } + elements.length = count; + return jqLite(elements); + } + + function runAnimationPostDigest(fn) { + var cancelFn, defer = $$q.defer(); + if (!$rootScope.$$phase) { + fn(noop); + defer.resolve(); + } + 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 map = {}; + + forEach(cache.add, function(className) { + if (className && className.length) { + map[className] = map[className] || 0; + map[className]++; + } + }); + + forEach(cache.remove, function(className) { + if (className && className.length) { + map[className] = map[className] || 0; + map[className]--; + } + }); + + var toAdd = [], toRemove = []; + forEach(map, function(status, className) { + var hasClass = jqLiteHasClass(element[0], className); + + if (status < 0 && hasClass) toRemove.push(className); + else if (status > 0 && !hasClass) toAdd.push(className); + }); + + return (toAdd.length + toRemove.length) > 0 && [toAdd.join(' '), toRemove.join(' ')]; + } + function asyncPromise() { // only serve one instance of a promise in order to save CPU cycles if (!currentDefer) { @@ -187,6 +247,11 @@ 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; @@ -209,6 +274,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; @@ -232,9 +302,39 @@ var $AnimateProvider = ['$provide', function($provide) { * @return {Promise} the animation callback promise */ setClass : function(element, add, remove) { - this.addClass(element, add); - this.removeClass(element, remove); - return asyncPromise(); + var self = this; + var STORAGE_KEY = '$$animateClasses'; + element = extractElementNodes(jqLite(element)); + + add = isArray(add) ? add : add.split(' '); + remove = isArray(remove) ? remove : remove.split(' '); + + var cache = element.data(STORAGE_KEY); + if (cache) { + cache.add = cache.add.concat(add); + cache.remove = cache.remove.concat(remove); + //the digest cycle will combine all the animations into one function + return cache.promise; + } else { + element.data(STORAGE_KEY, cache = { + add : add, + remove : remove + }); + } + + return 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].length) self.$$addClassImmediately(element, classes[0]); + if (classes[1].length) self.$$removeClassImmediately(element, classes[1]); + } + + done(); + }); }, enabled : noop, diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index a8cb6263a0cf..5fca76c7e37c 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -979,7 +979,9 @@ angular.module('ngAnimate', ['ng']) element = stripCommentsFromElement(element); if (classBasedAnimationsBlocked(element)) { - return $delegate.setClass(element, add, remove); + if (add) $delegate.$$addClassImmediately(element, add); + if (remove) $delegate.$$removeClassImmediately(element, remove); + return; } // we're using a combined array for both the add and remove @@ -1032,8 +1034,9 @@ angular.module('ngAnimate', ['ng']) var classes = resolveElementClasses(element, cache, state.active); return !classes ? done() - : performAnimation('setClass', classes, element, parentElement, null, function() { - $delegate.setClass(element, classes[0], classes[1]); + : performAnimation('setClass', classes, element, null, null, function() { + if (classes[0]) $delegate.$$addClassImmediately(element, classes[0]); + if (classes[1]) $delegate.$$removeClassImmediately(element, classes[1]); }, done); }); }, diff --git a/test/ng/animateSpec.js b/test/ng/animateSpec.js index 912012b160c4..52a71c6e4c40 100644 --- a/test/ng/animateSpec.js +++ b/test/ng/animateSpec.js @@ -100,4 +100,243 @@ describe("$animate", function() { inject(); }); }); + + describe('class API', function() { + var element; + var addClass; + var removeClass; + + beforeEach(module(provideLog)); + + afterEach(function() { + dealoc(element); + }); + + function setupClassManipulationSpies() { + inject(function($animate) { + addClass = spyOn($animate, '$$addClassImmediately').andCallThrough(); + removeClass = spyOn($animate, '$$removeClassImmediately').andCallThrough(); + }); + } + + function setupClassManipulationLogger(log) { + inject(function($animate) { + var addClassImmediately = $animate.$$addClassImmediately; + var removeClassImmediately = $animate.$$removeClassImmediately; + addClass = spyOn($animate, '$$addClassImmediately').andCallFake(function(element, classes) { + var names = classes; + if (Object.prototype.toString.call(classes) === '[object Array]') names = classes.join( ' '); + log('addClass(' + names + ')'); + return addClassImmediately.call($animate, element, classes); + }); + removeClass = spyOn($animate, '$$removeClassImmediately').andCallFake(function(element, classes) { + var names = classes; + if (Object.prototype.toString.call(classes) === '[object Array]') names = classes.join( ' '); + log('removeClass(' + names + ')'); + return removeClassImmediately.call($animate, element, classes); + }); + }); + } + + it('should defer class manipulation until end of digest', inject(function($rootScope, $animate) { + setupClassManipulationSpies(); + element = jqLite('

test

'); + + $rootScope.$apply(function() { + $animate.addClass(element, 'test-class1'); + expect(element).not.toHaveClass('test-class1'); + + $animate.removeClass(element, 'test-class1'); + + $animate.addClass(element, 'test-class2'); + expect(element).not.toHaveClass('test-class2'); + + $animate.setClass(element, 'test-class3', 'test-class4'); + expect(element).not.toHaveClass('test-class3'); + expect(element).not.toHaveClass('test-class4'); + }); + + expect(element).not.toHaveClass('test-class1'); + expect(element).not.toHaveClass('test-class4'); + expect(element).toHaveClass('test-class2'); + expect(element).toHaveClass('test-class3'); + expect(addClass.callCount).toBe(1); + expect(removeClass.callCount).toBe(0); + })); + + + it('should perform class manipulation immediately outside of digest', inject(function($rootScope, $animate) { + setupClassManipulationSpies(); + element = jqLite('

test

'); + + $animate.addClass(element, 'test-class1'); + expect(element).toHaveClass('test-class1'); + + $animate.removeClass(element, 'test-class1'); + expect(element).not.toHaveClass('test-class1'); + + $animate.addClass(element, 'test-class2'); + expect(element).toHaveClass('test-class2'); + + $animate.setClass(element, 'test-class3', 'test-class4'); + expect(element).toHaveClass('test-class3'); + expect(element).not.toHaveClass('test-class4'); + + expect(element).not.toHaveClass('test-class1'); + expect(element).toHaveClass('test-class2'); + expect(addClass.callCount).toBe(3); + expect(removeClass.callCount).toBe(1); + })); + + + it('should perform class manipulation in expected order at end of digest', inject(function($rootScope, $animate, log) { + element = jqLite('

test

'); + + setupClassManipulationLogger(log); + + $rootScope.$apply(function() { + $animate.addClass(element, 'test-class1'); + $animate.addClass(element, 'test-class2'); + $animate.removeClass(element, 'test-class1'); + $animate.removeClass(element, 'test-class3'); + $animate.addClass(element, 'test-class3'); + }); + expect(log).toEqual(['addClass(test-class2)']); + })); + + + it('should perform class manipulation in expected order outside of digest', inject(function($rootScope, $animate, log) { + element = jqLite('

test

'); + + setupClassManipulationLogger(log); + + $animate.addClass(element, 'test-class1'); + $animate.addClass(element, 'test-class2'); + $animate.removeClass(element, 'test-class1'); + $animate.removeClass(element, 'test-class3'); + $animate.addClass(element, 'test-class3'); + + expect(log).toEqual([ + 'addClass(test-class1)', + 'addClass(test-class2)', + 'removeClass(test-class1)', + 'removeClass(test-class3)', + 'addClass(test-class3)']); + })); + + + it('should return a promise which is resolved on a different turn', inject(function(log, $animate, $browser, $rootScope) { + element = jqLite('

test

'); + + $animate.addClass(element, 'test1').then(log.fn('addClass(test1)')); + $animate.removeClass(element, 'test2').then(log.fn('removeClass(test2)')); + + $browser.defer.flush(); + expect(log).toEqual(['addClass(test1)', 'removeClass(test2)']); + + log.reset(); + element = jqLite('

test

'); + + $rootScope.$apply(function() { + $animate.addClass(element, 'test3').then(log.fn('addClass(test3)')); + $animate.removeClass(element, 'test4').then(log.fn('removeClass(test4)')); + }); + + $browser.defer.flush(); + expect(log).toEqual(['addClass(test3)', 'removeClass(test4)']); + })); + + + it('should defer class manipulation until end of digest for SVG', inject(function($rootScope, $animate) { + if (!window.SVGElement) return; + setupClassManipulationSpies(); + element = jqLite(''); + var target = element.children().eq(0); + + $rootScope.$apply(function() { + $animate.addClass(target, 'test-class1'); + expect(target).not.toHaveClass('test-class1'); + + $animate.removeClass(target, 'test-class1'); + + $animate.addClass(target, 'test-class2'); + expect(target).not.toHaveClass('test-class2'); + + $animate.setClass(target, 'test-class3', 'test-class4'); + expect(target).not.toHaveClass('test-class3'); + expect(target).not.toHaveClass('test-class4'); + }); + + expect(target).not.toHaveClass('test-class1'); + expect(target).toHaveClass('test-class2'); + expect(addClass.callCount).toBe(1); + expect(removeClass.callCount).toBe(0); + })); + + + it('should perform class manipulation immediately outside of digest for SVG', inject(function($rootScope, $animate) { + if (!window.SVGElement) return; + setupClassManipulationSpies(); + element = jqLite(''); + var target = element.children().eq(0); + + $animate.addClass(target, 'test-class1'); + expect(target).toHaveClass('test-class1'); + + $animate.removeClass(target, 'test-class1'); + expect(target).not.toHaveClass('test-class1'); + + $animate.addClass(target, 'test-class2'); + expect(target).toHaveClass('test-class2'); + + $animate.setClass(target, 'test-class3', 'test-class4'); + expect(target).toHaveClass('test-class3'); + expect(target).not.toHaveClass('test-class4'); + + expect(target).not.toHaveClass('test-class1'); + expect(target).toHaveClass('test-class2'); + expect(addClass.callCount).toBe(3); + expect(removeClass.callCount).toBe(1); + })); + + + it('should perform class manipulation in expected order at end of digest for SVG', inject(function($rootScope, $animate, log) { + if (!window.SVGElement) return; + element = jqLite(''); + var target = element.children().eq(0); + + setupClassManipulationLogger(log); + + $rootScope.$apply(function() { + $animate.addClass(target, 'test-class1'); + $animate.addClass(target, 'test-class2'); + $animate.removeClass(target, 'test-class1'); + $animate.removeClass(target, 'test-class3'); + $animate.addClass(target, 'test-class3'); + }); + expect(log).toEqual(['addClass(test-class2)']); + })); + + + it('should perform class manipulation in expected order outside of digest for SVG', inject(function($rootScope, $animate, log) { + if (!window.SVGElement) return; + element = jqLite(''); + var target = element.children().eq(0); + + setupClassManipulationLogger(log); + + $animate.addClass(target, 'test-class1'); + $animate.addClass(target, 'test-class2'); + $animate.removeClass(target, 'test-class1'); + $animate.removeClass(target, 'test-class3'); + $animate.addClass(target, 'test-class3'); + + expect(log).toEqual([ + 'addClass(test-class1)', + 'addClass(test-class2)', + 'removeClass(test-class1)', + 'removeClass(test-class3)', + 'addClass(test-class3)']); + })); + }); }); diff --git a/test/ng/directive/inputSpec.js b/test/ng/directive/inputSpec.js index 6b0dd0d1df43..17aab79183c4 100644 --- a/test/ng/directive/inputSpec.js +++ b/test/ng/directive/inputSpec.js @@ -892,6 +892,45 @@ describe('NgModelController', function() { dealoc(element); })); + + it('should minimize janky setting of classes during $validate() and ngModelWatch', inject(function($animate, $compile, $rootScope) { + var addClass = $animate.$$addClassImmediately; + var removeClass = $animate.$$removeClassImmediately; + var addClassCallCount = 0; + var removeClassCallCount = 0; + var input; + $animate.$$addClassImmediately = function(element, className) { + if (input && element[0] === input[0]) ++addClassCallCount; + return addClass.call($animate, element, className); + }; + + $animate.$$removeClassImmediately = function(element, className) { + if (input && element[0] === input[0]) ++removeClassCallCount; + return removeClass.call($animate, element, className); + }; + + dealoc(element); + + $rootScope.value = "123456789"; + element = $compile( + '
' + + '' + + '
' + )($rootScope); + + var form = $rootScope.form; + input = element.children().eq(0); + + $rootScope.$digest(); + + expect(input).toBeValid(); + expect(input).not.toHaveClass('ng-invalid-maxlength'); + expect(input).toHaveClass('ng-valid-maxlength'); + expect(addClassCallCount).toBe(1); + expect(removeClassCallCount).toBe(0); + + dealoc(element); + })); }); }); From 43dcb3637128ebed23bd42d4c1d9e117afc9e730 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Tue, 30 Sep 2014 21:47:35 -0400 Subject: [PATCH 3/9] WIP: Fixuppin Always defer class manipulation DOM writes until the end of digest on the root scope, even when animations are disabled. BREAKING CHANGE: The $animate class API will always defer changes until the end of the next digest. This allows ngAnimate to coalesce class changes which occur over a short period of time into 1 or 2 DOM writes, rather than many. This prevents jank in browsers such as IE, and is generally a good thing. If you're finding that your classes are not being immediately applied, be sure to invoke $digest(). --- src/ng/animate.js | 26 ++---- src/ngAnimate/animate.js | 4 +- test/ng/animateSpec.js | 134 +++++----------------------- test/ng/directive/formSpec.js | 9 ++ test/ngAnimate/animateSpec.js | 160 +++++++++++++++++++++++++++++++++- 5 files changed, 198 insertions(+), 135 deletions(-) diff --git a/src/ng/animate.js b/src/ng/animate.js index 209247fecb69..e02a46623826 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -86,33 +86,18 @@ var $AnimateProvider = ['$provide', function($provide) { var currentDefer; var ELEMENT_NODE = 1; - function extractElementNodes(element) { - var elements = new Array(element.length); - var count = 0; - for(var i = 0; i < element.length; i++) { - var elm = element[i]; - if (elm.nodeType == ELEMENT_NODE) { - elements[count++] = elm; - } - } - elements.length = count; - return jqLite(elements); - } - function runAnimationPostDigest(fn) { var cancelFn, defer = $$q.defer(); - if (!$rootScope.$$phase) { - fn(noop); - defer.resolve(); - } defer.promise.$$cancelFn = function ngAnimateMaybeCancel() { cancelFn && cancelFn(); }; + $rootScope.$$postDigest(function ngAnimatePostDigest() { cancelFn = fn(function ngAnimateNotifyComplete() { defer.resolve(); }); }); + return defer.promise; } @@ -258,7 +243,6 @@ var $AnimateProvider = ['$provide', function($provide) { forEach(element, function (element) { jqLiteAddClass(element, className); }); - return asyncPromise(); }, /** @@ -304,7 +288,7 @@ var $AnimateProvider = ['$provide', function($provide) { setClass : function(element, add, remove) { var self = this; var STORAGE_KEY = '$$animateClasses'; - element = extractElementNodes(jqLite(element)); + element = jqLite(element); add = isArray(add) ? add : add.split(' '); remove = isArray(remove) ? remove : remove.split(' '); @@ -329,8 +313,8 @@ var $AnimateProvider = ['$provide', function($provide) { var classes = cache && resolveElementClasses(element, cache); if (classes) { - if (classes[0].length) self.$$addClassImmediately(element, classes[0]); - if (classes[1].length) self.$$removeClassImmediately(element, classes[1]); + if (classes[0]) self.$$addClassImmediately(element, classes[0]); + if (classes[1]) self.$$removeClassImmediately(element, classes[1]); } done(); diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index 5fca76c7e37c..9ae150f8c1df 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -979,8 +979,8 @@ angular.module('ngAnimate', ['ng']) element = stripCommentsFromElement(element); if (classBasedAnimationsBlocked(element)) { - if (add) $delegate.$$addClassImmediately(element, add); - if (remove) $delegate.$$removeClassImmediately(element, remove); + $delegate.$$addClassImmediately(element, add); + $delegate.$$removeClassImmediately(element, remove); return; } diff --git a/test/ng/animateSpec.js b/test/ng/animateSpec.js index 52a71c6e4c40..36bad5ad231e 100644 --- a/test/ng/animateSpec.js +++ b/test/ng/animateSpec.js @@ -50,10 +50,11 @@ describe("$animate", function() { expect(element.text()).toBe('21'); })); - it("should still perform DOM operations even if animations are disabled", inject(function($animate) { + it("should still perform DOM operations even if animations are disabled (post-digest)", inject(function($animate, $rootScope) { $animate.enabled(false); expect(element).toBeShown(); $animate.addClass(element, 'ng-hide'); + $rootScope.$digest(); expect(element).toBeHidden(); })); @@ -79,15 +80,17 @@ describe("$animate", function() { expect($animate.cancel()).toBeUndefined(); })); - it("should add and remove classes on SVG elements", inject(function($animate) { + it("should add and remove classes on SVG elements", inject(function($animate, $rootScope) { if (!window.SVGElement) return; var svg = jqLite(''); var rect = svg.children(); $animate.enabled(false); expect(rect).toBeShown(); $animate.addClass(rect, 'ng-hide'); + $rootScope.$digest(); expect(rect).toBeHidden(); $animate.removeClass(rect, 'ng-hide'); + $rootScope.$digest(); expect(rect).not.toBeHidden(); })); @@ -101,7 +104,7 @@ describe("$animate", function() { }); }); - describe('class API', function() { + describe('CSS class DOM manipulation', function() { var element; var addClass; var removeClass; @@ -138,8 +141,9 @@ describe("$animate", function() { }); } - it('should defer class manipulation until end of digest', inject(function($rootScope, $animate) { - setupClassManipulationSpies(); + + it('should defer class manipulation until end of digest', inject(function($rootScope, $animate, log) { + setupClassManipulationLogger(log); element = jqLite('

test

'); $rootScope.$apply(function() { @@ -154,96 +158,28 @@ describe("$animate", function() { $animate.setClass(element, 'test-class3', 'test-class4'); expect(element).not.toHaveClass('test-class3'); expect(element).not.toHaveClass('test-class4'); + expect(log).toEqual([]); }); expect(element).not.toHaveClass('test-class1'); expect(element).not.toHaveClass('test-class4'); expect(element).toHaveClass('test-class2'); expect(element).toHaveClass('test-class3'); + expect(log).toEqual(['addClass(test-class2 test-class3)']); expect(addClass.callCount).toBe(1); expect(removeClass.callCount).toBe(0); })); - it('should perform class manipulation immediately outside of digest', inject(function($rootScope, $animate) { - setupClassManipulationSpies(); - element = jqLite('

test

'); - - $animate.addClass(element, 'test-class1'); - expect(element).toHaveClass('test-class1'); - - $animate.removeClass(element, 'test-class1'); - expect(element).not.toHaveClass('test-class1'); - - $animate.addClass(element, 'test-class2'); - expect(element).toHaveClass('test-class2'); - - $animate.setClass(element, 'test-class3', 'test-class4'); - expect(element).toHaveClass('test-class3'); - expect(element).not.toHaveClass('test-class4'); - - expect(element).not.toHaveClass('test-class1'); - expect(element).toHaveClass('test-class2'); - expect(addClass.callCount).toBe(3); - expect(removeClass.callCount).toBe(1); - })); - - - it('should perform class manipulation in expected order at end of digest', inject(function($rootScope, $animate, log) { - element = jqLite('

test

'); - - setupClassManipulationLogger(log); - - $rootScope.$apply(function() { - $animate.addClass(element, 'test-class1'); - $animate.addClass(element, 'test-class2'); - $animate.removeClass(element, 'test-class1'); - $animate.removeClass(element, 'test-class3'); - $animate.addClass(element, 'test-class3'); - }); - expect(log).toEqual(['addClass(test-class2)']); - })); - - - it('should perform class manipulation in expected order outside of digest', inject(function($rootScope, $animate, log) { - element = jqLite('

test

'); - - setupClassManipulationLogger(log); - - $animate.addClass(element, 'test-class1'); - $animate.addClass(element, 'test-class2'); - $animate.removeClass(element, 'test-class1'); - $animate.removeClass(element, 'test-class3'); - $animate.addClass(element, 'test-class3'); - - expect(log).toEqual([ - 'addClass(test-class1)', - 'addClass(test-class2)', - 'removeClass(test-class1)', - 'removeClass(test-class3)', - 'addClass(test-class3)']); - })); - - - it('should return a promise which is resolved on a different turn', inject(function(log, $animate, $browser, $rootScope) { + it('should return a promise which is resolved on a different turn digest', inject(function(log, $animate, $browser, $rootScope) { element = jqLite('

test

'); $animate.addClass(element, 'test1').then(log.fn('addClass(test1)')); $animate.removeClass(element, 'test2').then(log.fn('removeClass(test2)')); + $rootScope.$digest(); $browser.defer.flush(); expect(log).toEqual(['addClass(test1)', 'removeClass(test2)']); - - log.reset(); - element = jqLite('

test

'); - - $rootScope.$apply(function() { - $animate.addClass(element, 'test3').then(log.fn('addClass(test3)')); - $animate.removeClass(element, 'test4').then(log.fn('removeClass(test4)')); - }); - - $browser.defer.flush(); - expect(log).toEqual(['addClass(test3)', 'removeClass(test4)']); })); @@ -274,29 +210,27 @@ describe("$animate", function() { })); - it('should perform class manipulation immediately outside of digest for SVG', inject(function($rootScope, $animate) { + it('should defer class manipulation until digest outside of digest for SVG', inject(function($rootScope, $animate, log) { if (!window.SVGElement) return; - setupClassManipulationSpies(); + setupClassManipulationLogger(log); element = jqLite(''); var target = element.children().eq(0); $animate.addClass(target, 'test-class1'); - expect(target).toHaveClass('test-class1'); - $animate.removeClass(target, 'test-class1'); - expect(target).not.toHaveClass('test-class1'); - $animate.addClass(target, 'test-class2'); - expect(target).toHaveClass('test-class2'); - $animate.setClass(target, 'test-class3', 'test-class4'); - expect(target).toHaveClass('test-class3'); - expect(target).not.toHaveClass('test-class4'); + expect(log).toEqual([]); + + $rootScope.$digest(); + + expect(log).toEqual(['addClass(test-class2 test-class3)']); expect(target).not.toHaveClass('test-class1'); expect(target).toHaveClass('test-class2'); - expect(addClass.callCount).toBe(3); - expect(removeClass.callCount).toBe(1); + expect(target).toHaveClass('test-class3'); + expect(addClass.callCount).toBe(1); + expect(removeClass.callCount).toBe(0); })); @@ -316,27 +250,5 @@ describe("$animate", function() { }); expect(log).toEqual(['addClass(test-class2)']); })); - - - it('should perform class manipulation in expected order outside of digest for SVG', inject(function($rootScope, $animate, log) { - if (!window.SVGElement) return; - element = jqLite(''); - var target = element.children().eq(0); - - setupClassManipulationLogger(log); - - $animate.addClass(target, 'test-class1'); - $animate.addClass(target, 'test-class2'); - $animate.removeClass(target, 'test-class1'); - $animate.removeClass(target, 'test-class3'); - $animate.addClass(target, 'test-class3'); - - expect(log).toEqual([ - 'addClass(test-class1)', - 'addClass(test-class2)', - 'removeClass(test-class1)', - 'removeClass(test-class3)', - 'addClass(test-class3)']); - })); }); }); diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js index febe0d8ebe54..757d4b6f8258 100644 --- a/test/ng/directive/formSpec.js +++ b/test/ng/directive/formSpec.js @@ -554,17 +554,20 @@ describe('form', function() { expect(doc).toBeValid(); control.$setValidity('error', false); + scope.$digest(); expect(doc).toBeInvalid(); expect(doc.hasClass('ng-valid-error')).toBe(false); expect(doc.hasClass('ng-invalid-error')).toBe(true); control.$setValidity('another', false); + scope.$digest(); expect(doc.hasClass('ng-valid-error')).toBe(false); expect(doc.hasClass('ng-invalid-error')).toBe(true); expect(doc.hasClass('ng-valid-another')).toBe(false); expect(doc.hasClass('ng-invalid-another')).toBe(true); control.$setValidity('error', true); + scope.$digest(); expect(doc).toBeInvalid(); expect(doc.hasClass('ng-valid-error')).toBe(true); expect(doc.hasClass('ng-invalid-error')).toBe(false); @@ -572,6 +575,7 @@ describe('form', function() { expect(doc.hasClass('ng-invalid-another')).toBe(true); control.$setValidity('another', true); + scope.$digest(); expect(doc).toBeValid(); expect(doc.hasClass('ng-valid-error')).toBe(true); expect(doc.hasClass('ng-invalid-error')).toBe(false); @@ -581,6 +585,7 @@ describe('form', function() { // validators are skipped, e.g. becuase of a parser error control.$setValidity('error', null); control.$setValidity('another', null); + scope.$digest(); expect(doc.hasClass('ng-valid-error')).toBe(false); expect(doc.hasClass('ng-invalid-error')).toBe(false); expect(doc.hasClass('ng-valid-another')).toBe(false); @@ -652,7 +657,9 @@ describe('form', function() { expect(input1).toBeDirty(); expect(input2).toBeDirty(); + formCtrl.$setPristine(); + scope.$digest(); expect(form).toBePristine(); expect(formCtrl.$pristine).toBe(true); expect(formCtrl.$dirty).toBe(false); @@ -685,6 +692,7 @@ describe('form', function() { expect(input).toBeDirty(); formCtrl.$setPristine(); + scope.$digest(); expect(form).toBePristine(); expect(formCtrl.$pristine).toBe(true); expect(formCtrl.$dirty).toBe(false); @@ -719,6 +727,7 @@ describe('form', function() { expect(nestedInput).toBeDirty(); formCtrl.$setPristine(); + scope.$digest(); expect(form).toBePristine(); expect(formCtrl.$pristine).toBe(true); expect(formCtrl.$dirty).toBe(false); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index e2440b343c1a..eee193e20df0 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -1,7 +1,13 @@ 'use strict'; describe("ngAnimate", function() { - + var $animateCore; + beforeEach(module(function($provide) { + $provide.decorator('$animate', function($delegate) { + $animateCore = $delegate; + return $delegate; + }); + })); beforeEach(module('ngAnimate')); beforeEach(module('ngAnimateMock')); @@ -4871,4 +4877,156 @@ describe("ngAnimate", function() { })); }); }); + + + describe('CSS class DOM manipulation', function() { + var element; + var addClass; + var removeClass; + + beforeEach(module(provideLog)); + + afterEach(function() { + dealoc(element); + }); + + function setupClassManipulationSpies() { + inject(function($animate) { + addClass = spyOn($animateCore, '$$addClassImmediately').andCallThrough(); + removeClass = spyOn($animateCore, '$$removeClassImmediately').andCallThrough(); + }); + } + + function setupClassManipulationLogger(log) { + inject(function($animate) { + var addClassImmediately = $animateCore.$$addClassImmediately; + var removeClassImmediately = $animateCore.$$removeClassImmediately; + addClass = spyOn($animateCore, '$$addClassImmediately').andCallFake(function(element, classes) { + var names = classes; + if (Object.prototype.toString.call(classes) === '[object Array]') names = classes.join( ' '); + log('addClass(' + names + ')'); + return addClassImmediately.call($animateCore, element, classes); + }); + removeClass = spyOn($animateCore, '$$removeClassImmediately').andCallFake(function(element, classes) { + var names = classes; + if (Object.prototype.toString.call(classes) === '[object Array]') names = classes.join( ' '); + log('removeClass(' + names + ')'); + return removeClassImmediately.call($animateCore, element, classes); + }); + }); + } + + it('should defer class manipulation until end of digest', inject(function($rootScope, $animate, log) { + setupClassManipulationLogger(log); + element = jqLite('

test

'); + + $rootScope.$apply(function() { + $animate.addClass(element, 'test-class1'); + expect(element).not.toHaveClass('test-class1'); + + $animate.removeClass(element, 'test-class1'); + + $animate.addClass(element, 'test-class2'); + expect(element).not.toHaveClass('test-class2'); + + $animate.setClass(element, 'test-class3', 'test-class4'); + expect(element).not.toHaveClass('test-class3'); + expect(element).not.toHaveClass('test-class4'); + expect(log).toEqual([]); + }); + + expect(element).not.toHaveClass('test-class1'); + expect(element).not.toHaveClass('test-class4'); + expect(element).toHaveClass('test-class2'); + expect(element).toHaveClass('test-class3'); + expect(log).toEqual(['addClass(test-class2 test-class3)']); + expect(addClass.callCount).toBe(1); + expect(removeClass.callCount).toBe(0); + })); + + + it('should defer class manipulation until digest outside of digest', inject(function($rootScope, $animate, log) { + setupClassManipulationLogger(log); + element = jqLite('

test

'); + + $animate.addClass(element, 'test-class1'); + $animate.removeClass(element, 'test-class1'); + $animate.addClass(element, 'test-class2'); + $animate.setClass(element, 'test-class3', 'test-class4'); + + expect(log).toEqual([]); + + $rootScope.$digest(); + + expect(log).toEqual(['addClass(test-class2 test-class3)']); + expect(element).not.toHaveClass('test-class1'); + expect(element).toHaveClass('test-class2'); + expect(element).toHaveClass('test-class3'); + expect(addClass.callCount).toBe(1); + expect(removeClass.callCount).toBe(0); + })); + + + it('should return a promise which is resolved on a different turn digest', inject(function(log, $animate, $browser, $rootScope) { + element = jqLite('

test

'); + + $animate.addClass(element, 'test1').then(log.fn('addClass(test1)')); + $animate.removeClass(element, 'test2').then(log.fn('removeClass(test2)')); + + $rootScope.$digest(); + $browser.defer.flush(); + expect(log).toEqual(['addClass(test1)', 'removeClass(test2)']); + })); + + + it('should defer class manipulation until end of digest for SVG', inject(function($rootScope, $animate) { + if (!window.SVGElement) return; + setupClassManipulationSpies(); + element = jqLite(''); + var target = element.children().eq(0); + + $rootScope.$apply(function() { + $animate.addClass(target, 'test-class1'); + expect(target).not.toHaveClass('test-class1'); + + $animate.removeClass(target, 'test-class1'); + + $animate.addClass(target, 'test-class2'); + expect(target).not.toHaveClass('test-class2'); + + $animate.setClass(target, 'test-class3', 'test-class4'); + expect(target).not.toHaveClass('test-class3'); + expect(target).not.toHaveClass('test-class4'); + }); + + expect(target).not.toHaveClass('test-class1'); + expect(target).toHaveClass('test-class2'); + expect(addClass.callCount).toBe(1); + expect(removeClass.callCount).toBe(0); + })); + + + it('should defer class manipulation until digest outside of digest for SVG', inject(function($rootScope, $animate, log) { + if (!window.SVGElement) return; + setupClassManipulationLogger(log); + element = jqLite(''); + var target = element.children().eq(0); + + $animate.addClass(target, 'test-class1'); + $animate.removeClass(target, 'test-class1'); + $animate.addClass(target, 'test-class2'); + $animate.setClass(target, 'test-class3', 'test-class4'); + + expect(log).toEqual([]); + + $rootScope.$digest(); + + expect(log).toEqual(['addClass(test-class2 test-class3)']); + expect(target).not.toHaveClass('test-class1'); + expect(target).toHaveClass('test-class2'); + expect(target).toHaveClass('test-class3'); + expect(addClass.callCount).toBe(1); + expect(removeClass.callCount).toBe(0); + })); + }); }); From a6ee5723f67ec8799bd27978950c6adb6e893936 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Tue, 7 Oct 2014 22:33:21 -0400 Subject: [PATCH 4/9] WIP --- fixups --- src/ng/animate.js | 107 ++++++++++++++++++++-------------- src/ngAnimate/animate.js | 6 +- test/ng/animateSpec.js | 64 +++++++++++++++++--- test/ng/directive/formSpec.js | 1 + test/ngAnimate/animateSpec.js | 88 +++++++++++++++++++++------- 5 files changed, 191 insertions(+), 75 deletions(-) diff --git a/src/ng/animate.js b/src/ng/animate.js index e02a46623826..f00c9dd33a90 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -86,6 +86,19 @@ var $AnimateProvider = ['$provide', function($provide) { var currentDefer; var ELEMENT_NODE = 1; + function extractElementNodes(element) { + var elements = new Array(element.length); + var count = 0; + for(var i = 0; i < element.length; i++) { + var elm = element[i]; + if (elm.nodeType == ELEMENT_NODE) { + elements[count++] = elm; + } + } + elements.length = count; + return jqLite(elements); + } + function runAnimationPostDigest(fn) { var cancelFn, defer = $$q.defer(); defer.promise.$$cancelFn = function ngAnimateMaybeCancel() { @@ -102,33 +115,31 @@ var $AnimateProvider = ['$provide', function($provide) { } function resolveElementClasses(element, cache) { - var map = {}; - - forEach(cache.add, function(className) { - if (className && className.length) { - map[className] = map[className] || 0; - map[className]++; - } - }); - - forEach(cache.remove, function(className) { - if (className && className.length) { - map[className] = map[className] || 0; - map[className]--; - } - }); - var toAdd = [], toRemove = []; - forEach(map, function(status, className) { + forEach(cache.classes, function(status, className) { var hasClass = jqLiteHasClass(element[0], className); - if (status < 0 && hasClass) toRemove.push(className); - else if (status > 0 && !hasClass) toAdd.push(className); + // 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); + } }); return (toAdd.length + toRemove.length) > 0 && [toAdd.join(' '), toRemove.join(' ')]; } + 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) { @@ -285,40 +296,50 @@ 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) { + setClass : function(element, add, remove, runSynchronously) { var self = this; var STORAGE_KEY = '$$animateClasses'; - element = jqLite(element); + element = extractElementNodes(jqLite(element)); - add = isArray(add) ? add : add.split(' '); - remove = isArray(remove) ? remove : remove.split(' '); + if (runSynchronously) { + self.$$addClassImmediately(element, add); + self.$$removeClassImmediately(element, remove); + return asyncPromise(); + } var cache = element.data(STORAGE_KEY); - if (cache) { - cache.add = cache.add.concat(add); - cache.remove = cache.remove.concat(remove); - //the digest cycle will combine all the animations into one function - return cache.promise; - } else { - element.data(STORAGE_KEY, cache = { - add : add, - remove : remove - }); + if (!cache) { + cache = { + classes: {} + }; + var createdCache = true; } - return cache.promise = runAnimationPostDigest(function(done) { - var cache = element.data(STORAGE_KEY); - element.removeData(STORAGE_KEY); + var classes = cache.classes; + + add = isArray(add) ? add : add.split(' '); + remove = isArray(remove) ? remove : remove.split(' '); + cachedClassManipulation(classes, add, true); + cachedClassManipulation(classes, remove, false); - var classes = cache && resolveElementClasses(element, cache); + if (createdCache) { + cache.promise = runAnimationPostDigest(function(done) { + var cache = element.data(STORAGE_KEY); + element.removeData(STORAGE_KEY); - if (classes) { - if (classes[0]) self.$$addClassImmediately(element, classes[0]); - if (classes[1]) self.$$removeClassImmediately(element, classes[1]); - } + var classes = cache && resolveElementClasses(element, cache); - done(); - }); + if (classes) { + if (classes[0]) self.$$addClassImmediately(element, classes[0]); + if (classes[1]) self.$$removeClassImmediately(element, classes[1]); + } + + done(); + }); + element.data(STORAGE_KEY, cache); + } + + return cache.promise; }, enabled : noop, diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index 9ae150f8c1df..e59e63ba3602 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -979,9 +979,7 @@ angular.module('ngAnimate', ['ng']) element = stripCommentsFromElement(element); if (classBasedAnimationsBlocked(element)) { - $delegate.$$addClassImmediately(element, add); - $delegate.$$removeClassImmediately(element, remove); - return; + return $delegate.setClass(element, add, remove, true); } // we're using a combined array for both the add and remove @@ -1034,7 +1032,7 @@ angular.module('ngAnimate', ['ng']) var classes = resolveElementClasses(element, cache, state.active); return !classes ? done() - : performAnimation('setClass', classes, element, null, null, function() { + : performAnimation('setClass', classes, element, parentElement, null, function() { if (classes[0]) $delegate.$$addClassImmediately(element, classes[0]); if (classes[1]) $delegate.$$removeClassImmediately(element, classes[1]); }, done); diff --git a/test/ng/animateSpec.js b/test/ng/animateSpec.js index 36bad5ad231e..ff1f15cbed8f 100644 --- a/test/ng/animateSpec.js +++ b/test/ng/animateSpec.js @@ -50,6 +50,7 @@ describe("$animate", function() { expect(element.text()).toBe('21'); })); + it("should still perform DOM operations even if animations are disabled (post-digest)", inject(function($animate, $rootScope) { $animate.enabled(false); expect(element).toBeShown(); @@ -141,7 +142,6 @@ describe("$animate", function() { }); } - it('should defer class manipulation until end of digest', inject(function($rootScope, $animate, log) { setupClassManipulationLogger(log); element = jqLite('

test

'); @@ -171,15 +171,66 @@ describe("$animate", function() { })); - it('should return a promise which is resolved on a different turn digest', inject(function(log, $animate, $browser, $rootScope) { + it('should defer class manipulation until postDigest when outside of digest', inject(function($rootScope, $animate, log) { + setupClassManipulationLogger(log); + element = jqLite('

test

'); + + $animate.addClass(element, 'test-class1'); + $animate.removeClass(element, 'test-class1'); + $animate.addClass(element, 'test-class2'); + $animate.setClass(element, 'test-class3', 'test-class4'); + + expect(log).toEqual([]); + $rootScope.$digest(); + + + expect(log).toEqual(['addClass(test-class2 test-class3)', 'removeClass(test-class4)']); + expect(element).not.toHaveClass('test-class1'); + expect(element).toHaveClass('test-class2'); + expect(element).toHaveClass('test-class3'); + expect(addClass.callCount).toBe(1); + expect(removeClass.callCount).toBe(1); + })); + + + it('should perform class manipulation in expected order at end of digest', inject(function($rootScope, $animate, log) { + element = jqLite('

test

'); + + setupClassManipulationLogger(log); + + $rootScope.$apply(function() { + $animate.addClass(element, 'test-class1'); + $animate.addClass(element, 'test-class2'); + $animate.removeClass(element, 'test-class1'); + $animate.removeClass(element, 'test-class3'); + $animate.addClass(element, 'test-class3'); + }); + expect(log).toEqual(['addClass(test-class2)']); + })); + + + it('should return a promise which is resolved on a different turn', inject(function(log, $animate, $browser, $rootScope) { element = jqLite('

test

'); $animate.addClass(element, 'test1').then(log.fn('addClass(test1)')); $animate.removeClass(element, 'test2').then(log.fn('removeClass(test2)')); $rootScope.$digest(); + expect(log).toEqual([]); $browser.defer.flush(); expect(log).toEqual(['addClass(test1)', 'removeClass(test2)']); + + log.reset(); + element = jqLite('

test

'); + + $rootScope.$apply(function() { + $animate.addClass(element, 'test3').then(log.fn('addClass(test3)')); + $animate.removeClass(element, 'test4').then(log.fn('removeClass(test4)')); + expect(log).toEqual([]); + }); + + $browser.defer.flush(); + expect(log).toEqual(['addClass(test3)', 'removeClass(test4)']); })); @@ -210,10 +261,10 @@ describe("$animate", function() { })); - it('should defer class manipulation until digest outside of digest for SVG', inject(function($rootScope, $animate, log) { + it('should defer class manipulation until postDigest when outside of digest for SVG', inject(function($rootScope, $animate, log) { if (!window.SVGElement) return; setupClassManipulationLogger(log); - element = jqLite(''); + element = jqLite(''); var target = element.children().eq(0); $animate.addClass(target, 'test-class1'); @@ -222,15 +273,14 @@ describe("$animate", function() { $animate.setClass(target, 'test-class3', 'test-class4'); expect(log).toEqual([]); - $rootScope.$digest(); - expect(log).toEqual(['addClass(test-class2 test-class3)']); + expect(log).toEqual(['addClass(test-class2 test-class3)', 'removeClass(test-class4)']); expect(target).not.toHaveClass('test-class1'); expect(target).toHaveClass('test-class2'); expect(target).toHaveClass('test-class3'); expect(addClass.callCount).toBe(1); - expect(removeClass.callCount).toBe(0); + expect(removeClass.callCount).toBe(1); })); diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js index 757d4b6f8258..6595ef9d6457 100644 --- a/test/ng/directive/formSpec.js +++ b/test/ng/directive/formSpec.js @@ -729,6 +729,7 @@ describe('form', function() { formCtrl.$setPristine(); scope.$digest(); expect(form).toBePristine(); + scope.$digest(); expect(formCtrl.$pristine).toBe(true); expect(formCtrl.$dirty).toBe(false); expect(nestedForm).toBePristine(); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index eee193e20df0..43748bfdee98 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -1,10 +1,10 @@ 'use strict'; describe("ngAnimate", function() { - var $animateCore; + var $originalAnimate; beforeEach(module(function($provide) { $provide.decorator('$animate', function($delegate) { - $animateCore = $delegate; + $originalAnimate = $delegate; return $delegate; }); })); @@ -4892,30 +4892,31 @@ describe("ngAnimate", function() { function setupClassManipulationSpies() { inject(function($animate) { - addClass = spyOn($animateCore, '$$addClassImmediately').andCallThrough(); - removeClass = spyOn($animateCore, '$$removeClassImmediately').andCallThrough(); + addClass = spyOn($originalAnimate, '$$addClassImmediately').andCallThrough(); + removeClass = spyOn($originalAnimate, '$$removeClassImmediately').andCallThrough(); }); } function setupClassManipulationLogger(log) { inject(function($animate) { - var addClassImmediately = $animateCore.$$addClassImmediately; - var removeClassImmediately = $animateCore.$$removeClassImmediately; - addClass = spyOn($animateCore, '$$addClassImmediately').andCallFake(function(element, classes) { + var addClassImmediately = $originalAnimate.$$addClassImmediately; + var removeClassImmediately = $originalAnimate.$$removeClassImmediately; + addClass = spyOn($originalAnimate, '$$addClassImmediately').andCallFake(function(element, classes) { var names = classes; if (Object.prototype.toString.call(classes) === '[object Array]') names = classes.join( ' '); log('addClass(' + names + ')'); - return addClassImmediately.call($animateCore, element, classes); + return addClassImmediately.call($originalAnimate, element, classes); }); - removeClass = spyOn($animateCore, '$$removeClassImmediately').andCallFake(function(element, classes) { + removeClass = spyOn($originalAnimate, '$$removeClassImmediately').andCallFake(function(element, classes) { var names = classes; if (Object.prototype.toString.call(classes) === '[object Array]') names = classes.join( ' '); log('removeClass(' + names + ')'); - return removeClassImmediately.call($animateCore, element, classes); + return removeClassImmediately.call($originalAnimate, element, classes); }); }); } + it('should defer class manipulation until end of digest', inject(function($rootScope, $animate, log) { setupClassManipulationLogger(log); element = jqLite('

test

'); @@ -4945,9 +4946,9 @@ describe("ngAnimate", function() { })); - it('should defer class manipulation until digest outside of digest', inject(function($rootScope, $animate, log) { + it('should defer class manipulation until postDigest when outside of digest', inject(function($rootScope, $animate, log) { setupClassManipulationLogger(log); - element = jqLite('

test

'); + element = jqLite('

test

'); $animate.addClass(element, 'test-class1'); $animate.removeClass(element, 'test-class1'); @@ -4955,27 +4956,55 @@ describe("ngAnimate", function() { $animate.setClass(element, 'test-class3', 'test-class4'); expect(log).toEqual([]); - $rootScope.$digest(); - expect(log).toEqual(['addClass(test-class2 test-class3)']); + expect(log).toEqual(['addClass(test-class2 test-class3)', 'removeClass(test-class4)']); expect(element).not.toHaveClass('test-class1'); expect(element).toHaveClass('test-class2'); expect(element).toHaveClass('test-class3'); expect(addClass.callCount).toBe(1); - expect(removeClass.callCount).toBe(0); + expect(removeClass.callCount).toBe(1); })); - it('should return a promise which is resolved on a different turn digest', inject(function(log, $animate, $browser, $rootScope) { + it('should perform class manipulation in expected order at end of digest', inject(function($rootScope, $animate, log) { + element = jqLite('

test

'); + + setupClassManipulationLogger(log); + + $rootScope.$apply(function() { + $animate.addClass(element, 'test-class1'); + $animate.addClass(element, 'test-class2'); + $animate.removeClass(element, 'test-class1'); + $animate.removeClass(element, 'test-class3'); + $animate.addClass(element, 'test-class3'); + }); + expect(log).toEqual(['addClass(test-class2)']); + })); + + + it('should return a promise which is resolved on a different turn', inject(function(log, $animate, $browser, $rootScope) { element = jqLite('

test

'); $animate.addClass(element, 'test1').then(log.fn('addClass(test1)')); $animate.removeClass(element, 'test2').then(log.fn('removeClass(test2)')); $rootScope.$digest(); + expect(log).toEqual([]); $browser.defer.flush(); expect(log).toEqual(['addClass(test1)', 'removeClass(test2)']); + + log.reset(); + element = jqLite('

test

'); + + $rootScope.$apply(function() { + $animate.addClass(element, 'test3').then(log.fn('addClass(test3)')); + $animate.removeClass(element, 'test4').then(log.fn('removeClass(test4)')); + expect(log).toEqual([]); + }); + + $browser.defer.flush(); + expect(log).toEqual(['addClass(test3)', 'removeClass(test4)']); })); @@ -5006,10 +5035,10 @@ describe("ngAnimate", function() { })); - it('should defer class manipulation until digest outside of digest for SVG', inject(function($rootScope, $animate, log) { + it('should defer class manipulation until postDigest when outside of digest for SVG', inject(function($rootScope, $animate, log) { if (!window.SVGElement) return; setupClassManipulationLogger(log); - element = jqLite(''); + element = jqLite(''); var target = element.children().eq(0); $animate.addClass(target, 'test-class1'); @@ -5018,15 +5047,32 @@ describe("ngAnimate", function() { $animate.setClass(target, 'test-class3', 'test-class4'); expect(log).toEqual([]); - $rootScope.$digest(); - expect(log).toEqual(['addClass(test-class2 test-class3)']); + expect(log).toEqual(['addClass(test-class2 test-class3)', 'removeClass(test-class4)']); expect(target).not.toHaveClass('test-class1'); expect(target).toHaveClass('test-class2'); expect(target).toHaveClass('test-class3'); expect(addClass.callCount).toBe(1); - expect(removeClass.callCount).toBe(0); + expect(removeClass.callCount).toBe(1); + })); + + + it('should perform class manipulation in expected order at end of digest for SVG', inject(function($rootScope, $animate, log) { + if (!window.SVGElement) return; + element = jqLite(''); + var target = element.children().eq(0); + + setupClassManipulationLogger(log); + + $rootScope.$apply(function() { + $animate.addClass(target, 'test-class1'); + $animate.addClass(target, 'test-class2'); + $animate.removeClass(target, 'test-class1'); + $animate.removeClass(target, 'test-class3'); + $animate.addClass(target, 'test-class3'); + }); + expect(log).toEqual(['addClass(test-class2)']); })); }); }); From 3e2ad02c4666072f7b7980e6efaa0c19d2f0801c Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Tue, 7 Oct 2014 22:42:59 -0400 Subject: [PATCH 5/9] refactor($compile): use labaled variables to represent nodeType values This also does some cleanup in $animate --- src/.jshintrc | 7 +++++++ src/Angular.js | 18 ++++++++++++++---- src/jqLite.js | 16 ++++++++-------- src/ng/animate.js | 19 +++---------------- src/ng/compile.js | 16 ++++++++-------- 5 files changed, 40 insertions(+), 36 deletions(-) diff --git a/src/.jshintrc b/src/.jshintrc index e2d6c01b7474..d11760dc1e26 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -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, diff --git a/src/Angular.js b/src/Angular.js index 2a7540eb224e..2e37d8925bf8 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -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, */ //////////////////////////////////// @@ -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; } @@ -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('
').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); }); @@ -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; diff --git a/src/jqLite.js b/src/jqLite.js index b199f390e329..0ed760801b73 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -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) { @@ -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]; @@ -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); } } @@ -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; } @@ -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; @@ -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); @@ -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); @@ -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) { diff --git a/src/ng/animate.js b/src/ng/animate.js index f00c9dd33a90..9b5c5a82b13f 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -84,20 +84,6 @@ var $AnimateProvider = ['$provide', function($provide) { this.$get = ['$$q', '$$asyncCallback', '$rootScope', function($$q, $$asyncCallback, $rootScope) { var currentDefer; - var ELEMENT_NODE = 1; - - function extractElementNodes(element) { - var elements = new Array(element.length); - var count = 0; - for(var i = 0; i < element.length; i++) { - var elm = element[i]; - if (elm.nodeType == ELEMENT_NODE) { - elements[count++] = elm; - } - } - elements.length = count; - return jqLite(elements); - } function runAnimationPostDigest(fn) { var cancelFn, defer = $$q.defer(); @@ -299,7 +285,8 @@ var $AnimateProvider = ['$provide', function($provide) { setClass : function(element, add, remove, runSynchronously) { var self = this; var STORAGE_KEY = '$$animateClasses'; - element = extractElementNodes(jqLite(element)); + var createdCache = false; + element = jqLite(element); if (runSynchronously) { self.$$addClassImmediately(element, add); @@ -312,7 +299,7 @@ var $AnimateProvider = ['$provide', function($provide) { cache = { classes: {} }; - var createdCache = true; + createdCache = true; } var classes = cache.classes; diff --git a/src/ng/compile.js b/src/ng/compile.js index 8f93f7f6d76a..043bab07c860 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -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 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('').parent()[0]; } }); @@ -1344,7 +1344,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { className; switch(nodeType) { - case 1: /* Element */ + case NODE_TYPE_ELEMENT: /* Element */ // use the node name: addDirective(directives, directiveNormalize(nodeName_(node)), 'E', maxPriority, ignoreDirective); @@ -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) { @@ -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--; } @@ -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, ''); @@ -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); @@ -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); } } From bf84fd1af21a69c462f2cc1cf6d6106560a1712d Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Wed, 8 Oct 2014 14:18:22 -0400 Subject: [PATCH 6/9] chore(ngAnimate): add TODO messages indicating desire to remove hack --- src/ng/animate.js | 2 ++ src/ngAnimate/animate.js | 3 +++ 2 files changed, 5 insertions(+) diff --git a/src/ng/animate.js b/src/ng/animate.js index 9b5c5a82b13f..7a27b0dbe890 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -289,6 +289,8 @@ var $AnimateProvider = ['$provide', function($provide) { element = jqLite(element); if (runSynchronously) { + // 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(); diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index e59e63ba3602..aab2d3ca9bdc 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -979,6 +979,9 @@ angular.module('ngAnimate', ['ng']) element = stripCommentsFromElement(element); if (classBasedAnimationsBlocked(element)) { + // 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); } From e954d22a564acdfd8693fb4e901a7abacabd2d67 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Wed, 8 Oct 2014 14:22:48 -0400 Subject: [PATCH 7/9] perf($animate): don't join classes before it's necessary in resolveElementClasses In ngAnimate, we can't do this because the behaviour is exposed via the API. But in core, we can avoid a bit of work. --- src/ng/animate.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/animate.js b/src/ng/animate.js index 7a27b0dbe890..13a0049a77dc 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -116,7 +116,7 @@ var $AnimateProvider = ['$provide', function($provide) { } }); - return (toAdd.length + toRemove.length) > 0 && [toAdd.join(' '), toRemove.join(' ')]; + return (toAdd.length + toRemove.length) > 0 && [toAdd.length && toAdd, toRemove.length && toRemove]; } function cachedClassManipulation(cache, classes, op) { From cbf7d922ecc715d2ec25478dd6dbc88592ebea37 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Wed, 8 Oct 2014 14:34:26 -0400 Subject: [PATCH 8/9] perf($animate): access DOM less in resolveElementClasses Previously we were reading DOM attributes frequently, now we can do it just once. --- src/ng/animate.js | 8 +++++++- src/ngAnimate/animate.js | 7 ++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/ng/animate.js b/src/ng/animate.js index 13a0049a77dc..f26099e7dac7 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -102,8 +102,14 @@ var $AnimateProvider = ['$provide', function($provide) { 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 = jqLiteHasClass(element[0], className); + var hasClass = hasClasses[className] === true; // 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 diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index aab2d3ca9bdc..55eb45a41ac7 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -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 From 6949e5d2f6471650f437ba947891ba41e8511061 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Wed, 8 Oct 2014 14:35:55 -0400 Subject: [PATCH 9/9] chore(AngularPublic): remove `$$hasClass` from angular exports It was previously used for ngAnimate, but is no longer needed --- src/AngularPublic.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/AngularPublic.js b/src/AngularPublic.js index aaa4bb0376ac..c263e1a9c8cf 100644 --- a/src/AngularPublic.js +++ b/src/AngularPublic.js @@ -140,8 +140,7 @@ function publishExternalAPI(angular){ 'getTestability': getTestability, '$$minErr': minErr, '$$csp': csp, - 'reloadWithDebugInfo': reloadWithDebugInfo, - '$$hasClass': jqLiteHasClass + 'reloadWithDebugInfo': reloadWithDebugInfo }); angularModule = setupModuleLoader(window);