From 1933488ca68f418d8cc290335036e6353fc230ad Mon Sep 17 00:00:00 2001 From: Chris Chua Date: Sun, 15 Dec 2013 23:04:56 -0800 Subject: [PATCH] fix(modal): support close animation Tests now have to wait for animation before checking for open modals. Tests should actually check for the 'in' class. Setting up both the transition end handler and a timeout ensures that the modal is always closed even if there was an issue with the transition. This was the implementation used by Twitter Bootstrap modal JS code. Note that unbinding the transition end event within the event handler isn't necessary, and, worse is currently buggy in jqLite (see https://github.com/angular/angular.js/pull/5109 ). Note that the scope is already destroyed when the dom is removed so the $destroy call isn't needed. Closes #1341 --- src/modal/modal.js | 61 ++++++++++++++++++++++++++++-------- src/modal/test/modal.spec.js | 12 +++++++ 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/src/modal/modal.js b/src/modal/modal.js index f04e02d58d..ce1ec83d34 100644 --- a/src/modal/modal.js +++ b/src/modal/modal.js @@ -1,4 +1,4 @@ -angular.module('ui.bootstrap.modal', []) +angular.module('ui.bootstrap.modal', ['ui.bootstrap.transition']) /** * A helper, internal data structure that acts as a map but also allows getting / removing @@ -78,7 +78,8 @@ angular.module('ui.bootstrap.modal', []) return { restrict: 'EA', scope: { - index: '@' + index: '@', + animate: '=' }, replace: true, transclude: true, @@ -105,8 +106,8 @@ angular.module('ui.bootstrap.modal', []) }; }]) - .factory('$modalStack', ['$document', '$compile', '$rootScope', '$$stackedMap', - function ($document, $compile, $rootScope, $$stackedMap) { + .factory('$modalStack', ['$transition', '$timeout', '$document', '$compile', '$rootScope', '$$stackedMap', + function ($transition, $timeout, $document, $compile, $rootScope, $$stackedMap) { var OPENED_MODAL_CLASS = 'modal-open'; @@ -140,20 +141,53 @@ angular.module('ui.bootstrap.modal', []) openedWindows.remove(modalInstance); //remove window DOM element - modalWindow.modalDomEl.remove(); + removeAfterAnimate(modalWindow.modalDomEl, modalWindow.modalScope, 300, checkRemoveBackdrop); body.toggleClass(OPENED_MODAL_CLASS, openedWindows.length() > 0); + } + + function checkRemoveBackdrop() { + //remove backdrop if no longer needed + if (backdropDomEl && backdropIndex() == -1) { + var backdropScopeRef = backdropScope; + removeAfterAnimate(backdropDomEl, backdropScope, 150, function () { + backdropScopeRef.$destroy(); + backdropScopeRef = null; + }); + backdropDomEl = undefined; + backdropScope = undefined; + } + } - //remove backdrop if no longer needed - if (backdropDomEl && backdropIndex() == -1) { - backdropDomEl.remove(); - backdropDomEl = undefined; + function removeAfterAnimate(domEl, scope, emulateTime, done) { + // Closing animation + scope.animate = false; - backdropScope.$destroy(); - backdropScope = undefined; + var transitionEndEventName = $transition.transitionEndEventName; + if (transitionEndEventName) { + // transition out + var timeout = $timeout(afterAnimating, emulateTime); + + domEl.bind(transitionEndEventName, function () { + $timeout.cancel(timeout); + afterAnimating(); + scope.$apply(); + }); + } else { + // Ensure this call is async + $timeout(afterAnimating, 0); } - //destroy scope - modalWindow.modalScope.$destroy(); + function afterAnimating() { + if (afterAnimating.done) { + return; + } + afterAnimating.done = true; + + domEl.remove(); + if (done) { + done(); + } + } } $document.bind('keydown', function (evt) { @@ -191,6 +225,7 @@ angular.module('ui.bootstrap.modal', []) var angularDomEl = angular.element('
'); angularDomEl.attr('window-class', modal.windowClass); angularDomEl.attr('index', openedWindows.length() - 1); + angularDomEl.attr('animate', 'animate'); angularDomEl.html(modal.content); var modalDomEl = $compile(angularDomEl)(modal.scope); diff --git a/src/modal/test/modal.spec.js b/src/modal/test/modal.spec.js index 96abff51a1..0ffd3a29bd 100644 --- a/src/modal/test/modal.spec.js +++ b/src/modal/test/modal.spec.js @@ -104,6 +104,7 @@ describe('$modal', function () { function dismiss(modal, reason) { modal.dismiss(reason); + $timeout.flush(); $rootScope.$digest(); } @@ -120,6 +121,9 @@ describe('$modal', function () { dismiss(modal, 'closing in test'); expect($document).toHaveModalsOpen(0); + + // Backdrop animation + $timeout.flush(); expect($document).not.toHaveBackdrop(); }); @@ -135,6 +139,9 @@ describe('$modal', function () { dismiss(modal, 'closing in test'); expect($document).toHaveModalsOpen(0); + + // Backdrop animation + $timeout.flush(); expect($document).not.toHaveBackdrop(); }); @@ -144,6 +151,7 @@ describe('$modal', function () { expect($document).toHaveModalsOpen(1); triggerKeyDown($document, 27); + $timeout.flush(); $rootScope.$digest(); expect($document).toHaveModalsOpen(0); @@ -155,6 +163,7 @@ describe('$modal', function () { expect($document).toHaveModalsOpen(1); $document.find('body > div.modal').click(); + $timeout.flush(); $rootScope.$digest(); expect($document).toHaveModalsOpen(0); @@ -386,6 +395,9 @@ describe('$modal', function () { expect(backdropEl).toHaveClass('in'); dismiss(modal); + // Backdrop animation + $timeout.flush(); + modal = open({ template: '
With backdrop
' }); backdropEl = $document.find('body > div.modal-backdrop'); expect(backdropEl).not.toHaveClass('in');