Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Commit

Permalink
fix(modal): support close animation
Browse files Browse the repository at this point in the history
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
angular/angular.js#5109 ).

Note that the scope is already destroyed when the dom is removed so the
$destroy call isn't needed.

Closes #1341
  • Loading branch information
chrisirhc authored and pkozlowski-opensource committed Jan 7, 2014
1 parent f75f85a commit 1933488
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 13 deletions.
61 changes: 48 additions & 13 deletions src/modal/modal.js
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -78,7 +78,8 @@ angular.module('ui.bootstrap.modal', [])
return {
restrict: 'EA',
scope: {
index: '@'
index: '@',
animate: '='
},
replace: true,
transclude: true,
Expand All @@ -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';

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -191,6 +225,7 @@ angular.module('ui.bootstrap.modal', [])
var angularDomEl = angular.element('<div modal-window></div>');
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);
Expand Down
12 changes: 12 additions & 0 deletions src/modal/test/modal.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ describe('$modal', function () {

function dismiss(modal, reason) {
modal.dismiss(reason);
$timeout.flush();
$rootScope.$digest();
}

Expand All @@ -120,6 +121,9 @@ describe('$modal', function () {
dismiss(modal, 'closing in test');

expect($document).toHaveModalsOpen(0);

// Backdrop animation
$timeout.flush();
expect($document).not.toHaveBackdrop();
});

Expand All @@ -135,6 +139,9 @@ describe('$modal', function () {
dismiss(modal, 'closing in test');

expect($document).toHaveModalsOpen(0);

// Backdrop animation
$timeout.flush();
expect($document).not.toHaveBackdrop();
});

Expand All @@ -144,6 +151,7 @@ describe('$modal', function () {
expect($document).toHaveModalsOpen(1);

triggerKeyDown($document, 27);
$timeout.flush();
$rootScope.$digest();

expect($document).toHaveModalsOpen(0);
Expand All @@ -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);
Expand Down Expand Up @@ -386,6 +395,9 @@ describe('$modal', function () {
expect(backdropEl).toHaveClass('in');

dismiss(modal);
// Backdrop animation
$timeout.flush();

modal = open({ template: '<div>With backdrop</div>' });
backdropEl = $document.find('body > div.modal-backdrop');
expect(backdropEl).not.toHaveClass('in');
Expand Down

0 comments on commit 1933488

Please sign in to comment.