fix(portal): run change detection before inserting portal content

In #16407 we made some changes where we'd run change detection after transferring the portal content to the outlet, in order to handle a case where focus traps may be attached too early. This PR reverts back to triggering change detection before moving the content, because doing it after may leave behind nodes that were inserted through `ViewContainerRef` inside one one of the init hooks. The initial focus trap issue is resolved by delaying the attachment instead.

These changes fix another issue where we were assuming that component portals can only have one root node. This is true for most cases, except for when a sibling is inserted through `ViewContainerRef`.

Finally, these changes add a `detectChanges` call before moving the contents of a component portal for consistency between portals. Previously we were only doing in for template portals.

Fixes #20343.

Author: crisbeto

src/cdk/a11y/focus-trap/focus-trap.spec.ts

@@ -1,6 +1,6 @@
 import {Platform} from '@angular/cdk/platform';
 import {Component, ViewChild, TemplateRef, ViewContainerRef} from '@angular/core';
-import {async, ComponentFixture, TestBed} from '@angular/core/testing';
+import {async, ComponentFixture, TestBed, fakeAsync, tick} from '@angular/core/testing';
 import {PortalModule, CdkPortalOutlet, TemplatePortal} from '@angular/cdk/portal';
 import {A11yModule, FocusTrap, CdkTrapFocus} from '../index';
 
@@ -81,10 +81,11 @@ describe('FocusTrap', () => {
   describe('with bindings', () => {
     let fixture: ComponentFixture<FocusTrapWithBindings>;
 
-    beforeEach(() => {
+    beforeEach(fakeAsync(() => {
       fixture = TestBed.createComponent(FocusTrapWithBindings);
       fixture.detectChanges();
-    });
+      tick();
+    }));
 
     it('should clean up its anchor sibling elements on destroy', () => {
       const rootElement = fixture.debugElement.nativeElement as HTMLElement;
@@ -212,26 +213,28 @@ describe('FocusTrap', () => {
 
   });
 
-  it('should put anchors inside the outlet when set at the root of a template portal', () => {
-    const fixture = TestBed.createComponent(FocusTrapInsidePortal);
-    const instance = fixture.componentInstance;
-    fixture.detectChanges();
-    const outlet: HTMLElement = fixture.nativeElement.querySelector('.portal-outlet');
-
-    expect(outlet.querySelectorAll('button').length)
-      .toBe(0, 'Expected no buttons inside the outlet on init.');
-    expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length)
-      .toBe(0, 'Expected no focus trap anchors inside the outlet on init.');
-
-    const portal = new TemplatePortal(instance.template, instance.viewContainerRef);
-    instance.portalOutlet.attachTemplatePortal(portal);
-    fixture.detectChanges();
-
-    expect(outlet.querySelectorAll('button').length)
-      .toBe(1, 'Expected one button inside the outlet after attaching.');
-    expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length)
-      .toBe(2, 'Expected two focus trap anchors in the outlet after attaching.');
-  });
+  it('should put anchors inside the outlet when set at the root of a template portal',
+    fakeAsync(() => {
+      const fixture = TestBed.createComponent(FocusTrapInsidePortal);
+      const instance = fixture.componentInstance;
+      fixture.detectChanges();
+      const outlet: HTMLElement = fixture.nativeElement.querySelector('.portal-outlet');
+
+      expect(outlet.querySelectorAll('button').length)
+        .toBe(0, 'Expected no buttons inside the outlet on init.');
+      expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length)
+        .toBe(0, 'Expected no focus trap anchors inside the outlet on init.');
+
+      const portal = new TemplatePortal(instance.template, instance.viewContainerRef);
+      instance.portalOutlet.attachTemplatePortal(portal);
+      fixture.detectChanges();
+      tick();
+
+      expect(outlet.querySelectorAll('button').length)
+        .toBe(1, 'Expected one button inside the outlet after attaching.');
+      expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length)
+        .toBe(2, 'Expected two focus trap anchors in the outlet after attaching.');
+    }));
 });
 
 

src/cdk/a11y/focus-trap/focus-trap.ts

@@ -390,6 +390,9 @@ export class CdkTrapFocus implements OnDestroy, AfterContentInit, OnChanges, DoC
   /** Previously focused element to restore focus to upon destroy when using autoCapture. */
   private _previouslyFocusedElement: HTMLElement | null = null;
 
+  /** Whether the focus trap has been initialized. */
+  private _hasInitialized: boolean;
+
   /** Whether the focus trap is active. */
   @Input('cdkTrapFocus')
   get enabled(): boolean { return this.focusTrap.enabled; }
