From 0d790d0b6ea3c7c5bfa3d911771cf909bac70ffa Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 29 Jun 2019 12:19:49 +0200 Subject: [PATCH] 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. --- src/cdk/a11y/BUILD.bazel | 1 + src/cdk/a11y/focus-trap/focus-trap.spec.ts | 48 +++++++++++++++++++++- src/cdk/portal/dom-portal-outlet.ts | 6 ++- src/cdk/portal/portal.spec.ts | 47 +++++++++++++++++++-- 4 files changed, 96 insertions(+), 6 deletions(-) diff --git a/src/cdk/a11y/BUILD.bazel b/src/cdk/a11y/BUILD.bazel index 2b683c2ea75e..992b525d754d 100644 --- a/src/cdk/a11y/BUILD.bazel +++ b/src/cdk/a11y/BUILD.bazel @@ -49,6 +49,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", diff --git a/src/cdk/a11y/focus-trap/focus-trap.spec.ts b/src/cdk/a11y/focus-trap/focus-trap.spec.ts index b0ec9a9a317c..fc49b2f1dbb8 100644 --- a/src/cdk/a11y/focus-trap/focus-trap.spec.ts +++ b/src/cdk/a11y/focus-trap/focus-trap.spec.ts @@ -1,6 +1,7 @@ import {Platform} from '@angular/cdk/platform'; -import {Component, ViewChild} from '@angular/core'; +import {Component, 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'; @@ -8,7 +9,7 @@ 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: ` +
+ +
+ + +
+ +
+ + `, +}) +class FocusTrapInsidePortal { + @ViewChild('template', {static: false}) template: TemplateRef; + @ViewChild(CdkPortalOutlet, {static: false}) portalOutlet: CdkPortalOutlet; + + constructor(public viewContainerRef: ViewContainerRef) {} +} diff --git a/src/cdk/portal/dom-portal-outlet.ts b/src/cdk/portal/dom-portal-outlet.ts index 78ed0f3df2d8..b30f3bd110a5 100644 --- a/src/cdk/portal/dom-portal-outlet.ts +++ b/src/cdk/portal/dom-portal-outlet.ts @@ -83,7 +83,6 @@ export class DomPortalOutlet extends BasePortalOutlet { attachTemplatePortal(portal: TemplatePortal): EmbeddedViewRef { 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 @@ -91,6 +90,11 @@ export class DomPortalOutlet extends BasePortalOutlet { // 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) { diff --git a/src/cdk/portal/portal.spec.ts b/src/cdk/portal/portal.spec.ts index 58ef243d919f..aab283087043 100644 --- a/src/cdk/portal/portal.spec.ts +++ b/src/cdk/portal/portal.spec.ts @@ -14,6 +14,8 @@ import { ViewChild, ViewChildren, ViewContainerRef, + Directive, + AfterViewInit, } from '@angular/core'; import {ComponentFixture, inject, TestBed} from '@angular/core/testing'; import {DomPortalOutlet} from './dom-portal-outlet'; @@ -404,7 +406,7 @@ describe('Portals', () => { let componentFactoryResolver: ComponentFactoryResolver; let someViewContainerRef: ViewContainerRef; let someInjector: Injector; - let someFixture: ComponentFixture; + let someFixture: ComponentFixture; let someDomElement: HTMLElement; let host: DomPortalOutlet; let injector: Injector; @@ -440,6 +442,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; @@ -634,12 +649,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) {} + + ngAfterViewInit() { + this.parentOnViewInit = this._elementRef.nativeElement.parentElement!; + } +} + /** Simple component to grab an arbitrary ViewContainerRef */ @Component({ selector: 'some-placeholder', - template: '

Hello

' + template: ` +

Hello

+ + +
+ + ` }) class ArbitraryViewContainerRefComponent { + @ViewChild('template', {static: false}) template: TemplateRef; + @ViewChild(SaveParentNodeOnInit, {static: false}) saveParentNodeOnInit: SaveParentNodeOnInit; + constructor(public viewContainerRef: ViewContainerRef, public injector: Injector) { } } @@ -731,7 +772,7 @@ const TEST_COMPONENTS = [ @NgModule({ imports: [CommonModule, PortalModule], exports: TEST_COMPONENTS, - declarations: TEST_COMPONENTS, + declarations: [...TEST_COMPONENTS, SaveParentNodeOnInit], entryComponents: TEST_COMPONENTS, }) class PortalTestModule { }