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

Commit 20604e7

Browse files
NarretzNarretz
committed
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. Closes #10387
1 parent b7b06d8 commit 20604e7

File tree

3 files changed

+145
-37
lines changed

3 files changed

+145
-37
lines changed

‎src/ngAnimate/animateCss.js‎

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -474,6 +474,8 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
474474
var maxDelayTime;
475475
var maxDuration;
476476
var maxDurationTime;
477+
var startTime;
478+
var events = [];
477479

478480
if (options.duration === 0 || (!$sniffer.animations && !$sniffer.transitions)) {
479481
return closeAndReturnNoopAnimator();
@@ -747,6 +749,11 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
747749
options.onDone();
748750
}
749751

752+
// Remove the transitionend / animationend listener(s)
753+
if (events) {
754+
element.off(events.join(' '), onAnimationProgress);
755+
}
756+
750757
// if the preparation function fails then the promise is not setup
751758
if (runner) {
752759
runner.complete(!rejected);
@@ -782,15 +789,37 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
782789
};
783790
}
784791

792+
function onAnimationProgress(event) {
793+
event.stopPropagation();
794+
var ev = event.originalEvent || event;
795+
var timeStamp = ev.$manualTimeStamp || ev.timeStamp || Date.now();
796+
797+
/* Firefox (or possibly just Gecko) likes to not round values up
798+
* when a ms measurement is used for the animation */
799+
var elapsedTime = parseFloat(ev.elapsedTime.toFixed(ELAPSED_TIME_MAX_DECIMAL_PLACES));
800+
801+
/* $manualTimeStamp is a mocked timeStamp value which is set
802+
* within browserTrigger(). This is only here so that tests can
803+
* mock animations properly. Real events fallback to event.timeStamp,
804+
* or, if they don't, then a timeStamp is automatically created for them.
805+
* We're checking to see if the timeStamp surpasses the expected delay,
806+
* but we're using elapsedTime instead of the timeStamp on the 2nd
807+
* pre-condition since animationPauseds sometimes close off early */
808+
if (Math.max(timeStamp - startTime, 0) >= maxDelayTime && elapsedTime >= maxDuration) {
809+
// we set this flag to ensure that if the transition is paused then, when resumed,
810+
// the animation will automatically close itself since transitions cannot be paused.
811+
animationCompleted = true;
812+
close();
813+
}
814+
}
815+
785816
function start() {
786817
if (animationClosed) return;
787818
if (!node.parentNode) {
788819
close();
789820
return;
790821
}
791822

792-
var startTime, events = [];
793-
794823
// even though we only pause keyframe animations here the pause flag
795824
// will still happen when transitions are used. Only the transition will
796825
// not be paused since that is not possible. If the animation ends when
@@ -953,30 +982,6 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
953982
element.removeData(ANIMATE_TIMER_KEY);
954983
}
955984
}
956-
957-
function onAnimationProgress(event) {
958-
event.stopPropagation();
959-
var ev = event.originalEvent || event;
960-
var timeStamp = ev.$manualTimeStamp || ev.timeStamp || Date.now();
961-
962-
/* Firefox (or possibly just Gecko) likes to not round values up
963-
* when a ms measurement is used for the animation */
964-
var elapsedTime = parseFloat(ev.elapsedTime.toFixed(ELAPSED_TIME_MAX_DECIMAL_PLACES));
965-
966-
/* $manualTimeStamp is a mocked timeStamp value which is set
967-
* within browserTrigger(). This is only here so that tests can
968-
* mock animations properly. Real events fallback to event.timeStamp,
969-
* or, if they don't, then a timeStamp is automatically created for them.
970-
* We're checking to see if the timeStamp surpasses the expected delay,
971-
* but we're using elapsedTime instead of the timeStamp on the 2nd
972-
* pre-condition since animations sometimes close off early */
973-
if (Math.max(timeStamp - startTime, 0) >= maxDelayTime && elapsedTime >= maxDuration) {
974-
// we set this flag to ensure that if the transition is paused then, when resumed,
975-
// the animation will automatically close itself since transitions cannot be paused.
976-
animationCompleted = true;
977-
close();
978-
}
979-
}
980985
}
981986
};
982987
}];

‎test/ngAnimate/.jshintrc‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@
88
"applyAnimationStyles": false,
99
"applyAnimationFromStyles": false,
1010
"applyAnimationToStyles": false,
11-
"applyAnimationClassesFactory": false
11+
"applyAnimationClassesFactory": false,
12+
"TRANSITIONEND_EVENT": false,
13+
"TRANSITION_PROP": false,
14+
"ANIMATION_PROP": false,
15+
"ANIMATIONEND_EVENT": false
1216
}
1317
}

‎test/ngAnimate/animateCssSpec.js‎

