From 6f3b5db898312c70d12d37f5412663d5a39131b4 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 6 Sep 2021 21:55:42 +0200 Subject: [PATCH] fix(cdk/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`. Fixes #20343. --- src/cdk/a11y/focus-trap/focus-trap.spec.ts | 72 +++++++++++++++------- src/cdk/a11y/focus-trap/focus-trap.ts | 37 +++++++++-- src/cdk/overlay/overlay.spec.ts | 62 ++++++++++++++++++- src/cdk/portal/dom-portal-outlet.ts | 28 ++++----- src/cdk/portal/portal.spec.ts | 39 +----------- tools/public_api_guard/cdk/a11y.md | 3 +- 6 files changed, 158 insertions(+), 83 deletions(-) diff --git a/src/cdk/a11y/focus-trap/focus-trap.spec.ts b/src/cdk/a11y/focus-trap/focus-trap.spec.ts index f6beedc5100a..106ed330409d 100644 --- a/src/cdk/a11y/focus-trap/focus-trap.spec.ts +++ b/src/cdk/a11y/focus-trap/focus-trap.spec.ts @@ -6,7 +6,7 @@ import { ViewContainerRef, ViewEncapsulation, } from '@angular/core'; -import {waitForAsync, ComponentFixture, TestBed} from '@angular/core/testing'; +import {waitForAsync, ComponentFixture, TestBed, fakeAsync, tick} from '@angular/core/testing'; import {PortalModule, CdkPortalOutlet, TemplatePortal} from '@angular/cdk/portal'; import {A11yModule, FocusTrap, CdkTrapFocus} from '../index'; import {By} from '@angular/platform-browser'; @@ -91,10 +91,11 @@ describe('FocusTrap', () => { describe('with bindings', () => { let fixture: ComponentFixture; - 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; @@ -298,26 +299,51 @@ 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) - .withContext('Expected no buttons inside the outlet on init.').toBe(0); - expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length) - .withContext('Expected no focus trap anchors inside the outlet on init.').toBe(0); - - const portal = new TemplatePortal(instance.template, instance.viewContainerRef); - instance.portalOutlet.attachTemplatePortal(portal); - fixture.detectChanges(); - - expect(outlet.querySelectorAll('button').length) - .withContext('Expected one button inside the outlet after attaching.').toBe(1); - expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length) - .withContext('Expected two focus trap anchors in the outlet after attaching.').toBe(2); - }); + 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) + .withContext('Expected no buttons inside the outlet on init.').toBe(0); + expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length) + .withContext('Expected no focus trap anchors inside the outlet on init.').toBe(0); + + const portal = new TemplatePortal(instance.template, instance.viewContainerRef); + instance.portalOutlet.attachTemplatePortal(portal); + fixture.detectChanges(); + tick(); + + expect(outlet.querySelectorAll('button').length) + .withContext('Expected one button inside the outlet after attaching.').toBe(1); + expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length) + .withContext('Expected two focus trap anchors in the outlet after attaching.').toBe(2); + })); + + 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) + .withContext('Expected no buttons inside the outlet on init.').toBe(0); + expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length) + .withContext('Expected no focus trap anchors inside the outlet on init.').toBe(0); + + const portal = new TemplatePortal(instance.template, instance.viewContainerRef); + instance.portalOutlet.attachTemplatePortal(portal); + fixture.detectChanges(); + tick(); + + expect(outlet.querySelectorAll('button').length) + .withContext('Expected one button inside the outlet after attaching.').toBe(1); + expect(outlet.querySelectorAll('.cdk-focus-trap-anchor').length) + .withContext('Expected two focus trap anchors in the outlet after attaching.').toBe(2); + })); }); /** Gets the currently-focused element while accounting for the shadow DOM. */ diff --git a/src/cdk/a11y/focus-trap/focus-trap.ts b/src/cdk/a11y/focus-trap/focus-trap.ts index 31c6ac98e611..8e3cde87c6f6 100644 --- a/src/cdk/a11y/focus-trap/focus-trap.ts +++ b/src/cdk/a11y/focus-trap/focus-trap.ts @@ -392,6 +392,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; } @@ -413,7 +416,12 @@ export class CdkTrapFocus implements OnDestroy, AfterContentInit, OnChanges, DoC * @deprecated No longer being used. To be removed. * @breaking-change 13.0.0 */ - @Inject(DOCUMENT) _document: any) { + @Inject(DOCUMENT) _document: any, + /** + * @deprecated _ngZone parameter to become required. + * @breaking-change 14.0.0 + */ + private _ngZone?: NgZone) { this.focusTrap = this._focusTrapFactory.create(this._elementRef.nativeElement, true); } @@ -429,15 +437,32 @@ export class CdkTrapFocus implements OnDestroy, AfterContentInit, OnChanges, DoC } ngAfterContentInit() { - this.focusTrap.attachAnchors(); - - if (this.autoCapture) { - this._captureFocus(); + // 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). + // It has to happen outside the `NgZone`, because the promise can delay `NgZone.onStable` which + // overlays depend on for positioning. + const attachAnchor = () => { + Promise.resolve().then(() => { + this.focusTrap.attachAnchors(); + + if (this.autoCapture) { + this._captureFocus(); + } + + this._hasInitialized = true; + }); + }; + + // @breaking-change 11.0.0 Remove null check for `_ngZone`. + if (this._ngZone) { + this._ngZone.runOutsideAngular(attachAnchor); + } else { + attachAnchor(); } } ngDoCheck() { - if (!this.focusTrap.hasAttached()) { + if (this._hasInitialized && !this.focusTrap.hasAttached()) { this.focusTrap.attachAnchors(); } } diff --git a/src/cdk/overlay/overlay.spec.ts b/src/cdk/overlay/overlay.spec.ts index bd20090d7a7b..1a7d87a0c5d1 100644 --- a/src/cdk/overlay/overlay.spec.ts +++ b/src/cdk/overlay/overlay.spec.ts @@ -15,6 +15,8 @@ import { Injectable, EventEmitter, NgZone, + ComponentFactoryResolver, + TemplateRef, } from '@angular/core'; import {Direction, Directionality} from '@angular/cdk/bidi'; import {MockNgZone, dispatchFakeEvent} from '../testing/private'; @@ -1032,6 +1034,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. */ @@ -1050,9 +1077,42 @@ class TestComponentWithTemplatePortals { constructor(public viewContainerRef: ViewContainerRef) { } } +@Component({ + selector: 'food-renderer', + template: '

Food:

', +}) +class FoodRenderer { + constructor( + private _componentFactoryResolver: ComponentFactoryResolver, + private _viewContainerRef: ViewContainerRef) { + const factory = this._componentFactoryResolver.resolveComponentFactory(PizzaMsg); + this._viewContainerRef.clear(); + this._viewContainerRef.createComponent(factory); + } +} + +@Component({ + template: ` + + + + ` +}) +class FoodRendererInTemplate { + @ViewChild('foodRenderer') foodRendererTemplate: TemplateRef; + + 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, diff --git a/src/cdk/portal/dom-portal-outlet.ts b/src/cdk/portal/dom-portal-outlet.ts index e48dbcdca9d5..04b424375561 100644 --- a/src/cdk/portal/dom-portal-outlet.ts +++ b/src/cdk/portal/dom-portal-outlet.ts @@ -70,7 +70,7 @@ export class DomPortalOutlet extends BasePortalOutlet { } // 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); this._attachedPortal = portal; return componentRef; @@ -82,23 +82,23 @@ export class DomPortalOutlet extends BasePortalOutlet { * @returns Reference to the created embedded view. */ attachTemplatePortal(portal: TemplatePortal): EmbeddedViewRef { - 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); } })); @@ -153,9 +153,9 @@ export class DomPortalOutlet extends BasePortalOutlet { } } - /** Gets the root HTMLElement for an instantiated component. */ - private _getComponentRootNode(componentRef: ComponentRef): HTMLElement { - return (componentRef.hostView as EmbeddedViewRef).rootNodes[0] as HTMLElement; + /** Moves all of the root nodes of an embedded view into the outlet element. */ + private _transferRootNodes(viewRef: EmbeddedViewRef): void { + viewRef.rootNodes.forEach(node => this.outletElement.appendChild(node)); } } diff --git a/src/cdk/portal/portal.spec.ts b/src/cdk/portal/portal.spec.ts index 16bc761a0e9e..e2a46361d12b 100644 --- a/src/cdk/portal/portal.spec.ts +++ b/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'; @@ -448,19 +446,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; @@ -682,37 +667,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) {} - - ngAfterViewInit() { - this.parentOnViewInit = this._elementRef.nativeElement.parentElement!; - } -} - /** Simple component to grab an arbitrary ViewContainerRef */ @Component({ selector: 'some-placeholder', template: `

Hello

- - -
- ` }) class ArbitraryViewContainerRefComponent { @ViewChild('template') template: TemplateRef; - @ViewChild(SaveParentNodeOnInit) saveParentNodeOnInit: SaveParentNodeOnInit; constructor(public viewContainerRef: ViewContainerRef, public injector: Injector) { } } @@ -805,7 +768,7 @@ const TEST_COMPONENTS = [ @NgModule({ imports: [CommonModule, PortalModule], exports: TEST_COMPONENTS, - declarations: [...TEST_COMPONENTS, SaveParentNodeOnInit], + declarations: TEST_COMPONENTS, entryComponents: TEST_COMPONENTS, }) class PortalTestModule { } diff --git a/tools/public_api_guard/cdk/a11y.md b/tools/public_api_guard/cdk/a11y.md index 4e3ef6b329a4..7851ef5dac98 100644 --- a/tools/public_api_guard/cdk/a11y.md +++ b/tools/public_api_guard/cdk/a11y.md @@ -95,7 +95,8 @@ export class CdkMonitorFocus implements AfterViewInit, OnDestroy { // @public export class CdkTrapFocus implements OnDestroy, AfterContentInit, OnChanges, DoCheck { constructor(_elementRef: ElementRef, _focusTrapFactory: FocusTrapFactory, - _document: any); + _document: any, + _ngZone?: NgZone | undefined); get autoCapture(): boolean; set autoCapture(value: boolean); get enabled(): boolean;