@@ -425,15 +428,21 @@ export class CdkTrapFocus implements OnDestroy, AfterContentInit, OnChanges, DoC
   }
 
   ngAfterContentInit() {
-    this.focusTrap.attachAnchors();
+    // We have to delay attaching the anchors, because the lifecycle hook may be fired before the
+    // focus trap element is at its final place in the DOM when inserted via a portal (see #16346).
+    Promise.resolve().then(() => {
+      this.focusTrap.attachAnchors();
 
-    if (this.autoCapture) {
-      this._captureFocus();
-    }
+      if (this.autoCapture) {
+        this._captureFocus();
+      }
+
+      this._hasInitialized = true;
+    });
   }
 
   ngDoCheck() {
-    if (!this.focusTrap.hasAttached()) {
+    if (this._hasInitialized && !this.focusTrap.hasAttached()) {
       this.focusTrap.attachAnchors();
     }
   }

src/cdk/overlay/overlay.spec.ts

@@ -8,6 +8,9 @@ import {
   Injectable,
   EventEmitter,
   NgZone,
+  ComponentFactoryResolver,
+  OnInit,
+  TemplateRef,
 } from '@angular/core';
 import {Direction, Directionality} from '@angular/cdk/bidi';
 import {MockNgZone, dispatchFakeEvent} from '@angular/cdk/testing/private';
@@ -990,6 +993,31 @@ describe('Overlay', () => {
     }));
 
   });
+
+  it('should insert component that adds content through ViewContainerRef', () => {
+    componentPortal = new ComponentPortal(FoodRenderer);
+    const overlayRef = overlay.create();
+    overlayRef.attach(componentPortal);
+
+    expect(overlayContainerElement.textContent).toContain('Food:Pizza');
+    overlayRef.dispose();
+  });
+
+  it('should insert template that has component that adds content via ViewContainerRef', () => {
+    const fixture = TestBed.createComponent(FoodRendererInTemplate);
+    fixture.detectChanges();
+
+    const portal = new TemplatePortal(
+      fixture.componentInstance.foodRendererTemplate,
+      fixture.componentInstance.viewContainerRef
+    );
+    const overlayRef = overlay.create();
+    overlayRef.attach(portal);
+
+    expect(overlayContainerElement.textContent).toContain('Food:Pizza');
+    overlayRef.dispose();
+  });
+
 });
 
 /** Simple component for testing ComponentPortal. */
@@ -1008,9 +1036,45 @@ class TestComponentWithTemplatePortals {
   constructor(public viewContainerRef: ViewContainerRef) { }
 }
 
+@Component({
+  selector: 'food-renderer',
+  template: '<p>Food:</p>',
+})
+class FoodRenderer implements OnInit {
+  constructor(
+    private _componentFactoryResolver: ComponentFactoryResolver,
+    private _viewContainerRef: ViewContainerRef) {
+  }
+
+  ngOnInit() {
+    const factory = this._componentFactoryResolver.resolveComponentFactory(PizzaMsg);
+    this._viewContainerRef.clear();
+    this._viewContainerRef.createComponent(factory);
+  }
+}
+
+@Component({
+  template: `
+    <ng-template #foodRenderer>
+      <food-renderer></food-renderer>
+    </ng-template>
+  `
+})
+class FoodRendererInTemplate {
+  @ViewChild('foodRenderer') foodRendererTemplate: TemplateRef<any>;
+
+  constructor(public viewContainerRef: ViewContainerRef) { }
+}
+
 // Create a real (non-test) NgModule as a workaround for
 // https://github.com/angular/angular/issues/10760
-const TEST_COMPONENTS = [PizzaMsg, TestComponentWithTemplatePortals];
+const TEST_COMPONENTS = [
+  PizzaMsg,
+  TestComponentWithTemplatePortals,
+  FoodRenderer,
+  FoodRendererInTemplate,
+];
+
 @NgModule({
   imports: [OverlayModule, PortalModule],
   exports: TEST_COMPONENTS,

src/cdk/portal/dom-portal-outlet.ts

@@ -68,9 +68,15 @@ export class DomPortalOutlet extends BasePortalOutlet {
         componentRef.destroy();
       });
     }
+
+    // Trigger change detection so that we invoke the lifecycle hooks of any components inside the
+    // component **before** moving the DOM nodes into their new position, because it can have an
+    // effect on the amount of `rootNodes` (e.g. if something is inserted via a `ViewContainerRef`).
+    componentRef.changeDetectorRef.detectChanges();
+
     // At this point the component has been instantiated, so we move it to the location in the DOM
     // where we want it to be rendered.
-    this.outletElement.appendChild(this._getComponentRootNode(componentRef));
+    this._transferRootNodes(componentRef.hostView as EmbeddedViewRef<any>);
 
     return componentRef;
   }