Lines changed: 109 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,16 @@ describe("ngAnimate $animateCss", function() {
1212
: expect(className).not.toMatch(regex);
1313
}
1414

15+
function keyframeProgress(element, duration, delay) {
16+
browserTrigger(element, 'animationend',
17+
{ timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration });
18+
}
19+
20+
function transitionProgress(element, duration, delay) {
21+
browserTrigger(element, 'transitionend',
22+
{ timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration });
23+
}
24+
1525
var fakeStyle = {
1626
color: 'blue'
1727
};
@@ -355,16 +365,6 @@ describe("ngAnimate $animateCss", function() {
355365
assert.toHaveClass('ng-enter-active');
356366
}
357367

358-
function keyframeProgress(element, duration, delay) {
359-
browserTrigger(element, 'animationend',
360-
{ timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration });
361-
}
362-
363-
function transitionProgress(element, duration, delay) {
364-
browserTrigger(element, 'transitionend',
365-
{ timeStamp: Date.now() + ((delay || 1) * 1000), elapsedTime: duration });
366-
}
367-
368368
beforeEach(inject(function($rootElement, $document) {
369369
element = jqLite('<div></div>');
370370
$rootElement.append(element);
@@ -1404,6 +1404,105 @@ describe("ngAnimate $animateCss", function() {
14041404
expect(count.stagger).toBe(2);
14051405
}));
14061406
});
1407+
1408+
describe('transitionend/animationend event listeners', function() {
1409+
var element, elementOnSpy, elementOffSpy, progress;
1410+
1411+
function setStyles(event) {
1412+
switch (event) {
1413+
case TRANSITIONEND_EVENT:
1414+
ss.addRule('.ng-enter', 'transition: 10s linear all;');
1415+
progress = transitionProgress;
1416+
break;
1417+
case ANIMATIONEND_EVENT:
1418+
ss.addRule('.ng-enter', '-webkit-animation: animation 10s;' +
1419+
'animation: animation 10s;');
1420+
progress = keyframeProgress;
1421+
break;
1422+
}
1423+
}
1424+
1425+
beforeEach(inject(function($rootElement, $document) {
1426+
element = jqLite('<div></div>');
1427+
$rootElement.append(element);
1428+
jqLite($document[0].body).append($rootElement);
1429+
1430+
elementOnSpy = spyOn(element, 'on').andCallThrough();
1431+
elementOffSpy = spyOn(element, 'off').andCallThrough();
1432+
}));
1433+
1434+
they('should remove the $prop event listeners on cancel',
1435+
[TRANSITIONEND_EVENT, ANIMATIONEND_EVENT], function(event) {
1436+
inject(function($animateCss) {
1437+
1438+
setStyles(event);
1439+
1440+
var animator = $animateCss(element, {
1441+
event: 'enter',
1442+
structural: true
1443+
});
1444+
1445+
var runner = animator.start();
1446+
triggerAnimationStartFrame();
1447+
1448+
expect(elementOnSpy).toHaveBeenCalledOnce();
1449+
expect(elementOnSpy.mostRecentCall.args[0]).toBe(event);
1450+
1451+
runner.cancel();
1452+
1453+
expect(elementOffSpy).toHaveBeenCalledOnce();
1454+
expect(elementOffSpy.mostRecentCall.args[0]).toBe(event);
1455+
});
1456+
});
1457+
1458+
they("should remove the $prop event listener when the animation is closed",
1459+
[TRANSITIONEND_EVENT, ANIMATIONEND_EVENT], function(event) {
1460+
inject(function($animateCss) {
1461+
1462+
setStyles(event);
1463+
1464+
var animator = $animateCss(element, {
1465+
event: 'enter',
1466+
structural: true
1467+
});
1468+
1469+
var runner = animator.start();
1470+
triggerAnimationStartFrame();
1471+
1472+
expect(elementOnSpy).toHaveBeenCalledOnce();
1473+
expect(elementOnSpy.mostRecentCall.args[0]).toBe(event);
1474+
1475+
progress(element, 10);
1476+
1477+
expect(elementOffSpy).toHaveBeenCalledOnce();
1478+
expect(elementOffSpy.mostRecentCall.args[0]).toBe(event);
1479+
});
1480+
});
1481+
1482+
they("should remove the $prop event listener when the closing timeout occurs",
1483+
[TRANSITIONEND_EVENT, ANIMATIONEND_EVENT], function(event) {
1484+
inject(function($animateCss, $timeout) {
1485+
1486+
setStyles(event);
1487+
1488+
var animator = $animateCss(element, {
1489+
event: 'enter',
1490+
structural: true
1491+
});
1492+
1493+
animator.start();
1494+
triggerAnimationStartFrame();
1495+
1496+
expect(elementOnSpy).toHaveBeenCalledOnce();
1497+
expect(elementOnSpy.mostRecentCall.args[0]).toBe(event);
1498+
1499+
$timeout.flush(15000);
1500+
1501+
expect(elementOffSpy).toHaveBeenCalledOnce();
1502+
expect(elementOffSpy.mostRecentCall.args[0]).toBe(event);
1503+
});
1504+
});
1505+
});
14071506
});
14081507

14091508
it('should avoid applying the same cache to an element a follow-up animation is run on the same element',

0 commit comments

Comments
 (0)