Skip to content

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 ).

Closes angular-ui#1341
  • Loading branch information
chrisirhc committed Dec 16, 2013
1 parent 41bea46 commit ae7cd5a
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 7 deletions.
32 changes: 25 additions & 7 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 @@ -101,8 +101,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 backdropjqLiteEl, backdropDomEl;
var backdropScope = $rootScope.$new(true);
Expand Down Expand Up @@ -133,16 +133,34 @@ angular.module('ui.bootstrap.modal', [])
openedWindows.remove(modalInstance);

//remove window DOM element
modalWindow.modalDomEl.remove();
// Scope should be destroyed when element is removed
removeAfterAnimating(modalWindow.modalDomEl, modalWindow.modalScope);

//remove backdrop if no longer needed
if (backdropDomEl && backdropIndex() == -1) {
backdropDomEl.remove();
removeAfterAnimating(backdropDomEl, backdropScope);
backdropDomEl = undefined;
}
}

//destroy scope
modalWindow.modalScope.$destroy();
function removeAfterAnimating(domEl, scope) {
scope.$destroy();
domEl.removeClass('in');

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Dec 23, 2013

So, the real reason why I didn't get round to adding support for close animations is this one line of code - or rather - the name of the CSS class "hard-coded" in JavaScript. I was kind of dreaming that we can write "CSS-free" JavaScript....


var transitionEndEventName = $transition.transitionEndEventName;
if (transitionEndEventName) {
// transition out
var timeout = $timeout(function () {
domEl.remove();
}, 500);

domEl.bind(transitionEndEventName, function () {
$timeout.cancel(timeout);
domEl.remove();
});
} else {
domEl.remove();
}
}

$document.bind('keydown', function (evt) {
Expand Down
3 changes: 3 additions & 0 deletions src/modal/test/modal.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ describe('$modal', function () {

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

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

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

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

$document.find('body > div.modal-backdrop').click();
$timeout.flush();
$rootScope.$digest();

expect($document).toHaveModalsOpen(0);
Expand Down

0 comments on commit ae7cd5a

Please sign in to comment.