From 6b326a5fd00c2937e42eefe3742941439f49d5be Mon Sep 17 00:00:00 2001 From: Oscar Hinton Date: Thu, 26 Feb 2026 17:44:49 +0100 Subject: [PATCH] [CL-1051] Generic closeOnNavigation for drawers (#19031) Exposes closeOnNavigation from cdk config and implements a drawer close solution on navigation. More complex scenarios may still require custom handling. DialogService is referenced in imported components in some tests meaning we need to use overrideProvider rather than providers. --- ...ult-list-items-container.component.spec.ts | 5 +- .../popup/settings/archive.component.spec.ts | 5 +- .../credentials/fido2-vault.component.spec.ts | 5 +- .../vault-item-dialog.component.spec.ts | 5 +- .../add-edit-v2.component.spec.ts | 5 +- .../src/dialog/dialog.service.spec.ts | 115 ++++++++++++++++++ libs/components/src/dialog/dialog.service.ts | 50 +++++++- libs/components/src/dialog/dialogs.mdx | 13 +- 8 files changed, 186 insertions(+), 17 deletions(-) create mode 100644 libs/components/src/dialog/dialog.service.spec.ts diff --git a/apps/browser/src/vault/popup/components/vault/vault-list-items-container/vault-list-items-container.component.spec.ts b/apps/browser/src/vault/popup/components/vault/vault-list-items-container/vault-list-items-container.component.spec.ts index eda84265e90..22eb12e3d05 100644 --- a/apps/browser/src/vault/popup/components/vault/vault-list-items-container/vault-list-items-container.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault/vault-list-items-container/vault-list-items-container.component.spec.ts @@ -84,11 +84,12 @@ describe("VaultListItemsContainerComponent", () => { { provide: CipherService, useValue: mock() }, { provide: Router, useValue: { navigate: jest.fn() } }, { provide: PlatformUtilsService, useValue: { getAutofillKeyboardShortcut: () => "" } }, - { provide: DialogService, useValue: mock() }, { provide: PasswordRepromptService, useValue: mock() }, ], schemas: [CUSTOM_ELEMENTS_SCHEMA], - }).compileComponents(); + }) + .overrideProvider(DialogService, { useValue: mock() }) + .compileComponents(); fixture = TestBed.createComponent(VaultListItemsContainerComponent); component = fixture.componentInstance; diff --git a/apps/browser/src/vault/popup/settings/archive.component.spec.ts b/apps/browser/src/vault/popup/settings/archive.component.spec.ts index bbb61b57a84..810ab85c4c6 100644 --- a/apps/browser/src/vault/popup/settings/archive.component.spec.ts +++ b/apps/browser/src/vault/popup/settings/archive.component.spec.ts @@ -62,7 +62,6 @@ describe("ArchiveComponent", () => { useValue: { hasOrganizations, organizations$: () => of([]) }, }, { provide: CollectionService, useValue: { decryptedCollections$ } }, - { provide: DialogService, useValue: mock() }, { provide: CipherService, useValue: mock() }, { provide: CipherArchiveService, @@ -99,7 +98,9 @@ describe("ArchiveComponent", () => { }, }, ], - }).compileComponents(); + }) + .overrideProvider(DialogService, { useValue: mock() }) + .compileComponents(); fixture = TestBed.createComponent(ArchiveComponent); component = fixture.componentInstance; diff --git a/apps/desktop/src/autofill/modal/credentials/fido2-vault.component.spec.ts b/apps/desktop/src/autofill/modal/credentials/fido2-vault.component.spec.ts index 70ef4461f6a..2a5da1b766b 100644 --- a/apps/desktop/src/autofill/modal/credentials/fido2-vault.component.spec.ts +++ b/apps/desktop/src/autofill/modal/credentials/fido2-vault.component.spec.ts @@ -10,6 +10,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service" import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherRepromptType, CipherType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; +import { DialogService } from "@bitwarden/components"; import { PasswordRepromptService } from "@bitwarden/vault"; import { DesktopSettingsService } from "../../../platform/services/desktop-settings.service"; @@ -65,7 +66,9 @@ describe("Fido2VaultComponent", () => { { provide: I18nService, useValue: mockI18nService }, ], schemas: [NO_ERRORS_SCHEMA], - }).compileComponents(); + }) + .overrideProvider(DialogService, { useValue: mock() }) + .compileComponents(); fixture = TestBed.createComponent(Fido2VaultComponent); component = fixture.componentInstance; diff --git a/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.spec.ts b/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.spec.ts index 3409fa9b8d3..0293245c733 100644 --- a/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.spec.ts +++ b/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.spec.ts @@ -86,7 +86,6 @@ describe("VaultItemDialogComponent", () => { { provide: I18nService, useValue: { t: (key: string) => key } }, { provide: DIALOG_DATA, useValue: { ...baseParams } }, { provide: DialogRef, useValue: {} }, - { provide: DialogService, useValue: {} }, { provide: ToastService, useValue: { @@ -173,7 +172,9 @@ describe("VaultItemDialogComponent", () => { { provide: SyncService, useValue: {} }, { provide: CipherRiskService, useValue: {} }, ], - }).compileComponents(); + }) + .overrideProvider(DialogService, { useValue: {} }) + .compileComponents(); fixture = TestBed.createComponent(TestVaultItemDialogComponent); component = fixture.componentInstance; diff --git a/apps/web/src/app/vault/individual-vault/add-edit-v2.component.spec.ts b/apps/web/src/app/vault/individual-vault/add-edit-v2.component.spec.ts index 3eecd4ee549..1f3f285d153 100644 --- a/apps/web/src/app/vault/individual-vault/add-edit-v2.component.spec.ts +++ b/apps/web/src/app/vault/individual-vault/add-edit-v2.component.spec.ts @@ -87,7 +87,6 @@ describe("AddEditComponentV2", () => { { provide: DIALOG_DATA, useValue: mockParams }, { provide: DialogRef, useValue: dialogRef }, { provide: I18nService, useValue: { t: jest.fn().mockReturnValue("login") } }, - { provide: DialogService, useValue: dialogService }, { provide: CipherService, useValue: cipherService }, { provide: MessagingService, useValue: messagingService }, { provide: OrganizationService, useValue: organizationService }, @@ -105,7 +104,9 @@ describe("AddEditComponentV2", () => { }, { provide: AccountService, useValue: accountService }, ], - }).compileComponents(); + }) + .overrideProvider(DialogService, { useValue: dialogService }) + .compileComponents(); fixture = TestBed.createComponent(AddEditComponentV2); component = fixture.componentInstance; diff --git a/libs/components/src/dialog/dialog.service.spec.ts b/libs/components/src/dialog/dialog.service.spec.ts new file mode 100644 index 00000000000..b4c40c066b3 --- /dev/null +++ b/libs/components/src/dialog/dialog.service.spec.ts @@ -0,0 +1,115 @@ +import { Dialog as CdkDialog } from "@angular/cdk/dialog"; +import { ChangeDetectionStrategy, Component } from "@angular/core"; +import { TestBed } from "@angular/core/testing"; +import { provideRouter } from "@angular/router"; +import { RouterTestingHarness } from "@angular/router/testing"; +import { mock, MockProxy } from "jest-mock-extended"; +import { BehaviorSubject } from "rxjs"; + +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; + +import { DialogService } from "./dialog.service"; +import { DrawerService } from "./drawer.service"; + +@Component({ + selector: "test-drawer", + template: "", + changeDetection: ChangeDetectionStrategy.OnPush, +}) +class TestDrawerComponent {} + +@Component({ + selector: "test-initial-route", + template: "

Initial Route

", + changeDetection: ChangeDetectionStrategy.OnPush, +}) +class InitialRouteComponent {} + +@Component({ + selector: "test-other-route", + template: "

Other Route

", + changeDetection: ChangeDetectionStrategy.OnPush, +}) +class OtherRouteComponent {} + +describe("DialogService", () => { + let service: DialogService; + let drawerService: MockProxy; + let cdkDialog: MockProxy; + let routerHarness: RouterTestingHarness; + let authStatus$: BehaviorSubject; + + beforeEach(async () => { + drawerService = mock(); + cdkDialog = mock(); + authStatus$ = new BehaviorSubject(AuthenticationStatus.Unlocked); + + TestBed.configureTestingModule({ + providers: [ + DialogService, + { provide: DrawerService, useValue: drawerService }, + { provide: CdkDialog, useValue: cdkDialog }, + { + provide: AuthService, + useValue: { + getAuthStatus: () => authStatus$, + }, + }, + provideRouter([ + { path: "", component: InitialRouteComponent }, + { path: "other-route", component: OtherRouteComponent }, + { path: "another-route", component: OtherRouteComponent }, + ]), + ], + }); + + routerHarness = await RouterTestingHarness.create(); + // Navigate to the initial route to set up the router state + await routerHarness.navigateByUrl("/"); + + service = TestBed.inject(DialogService); + }); + + describe("close drawer on navigation", () => { + it("closes the drawer when navigating to a different route with closeOnNavigation enabled", async () => { + service.openDrawer(TestDrawerComponent, { closeOnNavigation: true }); + + await routerHarness.navigateByUrl("/other-route"); + + expect(drawerService.close).toHaveBeenCalled(); + }); + + it("does not close the drawer when navigating if closeOnNavigation is disabled", async () => { + service.openDrawer(TestDrawerComponent, { closeOnNavigation: false }); + + await routerHarness.navigateByUrl("/other-route"); + + expect(drawerService.close).not.toHaveBeenCalled(); + }); + + it("does not close the drawer when only query params change", async () => { + service.openDrawer(TestDrawerComponent, { closeOnNavigation: true }); + + await routerHarness.navigateByUrl("/?foo=bar"); + + expect(drawerService.close).not.toHaveBeenCalled(); + }); + + it("closes the drawer when the path changes but query params remain", async () => { + service.openDrawer(TestDrawerComponent, { closeOnNavigation: true }); + + await routerHarness.navigateByUrl("/other-route?foo=bar"); + + expect(drawerService.close).toHaveBeenCalled(); + }); + + it("does not close the drawer by default when closeOnNavigation is not specified", async () => { + service.openDrawer(TestDrawerComponent); + + await routerHarness.navigateByUrl("/other-route"); + + expect(drawerService.close).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/libs/components/src/dialog/dialog.service.ts b/libs/components/src/dialog/dialog.service.ts index d4cff75eddd..09aac449d6d 100644 --- a/libs/components/src/dialog/dialog.service.ts +++ b/libs/components/src/dialog/dialog.service.ts @@ -10,7 +10,17 @@ import { ComponentPortal, Portal } from "@angular/cdk/portal"; import { Injectable, Injector, TemplateRef, inject } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { NavigationEnd, Router } from "@angular/router"; -import { filter, firstValueFrom, map, Observable, Subject, switchMap, take } from "rxjs"; +import { + distinctUntilChanged, + filter, + firstValueFrom, + map, + Observable, + startWith, + Subject, + switchMap, + take, +} from "rxjs"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; @@ -61,7 +71,14 @@ export abstract class DialogRef implements Pick< export type DialogConfig = Pick< CdkDialogConfig, - "data" | "disableClose" | "ariaModal" | "positionStrategy" | "height" | "width" | "restoreFocus" + | "data" + | "disableClose" + | "ariaModal" + | "positionStrategy" + | "height" + | "width" + | "restoreFocus" + | "closeOnNavigation" >; /** @@ -136,7 +153,11 @@ class DrawerDialogRef implements DialogRef { /** The portal containing the drawer */ portal?: Portal; - constructor(private drawerService: DrawerService) {} + constructor( + private drawerService: DrawerService, + /** Whether to close this drawer when navigating to a different route */ + readonly closeOnNavigation = false, + ) {} close(result?: R, _options?: DialogCloseOptions): void { if (this.disableClose) { @@ -187,7 +208,7 @@ export class DialogService { private dialog = inject(CdkDialog); private drawerService = inject(DrawerService); private injector = inject(Injector); - private router = inject(Router, { optional: true }); + private router = inject(Router); private authService = inject(AuthService, { optional: true }); private backDropClasses = ["tw-fixed", "tw-bg-black", "tw-bg-opacity-30", "tw-inset-0"]; @@ -210,6 +231,24 @@ export class DialogService { ) .subscribe(() => this.closeAll()); } + + /** + * Close the active drawer on route navigation if configured. + * Note: CDK dialogs have their own `closeOnNavigation` config option, + * but drawers use a custom implementation that requires manual cleanup. + */ + if (this.router) { + this.router.events + .pipe( + filter((event): event is NavigationEnd => event instanceof NavigationEnd), + map((event) => event.urlAfterRedirects.split("?")[0]), + startWith(this.router.url.split("?")[0]), + distinctUntilChanged(), + filter(() => this.activeDrawer?.closeOnNavigation === true), + takeUntilDestroyed(), + ) + .subscribe(() => this.closeDrawer()); + } } open( @@ -235,6 +274,7 @@ export class DialogService { backdropClass: this.backDropClasses, scrollStrategy: this.defaultScrollStrategy, positionStrategy: config?.positionStrategy ?? new ResponsivePositionStrategy(), + closeOnNavigation: config?.closeOnNavigation, injector, ...config, }; @@ -258,7 +298,7 @@ export class DialogService { * This is also circular. When creating the DrawerDialogRef, we do not yet have a portal instance to provide to the injector. * Similar to `this.open`, we get around this with mutability. */ - this.activeDrawer = new DrawerDialogRef(this.drawerService); + this.activeDrawer = new DrawerDialogRef(this.drawerService, config?.closeOnNavigation ?? false); const portal = new ComponentPortal( component, null, diff --git a/libs/components/src/dialog/dialogs.mdx b/libs/components/src/dialog/dialogs.mdx index 2b8afb06783..c2e674273ca 100644 --- a/libs/components/src/dialog/dialogs.mdx +++ b/libs/components/src/dialog/dialogs.mdx @@ -28,7 +28,16 @@ For non-blocking, supplementary content, open dialogs as a ### Closing Drawers on Navigation When using drawers, you may want to close them automatically when the user navigates to another page -to prevent the drawer from persisting across route changes. To implement this functionality: +to prevent the drawer from persisting across route changes. In most cases you can achieve this by +using the `closeOnNavigation` option when opening the drawer. + +```ts +this.dialogService.open(MyDialogComponent, { closeOnNavigation: true }); +``` + +In some cases you may want more control of when the drawer is closed, such as if you have nested +routes and want the drawer to persist across some route changes but not others. In this case, you +can manually close the drawer in `ngOnDestroy`. To do this: 1. Store a reference to the dialog when opening it 2. Implement `OnDestroy` and close the dialog in `ngOnDestroy` @@ -52,8 +61,6 @@ export class MyComponent implements OnDestroy { } ``` -This ensures drawers are closed when the component is destroyed during navigation. - ## Placement Dialogs should be centered vertically and horizontally on screen. Dialogs height should expand to