From ea4120bf3524c270cebbf3e4dd0ba31552de6f1f Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Tue, 29 Mar 2016 19:57:30 +0200 Subject: [PATCH 1/2] feat(ngAnimate): let $animate.off() remove all listeners for an element --- src/ng/animate.js | 7 +++- src/ngAnimate/animateQueue.js | 27 ++++++++++----- test/ngAnimate/animateSpec.js | 62 +++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 10 deletions(-) diff --git a/src/ng/animate.js b/src/ng/animate.js index 96df569176c8..67c852eb0eae 100644 --- a/src/ng/animate.js +++ b/src/ng/animate.js @@ -326,6 +326,9 @@ var $AnimateProvider = ['$provide', function($provide) { * // remove all the animation event listeners listening for `enter` * $animate.off('enter'); * + * // remove listeners for all animation events from the container element + * $animate.off(container); + * * // remove all the animation event listeners listening for `enter` on the given element and its children * $animate.off('enter', container); * @@ -334,7 +337,9 @@ var $AnimateProvider = ['$provide', function($provide) { * $animate.off('enter', container, callback); * ``` * - * @param {string} event the animation event (e.g. enter, leave, move, addClass, removeClass, etc...) + * @param {string|DOMElement} event|container the animation event (e.g. enter, leave, move, + * addClass, removeClass, etc...), or the container element. If it is the element, all other + * arguments are ignored. * @param {DOMElement=} container the container element the event listener was placed on * @param {Function=} callback the callback function that was registered as the listener */ diff --git a/src/ngAnimate/animateQueue.js b/src/ngAnimate/animateQueue.js index 07fa81f3f9e6..e9e785117818 100644 --- a/src/ngAnimate/animateQueue.js +++ b/src/ngAnimate/animateQueue.js @@ -199,6 +199,15 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { return matches; } + function filterFromRegistry(list, matchContainer, matchCallback) { + var containerNode = extractElementNode(matchContainer); + return list.filter(function(entry) { + var isMatch = entry.node === containerNode && + (!matchCallback || entry.callback === matchCallback); + return !isMatch; + }); + } + var $animate = { on: function(event, container, callback) { var node = extractElementNode(container); @@ -215,21 +224,21 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { }, off: function(event, container, callback) { + if (arguments.length === 1 && !angular.isString(arguments[0])) { + container = arguments[0]; + for (var eventType in callbackRegistry) { + callbackRegistry[eventType] = filterFromRegistry(callbackRegistry[eventType], container); + } + + return; + } + var entries = callbackRegistry[event]; if (!entries) return; callbackRegistry[event] = arguments.length === 1 ? null : filterFromRegistry(entries, container, callback); - - function filterFromRegistry(list, matchContainer, matchCallback) { - var containerNode = extractElementNode(matchContainer); - return list.filter(function(entry) { - var isMatch = entry.node === containerNode && - (!matchCallback || entry.callback === matchCallback); - return !isMatch; - }); - } }, pin: function(element, parentElement) { diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index 0ae259a67d96..2363dc458dfd 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -1896,6 +1896,68 @@ describe("animations", function() { expect(count).toBe(3); })); + it('should remove all event listeners for an element when $animate.off(element) is called', + inject(function($animate, $rootScope, $rootElement, $document, $$rAF) { + + element = jqLite('
'); + var otherElement = jqLite('
'); + $rootElement.append(otherElement); + + var count = 0; + var runner; + $animate.on('enter', element, counter); + $animate.on('leave', element, counter); + $animate.on('addClass', element, counter); + $animate.on('addClass', otherElement, counter); + + function counter(element, phase) { + count++; + } + + runner = $animate.enter(element, $rootElement); + $rootScope.$digest(); + $animate.flush(); + runner.end(); + + runner = $animate.addClass(element, 'blue'); + $rootScope.$digest(); + $animate.flush(); + + runner.end(); + $$rAF.flush(); + + expect(count).toBe(4); + + $animate.off(element); + + runner = $animate.enter(element, $rootElement); + $animate.flush(); + expect(capturedAnimation[1]).toBe('enter'); + runner.end(); + + runner = $animate.addClass(element, 'red'); + $animate.flush(); + expect(capturedAnimation[1]).toBe('addClass'); + runner.end(); + + runner = $animate.leave(element); + $animate.flush(); + expect(capturedAnimation[1]).toBe('leave'); + runner.end(); + + // Try to flush all remaining callbacks + expect(function() { + $$rAF.flush(); + }).toThrowError('No rAF callbacks present'); + + expect(count).toBe(4); + + // Check that other elements' event listeners are not affected + $animate.addClass(otherElement, 'green'); + $animate.flush(); + expect(count).toBe(5); + })); + it('should fire a `start` callback when the animation starts with the matching element', inject(function($animate, $rootScope, $rootElement, $document) { From f8e6a4bcba33e70c0d09cfedd17be628af33c206 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Tue, 29 Mar 2016 16:56:39 +0200 Subject: [PATCH 2/2] fix(ngAnimate): remove event listeners only after all listeners have been called The fix for removing the event callbacks on destroy introduced in ce7f400011e1e2e1b9316f18ce87b87b79d878b4 removed the events too early, so that the event callbacks for the "close" phase in "leave" animations were not called. This commit fixes the behavior so that the event callbacks are only removed during on('$destroy') when no animation is currently active on the element. When an animation is active, the event callbacks will be removed after all callbacks have run, and if the element has no parent (has been removed from the DOM). Closes #14321 --- src/ngAnimate/animateQueue.js | 20 +- test/ngAnimate/animateSpec.js | 342 +++++++++++++++++++++++----------- 2 files changed, 248 insertions(+), 114 deletions(-) diff --git a/src/ngAnimate/animateQueue.js b/src/ngAnimate/animateQueue.js index e9e785117818..5577d6f919f2 100644 --- a/src/ngAnimate/animateQueue.js +++ b/src/ngAnimate/animateQueue.js @@ -208,6 +208,14 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { }); } + function cleanupEventListeners(phase, element) { + if (phase === 'close' && !element[0].parentNode) { + // If the element is not attached to a parentNode, it has been removed by + // the domOperation, and we can safely remove the event callbacks + $animate.off(element); + } + } + var $animate = { on: function(event, container, callback) { var node = extractElementNode(container); @@ -219,7 +227,14 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { // Remove the callback when the element is removed from the DOM jqLite(container).on('$destroy', function() { - $animate.off(event, container, callback); + var animationDetails = activeAnimationsLookup.get(node); + + if (!animationDetails) { + // If there's an animation ongoing, the callback calling code will remove + // the event listeners. If we'd remove here, the callbacks would be removed + // before the animation ends + $animate.off(event, container, callback); + } }); }, @@ -552,7 +567,10 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { forEach(callbacks, function(callback) { callback(element, phase, data); }); + cleanupEventListeners(phase, element); }); + } else { + cleanupEventListeners(phase, element); } }); runner.progress(event, phase, data); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index 2363dc458dfd..2d1d89b256d5 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -1999,63 +1999,96 @@ describe("animations", function() { expect(capturedElement).toBe(element); })); - it('should remove all event listeners when the element is removed', - inject(function($animate, $rootScope, $rootElement) { - element = jqLite('
'); + they('should remove all event listeners when the element is removed via $prop', + ['leave()', 'remove()'], function(method) { + inject(function($animate, $rootScope, $rootElement, $$rAF) { - var count = 0; - var runner; + element = jqLite('
'); - $animate.on('enter', element, counter); - $animate.on('addClass', element[0], counter); + var count = 0; + var enterSpy = jasmine.createSpy(); + var addClassSpy = jasmine.createSpy(); + var runner; - function counter(element, phase) { - if (phase === 'start') { - count++; + $animate.on('enter', element, enterSpy); + $animate.on('addClass', element[0], addClassSpy); + + function counter(element, phase) { + if (phase === 'start') { + count++; + } } - } - runner = $animate.enter(element, $rootElement); - $rootScope.$digest(); - expect(capturedAnimation).toBeTruthy(); + runner = $animate.enter(element, $rootElement); + $rootScope.$digest(); + expect(capturedAnimation).toBeTruthy(); - $animate.flush(); - runner.end(); // Otherwise the class animation won't run because enter is still in progress - expect(count).toBe(1); + $animate.flush(); + expect(enterSpy.calls.count()).toBe(1); + expect(enterSpy.calls.mostRecent().args[1]).toBe('start'); - capturedAnimation = null; + runner.end(); // Otherwise the class animation won't run because enter is still in progress + $$rAF.flush(); + expect(enterSpy.calls.count()).toBe(2); + expect(enterSpy.calls.mostRecent().args[1]).toBe('close'); - $animate.addClass(element, 'blue'); - $rootScope.$digest(); - expect(capturedAnimation).toBeTruthy(); + enterSpy.calls.reset(); + capturedAnimation = null; - $animate.flush(); - expect(count).toBe(2); + runner = $animate.addClass(element, 'blue'); + $rootScope.$digest(); + expect(capturedAnimation).toBeTruthy(); - capturedAnimation = null; + $animate.flush(); + expect(addClassSpy.calls.count()).toBe(1); + expect(addClassSpy.calls.mostRecent().args[1]).toBe('start'); - element.remove(); + runner.end(); + $$rAF.flush(); + expect(addClassSpy.calls.count()).toBe(2); + expect(addClassSpy.calls.mostRecent().args[1]).toBe('close'); - runner = $animate.enter(element, $rootElement); - $rootScope.$digest(); + addClassSpy.calls.reset(); + capturedAnimation = null; - expect(capturedAnimation).toBeTruthy(); + if (method === 'leave()') { + runner = $animate.leave(element); + $animate.flush(); + runner.end(); + } else if (method === 'remove()') { + element.remove(); + } - $animate.flush(); - runner.end(); // Otherwise the class animation won't run because enter is still in progress + runner = $animate.enter(element, $rootElement); + $rootScope.$digest(); - expect(count).toBe(2); + $animate.flush(); + expect(enterSpy.calls.count()).toBe(0); - capturedAnimation = null; + runner.end(); // Otherwise the class animation won't run because enter is still in progress + expect(function() { + $$rAF.flush(); + }).toThrowError('No rAF callbacks present'); // Try to flush any callbacks + expect(enterSpy.calls.count()).toBe(0); - $animate.addClass(element, 'red'); - $rootScope.$digest(); - expect(capturedAnimation).toBeTruthy(); + capturedAnimation = null; - $animate.flush(); - expect(count).toBe(2); - })); + $animate.addClass(element, 'red'); + $rootScope.$digest(); + expect(capturedAnimation).toBeTruthy(); + + $animate.flush(); + expect(addClassSpy.calls.count()).toBe(0); + + runner.end(); + expect(function() { + $$rAF.flush(); + }).toThrowError('No rAF callbacks present'); // Try to flush any callbacks + expect(addClassSpy.calls.count()).toBe(0); + expect(enterSpy.calls.count()).toBe(0); + }); + }); it('should always detect registered callbacks after one postDigest has fired', inject(function($animate, $rootScope, $rootElement) { @@ -2120,104 +2153,187 @@ describe("animations", function() { } })); - it('leave: should remove the element even if another animation is called after', - inject(function($animate, $rootScope, $rootElement) { + describe('for leave', function() { - var outerContainer = jqLite('
'); - element = jqLite('
'); - outerContainer.append(element); - $rootElement.append(outerContainer); + it('should remove the element even if another animation is called afterwards', + inject(function($animate, $rootScope, $rootElement) { - var runner = $animate.leave(element, $rootElement); - $animate.removeClass(element,'rclass'); - $rootScope.$digest(); - runner.end(); - $animate.flush(); + var outerContainer = jqLite('
'); + element = jqLite('
'); + outerContainer.append(element); + $rootElement.append(outerContainer); - var isElementRemoved = !outerContainer[0].contains(element[0]); - expect(isElementRemoved).toBe(true); - })); + var runner = $animate.leave(element, $rootElement); + $animate.removeClass(element,'rclass'); + $rootScope.$digest(); + runner.end(); + $animate.flush(); - it('leave : should trigger a callback for an leave animation', - inject(function($animate, $rootScope, $$rAF, $rootElement, $document) { + var isElementRemoved = !outerContainer[0].contains(element[0]); + expect(isElementRemoved).toBe(true); + })); - var callbackTriggered = false; - $animate.on('leave', jqLite($document[0].body), function() { - callbackTriggered = true; - }); + they('should trigger callbacks when the listener is on the $prop element', ['same', 'parent'], + function(elementRelation) { + inject(function($animate, $rootScope, $$rAF, $rootElement, $document) { + var listenerElement, callbackSpy = jasmine.createSpy(); - element = jqLite('
'); - $rootElement.append(element); - $animate.leave(element, $rootElement); - $rootScope.$digest(); + element = jqLite('
'); + listenerElement = elementRelation === 'same' ? element : jqLite($document[0].body); + $animate.on('leave', listenerElement, callbackSpy); + $rootElement.append(element); + var runner = $animate.leave(element, $rootElement); + $rootScope.$digest(); - $$rAF.flush(); + $$rAF.flush(); - expect(callbackTriggered).toBe(true); - })); + expect(callbackSpy.calls.count()).toBe(1); + expect(callbackSpy.calls.mostRecent().args[1]).toBe('start'); + callbackSpy.calls.reset(); - it('leave : should not fire a callback if the element is outside of the given container', - inject(function($animate, $rootScope, $$rAF, $rootElement) { + runner.end(); + $$rAF.flush(); - var callbackTriggered = false; - var innerContainer = jqLite('
'); - $rootElement.append(innerContainer); + expect(callbackSpy.calls.count()).toBe(1); + expect(callbackSpy.calls.mostRecent().args[1]).toBe('close'); + }); + } + ); - $animate.on('leave', innerContainer, - function(element, phase, data) { - callbackTriggered = true; - }); + it('should trigger callbacks for a leave animation', + inject(function($animate, $rootScope, $$rAF, $rootElement, $document) { - element = jqLite('
'); - $rootElement.append(element); - $animate.leave(element, $rootElement); - $rootScope.$digest(); + var callbackSpy = jasmine.createSpy(); + $animate.on('leave', jqLite($document[0].body), callbackSpy); - expect(callbackTriggered).toBe(false); - })); + element = jqLite('
'); + $rootElement.append(element); + $animate.leave(element, $rootElement); + $rootScope.$digest(); - it('leave : should fire a `start` callback when the animation starts with the matching element', - inject(function($animate, $rootScope, $$rAF, $rootElement, $document) { + $$rAF.flush(); - element = jqLite('
'); + expect(callbackSpy).toHaveBeenCalled(); + expect(callbackSpy.calls.count()).toBe(1); + })); - var capturedState; - var capturedElement; - $animate.on('leave', jqLite($document[0].body), function(element, phase) { - capturedState = phase; - capturedElement = element; - }); + it('should trigger a callback for an leave animation (same element)', + inject(function($animate, $rootScope, $$rAF, $rootElement, $document) { - $rootElement.append(element); - $animate.leave(element, $rootElement); - $rootScope.$digest(); - $$rAF.flush(); + var callbackSpy = jasmine.createSpy(); - expect(capturedState).toBe('start'); - expect(capturedElement).toBe(element); - })); + element = jqLite('
'); + $animate.on('leave', element, callbackSpy); + $rootElement.append(element); + var runner = $animate.leave(element, $rootElement); + $rootScope.$digest(); - it('leave : should fire a `close` callback when the animation ends with the matching element', - inject(function($animate, $rootScope, $$rAF, $rootElement, $document) { + $$rAF.flush(); - element = jqLite('
'); + expect(callbackSpy.calls.count()).toBe(1); + expect(callbackSpy.calls.mostRecent().args[1]).toBe('start'); + callbackSpy.calls.reset(); - var capturedState; - var capturedElement; - $animate.on('leave', jqLite($document[0].body), function(element, phase) { - capturedState = phase; - capturedElement = element; - }); + runner.end(); + $$rAF.flush(); - $rootElement.append(element); - var runner = $animate.leave(element, $rootElement); - $rootScope.$digest(); - runner.end(); - $$rAF.flush(); + expect(callbackSpy.calls.count()).toBe(1); + expect(callbackSpy.calls.mostRecent().args[1]).toBe('close'); + })); - expect(capturedState).toBe('close'); - expect(capturedElement).toBe(element); - })); + it('should not fire a callback if the element is outside of the given container', + inject(function($animate, $rootScope, $$rAF, $rootElement) { + + var callbackTriggered = false; + var innerContainer = jqLite('
'); + $rootElement.append(innerContainer); + + $animate.on('leave', innerContainer, + function(element, phase, data) { + callbackTriggered = true; + }); + + element = jqLite('
'); + $rootElement.append(element); + $animate.leave(element, $rootElement); + $rootScope.$digest(); + + expect(callbackTriggered).toBe(false); + })); + + it('should fire a `start` callback when the animation starts', + inject(function($animate, $rootScope, $$rAF, $rootElement, $document) { + + element = jqLite('
'); + + var capturedState; + var capturedElement; + $animate.on('leave', jqLite($document[0].body), function(element, phase) { + capturedState = phase; + capturedElement = element; + }); + + $rootElement.append(element); + $animate.leave(element, $rootElement); + $rootScope.$digest(); + $$rAF.flush(); + + expect(capturedState).toBe('start'); + expect(capturedElement).toBe(element); + })); + + it('should fire a `close` callback when the animation ends', + inject(function($animate, $rootScope, $$rAF, $rootElement, $document) { + + element = jqLite('
'); + + var capturedState; + var capturedElement; + $animate.on('leave', jqLite($document[0].body), function(element, phase) { + capturedState = phase; + capturedElement = element; + }); + + $rootElement.append(element); + var runner = $animate.leave(element, $rootElement); + $rootScope.$digest(); + runner.end(); + $$rAF.flush(); + + expect(capturedState).toBe('close'); + expect(capturedElement).toBe(element); + })); + + it('should remove all event listeners after all callbacks for the "leave:close" phase have been called', + inject(function($animate, $rootScope, $rootElement, $$rAF) { + + var leaveSpy = jasmine.createSpy(); + var addClassSpy = jasmine.createSpy(); + + element = jqLite('
'); + $animate.on('leave', element, leaveSpy); + $animate.on('addClass', element, addClassSpy); + $rootElement.append(element); + var runner = $animate.leave(element, $rootElement); + $animate.flush(); + + runner.end(); + $$rAF.flush(); + + expect(leaveSpy.calls.mostRecent().args[1]).toBe('close'); + + $animate.addClass(element, 'blue'); + + $animate.flush(); + runner.end(); + expect(function() { + $$rAF.flush(); + }).toThrowError('No rAF callbacks present'); + + expect(addClassSpy.calls.count()).toBe(0); + })); + + }); they('should trigger a callback for a $prop animation if the listener is on the document', ['enter', 'leave'], function($event) {