@@ -81,23 +87,23 @@ export class DomPortalOutlet extends BasePortalOutlet {
    * @returns Reference to the created embedded view.
    */
   attachTemplatePortal<C>(portal: TemplatePortal<C>): EmbeddedViewRef<C> {
-    let viewContainer = portal.viewContainerRef;
-    let viewRef = viewContainer.createEmbeddedView(portal.templateRef, portal.context);
+    const viewContainer = portal.viewContainerRef;
+    const viewRef = viewContainer.createEmbeddedView(portal.templateRef, portal.context);
+
+    // Trigger change detection so that we invoke the lifecycle hooks of any components inside the
+    // template **before** moving the DOM nodes into their new position, because it can have an
+    // effect on the amount of `rootNodes` (e.g. if something is inserted via a `ViewContainerRef`).
+    viewRef.detectChanges();
 
     // The method `createEmbeddedView` will add the view as a child of the viewContainer.
     // But for the DomPortalOutlet the view can be added everywhere in the DOM
     // (e.g Overlay Container) To move the view to the specified host element. We just
     // re-append the existing root nodes.
-    viewRef.rootNodes.forEach(rootNode => this.outletElement.appendChild(rootNode));
-
-    // Note that we want to detect changes after the nodes have been moved so that
-    // any directives inside the portal that are looking at the DOM inside a lifecycle
-    // hook won't be invoked too early.
-    viewRef.detectChanges();
+    this._transferRootNodes(viewRef);
 
     this.setDisposeFn((() => {
-      let index = viewContainer.indexOf(viewRef);
-      if (index !== -1) {
+      const index = viewContainer.indexOf(viewRef);
+      if (index > -1) {
         viewContainer.remove(index);
       }
     }));
@@ -149,9 +155,9 @@ export class DomPortalOutlet extends BasePortalOutlet {
     }
   }
 
-  /** Gets the root HTMLElement for an instantiated component. */
-  private _getComponentRootNode(componentRef: ComponentRef<any>): HTMLElement {
-    return (componentRef.hostView as EmbeddedViewRef<any>).rootNodes[0] as HTMLElement;
+  /** Moves all of the root nodes of an embedded view into the outlet element. */
+  private _transferRootNodes(viewRef: EmbeddedViewRef<any>): void {
+    viewRef.rootNodes.forEach(node => this.outletElement.appendChild(node));
   }
 }
 

src/cdk/portal/portal.spec.ts

@@ -14,8 +14,6 @@ import {
   ViewChild,
   ViewChildren,
   ViewContainerRef,
-  Directive,
-  AfterViewInit,
 } from '@angular/core';
 import {ComponentFixture, inject, TestBed} from '@angular/core/testing';
 import {DomPortalOutlet} from './dom-portal-outlet';
@@ -442,19 +440,6 @@ describe('Portals', () => {
       expect(someDomElement.innerHTML).toBe('');
     });
 
-    it('should move the DOM nodes before running change detection', () => {
-      someFixture.detectChanges();
-      let portal = new TemplatePortal(someFixture.componentInstance.template, someViewContainerRef);
-
-      host.attachTemplatePortal(portal);
-      someFixture.detectChanges();
-
-      expect(someFixture.componentInstance.saveParentNodeOnInit.parentOnViewInit)
-          .toBe(someDomElement);
-
-      host.dispose();
-    });
-
     it('should attach and detach a component portal with a given injector', () => {
       let fixture = TestBed.createComponent(ArbitraryViewContainerRefComponent);
       someViewContainerRef = fixture.componentInstance.viewContainerRef;
@@ -649,37 +634,15 @@ class PizzaMsg {
   constructor(@Optional() public snack: Chocolate) { }
 }
 
-/**
- * Saves the parent node that the directive was attached to on init.
- * Useful to see where the element was in the DOM when it was first attached.
- */
-@Directive({
-  selector: '[savesParentNodeOnInit]'
-})
-class SaveParentNodeOnInit implements AfterViewInit {
-  parentOnViewInit: HTMLElement;
-
-  constructor(private _elementRef: ElementRef<HTMLElement>) {}
-
-  ngAfterViewInit() {
-    this.parentOnViewInit = this._elementRef.nativeElement.parentElement!;
-  }
-}
-
 /** Simple component to grab an arbitrary ViewContainerRef */
 @Component({
   selector: 'some-placeholder',
   template: `
     <p>Hello</p>
-
-    <ng-template #template>
-      <div savesParentNodeOnInit></div>
-    </ng-template>
   `
 })
 class ArbitraryViewContainerRefComponent {
   @ViewChild('template') template: TemplateRef<any>;
-  @ViewChild(SaveParentNodeOnInit) saveParentNodeOnInit: SaveParentNodeOnInit;
 
   constructor(public viewContainerRef: ViewContainerRef, public injector: Injector) { }
 }
@@ -772,7 +735,7 @@ const TEST_COMPONENTS = [
 @NgModule({
   imports: [CommonModule, PortalModule],
   exports: TEST_COMPONENTS,
-  declarations: [...TEST_COMPONENTS, SaveParentNodeOnInit],
+  declarations: TEST_COMPONENTS,
   entryComponents: TEST_COMPONENTS,
 })
 class PortalTestModule { }