Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Commit e8cfce2

Browse files
erikogenvikThomasBurleson
authored andcommitted
fix(dialog): make sure dialog only destroys itself.
Dialog destruction is async and deferred. As such, there's a risk of another dialog having been created by the time the current dialog's "destroy()" is called. * Added tests for testing dialog double opening. * Fix a bug which prevents a dialog from being closed if it's opened while another dialog is shown. Closes #5157.
1 parent 4205be7 commit e8cfce2

File tree

3 files changed

+83
-8
lines changed

3 files changed

+83
-8
lines changed

src/components/dialog/dialog.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ function MdDialogDirective($$rAF, $mdTheming, $mdDialog) {
2727
}
2828

2929
scope.$on('$destroy', function() {
30-
$mdDialog.destroy();
30+
$mdDialog.destroy(element);
3131
});
3232

3333
/**

src/components/dialog/dialog.spec.js

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -970,6 +970,65 @@ describe('$mdDialog', function() {
970970
expect(parent[0].querySelectorAll('md-dialog.two').length).toBe(1);
971971
}));
972972

973+
it('should hide dialog', inject(function($mdDialog, $rootScope, $animate) {
974+
var parent = angular.element('<div>');
975+
$mdDialog.show({
976+
template: '<md-dialog class="one">',
977+
parent: parent
978+
});
979+
runAnimation();
980+
981+
$mdDialog.hide();
982+
runAnimation();
983+
984+
expect(parent[0].querySelectorAll('md-dialog.one').length).toBe(0);
985+
}));
986+
987+
it('should allow opening new dialog after existing without corruption', inject(function($mdDialog, $rootScope, $animate) {
988+
var parent = angular.element('<div>');
989+
$mdDialog.show({
990+
template: '<md-dialog class="one">',
991+
parent: parent
992+
});
993+
runAnimation();
994+
$mdDialog.hide();
995+
runAnimation();
996+
997+
$mdDialog.show({
998+
template: '<md-dialog class="two">',
999+
parent: parent
1000+
});
1001+
runAnimation();
1002+
$mdDialog.hide();
1003+
runAnimation();
1004+
1005+
expect(parent[0].querySelectorAll('md-dialog.one').length).toBe(0);
1006+
expect(parent[0].querySelectorAll('md-dialog.two').length).toBe(0);
1007+
}));
1008+
1009+
it('should allow opening new dialog from existing without corruption', inject(function($mdDialog, $rootScope, $animate) {
1010+
var parent = angular.element('<div>');
1011+
$mdDialog.show({
1012+
template: '<md-dialog class="one">',
1013+
parent: parent
1014+
});
1015+
runAnimation();
1016+
1017+
$mdDialog.show({
1018+
template: '<md-dialog class="two">',
1019+
parent: parent
1020+
});
1021+
//First run is for the old dialog being hidden.
1022+
runAnimation();
1023+
//Second run is for the new dialog being shown.
1024+
runAnimation();
1025+
$mdDialog.hide();
1026+
runAnimation();
1027+
1028+
expect(parent[0].querySelectorAll('md-dialog.one').length).toBe(0);
1029+
expect(parent[0].querySelectorAll('md-dialog.two').length).toBe(0);
1030+
}));
1031+
9731032
it('should have the dialog role', inject(function($mdDialog, $rootScope) {
9741033
var template = '<md-dialog>Hello</md-dialog>';
9751034
var parent = angular.element('<div>');
@@ -1141,4 +1200,3 @@ describe('$mdDialog with custom interpolation symbols', function() {
11411200
expect(buttons.eq(1).text()).toBe('OK');
11421201
}));
11431202
});
1144-

src/core/services/interimElement/interimElement.js

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -370,14 +370,29 @@ function InterimElementProvider() {
370370

371371
/*
372372
* Special method to quick-remove the interim element without animations
373+
* Note: interim elements are in "interim containers"
373374
*/
374-
function destroy() {
375-
var interim = stack.shift();
375+
function destroy(target) {
376+
var interim = !target ? stack.shift() : null;
377+
var cntr = angular.element(target).length ? angular.element(target)[0].parentNode : null;
378+
379+
if (cntr) {
380+
// Try to find the interim element in the stack which corresponds to the supplied DOM element.
381+
var filtered = stack.filter(function(entry) {
382+
var currNode = entry.options.element[0];
383+
return (currNode === cntr);
384+
});
376385

377-
return interim ? interim.remove(SHOW_CANCELLED, false, {'$destroy':true}) :
378-
$q.when(SHOW_CANCELLED);
379-
}
386+
// Note: this function might be called when the element already has been removed, in which
387+
// case we won't find any matches. That's ok.
388+
if (filtered.length > 0) {
389+
interim = filtered[0];
390+
stack.splice(stack.indexOf(interim), 1);
391+
}
392+
}
380393

394+
return interim ? interim.remove(SHOW_CANCELLED, false, {'$destroy':true}) : $q.when(SHOW_CANCELLED);
395+
}
381396

382397
/*
383398
* Internal Interim Element Object
@@ -438,7 +453,9 @@ function InterimElementProvider() {
438453

439454
if ( options.$destroy === true ) {
440455

441-
return hideElement(options.element, options);
456+
return hideElement(options.element, options).then(function(){
457+
(isCancelled && rejectAll(response)) || resolveAll(response);
458+
});
442459

443460
} else {
444461

0 commit comments

Comments
 (0)