#2404 Modal open calls should complete in order #2443
Conversation
|
Is this for having multiple modals open? It should be noted that UI Bootstrap does not support multiple modals being opened at the same time. |
|
Use case: modals bound to router state. "main.detail" is a modal detail popup. "main.detail.edit" is a nested modal on top of that. If someone follows a deep link to the edit state, then the router will issue modal.open calls in a deterministic order, but without this fix the modal resolves could complete out of order resulting in the wrong modal being on top. There's no reason why modals shouldn't be allowed to open on top of each other. Nesting too deeply may not be great UI design, but consider a modal edit dialog with a fancy "are you sure?" exit prompt that also uses a modal. That would be two modals open at the same time. In practice, I haven't found a problem with an arbitrary level of nesting. |
| @@ -358,9 +360,26 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.transition']) | |||
| var templateAndResolvePromise = | |||
| $q.all([getTemplatePromise(modalOptions)].concat(getResolvePromises(modalOptions.resolve))); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's a good suggestion, but I don't think it can work because $modalStack isn't updated until templateAndResolvePromise is resolved.
There was a problem hiding this comment.
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.
|
I think this is a good idea. However, I think your PR doesn't work for more modals. I've made a suggestion above that should also minimize the changes you need to make. |
|
I've committed a change that should fix the problem with more modals and updated the test to cover more scenarios. |
| var previous = modalOpenedPromiseChain; | ||
| modalOpenedPromiseChain = $q.all([previous || $q.when(undefined), templateAndResolvePromise]) | ||
| .then(function() { return true; }, function(reason) { return reason[reason.length-1]; }) | ||
| ['finally'](function() { modalOpenedPromiseChain = previous; }); |
There was a problem hiding this comment.
This finally may call prematurely if 3 modals are opened and the first modal's templateAndResolvePromise fails. In such a scenario, if a 4th modal is opened, the modalOpenedPromiseChain would be reset to undefined, but that's not correct.
|
This is rather complex if |
|
@chrisirhc thank you for your comments (and your patience!). I should have realized that templateAndResolvePromise needs to remain chained because it controls insertion order onto the stack. Here's another revision that I think addresses the problems you've raised. This could still be reworked to coordinate through $modalStack if you prefer, but as is, it seems to resolve and cleanup correctly. |
|
Can you squash and rebase? |
| @@ -24,6 +24,7 @@ describe('$modal', function () { | |||
| $timeout = _$timeout_; | |||
| $q = _$q_; | |||
| $modal = _$modal_; | |||
| $modalStack = _$modalStack_; | |||
|
Not sure that rebase commit did what I wanted it to. I'll have to look at this further in another week when I'm back from vacation and have access to my dev machine. |
Modal open calls wait on template and resolve promises. Even though the
resolution order of the promises among different modals is
non-deterministic, the open calls should complete in the same order they
were invoked.