From 72e887dad20b971ba28b305dd308c2bc4dadd4ef Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 16 Jul 2016 21:44:30 +0200 Subject: [PATCH 1/5] feat(interimElement): properly handle multiple interims. * Adds support for multiple interim elements (like dialog) * When single interim (default) is enabled, then interims should hide properly (as in toasts). Fixes #8624. References #8630. --- src/components/dialog/dialog.spec.js | 2 + src/components/menu/js/menuServiceProvider.js | 2 +- .../services/interimElement/interimElement.js | 118 +++++++++++------- .../interimElement/interimElement.spec.js | 74 +++++++++++ 4 files changed, 149 insertions(+), 47 deletions(-) diff --git a/src/components/dialog/dialog.spec.js b/src/components/dialog/dialog.spec.js index 24772be9d24..ae3d70c7671 100644 --- a/src/components/dialog/dialog.spec.js +++ b/src/components/dialog/dialog.spec.js @@ -1607,6 +1607,8 @@ describe('$mdDialog', function() { document.body.appendChild(parent); $mdDialog.show({template: template, parent: parent}); + runAnimation(); + $rootScope.$apply(); // It should add two focus traps to the document around the dialog content. diff --git a/src/components/menu/js/menuServiceProvider.js b/src/components/menu/js/menuServiceProvider.js index 6520460f050..80c6a7df962 100644 --- a/src/components/menu/js/menuServiceProvider.js +++ b/src/components/menu/js/menuServiceProvider.js @@ -34,7 +34,7 @@ function MenuProvider($$interimElementProvider) { disableParentScroll: true, skipCompile: true, preserveScope: true, - skipHide: true, + multiple: true, themable: true }; diff --git a/src/core/services/interimElement/interimElement.js b/src/core/services/interimElement/interimElement.js index b5feff58a8b..e6d4b9ca5d6 100644 --- a/src/core/services/interimElement/interimElement.js +++ b/src/core/services/interimElement/interimElement.js @@ -257,7 +257,12 @@ function InterimElementProvider() { * A service used to control inserting and removing an element into the DOM. * */ - var service, stack = []; + + var service; + + var showPromises = []; // Promises for the interim's which are currently opening. + var hidePromises = []; // Promises for the interim's which are currently hiding. + var showingInterims = []; // Interim elements which are currently showing up. // Publish instance $$interimElement service; // ... used as $mdDialog, $mdToast, $mdMenu, and $mdSelect @@ -286,26 +291,35 @@ function InterimElementProvider() { function show(options) { options = options || {}; var interimElement = new InterimElement(options || {}); + // When an interim element is currently showing, we have to cancel it. // Just hiding it, will resolve the InterimElement's promise, the promise should be // rejected instead. - var hideExisting = !options.skipHide && stack.length ? service.cancel() : $q.when(true); - - // This hide()s only the current interim element before showing the next, new one - // NOTE: this is not reversible (e.g. interim elements are not stackable) + var hideAction = options.multiple ? $q.when(true) : $q.all(showPromises); + + if (!options.multiple) { + // Wait for all opening interim's to finish their transition. + hideAction = hideAction.then(function() { + // Wait for all closing and showing interim's to be completely closed. + var promiseArray = hidePromises.concat(showingInterims.map(service.cancel)); + return $q.all(promiseArray); + }); + } - hideExisting.finally(function() { + var showAction = hideAction.then(function() { - stack.push(interimElement); - interimElement + return interimElement .show() - .catch(function( reason ) { - //$log.error("InterimElement.show() error: " + reason ); - return reason; + .catch(function(reason) { return reason; }) + .finally(function() { + showPromises.splice(showPromises.indexOf(showAction), 1); + showingInterims.push(interimElement); }); }); + showPromises.push(showAction); + // Return a promise that will be resolved when the interim // element is hidden or cancelled... @@ -325,27 +339,34 @@ function InterimElementProvider() { * */ function hide(reason, options) { - if ( !stack.length ) return $q.when(reason); + if (!showingInterims.length) { + return $q.when(reason); + } + options = options || {}; if (options.closeAll) { - var promise = $q.all(stack.reverse().map(closeElement)); - stack = []; - return promise; + // We have to make a shallow copy of the array, because otherwise the map will break. + return $q.all(showingInterims.slice().reverse().map(closeElement)); } else if (options.closeTo !== undefined) { - return $q.all(stack.splice(options.closeTo).map(closeElement)); + return $q.all(showingInterims.slice(options.closeTo).map(closeElement)); } else { - var interim = stack.pop(); + var interim = showingInterims.pop(); return closeElement(interim); } function closeElement(interim) { - interim + + var hideAction = interim .remove(reason, false, options || { }) - .catch(function( reason ) { - //$log.error("InterimElement.hide() error: " + reason ); - return reason; + .catch(function(reason) { return reason; }) + .finally(function() { + hidePromises.splice(hidePromises.indexOf(hideAction), 1); }); + + showingInterims.splice(showingInterims.indexOf(interim), 1); + hidePromises.push(hideAction); + return interim.deferred.promise; } } @@ -363,16 +384,20 @@ function InterimElementProvider() { * */ function cancel(reason, options) { - var interim = stack.pop(); - if ( !interim ) return $q.when(reason); - - interim - .remove(reason, true, options || { }) - .catch(function( reason ) { - //$log.error("InterimElement.cancel() error: " + reason ); - return reason; + var interim = showingInterims.pop(); + if (!interim) { + return $q.when(reason); + } + + var cancelAction = interim + .remove(reason, true, options || {}) + .catch(function(reason) { return reason; }) + .finally(function() { + hidePromises.splice(hidePromises.indexOf(cancelAction), 1); }); + hidePromises.push(cancelAction); + // Since Angular 1.6.7, promises will be logged to $exceptionHandler when the promise // is not handling the rejection. We create a pseudo catch handler, which will prevent the // promise from being logged to the $exceptionHandler. @@ -383,26 +408,27 @@ function InterimElementProvider() { * Special method to quick-remove the interim element without animations * Note: interim elements are in "interim containers" */ - function destroy(target) { - var interim = !target ? stack.shift() : null; - var cntr = angular.element(target).length ? angular.element(target)[0].parentNode : null; - - if (cntr) { - // Try to find the interim element in the stack which corresponds to the supplied DOM element. - var filtered = stack.filter(function(entry) { - var currNode = entry.options.element[0]; - return (currNode === cntr); - }); + function destroy(targetEl) { + var interim = !targetEl ? showingInterims.shift() : null; - // Note: this function might be called when the element already has been removed, in which - // case we won't find any matches. That's ok. - if (filtered.length > 0) { - interim = filtered[0]; - stack.splice(stack.indexOf(interim), 1); - } + var parentEl = angular.element(targetEl).length && angular.element(targetEl)[0].parentNode; + + if (parentEl) { + // Try to find the interim in the stack which corresponds to the supplied DOM element. + var filtered = showingInterims.filter(function(entry) { + return entry.options.element[0] === parentEl; + }); + + // Note: This function might be called when the element already has been removed, + // in which case we won't find any matches. + if (filtered.length > 0) { + interim = filtered[0]; + showingInterims.splice(showingInterims.indexOf(interim), 1); + } } - return interim ? interim.remove(SHOW_CANCELLED, false, {'$destroy':true}) : $q.when(SHOW_CANCELLED); + return interim ? interim.remove(SHOW_CANCELLED, false, { '$destroy': true }) : + $q.when(SHOW_CANCELLED); } /* diff --git a/src/core/services/interimElement/interimElement.spec.js b/src/core/services/interimElement/interimElement.spec.js index 05200a83212..c3320402bec 100644 --- a/src/core/services/interimElement/interimElement.spec.js +++ b/src/core/services/interimElement/interimElement.spec.js @@ -342,6 +342,80 @@ describe('$$interimElement service', function() { })); + it('should show multiple interim elements', function() { + var showCount = 0; + + showInterim(); + expect(showCount).toBe(1); + + showInterim(); + expect(showCount).toBe(2); + + function showInterim() { + Service.show({ + template: '
First Interim
', + onShow: function() { + showCount++; + }, + onRemove: function() { + showCount--; + }, + multiple: true + }); + } + }); + + + it('should not show multiple interim elements by default', function() { + var showCount = 0; + + showInterim(); + expect(showCount).toBe(1); + + showInterim(); + expect(showCount).toBe(1); + + function showInterim() { + Service.show({ + template: '
First Interim
', + onShow: function() { + showCount++; + }, + onRemove: function() { + showCount--; + } + }); + } + }); + + it('should cancel a previous interim after it completes hiding', inject(function($q, $timeout) { + var hidePromise = $q.defer(); + var isShown = false; + + Service.show({ + template: '
First Interim
', + onRemove: function() { + return hidePromise.promise; + } + }); + + // Once we show the second interim, the first interim should be cancelled and new interim + // will successfully show up after the first interim hides completely. + Service.show({ + template: '
Second Interim
', + onShow: function() { + isShown = true; + } + }); + + expect(isShown).toBe(false); + + hidePromise.resolve(); + $timeout.flush(); + + expect(isShown).toBe(true); + })); + it('should cancel a previous shown interim element', inject(function() { var isCancelled = false; From 168ba08ad6e2cf6cbfebed4648d4de23c5a73b3a Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 29 Jul 2016 19:03:27 +0200 Subject: [PATCH 2/5] When hiding wait for currenlty opening interims --- src/core/services/interimElement/interimElement.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/core/services/interimElement/interimElement.js b/src/core/services/interimElement/interimElement.js index e6d4b9ca5d6..067a39cd38a 100644 --- a/src/core/services/interimElement/interimElement.js +++ b/src/core/services/interimElement/interimElement.js @@ -339,7 +339,16 @@ function InterimElementProvider() { * */ function hide(reason, options) { + if (!showingInterims.length) { + // When there are still interim's opening, then wait for the first interim element to + // finish its opening. + if (showPromises.length) { + return showPromises.shift().finally(function() { + return service.hide(reason, options) + }); + } + return $q.when(reason); } From dd959a8d6368e77f69a1fb0f51b8b72c3b86d4be Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 1 Sep 2016 23:29:21 +0200 Subject: [PATCH 3/5] Address EladBezalel's feedback --- src/core/services/interimElement/interimElement.js | 2 +- src/core/services/interimElement/interimElement.spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/services/interimElement/interimElement.js b/src/core/services/interimElement/interimElement.js index 067a39cd38a..f837f8165e1 100644 --- a/src/core/services/interimElement/interimElement.js +++ b/src/core/services/interimElement/interimElement.js @@ -430,7 +430,7 @@ function InterimElementProvider() { // Note: This function might be called when the element already has been removed, // in which case we won't find any matches. - if (filtered.length > 0) { + if (filtered.length) { interim = filtered[0]; showingInterims.splice(showingInterims.indexOf(interim), 1); } diff --git a/src/core/services/interimElement/interimElement.spec.js b/src/core/services/interimElement/interimElement.spec.js index c3320402bec..f85a5843483 100644 --- a/src/core/services/interimElement/interimElement.spec.js +++ b/src/core/services/interimElement/interimElement.spec.js @@ -388,7 +388,7 @@ describe('$$interimElement service', function() { } }); - it('should cancel a previous interim after it completes hiding', inject(function($q, $timeout) { + it('should cancel a previous interim after a second shows up', inject(function($q, $timeout) { var hidePromise = $q.defer(); var isShown = false; From f82a7ffa3cc06c7204b7042b14628f3789744e21 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 2 Sep 2016 01:40:01 +0200 Subject: [PATCH 4/5] Wait for interim element for cancel as well --- .../services/interimElement/interimElement.js | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/src/core/services/interimElement/interimElement.js b/src/core/services/interimElement/interimElement.js index f837f8165e1..c9fc6769d40 100644 --- a/src/core/services/interimElement/interimElement.js +++ b/src/core/services/interimElement/interimElement.js @@ -269,8 +269,8 @@ function InterimElementProvider() { return service = { show: show, - hide: hide, - cancel: cancel, + hide: waitForInterim(hide), + cancel: waitForInterim(cancel), destroy : destroy, $injector_: $injector }; @@ -339,19 +339,6 @@ function InterimElementProvider() { * */ function hide(reason, options) { - - if (!showingInterims.length) { - // When there are still interim's opening, then wait for the first interim element to - // finish its opening. - if (showPromises.length) { - return showPromises.shift().finally(function() { - return service.hide(reason, options) - }); - } - - return $q.when(reason); - } - options = options || {}; if (options.closeAll) { @@ -413,6 +400,31 @@ function InterimElementProvider() { return interim.deferred.promise.catch(angular.noop); } + /** + * Creates a function to wait for at least one interim element to be available. + * @param callbackFn Function to be used as callback + * @returns {Function} + */ + function waitForInterim(callbackFn) { + return function() { + var fnArguments = arguments; + + if (!showingInterims.length) { + // When there are still interim's opening, then wait for the first interim element to + // finish its open animation. + if (showPromises.length) { + return showPromises[0].finally(function () { + return callbackFn.apply(service, fnArguments); + }); + } + + return $q.when("No interim elements currently showing up."); + } + + return callbackFn.apply(service, fnArguments); + }; + } + /* * Special method to quick-remove the interim element without animations * Note: interim elements are in "interim containers" From 9debca2ea62bf883d69a2b4705a47883fac0a486 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 6 Sep 2016 12:31:54 +0200 Subject: [PATCH 5/5] Address EladBezalel's feedback --- src/core/services/interimElement/interimElement.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/services/interimElement/interimElement.js b/src/core/services/interimElement/interimElement.js index c9fc6769d40..e8d48703aa6 100644 --- a/src/core/services/interimElement/interimElement.js +++ b/src/core/services/interimElement/interimElement.js @@ -295,7 +295,7 @@ function InterimElementProvider() { // When an interim element is currently showing, we have to cancel it. // Just hiding it, will resolve the InterimElement's promise, the promise should be // rejected instead. - var hideAction = options.multiple ? $q.when(true) : $q.all(showPromises); + var hideAction = options.multiple ? $q.resolve() : $q.all(showPromises); if (!options.multiple) { // Wait for all opening interim's to finish their transition. @@ -346,11 +346,11 @@ function InterimElementProvider() { return $q.all(showingInterims.slice().reverse().map(closeElement)); } else if (options.closeTo !== undefined) { return $q.all(showingInterims.slice(options.closeTo).map(closeElement)); - } else { - var interim = showingInterims.pop(); - return closeElement(interim); } + // Hide the latest showing interim element. + return closeElement(showingInterims.pop()); + function closeElement(interim) { var hideAction = interim