Skip to content

Commit

Permalink
fix(portal): better handling when dom portal content can't be restored (
Browse files Browse the repository at this point in the history
#17851)

Avoids a cryptic error being thrown if we're unable to restore the content of a DOM portal. Also adds a better error when trying to attach an element without a parent to a DOM portal.
  • Loading branch information
crisbeto authored and jelbourn committed Dec 3, 2019
1 parent 0c10828 commit 2e6045c
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 9 deletions.
14 changes: 10 additions & 4 deletions src/cdk/portal/dom-portal-outlet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,23 @@ export class DomPortalOutlet extends BasePortalOutlet {
throw Error('Cannot attach DOM portal without _document constructor parameter');
}

const element = portal.element;
if (!element.parentNode) {
throw Error('DOM portal content must be attached to a parent node.');
}

// Anchor used to save the element's previous position so
// that we can restore it when the portal is detached.
let anchorNode = this._document.createComment('dom-portal');
let element = portal.element;
const anchorNode = this._document.createComment('dom-portal');

element.parentNode!.insertBefore(anchorNode, element);
element.parentNode.insertBefore(anchorNode, element);
this.outletElement.appendChild(element);

super.setDisposeFn(() => {
// We can't use `replaceWith` here because IE doesn't support it.
anchorNode.parentNode!.replaceChild(element, anchorNode);
if (anchorNode.parentNode) {
anchorNode.parentNode.replaceChild(element, anchorNode);
}
});
}

Expand Down
12 changes: 9 additions & 3 deletions src/cdk/portal/portal-directives.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,17 +202,23 @@ export class CdkPortalOutlet extends BasePortalOutlet implements OnInit, OnDestr
throw Error('Cannot attach DOM portal without _document constructor parameter');
}

const element = portal.element;
if (!element.parentNode) {
throw Error('DOM portal content must be attached to a parent node.');
}

// Anchor used to save the element's previous position so
// that we can restore it when the portal is detached.
const anchorNode = this._document.createComment('dom-portal');
const element = portal.element;

portal.setAttachedHost(this);
element.parentNode!.insertBefore(anchorNode, element);
element.parentNode.insertBefore(anchorNode, element);
this._getRootNode().appendChild(element);

super.setDisposeFn(() => {
anchorNode.parentNode!.replaceChild(element, anchorNode);
if (anchorNode.parentNode) {
anchorNode.parentNode!.replaceChild(element, anchorNode);
}
});
}

Expand Down
56 changes: 54 additions & 2 deletions src/cdk/portal/portal.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,33 @@ describe('Portals', () => {
.toBe(false, 'Expected content to be removed from outlet on detach.');
});

it('should throw when trying to load an element without a parent into a DOM portal', () => {
const testAppComponent = fixture.componentInstance;
const element = document.createElement('div');
const domPortal = new DomPortal(element);

expect(() => {
testAppComponent.selectedPortal = domPortal;
fixture.detectChanges();
}).toThrowError('DOM portal content must be attached to a parent node.');
});

it('should not throw when restoring if the outlet element was cleared', () => {
const testAppComponent = fixture.componentInstance;
const parent = fixture.nativeElement.querySelector('.dom-portal-parent');
const domPortal = new DomPortal(testAppComponent.domPortalContent);

testAppComponent.selectedPortal = domPortal;
fixture.detectChanges();

parent.innerHTML = '';

expect(() => {
testAppComponent.selectedPortal = undefined;
fixture.detectChanges();
}).not.toThrow();
});

it('should project template context bindings in the portal', () => {
let testAppComponent = fixture.componentInstance;
let hostContainer = fixture.nativeElement.querySelector('.portal-container');
Expand Down Expand Up @@ -558,6 +585,29 @@ describe('Portals', () => {
expect(someDomElement.textContent!.trim()).toBe('');
});

it('should throw when trying to load an element without a parent into a DOM portal', () => {
const fixture = TestBed.createComponent(PortalTestApp);
fixture.detectChanges();
const element = document.createElement('div');
const portal = new DomPortal(element);

expect(() => {
portal.attach(host);
fixture.detectChanges();
}).toThrowError('DOM portal content must be attached to a parent node.');
});

it('should not throw when restoring if the outlet element was cleared', () => {
const fixture = TestBed.createComponent(PortalTestApp);
fixture.detectChanges();
const portal = new DomPortal(fixture.componentInstance.domPortalContent);

portal.attach(host);
host.outletElement.innerHTML = '';

expect(() => host.detach()).not.toThrow();
});

});
});

Expand Down Expand Up @@ -618,8 +668,10 @@ class ArbitraryViewContainerRefComponent {
<ng-template #templateRef let-data> {{fruit}} - {{ data?.status }}!</ng-template>
<div #domPortalContent>
<p class="dom-portal-inner-content">Hello there</p>
<div class="dom-portal-parent">
<div #domPortalContent>
<p class="dom-portal-inner-content">Hello there</p>
</div>
</div>
`,
})
Expand Down

0 comments on commit 2e6045c

Please sign in to comment.