From d9157849df224a3a8d2e0bf03099d137f51499f6 Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Mon, 7 Dec 2015 20:11:06 +0100 Subject: [PATCH] fix($animateCss): remove animation end event listeners on close Previously the transition/animation end events were not removed when the animation was closed. This normally didn't matter, because the close function knows the animations are closed and won't do work twice. However, the listeners themselves do computation that could fail when the event was missing some data, for example when the event was triggered instead of natural. This commit includes the fix for a bug that was introduced by this change and landed on master separately: https://github.com/angular/angular.js/commit/959f2bbb2d12c23a74902433c6247290d8f2fb89 Closes #13672 --- src/ngAnimate/animateCss.js | 62 +++++++------ test/ngAnimate/.jshintrc | 6 +- test/ngAnimate/animateCssSpec.js | 152 +++++++++++++++++++++++++++++-- 3 files changed, 182 insertions(+), 38 deletions(-) diff --git a/src/ngAnimate/animateCss.js b/src/ngAnimate/animateCss.js index 664583198624..66134efe44e8 100644 --- a/src/ngAnimate/animateCss.js +++ b/src/ngAnimate/animateCss.js @@ -474,6 +474,8 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { var maxDelayTime; var maxDuration; var maxDurationTime; + var startTime; + var events = []; if (options.duration === 0 || (!$sniffer.animations && !$sniffer.transitions)) { return closeAndReturnNoopAnimator(); @@ -747,6 +749,11 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { options.onDone(); } + if (events && events.length) { + // Remove the transitionend / animationend listener(s) + element.off(events.join(' '), onAnimationProgress); + } + // if the preparation function fails then the promise is not setup if (runner) { runner.complete(!rejected); @@ -782,6 +789,30 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { }; } + function onAnimationProgress(event) { + event.stopPropagation(); + var ev = event.originalEvent || event; + var timeStamp = ev.$manualTimeStamp || ev.timeStamp || Date.now(); + + /* Firefox (or possibly just Gecko) likes to not round values up + * when a ms measurement is used for the animation */ + var elapsedTime = parseFloat(ev.elapsedTime.toFixed(ELAPSED_TIME_MAX_DECIMAL_PLACES)); + + /* $manualTimeStamp is a mocked timeStamp value which is set + * within browserTrigger(). This is only here so that tests can + * mock animations properly. Real events fallback to event.timeStamp, + * or, if they don't, then a timeStamp is automatically created for them. + * We're checking to see if the timeStamp surpasses the expected delay, + * but we're using elapsedTime instead of the timeStamp on the 2nd + * pre-condition since animationPauseds sometimes close off early */ + if (Math.max(timeStamp - startTime, 0) >= maxDelayTime && elapsedTime >= maxDuration) { + // we set this flag to ensure that if the transition is paused then, when resumed, + // the animation will automatically close itself since transitions cannot be paused. + animationCompleted = true; + close(); + } + } + function start() { if (animationClosed) return; if (!node.parentNode) { @@ -789,8 +820,6 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { return; } - var startTime, events = []; - // even though we only pause keyframe animations here the pause flag // will still happen when transitions are used. Only the transition will // not be paused since that is not possible. If the animation ends when @@ -931,7 +960,10 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { element.data(ANIMATE_TIMER_KEY, animationsData); } - element.on(events.join(' '), onAnimationProgress); + if (events.length) { + element.on(events.join(' '), onAnimationProgress); + } + if (options.to) { if (options.cleanupStyles) { registerRestorableStyles(restoreStyles, node, Object.keys(options.to)); @@ -953,30 +985,6 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { element.removeData(ANIMATE_TIMER_KEY); } } - - function onAnimationProgress(event) { - event.stopPropagation(); - var ev = event.originalEvent || event; - var timeStamp = ev.$manualTimeStamp || ev.timeStamp || Date.now(); - - /* Firefox (or possibly just Gecko) likes to not round values up - * when a ms measurement is used for the animation */ - var elapsedTime = parseFloat(ev.elapsedTime.toFixed(ELAPSED_TIME_MAX_DECIMAL_PLACES)); - - /* $manualTimeStamp is a mocked timeStamp value which is set - * within browserTrigger(). This is only here so that tests can - * mock animations properly. Real events fallback to event.timeStamp, - * or, if they don't, then a timeStamp is automatically created for them. - * We're checking to see if the timeStamp surpasses the expected delay, - * but we're using elapsedTime instead of the timeStamp on the 2nd - * pre-condition since animations sometimes close off early */ - if (Math.max(timeStamp - startTime, 0) >= maxDelayTime && elapsedTime >= maxDuration) { - // we set this flag to ensure that if the transition is paused then, when resumed, - // the animation will automatically close itself since transitions cannot be paused. - animationCompleted = true; - close(); - } - } } }; }]; diff --git a/test/ngAnimate/.jshintrc b/test/ngAnimate/.jshintrc index b307fb405952..fcaf04058079 100644 --- a/test/ngAnimate/.jshintrc +++ b/test/ngAnimate/.jshintrc @@ -8,6 +8,10 @@ "applyAnimationStyles": false, "applyAnimationFromStyles": false, "applyAnimationToStyles": false, - "applyAnimationClassesFactory": false + "applyAnimationClassesFactory": false, + "TRANSITIONEND_EVENT": false, + "TRANSITION_PROP": false, + "ANIMATION_PROP": false, + "ANIMATIONEND_EVENT": false } } diff --git a/test/ngAnimate/animateCssSpec.js b/test/ngAnimate/animateCssSpec.js index 42cc76c97239..e43dd89a912c 100644 --- a/test/ngAnimate/animateCssSpec.js +++ b/test/ngAnimate/animateCssSpec.js @@ -12,6 +12,16 @@ describe("ngAnimate $animateCss", function() { : expect(className).not.toMatch(regex); } + function keyframeProgress(element, duration, delay) { + browserTrigger(element, 'animationend', + { timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration }); + } + + function transitionProgress(element, duration, delay) { + browserTrigger(element, 'transitionend', + { timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration }); + } + var fakeStyle = { color: 'blue' }; @@ -355,16 +365,6 @@ describe("ngAnimate $animateCss", function() { assert.toHaveClass('ng-enter-active'); } - function keyframeProgress(element, duration, delay) { - browserTrigger(element, 'animationend', - { timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration }); - } - - function transitionProgress(element, duration, delay) { - browserTrigger(element, 'transitionend', - { timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration }); - } - beforeEach(inject(function($rootElement, $document) { element = jqLite('
'); $rootElement.append(element); @@ -1404,6 +1404,138 @@ describe("ngAnimate $animateCss", function() { expect(count.stagger).toBe(2); })); }); + + describe('transitionend/animationend event listeners', function() { + var element, elementOnSpy, elementOffSpy, progress; + + function setStyles(event) { + switch (event) { + case TRANSITIONEND_EVENT: + ss.addRule('.ng-enter', 'transition: 10s linear all;'); + progress = transitionProgress; + break; + case ANIMATIONEND_EVENT: + ss.addRule('.ng-enter', '-webkit-animation: animation 10s;' + + 'animation: animation 10s;'); + progress = keyframeProgress; + break; + } + } + + beforeEach(inject(function($rootElement, $document) { + element = jqLite('
'); + $rootElement.append(element); + jqLite($document[0].body).append($rootElement); + + elementOnSpy = spyOn(element, 'on').andCallThrough(); + elementOffSpy = spyOn(element, 'off').andCallThrough(); + })); + + they('should remove the $prop event listeners on cancel', + [TRANSITIONEND_EVENT, ANIMATIONEND_EVENT], function(event) { + inject(function($animateCss) { + + setStyles(event); + + var animator = $animateCss(element, { + event: 'enter', + structural: true + }); + + var runner = animator.start(); + triggerAnimationStartFrame(); + + expect(elementOnSpy).toHaveBeenCalledOnce(); + expect(elementOnSpy.mostRecentCall.args[0]).toBe(event); + + runner.cancel(); + + expect(elementOffSpy).toHaveBeenCalledOnce(); + expect(elementOffSpy.mostRecentCall.args[0]).toBe(event); + }); + }); + + they("should remove the $prop event listener when the animation is closed", + [TRANSITIONEND_EVENT, ANIMATIONEND_EVENT], function(event) { + inject(function($animateCss) { + + setStyles(event); + + var animator = $animateCss(element, { + event: 'enter', + structural: true + }); + + var runner = animator.start(); + triggerAnimationStartFrame(); + + expect(elementOnSpy).toHaveBeenCalledOnce(); + expect(elementOnSpy.mostRecentCall.args[0]).toBe(event); + + progress(element, 10); + + expect(elementOffSpy).toHaveBeenCalledOnce(); + expect(elementOffSpy.mostRecentCall.args[0]).toBe(event); + }); + }); + + they("should remove the $prop event listener when the closing timeout occurs", + [TRANSITIONEND_EVENT, ANIMATIONEND_EVENT], function(event) { + inject(function($animateCss, $timeout) { + + setStyles(event); + + var animator = $animateCss(element, { + event: 'enter', + structural: true + }); + + animator.start(); + triggerAnimationStartFrame(); + + expect(elementOnSpy).toHaveBeenCalledOnce(); + expect(elementOnSpy.mostRecentCall.args[0]).toBe(event); + + $timeout.flush(15000); + + expect(elementOffSpy).toHaveBeenCalledOnce(); + expect(elementOffSpy.mostRecentCall.args[0]).toBe(event); + }); + }); + + they("should not add or remove $prop event listeners when no animation styles are detected", + [TRANSITIONEND_EVENT, ANIMATIONEND_EVENT], function(event) { + inject(function($animateCss, $timeout) { + + progress = event === TRANSITIONEND_EVENT ? transitionProgress : keyframeProgress; + + // Make sure other event listeners are not affected + var otherEndSpy = jasmine.createSpy('otherEndSpy'); + element.on(event, otherEndSpy); + + expect(elementOnSpy).toHaveBeenCalledOnce(); + elementOnSpy.reset(); + + var animator = $animateCss(element, { + event: 'enter', + structural: true + }); + + expect(animator.$$willAnimate).toBeFalsy(); + + // This will close the animation because no styles have been detected + var runner = animator.start(); + triggerAnimationStartFrame(); + + expect(elementOnSpy).not.toHaveBeenCalled(); + expect(elementOffSpy).not.toHaveBeenCalled(); + + progress(element, 10); + expect(otherEndSpy).toHaveBeenCalledOnce(); + }); + }); + + }); }); it('should avoid applying the same cache to an element a follow-up animation is run on the same element',