From 99bd29afe6e01119be572d35982dc6dea306abe8 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 20 Jun 2020 11:03:45 +0200 Subject: [PATCH] fix(overlay): overlay directives not emitting when detached externally Currently the `ConnectedOverlayDirective` only emits the `detach` event when it _thinks_ that the overlay is detached (escape press, backdrop click etc.), but this won't necessarily be correct (e.g. when it was closed by a scroll strategy). These changes refactor the outputs to always be one-to-one with the `OverlayRef` detachments. --- src/cdk/overlay/overlay-directives.spec.ts | 71 +++++++++++++++++++--- src/cdk/overlay/overlay-directives.ts | 17 +++--- 2 files changed, 72 insertions(+), 16 deletions(-) diff --git a/src/cdk/overlay/overlay-directives.spec.ts b/src/cdk/overlay/overlay-directives.spec.ts index 97b1255ea9f3..d0813b167992 100644 --- a/src/cdk/overlay/overlay-directives.spec.ts +++ b/src/cdk/overlay/overlay-directives.spec.ts @@ -8,13 +8,21 @@ import { dispatchEvent, } from '@angular/cdk/testing/private'; import {ESCAPE, A} from '@angular/cdk/keycodes'; -import {Overlay, CdkConnectedOverlay, OverlayModule, CdkOverlayOrigin} from './index'; +import { + Overlay, + CdkConnectedOverlay, + OverlayModule, + CdkOverlayOrigin, + ScrollDispatcher, + ScrollStrategy, +} from './index'; import {OverlayContainer} from './overlay-container'; import { ConnectedOverlayPositionChange, ConnectionPositionPair, } from './position/connected-position'; import {FlexibleConnectedPositionStrategy} from './position/flexible-connected-position-strategy'; +import {Subject} from 'rxjs'; describe('Overlay directives', () => { @@ -23,12 +31,17 @@ describe('Overlay directives', () => { let overlayContainerElement: HTMLElement; let fixture: ComponentFixture; let dir: {value: string}; + let scrolledSubject = new Subject(); beforeEach(() => { TestBed.configureTestingModule({ imports: [OverlayModule], declarations: [ConnectedOverlayDirectiveTest, ConnectedOverlayPropertyInitOrder], - providers: [{provide: Directionality, useFactory: () => dir = {value: 'ltr'}}], + providers: [{provide: Directionality, useFactory: () => dir = {value: 'ltr'}}, + {provide: ScrollDispatcher, useFactory: () => ({ + scrolled: () => scrolledSubject.asObservable() + })} + ], }); }); @@ -529,7 +542,7 @@ describe('Overlay directives', () => { }); describe('outputs', () => { - it('should emit backdropClick appropriately', () => { + it('should emit when the backdrop was clicked', () => { fixture.componentInstance.hasBackdrop = true; fixture.componentInstance.isOpen = true; fixture.detectChanges(); @@ -543,7 +556,7 @@ describe('Overlay directives', () => { .toHaveBeenCalledWith(jasmine.any(MouseEvent)); }); - it('should emit positionChange appropriately', () => { + it('should emit when the position has changed', () => { expect(fixture.componentInstance.positionChangeHandler).not.toHaveBeenCalled(); fixture.componentInstance.isOpen = true; fixture.detectChanges(); @@ -556,15 +569,24 @@ describe('Overlay directives', () => { .toBe(true, `Expected directive to emit an instance of ConnectedOverlayPositionChange.`); }); - it('should emit attach and detach appropriately', () => { + it('should emit when attached', () => { expect(fixture.componentInstance.attachHandler).not.toHaveBeenCalled(); - expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled(); fixture.componentInstance.isOpen = true; fixture.detectChanges(); expect(fixture.componentInstance.attachHandler).toHaveBeenCalled(); expect(fixture.componentInstance.attachResult instanceof HTMLElement) .toBe(true, `Expected pane to be populated with HTML elements when attach was called.`); + + fixture.componentInstance.isOpen = false; + fixture.detectChanges(); + }); + + it('should emit when detached', () => { + expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled(); + fixture.componentInstance.isOpen = true; + fixture.detectChanges(); + expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled(); fixture.componentInstance.isOpen = false; @@ -584,11 +606,40 @@ describe('Overlay directives', () => { expect(fixture.componentInstance.keydownHandler).toHaveBeenCalledWith(event); }); + it('should emit when detached externally', () => { + expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled(); + fixture.componentInstance.scrollStrategy = overlay.scrollStrategies.close(); + fixture.componentInstance.isOpen = true; + fixture.detectChanges(); + + expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled(); + + scrolledSubject.next(); + fixture.detectChanges(); + + expect(fixture.componentInstance.detachHandler).toHaveBeenCalled(); + }); + + // This is intended as a simplified example of a more complicated bug in g3. Technically + // these events shouldn't invoke their listeners after destruction anyway, but in some + // tests it can happen. For more context: https://github.com/crisbeto/material2/pull/10 + it('should not emit after the directive is destroyed', () => { + const spy = jasmine.createSpy('detach spy'); + const subscription = + fixture.componentInstance.connectedOverlayDirective.detach.subscribe(spy); + + fixture.componentInstance.isOpen = true; + fixture.detectChanges(); + fixture.destroy(); + + expect(spy).not.toHaveBeenCalled(); + subscription.unsubscribe(); + }); + }); }); - @Component({ template: ` @@ -605,6 +656,7 @@ describe('Overlay directives', () => { [cdkConnectedOverlayFlexibleDimensions]="flexibleDimensions" [cdkConnectedOverlayGrowAfterOpen]="growAfterOpen" [cdkConnectedOverlayPush]="push" + [cdkConnectedOverlayScrollStrategy]="scrollStrategy" cdkConnectedOverlayBackdropClass="mat-test-class" cdkConnectedOverlayPanelClass="cdk-test-panel-class" (backdropClick)="backdropClickHandler($event)" @@ -640,13 +692,14 @@ class ConnectedOverlayDirectiveTest { flexibleDimensions: boolean; growAfterOpen: boolean; push: boolean; + scrollStrategy: ScrollStrategy; backdropClickHandler = jasmine.createSpy('backdropClick handler'); positionChangeHandler = jasmine.createSpy('positionChange handler'); keydownHandler = jasmine.createSpy('keydown handler'); positionOverrides: ConnectionPositionPair[]; attachHandler = jasmine.createSpy('attachHandler').and.callFake(() => { - this.attachResult = - this.connectedOverlayDirective.overlayRef.overlayElement.querySelector('p') as HTMLElement; + const overlayElement = this.connectedOverlayDirective.overlayRef.overlayElement; + this.attachResult = overlayElement.querySelector('p') as HTMLElement; }); detachHandler = jasmine.createSpy('detachHandler'); attachResult: HTMLElement; diff --git a/src/cdk/overlay/overlay-directives.ts b/src/cdk/overlay/overlay-directives.ts index ca5262aa7ed9..d0f3d0bba99c 100644 --- a/src/cdk/overlay/overlay-directives.ts +++ b/src/cdk/overlay/overlay-directives.ts @@ -111,6 +111,8 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges { private _flexibleDimensions = false; private _push = false; private _backdropSubscription = Subscription.EMPTY; + private _attachSubscription = Subscription.EMPTY; + private _detachSubscription = Subscription.EMPTY; private _offsetX: number; private _offsetY: number; private _position: FlexibleConnectedPositionStrategy; @@ -246,11 +248,13 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges { } ngOnDestroy() { + this._attachSubscription.unsubscribe(); + this._detachSubscription.unsubscribe(); + this._backdropSubscription.unsubscribe(); + if (this._overlayRef) { this._overlayRef.dispose(); } - - this._backdropSubscription.unsubscribe(); } ngOnChanges(changes: SimpleChanges) { @@ -279,9 +283,10 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges { this.positions = defaultPositionList; } - this._overlayRef = this._overlay.create(this._buildConfig()); - - this._overlayRef.keydownEvents().subscribe((event: KeyboardEvent) => { + const overlayRef = this._overlayRef = this._overlay.create(this._buildConfig()); + this._attachSubscription = overlayRef.attachments().subscribe(() => this.attach.emit()); + this._detachSubscription = overlayRef.detachments().subscribe(() => this.detach.emit()); + overlayRef.keydownEvents().subscribe((event: KeyboardEvent) => { this.overlayKeydown.next(event); if (event.keyCode === ESCAPE && !hasModifierKey(event)) { @@ -373,7 +378,6 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges { if (!this._overlayRef.hasAttached()) { this._overlayRef.attach(this._templatePortal); - this.attach.emit(); } if (this.hasBackdrop) { @@ -389,7 +393,6 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges { private _detachOverlay() { if (this._overlayRef) { this._overlayRef.detach(); - this.detach.emit(); } this._backdropSubscription.unsubscribe();