From 34c529dc2380058578c74fd21f67ed979470bd34 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sun, 12 Feb 2023 09:43:21 +0100 Subject: [PATCH] fix(cdk/dialog): not emitting closed event on external detachments Fixes that the CDK dialog wasn't emitting to the `closed` event when it is detached externally, e.g. by a scroll strategy or a navigation. We had unit tests for this on the Material side, but we had special logic to handle it there. Fixes #26581. --- src/cdk/dialog/BUILD.bazel | 1 + src/cdk/dialog/dialog-config.ts | 6 ++++ src/cdk/dialog/dialog-ref.ts | 32 +++++++++++------ src/cdk/dialog/dialog.spec.ts | 42 ++++++++++++++++++++--- src/material/bottom-sheet/bottom-sheet.ts | 2 ++ src/material/dialog/dialog.ts | 2 ++ tools/public_api_guard/cdk/dialog.md | 1 + 7 files changed, 71 insertions(+), 15 deletions(-) diff --git a/src/cdk/dialog/BUILD.bazel b/src/cdk/dialog/BUILD.bazel index 1007851774b9..ac98a7537e58 100644 --- a/src/cdk/dialog/BUILD.bazel +++ b/src/cdk/dialog/BUILD.bazel @@ -45,6 +45,7 @@ ng_test_library( "//src/cdk/testing/private", "@npm//@angular/common", "@npm//@angular/platform-browser", + "@npm//rxjs", ], ) diff --git a/src/cdk/dialog/dialog-config.ts b/src/cdk/dialog/dialog-config.ts index 8451c86833af..b39553ab4ec4 100644 --- a/src/cdk/dialog/dialog-config.ts +++ b/src/cdk/dialog/dialog-config.ts @@ -132,6 +132,12 @@ export class DialogConfig { this.close(undefined, {focusOrigin: 'mouse'}); } }); + + overlayRef.detachments().subscribe(() => { + // Check specifically for `false`, because we want `undefined` to be treated like `true`. + if (config.closeOnOverlayDetachments !== false) { + this._finishClose(); + } + }); } /** @@ -80,16 +87,8 @@ export class DialogRef { * @param options Additional options to customize the closing behavior. */ close(result?: R, options?: DialogCloseOptions): void { - if (this.containerInstance) { - const closedSubject = this.closed as Subject; - this.containerInstance._closeInteractionType = options?.focusOrigin || 'program'; - this.overlayRef.dispose(); - closedSubject.next(result); - closedSubject.complete(); - (this as {componentInstance: C}).componentInstance = ( - this as {containerInstance: BasePortalOutlet} - ).containerInstance = null!; - } + this._finishClose(result, options); + this.overlayRef.dispose(); } /** Updates the position of the dialog based on the current position strategy. */ @@ -119,4 +118,17 @@ export class DialogRef { this.overlayRef.removePanelClass(classes); return this; } + + /** Finalizes the closing sequence once the overlay is detached. */ + private _finishClose(result?: R, options?: DialogCloseOptions) { + if (this.containerInstance) { + const closedSubject = this.closed as Subject; + this.containerInstance._closeInteractionType = options?.focusOrigin || 'program'; + closedSubject.next(result); + closedSubject.complete(); + (this as {componentInstance: C}).componentInstance = ( + this as {containerInstance: BasePortalOutlet} + ).containerInstance = null!; + } + } } diff --git a/src/cdk/dialog/dialog.spec.ts b/src/cdk/dialog/dialog.spec.ts index 10886d768d88..b080954798ef 100644 --- a/src/cdk/dialog/dialog.spec.ts +++ b/src/cdk/dialog/dialog.spec.ts @@ -23,7 +23,7 @@ import {By} from '@angular/platform-browser'; import {Location} from '@angular/common'; import {SpyLocation} from '@angular/common/testing'; import {Directionality} from '@angular/cdk/bidi'; -import {Overlay, OverlayContainer} from '@angular/cdk/overlay'; +import {Overlay, OverlayContainer, ScrollDispatcher} from '@angular/cdk/overlay'; import {A, ESCAPE} from '@angular/cdk/keycodes'; import {_supportsShadowDom} from '@angular/cdk/platform'; import { @@ -31,6 +31,7 @@ import { createKeyboardEvent, dispatchEvent, } from '@angular/cdk/testing/private'; +import {Subject} from 'rxjs'; import {DIALOG_DATA, Dialog, DialogModule, DialogRef} from './index'; describe('Dialog', () => { @@ -41,6 +42,7 @@ describe('Dialog', () => { let viewContainerFixture: ComponentFixture; let mockLocation: SpyLocation; let overlay: Overlay; + let scrolledSubject = new Subject(); beforeEach(fakeAsync(() => { TestBed.configureTestingModule({ @@ -59,6 +61,10 @@ describe('Dialog', () => { providers: [ {provide: Location, useClass: SpyLocation}, {provide: TEMPLATE_INJECTOR_TEST_TOKEN, useValue: 'hello from test module'}, + { + provide: ScrollDispatcher, + useFactory: () => ({scrolled: () => scrolledSubject}), + }, ], }); @@ -504,24 +510,28 @@ describe('Dialog', () => { })); it('should close all dialogs when the user goes forwards/backwards in history', fakeAsync(() => { - dialog.open(PizzaMsg); + const closeSpy = jasmine.createSpy('closed'); + dialog.open(PizzaMsg).closed.subscribe(closeSpy); viewContainerFixture.detectChanges(); - dialog.open(PizzaMsg); + dialog.open(PizzaMsg).closed.subscribe(closeSpy); viewContainerFixture.detectChanges(); expect(overlayContainerElement.querySelectorAll('cdk-dialog-container').length).toBe(2); + expect(closeSpy).not.toHaveBeenCalled(); mockLocation.simulateUrlPop(''); viewContainerFixture.detectChanges(); flush(); expect(overlayContainerElement.querySelectorAll('cdk-dialog-container').length).toBe(0); + expect(closeSpy).toHaveBeenCalledTimes(2); })); it('should close all open dialogs when the location hash changes', fakeAsync(() => { - dialog.open(PizzaMsg); + const closeSpy = jasmine.createSpy('closed'); + dialog.open(PizzaMsg).closed.subscribe(closeSpy); viewContainerFixture.detectChanges(); - dialog.open(PizzaMsg); + dialog.open(PizzaMsg).closed.subscribe(closeSpy); viewContainerFixture.detectChanges(); expect(overlayContainerElement.querySelectorAll('cdk-dialog-container').length).toBe(2); @@ -533,6 +543,28 @@ describe('Dialog', () => { expect(overlayContainerElement.querySelectorAll('cdk-dialog-container').length).toBe(0); })); + it('should close the dialog when detached externally', fakeAsync(() => { + const closeSpy = jasmine.createSpy('closed'); + dialog + .open(PizzaMsg, {scrollStrategy: overlay.scrollStrategies.close()}) + .closed.subscribe(closeSpy); + viewContainerFixture.detectChanges(); + dialog + .open(PizzaMsg, {scrollStrategy: overlay.scrollStrategies.close()}) + .closed.subscribe(closeSpy); + viewContainerFixture.detectChanges(); + + expect(overlayContainerElement.querySelectorAll('cdk-dialog-container').length).toBe(2); + expect(closeSpy).not.toHaveBeenCalled(); + + scrolledSubject.next(); + viewContainerFixture.detectChanges(); + flush(); + + expect(overlayContainerElement.querySelectorAll('cdk-dialog-container').length).toBe(0); + expect(closeSpy).toHaveBeenCalledTimes(2); + })); + it('should have the componentInstance available in the afterClosed callback', fakeAsync(() => { let dialogRef = dialog.open(PizzaMsg); let spy = jasmine.createSpy('afterClosed spy'); diff --git a/src/material/bottom-sheet/bottom-sheet.ts b/src/material/bottom-sheet/bottom-sheet.ts index af410998f155..28f02e904648 100644 --- a/src/material/bottom-sheet/bottom-sheet.ts +++ b/src/material/bottom-sheet/bottom-sheet.ts @@ -95,6 +95,8 @@ export class MatBottomSheet implements OnDestroy { ..._config, // Disable closing since we need to sync it up to the animation ourselves. disableClose: true, + // Disable closing on detachments so that we can sync up the animation. + closeOnOverlayDetachments: false, maxWidth: '100%', container: MatBottomSheetContainer, scrollStrategy: _config.scrollStrategy || this._overlay.scrollStrategies.block(), diff --git a/src/material/dialog/dialog.ts b/src/material/dialog/dialog.ts index cd59c7626ca1..29e1db3510cd 100644 --- a/src/material/dialog/dialog.ts +++ b/src/material/dialog/dialog.ts @@ -170,6 +170,8 @@ export abstract class _MatDialogBase implemen // We want to do the cleanup here, rather than the CDK service, because the CDK destroys // the dialogs immediately whereas we want it to wait for the animations to finish. closeOnDestroy: false, + // Disable closing on detachments so that we can sync up the animation. + closeOnOverlayDetachments: false, container: { type: this._dialogContainerType, providers: () => [ diff --git a/tools/public_api_guard/cdk/dialog.md b/tools/public_api_guard/cdk/dialog.md index 7649929e8034..579b2578a627 100644 --- a/tools/public_api_guard/cdk/dialog.md +++ b/tools/public_api_guard/cdk/dialog.md @@ -127,6 +127,7 @@ export class DialogConfig | { type: Type;