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

@@ -83,14 +83,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

@@ -13,6 +13,8 @@ import {
   TemplateRef,
   ComponentRef,
   ElementRef,
+  Directive,
+  AfterViewInit,
 } from '@angular/core';
 import {CommonModule} from '@angular/common';
 import {CdkPortal, CdkPortalOutlet, PortalModule} from './portal-directives';
@@ -367,7 +369,7 @@ describe('Portals', () => {
     let componentFactoryResolver: ComponentFactoryResolver;
     let someViewContainerRef: ViewContainerRef;
     let someInjector: Injector;
-    let someFixture: ComponentFixture<any>;
+    let someFixture: ComponentFixture<ArbitraryViewContainerRefComponent>;
     let someDomElement: HTMLElement;
     let host: DomPortalOutlet;
     let injector: Injector;
@@ -403,6 +405,19 @@ 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;
@@ -575,12 +590,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) { }
 }
 
@@ -666,7 +707,7 @@ const TEST_COMPONENTS = [
 @NgModule({
   imports: [CommonModule, PortalModule],
   exports: TEST_COMPONENTS,
-  declarations: TEST_COMPONENTS,
+  declarations: [...TEST_COMPONENTS, SaveParentNodeOnInit],
   entryComponents: TEST_COMPONENTS,
 })
 class PortalTestModule { }