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

Commit 7c4b434

Browse files
devversionhansl
authored andcommitted
fix(dialog): do not compile an empty element when using a content element (#9303)
I recently noticed while playing with the pre-rendered dialog around, that despite of the pre-rendered dialog, the interim element triggers a compilation in the `$mdCompiler`. It doesn't introduce any bugs, but it's unnecessary and could damage the performance. Also moves the content element stuff into its own function, which makes the source more clean and allows us to maintain it easily.
1 parent 8f8ad78 commit 7c4b434

File tree

2 files changed

+100
-44
lines changed

2 files changed

+100
-44
lines changed

src/components/dialog/dialog.js

Lines changed: 56 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -679,6 +679,10 @@ function MdDialogProvider($$interimElementProvider) {
679679
// Those option changes need to be done, before the compilation has started, because otherwise
680680
// the option changes will be not available in the $mdCompilers locales.
681681
detectTheming(options);
682+
683+
if (options.contentElement) {
684+
options.restoreContentElement = installContentElement(options);
685+
}
682686
}
683687

684688
function beforeShow(scope, element, options, controller) {
@@ -702,29 +706,6 @@ function MdDialogProvider($$interimElementProvider) {
702706
function onShow(scope, element, options, controller) {
703707
angular.element($document[0].body).addClass('md-dialog-is-showing');
704708

705-
if (options.contentElement) {
706-
var contentEl = options.contentElement;
707-
708-
if (angular.isString(contentEl)) {
709-
contentEl = document.querySelector(contentEl);
710-
options.elementInsertionSibling = contentEl.nextElementSibling;
711-
options.elementInsertionParent = contentEl.parentNode;
712-
} else {
713-
contentEl = contentEl[0] || contentEl;
714-
// When the element is not visible in the DOM, then we can treat is as same
715-
// as a normal dialog would do. Removing it at close etc.
716-
// ---
717-
// When the element is visible in the DOM, then we restore it at close of the dialog.
718-
if (document.contains(contentEl)) {
719-
options.elementInsertionSibling = contentEl.nextElementSibling;
720-
options.elementInsertionParent = contentEl.parentNode;
721-
}
722-
}
723-
724-
options.elementInsertionEntry = contentEl;
725-
element = angular.element(contentEl);
726-
}
727-
728709
var dialogElement = element.find('md-dialog');
729710

730711
// Once a dialog has `ng-cloak` applied on his template the dialog animation will not work properly.
@@ -813,26 +794,6 @@ function MdDialogProvider($$interimElementProvider) {
813794
return dialogPopOut(element, options);
814795
}
815796

816-
function removeContentElement() {
817-
if (!options.contentElement) return;
818-
819-
options.reverseContainerStretch();
820-
821-
if (!options.elementInsertionParent) {
822-
// When the contentElement has no parent, then it's a virtual DOM element, which should
823-
// be removed at close, as same as normal templates inside of a dialog.
824-
options.elementInsertionEntry.parentNode.removeChild(options.elementInsertionEntry);
825-
} else if (!options.elementInsertionSibling) {
826-
// When the contentElement doesn't have any sibling, then it can be simply appended to the
827-
// parent, because it plays no role, which index it had before.
828-
options.elementInsertionParent.appendChild(options.elementInsertionEntry);
829-
} else {
830-
// When the contentElement has a sibling, which marks the previous position of the contentElement
831-
// in the DOM, we insert it correctly before the sibling, to have the same index as before.
832-
options.elementInsertionParent.insertBefore(options.elementInsertionEntry, options.elementInsertionSibling);
833-
}
834-
}
835-
836797
/**
837798
* Detach the element
838799
*/
@@ -842,7 +803,8 @@ function MdDialogProvider($$interimElementProvider) {
842803
if (!options.contentElement) {
843804
element.remove();
844805
} else {
845-
removeContentElement();
806+
options.reverseContainerStretch();
807+
options.restoreContentElement();
846808
}
847809

848810
if (!options.$destroy) options.origin.focus();
@@ -865,6 +827,56 @@ function MdDialogProvider($$interimElementProvider) {
865827

866828
}
867829

830+
/**
831+
* Installs a content element to the current $$interimElement provider options.
832+
* @returns {Function} Function to restore the content element at its old DOM location.
833+
*/
834+
function installContentElement(options) {
835+
var contentEl = options.contentElement;
836+
var restoreFn = null;
837+
838+
if (angular.isString(contentEl)) {
839+
contentEl = document.querySelector(contentEl);
840+
restoreFn = createRestoreFn(contentEl);
841+
} else {
842+
contentEl = contentEl[0] || contentEl;
843+
844+
// When the element is visible in the DOM, then we restore it at close of the dialog.
845+
// Otherwise it will be removed from the DOM after close.
846+
if (document.contains(contentEl)) {
847+
restoreFn = createRestoreFn(contentEl);
848+
} else {
849+
restoreFn = function() {
850+
contentEl.parentNode.removeChild(contentEl);
851+
}
852+
}
853+
}
854+
855+
// Overwrite the options to use the content element.
856+
options.element = angular.element(contentEl);
857+
options.skipCompile = true;
858+
859+
return restoreFn;
860+
861+
function createRestoreFn(element) {
862+
var parent = element.parentNode;
863+
var nextSibling = element.nextElementSibling;
864+
865+
return function() {
866+
if (!nextSibling) {
867+
// When the element didn't had any sibling, then it can be simply appended to the
868+
// parent, because it plays no role, which index it had before.
869+
parent.appendChild(element);
870+
} else {
871+
// When the element had a sibling, which marks the previous position of the element
872+
// in the DOM, we insert it correctly before the sibling, to have the same index as
873+
// before.
874+
parent.insertBefore(element, nextSibling);
875+
}
876+
}
877+
}
878+
}
879+
868880
/**
869881
* Capture originator/trigger/from/to element information (if available)
870882
* and the parent container for the dialog; defaults to the $rootElement

src/components/dialog/dialog.spec.js

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1390,6 +1390,8 @@ describe('$mdDialog', function() {
13901390

13911391
$mdDialog.hide();
13921392
runAnimation();
1393+
1394+
expect(contentElement[0].offsetParent).toBeFalsy();
13931395
});
13941396

13951397
it('should properly toggle the fullscreen class', function() {
@@ -1471,6 +1473,48 @@ describe('$mdDialog', function() {
14711473
expect(dialogEl).not.toHaveClass('md-transition-out');
14721474
});
14731475

1476+
it('should restore the contentElement at its previous position', function() {
1477+
var contentElement = $compile(
1478+
'<div class="md-dialog-container">' +
1479+
'<md-dialog>Dialog</md-dialog>' +
1480+
'</div>'
1481+
)($rootScope);
1482+
1483+
var dialogParent = angular.element('<div>');
1484+
var contentParent = angular.element(
1485+
'<div>' +
1486+
'<span>Child Element</span>' +
1487+
'</div>'
1488+
);
1489+
1490+
// Append the content parent to the document, otherwise contentElement is not able
1491+
// to detect it properly.
1492+
document.body.appendChild(contentParent[0]);
1493+
1494+
contentParent.prepend(contentElement);
1495+
1496+
$mdDialog.show({
1497+
contentElement: contentElement,
1498+
parent: dialogParent,
1499+
escapeToClose: true
1500+
});
1501+
1502+
$rootScope.$apply();
1503+
runAnimation();
1504+
1505+
expect(contentElement[0].parentNode).toBe(dialogParent[0]);
1506+
1507+
$mdDialog.hide();
1508+
runAnimation();
1509+
1510+
expect(contentElement[0].parentNode).toBe(contentParent[0]);
1511+
1512+
var childNodes = [].slice.call(contentParent[0].children);
1513+
expect(childNodes.indexOf(contentElement[0])).toBe(0);
1514+
1515+
document.body.removeChild(contentParent[0]);
1516+
})
1517+
14741518
});
14751519

14761520
it('should have the dialog role', inject(function($mdDialog, $rootScope) {

0 commit comments

Comments
 (0)