From b488b393c6ea2db75322cf76a588888bc0f2a4c4 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 8 Dec 2017 19:41:06 +0100 Subject: [PATCH] fix(portal): inaccurate hasAttahed result and portal being cleared if attached too early (#8642) Fixes the `hasAttached` method always returning false when the portal was attached using `CdkPortalOutlet.attachComponentPortal` or `CdkPortalOutlet.attachTemplatePortal`. This means that checks like https://github.com/angular/material2/blob/master/src/lib/dialog/dialog-container.ts#L118 always evaluate to false. Fixing `hasAttached` revealed another issue: if a portal outlet is unbound (e.g. `
`) and the consumer attaches something to it on the before the first CD round, the content will be cleared immediately due to Angular invoking the getter with an empty string. This has been fixed by not clearing any previously-attached portals if the reset value is set before `ngOnInit`. Fixes #8628. --- src/cdk/portal/portal-directives.ts | 53 ++++++++----- src/cdk/portal/portal.spec.ts | 113 +++++++++++++++++++++------- src/cdk/portal/portal.ts | 2 +- 3 files changed, 118 insertions(+), 50 deletions(-) diff --git a/src/cdk/portal/portal-directives.ts b/src/cdk/portal/portal-directives.ts index 935d6e68098..64e00bdecee 100644 --- a/src/cdk/portal/portal-directives.ts +++ b/src/cdk/portal/portal-directives.ts @@ -7,15 +7,16 @@ */ import { - NgModule, - ComponentRef, - Directive, - EmbeddedViewRef, - TemplateRef, - ComponentFactoryResolver, - ViewContainerRef, - OnDestroy, - Input, + NgModule, + ComponentRef, + Directive, + EmbeddedViewRef, + TemplateRef, + ComponentFactoryResolver, + ViewContainerRef, + OnDestroy, + OnInit, + Input, } from '@angular/core'; import {Portal, TemplatePortal, ComponentPortal, BasePortalOutlet} from './portal'; @@ -47,9 +48,9 @@ export class CdkPortal extends TemplatePortal { exportAs: 'cdkPortalOutlet, cdkPortalHost', inputs: ['portal: cdkPortalOutlet'] }) -export class CdkPortalOutlet extends BasePortalOutlet implements OnDestroy { - /** The attached portal. */ - private _portal: Portal | null = null; +export class CdkPortalOutlet extends BasePortalOutlet implements OnInit, OnDestroy { + /** Whether the portal component is initialized. */ + private _isInitialized = false; constructor( private _componentFactoryResolver: ComponentFactoryResolver, @@ -69,10 +70,18 @@ export class CdkPortalOutlet extends BasePortalOutlet implements OnDestroy { /** Portal associated with the Portal outlet. */ get portal(): Portal | null { - return this._portal; + return this._attachedPortal; } set portal(portal: Portal | null) { + // Ignore the cases where the `portal` is set to a falsy value before the lifecycle hooks have + // run. This handles the cases where the user might do something like `
` + // and attach a portal programmatically in the parent component. When Angular does the first CD + // round, it will fire the setter with empty string, causing the user's content to be cleared. + if (this.hasAttached() && !portal && !this._isInitialized) { + return; + } + if (this.hasAttached()) { super.detach(); } @@ -81,12 +90,16 @@ export class CdkPortalOutlet extends BasePortalOutlet implements OnDestroy { super.attach(portal); } - this._portal = portal; + this._attachedPortal = portal; + } + + ngOnInit() { + this._isInitialized = true; } ngOnDestroy() { super.dispose(); - this._portal = null; + this._attachedPortal = null; } /** @@ -100,18 +113,18 @@ export class CdkPortalOutlet extends BasePortalOutlet implements OnDestroy { // If the portal specifies an origin, use that as the logical location of the component // in the application tree. Otherwise use the location of this PortalOutlet. - let viewContainerRef = portal.viewContainerRef != null ? + const viewContainerRef = portal.viewContainerRef != null ? portal.viewContainerRef : this._viewContainerRef; - let componentFactory = + const componentFactory = this._componentFactoryResolver.resolveComponentFactory(portal.component); - let ref = viewContainerRef.createComponent( + const ref = viewContainerRef.createComponent( componentFactory, viewContainerRef.length, portal.injector || viewContainerRef.parentInjector); super.setDisposeFn(() => ref.destroy()); - this._portal = portal; + this._attachedPortal = portal; return ref; } @@ -126,7 +139,7 @@ export class CdkPortalOutlet extends BasePortalOutlet implements OnDestroy { const viewRef = this._viewContainerRef.createEmbeddedView(portal.templateRef, portal.context); super.setDisposeFn(() => this._viewContainerRef.clear()); - this._portal = portal; + this._attachedPortal = portal; return viewRef; } diff --git a/src/cdk/portal/portal.spec.ts b/src/cdk/portal/portal.spec.ts index 69659237f09..0016acc7c26 100644 --- a/src/cdk/portal/portal.spec.ts +++ b/src/cdk/portal/portal.spec.ts @@ -1,4 +1,4 @@ -import {inject, ComponentFixture, TestBed, async} from '@angular/core/testing'; +import {inject, ComponentFixture, TestBed} from '@angular/core/testing'; import { NgModule, Component, @@ -20,13 +20,11 @@ import {DomPortalOutlet} from './dom-portal-outlet'; describe('Portals', () => { - beforeEach(async(() => { - TestBed.configureTestingModule({ - imports: [PortalModule, PortalTestModule], - }); - - TestBed.compileComponents(); - })); + beforeEach(() => { + TestBed + .configureTestingModule({imports: [PortalModule, PortalTestModule]}) + .compileComponents(); + }); describe('CdkPortalOutlet', () => { let fixture: ComponentFixture; @@ -37,28 +35,33 @@ describe('Portals', () => { it('should load a component into the portal', () => { // Set the selectedHost to be a ComponentPortal. - let testAppComponent = fixture.debugElement.componentInstance; - testAppComponent.selectedPortal = new ComponentPortal(PizzaMsg); + let testAppComponent = fixture.componentInstance; + let componentPortal = new ComponentPortal(PizzaMsg); + let hostContainer = fixture.nativeElement.querySelector('.portal-container'); + + testAppComponent.selectedPortal = componentPortal; fixture.detectChanges(); // Expect that the content of the attached portal is present. - let hostContainer = fixture.nativeElement.querySelector('.portal-container'); expect(hostContainer.textContent).toContain('Pizza'); + expect(testAppComponent.portalOutlet.portal).toBe(componentPortal); }); it('should load a template into the portal', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; let hostContainer = fixture.nativeElement.querySelector('.portal-container'); - let templatePortal = new TemplatePortal(testAppComponent.templateRef, null!); + testAppComponent.selectedPortal = templatePortal; fixture.detectChanges(); + // Expect that the content of the attached portal is present and no context is projected expect(hostContainer.textContent).toContain('Banana'); + expect(testAppComponent.portalOutlet.portal).toBe(templatePortal); }); it('should project template context bindings in the portal', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; let hostContainer = fixture.nativeElement.querySelector('.portal-container'); // TemplatePortal without context: @@ -99,7 +102,7 @@ describe('Portals', () => { it('should dispose the host when destroyed', () => { // Set the selectedHost to be a ComponentPortal. - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; testAppComponent.selectedPortal = new ComponentPortal(PizzaMsg); fixture.detectChanges(); @@ -114,7 +117,7 @@ describe('Portals', () => { let chocolateInjector = new ChocolateInjector(fixture.componentInstance.injector); // Set the selectedHost to be a ComponentPortal. - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; testAppComponent.selectedPortal = new ComponentPortal(PizzaMsg, undefined, chocolateInjector); fixture.detectChanges(); @@ -125,7 +128,7 @@ describe('Portals', () => { }); it('should load a portal', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; // Detect changes initially so that the component's ViewChildren are resolved. fixture.detectChanges(); @@ -140,7 +143,7 @@ describe('Portals', () => { }); it('should load a portal with the `*` sugar', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; // Detect changes initially so that the component's ViewChildren are resolved. fixture.detectChanges(); @@ -155,7 +158,7 @@ describe('Portals', () => { }); it('should load a portal with a binding', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; // Detect changes initially so that the component's ViewChildren are resolved. fixture.detectChanges(); @@ -177,7 +180,7 @@ describe('Portals', () => { }); it('should load a portal with an inner template', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; // Detect changes initially so that the component's ViewChildren are resolved. fixture.detectChanges(); @@ -199,7 +202,7 @@ describe('Portals', () => { }); it('should change the attached portal', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; // Detect changes initially so that the component's ViewChildren are resolved. fixture.detectChanges(); @@ -219,14 +222,14 @@ describe('Portals', () => { }); it('should detach the portal when it is set to null', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; testAppComponent.selectedPortal = new ComponentPortal(PizzaMsg); fixture.detectChanges(); expect(testAppComponent.portalOutlet.hasAttached()).toBe(true); expect(testAppComponent.portalOutlet.portal).toBe(testAppComponent.selectedPortal); - testAppComponent.selectedPortal = null; + testAppComponent.selectedPortal = null!; fixture.detectChanges(); expect(testAppComponent.portalOutlet.hasAttached()).toBe(false); @@ -234,7 +237,7 @@ describe('Portals', () => { }); it('should set the `portal` when attaching a component portal programmatically', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; let portal = new ComponentPortal(PizzaMsg); testAppComponent.portalOutlet.attachComponentPortal(portal); @@ -243,7 +246,7 @@ describe('Portals', () => { }); it('should set the `portal` when attaching a template portal programmatically', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; fixture.detectChanges(); testAppComponent.portalOutlet.attachTemplatePortal(testAppComponent.cakePortal); @@ -252,7 +255,7 @@ describe('Portals', () => { }); it('should clear the portal reference on destroy', () => { - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; testAppComponent.selectedPortal = new ComponentPortal(PizzaMsg); fixture.detectChanges(); @@ -263,6 +266,40 @@ describe('Portals', () => { expect(testAppComponent.portalOutlet.portal).toBeNull(); }); + + it('should not clear programmatically-attached portals on init', () => { + fixture.destroy(); + + const unboundFixture = TestBed.createComponent(UnboundPortalTestApp); + + // Note: calling `detectChanges` here will cause a false positive. + // What we're testing is attaching before the first CD cycle. + unboundFixture.componentInstance.portalOutlet.attach(new ComponentPortal(PizzaMsg)); + unboundFixture.detectChanges(); + + expect(unboundFixture.nativeElement.querySelector('.portal-container').textContent) + .toContain('Pizza'); + }); + + it('should be considered attached when attaching using `attach`', () => { + expect(fixture.componentInstance.portalOutlet.hasAttached()).toBe(false); + fixture.componentInstance.portalOutlet.attach(new ComponentPortal(PizzaMsg)); + expect(fixture.componentInstance.portalOutlet.hasAttached()).toBe(true); + }); + + it('should be considered attached when attaching using `attachComponentPortal`', () => { + expect(fixture.componentInstance.portalOutlet.hasAttached()).toBe(false); + fixture.componentInstance.portalOutlet.attachComponentPortal(new ComponentPortal(PizzaMsg)); + expect(fixture.componentInstance.portalOutlet.hasAttached()).toBe(true); + }); + + it('should be considered attached when attaching using `attachTemplatePortal`', () => { + const instance = fixture.componentInstance; + expect(instance.portalOutlet.hasAttached()).toBe(false); + instance.portalOutlet.attachTemplatePortal(new TemplatePortal(instance.templateRef, null!)); + expect(instance.portalOutlet.hasAttached()).toBe(true); + }); + }); describe('DomPortalOutlet', () => { @@ -345,7 +382,7 @@ describe('Portals', () => { it('should attach and detach a template portal with a binding', () => { let fixture = TestBed.createComponent(PortalTestApp); - let testAppComponent = fixture.debugElement.componentInstance; + let testAppComponent = fixture.componentInstance; // Detect changes initially so that the component's ViewChildren are resolved. fixture.detectChanges(); @@ -484,7 +521,7 @@ class PortalTestApp { @ViewChild(CdkPortalOutlet) portalOutlet: CdkPortalOutlet; @ViewChild('templateRef', { read: TemplateRef }) templateRef: TemplateRef; - selectedPortal: Portal; + selectedPortal: Portal|undefined; fruit: string = 'Banana'; fruits = ['Apple', 'Pineapple', 'Durian']; @@ -508,9 +545,27 @@ class PortalTestApp { } +/** Test-bed component that contains a portal outlet and a couple of template portals. */ +@Component({ + template: ` +
+ +
+ `, +}) +class UnboundPortalTestApp { + @ViewChild(CdkPortalOutlet) portalOutlet: CdkPortalOutlet; +} + // Create a real (non-test) NgModule as a workaround for // https://github.com/angular/angular/issues/10760 -const TEST_COMPONENTS = [PortalTestApp, ArbitraryViewContainerRefComponent, PizzaMsg]; +const TEST_COMPONENTS = [ + PortalTestApp, + UnboundPortalTestApp, + ArbitraryViewContainerRefComponent, + PizzaMsg +]; + @NgModule({ imports: [CommonModule, PortalModule], exports: TEST_COMPONENTS, diff --git a/src/cdk/portal/portal.ts b/src/cdk/portal/portal.ts index 981f12f562e..08947ca21ce 100644 --- a/src/cdk/portal/portal.ts +++ b/src/cdk/portal/portal.ts @@ -168,7 +168,7 @@ export interface PortalOutlet { */ export abstract class BasePortalOutlet implements PortalOutlet { /** The portal currently attached to the host. */ - private _attachedPortal: Portal | null; + protected _attachedPortal: Portal | null; /** A function that will permanently dispose this host. */ private _disposeFn: (() => void) | null;