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

feat(interimElement): properly handle multiple interims. #9053

Merged
merged 5 commits into from
Sep 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/components/dialog/dialog.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/components/menu/js/menuServiceProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function MenuProvider($$interimElementProvider) {
disableParentScroll: true,
skipCompile: true,
preserveScope: true,
skipHide: true,
multiple: true,
themable: true
};

Expand Down
147 changes: 97 additions & 50 deletions src/core/services/interimElement/interimElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,15 +257,20 @@ 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

return service = {
show: show,
hide: hide,
cancel: cancel,
hide: waitForInterim(hide),
cancel: waitForInterim(cancel),
destroy : destroy,
$injector_: $injector
};
Expand All @@ -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.resolve() : $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; })
Copy link
Member

Choose a reason for hiding this comment

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

Why are we muting the errors and resolving the promise?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure about this. I did not write this.

I assume it's because we are resolving / rejecting the promises twice, and this one is just for internal use.

The real promise is returned below.

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

Expand All @@ -325,27 +339,30 @@ function InterimElementProvider() {
*
*/
function hide(reason, options) {
if ( !stack.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));
Copy link
Member

Choose a reason for hiding this comment

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

Why do we reverse it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to show them in reverse, last added interim should hide at first.

} else if (options.closeTo !== undefined) {
return $q.all(stack.splice(options.closeTo).map(closeElement));
} else {
var interim = stack.pop();
return closeElement(interim);
return $q.all(showingInterims.slice(options.closeTo).map(closeElement));
}

// Hide the latest showing interim element.
return closeElement(showingInterims.pop());

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;
}
}
Expand All @@ -363,46 +380,76 @@ function InterimElementProvider() {
*
*/
function cancel(reason, options) {
Copy link
Member

Choose a reason for hiding this comment

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

cancel, hide and show are pretty similar can't we make a more generic function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather not make big changes in the PR, since it's already big enough. Could be an extra PR.

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.
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) {
Copy link
Member

@EladBezalel EladBezalel Sep 2, 2016

Choose a reason for hiding this comment

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

why are we getting a callback function and not resolving a promise? this is weird!

Copy link
Member Author

Choose a reason for hiding this comment

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

This is function which waits for one interim element to be available. Doing it with promises means we need to repeat that logic multiple times (what I had before this part commit)

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 () {
Copy link
Member

Choose a reason for hiding this comment

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

showPromises[0].finally isn't the same as $q.all ?

Copy link
Member Author

@devversion devversion Sep 2, 2016

Choose a reason for hiding this comment

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

This is intentionally. We only want to wait for the first promise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why you only care about the first promise? This is strange.

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"
*/
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) {
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);
}

/*
Expand Down
74 changes: 74 additions & 0 deletions src/core/services/interimElement/interimElement.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<div>First Interim</div>',
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: '<div>First Interim</div>',
onShow: function() {
showCount++;
},
onRemove: function() {
showCount--;
}
});
}
});

it('should cancel a previous interim after a second shows up', inject(function($q, $timeout) {
var hidePromise = $q.defer();
var isShown = false;

Service.show({
template: '<div>First Interim</div>',
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: '<div>Second Interim</div>',
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;

Expand Down