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. `<div cdkPortalOutlet>`) 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.

Author: crisbeto

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<any> {
   exportAs: 'cdkPortalOutlet, cdkPortalHost',
   inputs: ['portal: cdkPortalOutlet']
 })
-export class CdkPortalOutlet extends BasePortalOutlet implements OnDestroy {
-  /** The attached portal. */
-  private _portal: Portal<any> | 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<any> | null {
-    return this._portal;
+    return this._attachedPortal;
   }
 
   set portal(portal: Portal<any> | 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 `<div cdkPortalOutlet>`
+    // 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;
   }

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<PortalTestApp>;
@@ -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 <ng-template> 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 <ng-template> 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 <ng-template> 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 <ng-template> 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,22 +222,22 @@ 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);
       expect(testAppComponent.portalOutlet.portal).toBeNull();
     });
 
     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<any>;
 
-  selectedPortal: Portal<any>;
+  selectedPortal: Portal<any>|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: `
+    <div class="portal-container">
+      <ng-template cdkPortalOutlet></ng-template>
+    </div>
+  `,
+})
+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,

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<any> | null;
+  protected _attachedPortal: Portal<any> | null;
 
   /** A function that will permanently dispose this host. */
   private _disposeFn: (() => void) | null;