fix(portal): running change detection before nodes have been moved to outlet

Currently we run change detection as soon as we create a portal's embedded view and afterwards we transfer its DOM nodes into the portal outlet. This is problematic, because running change detection also executes any lifecycle hooks which means that directives inside the portal, which are looking at the DOM, will be invoked while the element is still at its old location.

Fixes #16346.

Author: crisbeto

src/cdk/a11y/BUILD.bazel

@@ -50,6 +50,7 @@ ng_test_library(
         "//src/cdk/keycodes",
         "//src/cdk/observers",
         "//src/cdk/platform",
+        "//src/cdk/portal",
         "//src/cdk/testing/private",
         "@npm//@angular/platform-browser",
         "@npm//rxjs",

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

@@ -1,14 +1,15 @@
 import {Platform} from '@angular/cdk/platform';
-import {Component, PLATFORM_ID, ViewChild} from '@angular/core';
+import {Component, PLATFORM_ID, ViewChild, TemplateRef, ViewContainerRef} from '@angular/core';
 import {async, ComponentFixture, TestBed} from '@angular/core/testing';
+import {PortalModule, CdkPortalOutlet, TemplatePortal} from '@angular/cdk/portal';
 import {A11yModule, FocusTrap, CdkTrapFocus} from '../index';
 
 
 describe('FocusTrap', () => {
 
   beforeEach(async(() => {
     TestBed.configureTestingModule({
-      imports: [A11yModule],
+      imports: [A11yModule, PortalModule],
       declarations: [
         FocusTrapWithBindings,
         SimpleFocusTrap,
@@ -17,6 +18,7 @@ describe('FocusTrap', () => {
         FocusTrapWithoutFocusableElements,
         FocusTrapWithAutoCapture,
         FocusTrapUnfocusableTarget,
+        FocusTrapInsidePortal,
       ],
     });
 
@@ -187,6 +189,27 @@ 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.');
+  });
 });
 
 
@@ -283,3 +306,24 @@ class FocusTrapWithSvg {
 class FocusTrapWithoutFocusableElements {
   @ViewChild(CdkTrapFocus) focusTrapDirective: CdkTrapFocus;
 }
+
+
+@Component({
+  template: `
+  <div class="portal-outlet">
+    <ng-template cdkPortalOutlet></ng-template>
+  </div>
+
+  <ng-template #template>
+    <div cdkTrapFocus>
+      <button>Click me</button>
+    </div>
+  </ng-template>
+  `,
+})
+class FocusTrapInsidePortal {
+  @ViewChild('template', {static: false}) template: TemplateRef<any>;
+  @ViewChild(CdkPortalOutlet, {static: false}) portalOutlet: CdkPortalOutlet;
+
+  constructor(public viewContainerRef: ViewContainerRef) {}
+}

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

@@ -74,14 +74,18 @@ export class DomPortalOutlet extends BasePortalOutlet {
   attachTemplatePortal<C>(portal: TemplatePortal<C>): EmbeddedViewRef<C> {
     let viewContainer = portal.viewContainerRef;
     let viewRef = viewContainer.createEmbeddedView(portal.templateRef, portal.context);
-    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.setDisposeFn((() => {
       let index = viewContainer.indexOf(viewRef);
       if (index !== -1) {

src/cdk/portal/portal.spec.ts

@@ -12,6 +12,9 @@ import {
   ApplicationRef,
   TemplateRef,
   ComponentRef,
+  Directive,
+  AfterViewInit,
+  ElementRef,
 } from '@angular/core';
 import {CommonModule} from '@angular/common';
 import {CdkPortal, CdkPortalOutlet, PortalModule} from './portal-directives';
@@ -336,8 +339,8 @@ describe('Portals', () => {
     let componentFactoryResolver: ComponentFactoryResolver;
     let someViewContainerRef: ViewContainerRef;
     let someInjector: Injector;
-    let someFixture: ComponentFixture<any>;
-    let someDomElement: HTMLElement;
+    let someFixture: ComponentFixture<ArbitraryViewContainerRefComponent>;
+    let outletNode: HTMLElement;
     let host: DomPortalOutlet;
     let injector: Injector;
     let appRef: ApplicationRef;
@@ -350,8 +353,8 @@ describe('Portals', () => {
     }));
 
     beforeEach(() => {
-      someDomElement = document.createElement('div');
-      host = new DomPortalOutlet(someDomElement, componentFactoryResolver, appRef, injector);
+      outletNode = document.createElement('div');
+      host = new DomPortalOutlet(outletNode, componentFactoryResolver, appRef, injector);
 
       someFixture = TestBed.createComponent(ArbitraryViewContainerRefComponent);
       someViewContainerRef = someFixture.componentInstance.viewContainerRef;
@@ -364,11 +367,23 @@ describe('Portals', () => {
       let componentInstance: PizzaMsg = portal.attach(host).instance;
 
       expect(componentInstance instanceof PizzaMsg).toBe(true);
-      expect(someDomElement.textContent).toContain('Pizza');
+      expect(outletNode.textContent).toContain('Pizza');
 
       host.detach();
 
-      expect(someDomElement.innerHTML).toBe('');
+      expect(outletNode.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(outletNode);
+
+      host.dispose();
     });
 
     it('should attach and detach a component portal with a given injector', () => {
@@ -383,12 +398,12 @@ describe('Portals', () => {
       fixture.detectChanges();
 
       expect(componentInstance instanceof PizzaMsg).toBe(true);
-      expect(someDomElement.textContent).toContain('Pizza');
-      expect(someDomElement.textContent).toContain('Chocolate');
+      expect(outletNode.textContent).toContain('Pizza');
+      expect(outletNode.textContent).toContain('Chocolate');
 
       host.detach();
 
-      expect(someDomElement.innerHTML).toBe('');
+      expect(outletNode.innerHTML).toBe('');
     });
 
     it('should attach and detach a template portal', () => {
@@ -397,7 +412,7 @@ describe('Portals', () => {
 
       fixture.componentInstance.cakePortal.attach(host);
 
-      expect(someDomElement.textContent).toContain('Cake');
+      expect(outletNode.textContent).toContain('Cake');
     });
 
     it('should render a template portal with an inner template', () => {
@@ -406,7 +421,7 @@ describe('Portals', () => {
 
       fixture.componentInstance.portalWithTemplate.attach(host);
 
-      expect(someDomElement.textContent).toContain('Durian');
+      expect(outletNode.textContent).toContain('Durian');
     });
 
     it('should attach and detach a template portal with a binding', () => {
@@ -426,17 +441,17 @@ describe('Portals', () => {
       fixture.detectChanges();
 
       // Expect that the content of the attached portal is present.
-      expect(someDomElement.textContent).toContain('Banana - fresh');
+      expect(outletNode.textContent).toContain('Banana - fresh');
 
       // When updating the binding value.
       testAppComponent.fruit = 'Mango';
       fixture.detectChanges();
 
       // Expect the new value to be reflected in the rendered output.
-      expect(someDomElement.textContent).toContain('Mango');
+      expect(outletNode.textContent).toContain('Mango');
 
       host.detach();
-      expect(someDomElement.innerHTML).toBe('');
+      expect(outletNode.innerHTML).toBe('');
     });
 
     it('should change the attached portal', () => {
@@ -448,12 +463,12 @@ describe('Portals', () => {
 
       appFixture.componentInstance.piePortal.attach(host);
 
-      expect(someDomElement.textContent).toContain('Pie');
+      expect(outletNode.textContent).toContain('Pie');
 
       host.detach();
       host.attach(new ComponentPortal(PizzaMsg, someViewContainerRef));
 
-      expect(someDomElement.textContent).toContain('Pizza');
+      expect(outletNode.textContent).toContain('Pizza');
     });
 
     it('should attach and detach a component portal without a ViewContainerRef', () => {
@@ -463,17 +478,17 @@ describe('Portals', () => {
 
       expect(componentInstance instanceof PizzaMsg)
           .toBe(true, 'Expected a PizzaMsg component to be created');
-      expect(someDomElement.textContent)
+      expect(outletNode.textContent)
           .toContain('Pizza', 'Expected the static string "Pizza" in the DomPortalOutlet.');
 
       componentInstance.snack = new Chocolate();
       someFixture.detectChanges();
-      expect(someDomElement.textContent)
+      expect(outletNode.textContent)
           .toContain('Chocolate', 'Expected the bound string "Chocolate" in the DomPortalOutlet');
 
       host.detach();
 
-      expect(someDomElement.innerHTML)
+      expect(outletNode.innerHTML)
           .toBe('', 'Expected the DomPortalOutlet to be empty after detach');
     });
 
@@ -529,12 +544,38 @@ 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>'
+  template: `
+    <p>Hello</p>
+
+    <ng-template #template>
+      <div savesParentNodeOnInit></div>
+    </ng-template>
+  `
 })
 class ArbitraryViewContainerRefComponent {
+  @ViewChild('template', {static: false}) template: TemplateRef<any>;
+  @ViewChild(SaveParentNodeOnInit, {static: false}) saveParentNodeOnInit: SaveParentNodeOnInit;
+
   constructor(public viewContainerRef: ViewContainerRef, public injector: Injector) { }
 }
 
@@ -615,7 +656,7 @@ const TEST_COMPONENTS = [
 @NgModule({
   imports: [CommonModule, PortalModule],
   exports: TEST_COMPONENTS,
-  declarations: TEST_COMPONENTS,
+  declarations: [...TEST_COMPONENTS, SaveParentNodeOnInit],
   entryComponents: TEST_COMPONENTS,
 })
 class PortalTestModule { }