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;