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

#2404 Modal open calls should complete in order #2443

Closed
wants to merge 5 commits into from
Closed

#2404 Modal open calls should complete in order #2443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4391d9a
chore rebase from master
Jul 11, 2014
6b1d373
Revert "chore rebase from master"
Aug 23, 2015
01a7664
Merge remote-tracking branch 'angular-ui/master' into 2404
Aug 23, 2015
cb075a3
Merge branch 'master' into 2404
Aug 28, 2015
f6b509d
style: whitespace and finally
Aug 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 59 additions & 47 deletions src/modal/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,11 @@ angular.module('ui.bootstrap.modal', [])
return promisesArr;
}

var promiseChain = null;
$modal.getPromiseChain = function() {
return promiseChain;
};

$modal.open = function (modalOptions) {

var modalResultDeferred = $q.defer();
Expand Down Expand Up @@ -637,63 +642,70 @@ angular.module('ui.bootstrap.modal', [])
var templateAndResolvePromise =
$q.all([getTemplatePromise(modalOptions)].concat(getResolvePromises(modalOptions.resolve)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about getting the top opened window's opened promise and appending it here?

You can get it by doing $modalStack.getTop().opened. You should also handle the case that there's no top window.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion, but I don't think it can work because $modalStack isn't updated until templateAndResolvePromise is resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reorganized things so that the opened promise is the only one burdened with chaining concerns. It shuffles things around a little, but it leaves the templateAndResolvePromise alone.


// Wait for the resolution of the existing promise chain.
// Then switch to our own combined promise dependency (regardless of how the previous modal fared).
// Then add to $modalStack and resolve opened.
// Finally clean up the chain variable if no subsequent modal has overwritten it.
var samePromise;
samePromise = promiseChain = $q.all([promiseChain])
.then(function() { return templateAndResolvePromise; }, function() { return templateAndResolvePromise; })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second callback is forcing a resolve if there is an error?

.then(function resolveSuccess(tplAndVars) {

var modalScope = (modalOptions.scope || $rootScope).$new();
modalScope.$close = modalInstance.close;
modalScope.$dismiss = modalInstance.dismiss;

modalScope.$on('$destroy', function() {
if (!modalScope.$$uibDestructionScheduled) {
modalScope.$dismiss('$uibUnscheduledDestruction');
}
});

templateAndResolvePromise.then(function resolveSuccess(tplAndVars) {

var modalScope = (modalOptions.scope || $rootScope).$new();
modalScope.$close = modalInstance.close;
modalScope.$dismiss = modalInstance.dismiss;

modalScope.$on('$destroy', function() {
if (!modalScope.$$uibDestructionScheduled) {
modalScope.$dismiss('$uibUnscheduledDestruction');
}
});
var ctrlInstance, ctrlLocals = {};
var resolveIter = 1;

var ctrlInstance, ctrlLocals = {};
var resolveIter = 1;
//controllers
if (modalOptions.controller) {
ctrlLocals.$scope = modalScope;
ctrlLocals.$modalInstance = modalInstance;
angular.forEach(modalOptions.resolve, function(value, key) {
ctrlLocals[key] = tplAndVars[resolveIter++];
});

//controllers
if (modalOptions.controller) {
ctrlLocals.$scope = modalScope;
ctrlLocals.$modalInstance = modalInstance;
angular.forEach(modalOptions.resolve, function(value, key) {
ctrlLocals[key] = tplAndVars[resolveIter++];
});
ctrlInstance = $controller(modalOptions.controller, ctrlLocals);
if (modalOptions.controllerAs) {
if (modalOptions.bindToController) {
angular.extend(ctrlInstance, modalScope);
}

ctrlInstance = $controller(modalOptions.controller, ctrlLocals);
if (modalOptions.controllerAs) {
if (modalOptions.bindToController) {
angular.extend(ctrlInstance, modalScope);
modalScope[modalOptions.controllerAs] = ctrlInstance;
}

modalScope[modalOptions.controllerAs] = ctrlInstance;
}
}

$modalStack.open(modalInstance, {
scope: modalScope,
deferred: modalResultDeferred,
renderDeferred: modalRenderDeferred,
content: tplAndVars[0],
animation: modalOptions.animation,
backdrop: modalOptions.backdrop,
keyboard: modalOptions.keyboard,
backdropClass: modalOptions.backdropClass,
windowClass: modalOptions.windowClass,
windowTemplateUrl: modalOptions.windowTemplateUrl,
size: modalOptions.size,
openedClass: modalOptions.openedClass
});
$modalStack.open(modalInstance, {
scope: modalScope,
deferred: modalResultDeferred,
renderDeferred: modalRenderDeferred,
content: tplAndVars[0],
animation: modalOptions.animation,
backdrop: modalOptions.backdrop,
keyboard: modalOptions.keyboard,
backdropClass: modalOptions.backdropClass,
windowClass: modalOptions.windowClass,
windowTemplateUrl: modalOptions.windowTemplateUrl,
size: modalOptions.size,
openedClass: modalOptions.openedClass
});
modalOpenedDeferred.resolve(true);

}, function resolveError(reason) {
modalResultDeferred.reject(reason);
});

templateAndResolvePromise.then(function() {
modalOpenedDeferred.resolve(true);
}, function(reason) {
modalOpenedDeferred.reject(reason);
modalResultDeferred.reject(reason);
})
.finally(function() {
if (promiseChain === samePromise) {
promiseChain = null;
}
});

return modalInstance;
Expand Down
86 changes: 84 additions & 2 deletions src/modal/test/modal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
describe('$modal', function () {
var $animate, $controllerProvider, $rootScope, $document, $compile, $templateCache, $timeout, $q;
var $modal, $modalProvider;
var $modal, $modalStack, $modalProvider;

beforeEach(module('ngAnimateMock'));
beforeEach(module('ui.bootstrap.modal'));
Expand All @@ -11,7 +11,7 @@ describe('$modal', function () {
$modalProvider = _$modalProvider_;
}));

beforeEach(inject(function(_$animate_, _$rootScope_, _$document_, _$compile_, _$templateCache_, _$timeout_, _$q_, _$modal_) {
beforeEach(inject(function(_$animate_, _$rootScope_, _$document_, _$compile_, _$templateCache_, _$timeout_, _$q_, _$modal_, _$modalStack_) {
$animate = _$animate_;
$rootScope = _$rootScope_;
$document = _$document_;
Expand All @@ -20,6 +20,7 @@ describe('$modal', function () {
$timeout = _$timeout_;
$q = _$q_;
$modal = _$modal_;
$modalStack = _$modalStack_;
}));

beforeEach(function() {
Expand Down Expand Up @@ -925,6 +926,87 @@ describe('$modal', function () {

element.remove();
});

it('should open modals and resolve the opened promises in order', function() {
// Opens a modal for each element in array order.
// Order is an array of non-repeating integers from 0..length-1 representing when to resolve that modal's promise.
// For example [1,2,0] would resolve the 3rd modal's promise first and the 2nd modal's promise last.
// Tests that the modals are added to $modalStack and that each resolves its "opened" promise sequentially.
// If an element is {reject:n} then n is still the order, but the corresponding promise is rejected.
// A rejection earlier in the open sequence should not affect modals opened later.
function test(order) {
var ds = []; // {index, deferred, reject}
var expected = ''; // 0..length-1
var actual = '';
angular.forEach(order, function(x, i) {
var reject = x.reject !== undefined;
if (reject) {
x = x.reject;
} else {
expected += i;
}
ds[x] = {index:i, deferred:$q.defer(), reject:reject};

var scope = $rootScope.$new();
scope.index = i;
open({
template: '<div>' + i + '</div>',
scope: scope,
resolve: {
x: function() { return ds[x].deferred.promise; }
}
}).opened.then(function() {
expect($modalStack.getTop().value.modalScope.index).toEqual(i);
actual += i;
});
});

angular.forEach(ds, function(d, i) {
if (d.reject) {
d.deferred.reject('rejected:' + d.index );
} else {
d.deferred.resolve('resolved:' + d.index );
}
$rootScope.$digest();
});

expect(actual).toEqual(expected);
expect($modal.getPromiseChain()).toEqual(null);
}

// Calls emit n! times on arrays of length n containing all non-repeating permutations of the integers 0..n-1.
function permute(n, emit) {
if (n < 1 || typeof emit !== 'function') {
return;
}
var a = [];
function _permute(depth) {
index: for (var i=0; i < n; i++) {
for (var j=0; j < depth; j++) {
if (a[j] === i) {
continue index; // already used
}
}

a[depth] = i;
if (depth + 1 === n) {
emit(angular.copy(a));
} else {
_permute(depth+1);
}
}
}
_permute(0);
}

permute(2, function(a) { test(a); });
permute(2, function(a) { test(a.map(function(x, i) { return {reject:x}; })); });
permute(2, function(a) { test(a.map(function(x, i) { return i === 0 ? {reject:x} : x; })); });
permute(3, function(a) { test(a); });
permute(3, function(a) { test(a.map(function(x, i) { return {reject:x}; })); });
permute(3, function(a) { test(a.map(function(x, i) { return i === 0 ? {reject:x} : x; })); });
permute(3, function(a) { test(a.map(function(x, i) { return i === 1 ? {reject:x} : x; })); });
});
});

describe('modal.closing event', function() {
Expand Down