From 24f7383f1e8ad4fedc3e7dc0af65b322f9658fb3 Mon Sep 17 00:00:00 2001 From: Matti Airas Date: Sat, 15 Aug 2026 20:45:07 +0300 Subject: [PATCH 1/4] fix(nav): close the top overlay on Back instead of navigating Back both closed a widget-settings dialog and changed the page, abandoning an in-progress layout edit. Every dialog and registered bottom sheet now gets a history entry pushed in front of it at the URL the app is already on, so a Back pops an identical URL and closes exactly one overlay; with nothing left it navigates as before. An overlay closed by Esc, a button or the backdrop takes its entry down with it. Material's own closeOnNavigation had to move out of the way: it disposes the overlay from the CDK's location listener before this service sees the pop, and the orphaned guard entry then unwound into a second Back that changed the page anyway. Closing on a real route change stays this service's job. Closes halos-org/skip#393 --- .../action-menu/action-menu.component.ts | 11 +- .../toolbar/toolbar.component.spec.ts | 6 +- .../components/toolbar/toolbar.component.ts | 11 +- .../router-overlay-navigation.service.spec.ts | 166 ++++++++++++++++++ .../router-overlay-navigation.service.ts | 108 +++++++++++- src/main.ts | 7 +- 6 files changed, 291 insertions(+), 18 deletions(-) create mode 100644 src/app/core/services/router-overlay-navigation.service.spec.ts diff --git a/src/app/core/components/action-menu/action-menu.component.ts b/src/app/core/components/action-menu/action-menu.component.ts index c50fa1c6..c6f0e7b2 100644 --- a/src/app/core/components/action-menu/action-menu.component.ts +++ b/src/app/core/components/action-menu/action-menu.component.ts @@ -4,6 +4,7 @@ import { MatBottomSheet } from '@angular/material/bottom-sheet'; import { MatIconModule } from '@angular/material/icon'; import { BreakpointObserver } from '@angular/cdk/layout'; import { WidgetHostBottomSheetComponent } from '../widget-host-bottom-sheet/widget-host-bottom-sheet.component'; +import { RouterOverlayNavigationService } from '../../services/router-overlay-navigation.service'; import { ActionMenuItem } from './action-menu-item'; /** Below this width the menu is a bottom drawer; above it, a pop-over at the tap point. */ @@ -28,6 +29,7 @@ export class ActionMenuComponent { private readonly _breakpoint = inject(BreakpointObserver); private readonly _bottomSheet = inject(MatBottomSheet); + private readonly _overlayNavigation = inject(RouterOverlayNavigationService); private readonly _anchor = viewChild.required>('anchor'); private readonly _trigger = viewChild.required(MatMenuTrigger); @@ -37,11 +39,14 @@ export class ActionMenuComponent { // and immediately dismisses it on touch devices (first seen on Linux Firefox, but real mobile // browsers do it too — the drawer vanishes the instant it opens). Disable backdrop-close on the // phone path; the always-present Cancel row is the exit. - this._bottomSheet.open(WidgetHostBottomSheetComponent, { + const sheet = this._bottomSheet.open(WidgetHostBottomSheetComponent, { data: { items: this.items() }, disableClose: true, - }) - .afterDismissed() + }); + // A bottom sheet is as blocking as a dialog, and MatBottomSheet publishes no opened stream + // for the Back guard to pick it up on its own. + this._overlayNavigation.guardOverlay(() => sheet.dismiss(), sheet.afterDismissed()); + sheet.afterDismissed() .subscribe((id?: string) => { if (id && id !== 'cancel') this.selected.emit(id); }); diff --git a/src/app/core/components/toolbar/toolbar.component.spec.ts b/src/app/core/components/toolbar/toolbar.component.spec.ts index 6a952776..a1cf1975 100644 --- a/src/app/core/components/toolbar/toolbar.component.spec.ts +++ b/src/app/core/components/toolbar/toolbar.component.spec.ts @@ -2,7 +2,7 @@ import { signal } from '@angular/core'; import { ComponentFixture, TestBed } from '@angular/core/testing'; import { Router } from '@angular/router'; import { MatBottomSheet } from '@angular/material/bottom-sheet'; -import { of } from 'rxjs'; +import { EMPTY, of } from 'rxjs'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { AppService } from '../../services/app-service'; import { ChromeVisibilityService, CHROME_HOVER_DWELL_MS } from '../../services/chrome-visibility.service'; @@ -32,7 +32,7 @@ const dashboard = { // The edit and page-management controls are hidden in a session that cannot save page changes. isReadOnlySession: signal(false), }; -const bottomSheet = { open: vi.fn(() => ({ afterDismissed: () => of(undefined) })) }; +const bottomSheet = { open: vi.fn(() => ({ afterDismissed: () => of(undefined), dismiss: vi.fn() })), dismiss: vi.fn() }; const uiEvent = { toggleFullScreen: vi.fn(), fullscreenSupported: signal(true), @@ -41,7 +41,7 @@ const uiEvent = { const app = { isNightMode: signal(false), toggleDayNightMode: vi.fn(), toggleNightMode: vi.fn(), appVersion: signal('1.0.0') }; const settings = { autoNightMode: signal(false) }; const dialog = { openNotifications: vi.fn() }; -const router = { navigate: vi.fn() }; +const router = { navigate: vi.fn(), events: EMPTY }; const alarmCount = signal(0); const notifications = { observerNotificationsInfo: () => diff --git a/src/app/core/components/toolbar/toolbar.component.ts b/src/app/core/components/toolbar/toolbar.component.ts index 08ad2a89..1e46f1ed 100644 --- a/src/app/core/components/toolbar/toolbar.component.ts +++ b/src/app/core/components/toolbar/toolbar.component.ts @@ -13,6 +13,7 @@ import { AppService } from '../../services/app-service'; import { SettingsService } from '../../services/settings.service'; import { DialogService } from '../../services/dialog.service'; import { NotificationsService } from '../../services/notifications.service'; +import { RouterOverlayNavigationService } from '../../services/router-overlay-navigation.service'; import { PageNavControlComponent } from '../page-nav-control/page-nav-control.component'; import { PageManagerBottomSheetComponent } from '../page-manager-bottom-sheet/page-manager-bottom-sheet.component'; @@ -48,6 +49,7 @@ export class ToolbarComponent implements OnDestroy { private readonly router = inject(Router); private readonly notifications = inject(NotificationsService); private readonly bottomSheet = inject(MatBottomSheet); + private readonly overlayNavigation = inject(RouterOverlayNavigationService); protected readonly isNightMode = this.app.isNightMode; protected readonly autoNightMode = this.settings.autoNightMode; @@ -134,10 +136,11 @@ export class ToolbarComponent implements OnDestroy { // A sheet page op (e.g. deleting a lower page) can reassign the active page without // navigating, leaving the URL on a now-stale index; re-sync the route on dismiss so the // page dots stay tappable. - this.bottomSheet - .open(PageManagerBottomSheetComponent) - .afterDismissed() - .subscribe(() => this.dashboard.navigateToActive()); + const sheet = this.bottomSheet.open(PageManagerBottomSheetComponent); + // A bottom sheet is as blocking as a dialog, and MatBottomSheet publishes no opened stream for + // the Back guard to pick it up on its own. + this.overlayNavigation.guardOverlay(() => sheet.dismiss(), sheet.afterDismissed()); + sheet.afterDismissed().subscribe(() => this.dashboard.navigateToActive()); } protected openNotifications(): void { diff --git a/src/app/core/services/router-overlay-navigation.service.spec.ts b/src/app/core/services/router-overlay-navigation.service.spec.ts new file mode 100644 index 00000000..58293d2c --- /dev/null +++ b/src/app/core/services/router-overlay-navigation.service.spec.ts @@ -0,0 +1,166 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { Subject } from 'rxjs'; +import { TestBed } from '@angular/core/testing'; +import { MatDialog, MatDialogRef } from '@angular/material/dialog'; +import { MatBottomSheet } from '@angular/material/bottom-sheet'; +import { Router, NavigationStart } from '@angular/router'; +import { MAT_DIALOG_DEFAULT_OPTIONS } from '@angular/material/dialog'; +import { MAT_BOTTOM_SHEET_DEFAULT_OPTIONS } from '@angular/material/bottom-sheet'; +import { EmbedModeService } from './embed-mode.service'; +import { OVERLAY_BACK_GUARD_PROVIDERS, RouterOverlayNavigationService } from './router-overlay-navigation.service'; + +interface DialogStub { afterClosed: () => Subject; close: () => void } + +describe('RouterOverlayNavigationService', () => { + const routerEvents = new Subject(); + const dialogOpened = new Subject>(); + let openDialogs: DialogStub[]; + let dialogMock: { afterOpened: Subject>; openDialogs: DialogStub[]; closeAll: () => void }; + let bottomSheetMock: { dismiss: ReturnType void>> }; + let embed: boolean; + + const openDialog = (): { closed: Subject; close: ReturnType } => { + const closed = new Subject(); + const close = vi.fn(() => closed.next(undefined)); + const stub: DialogStub = { afterClosed: () => closed, close }; + openDialogs.push(stub); + dialogOpened.next(stub as unknown as MatDialogRef); + return { closed, close }; + }; + + const pop = (): void => { window.dispatchEvent(new PopStateEvent('popstate')); }; + + const create = (): RouterOverlayNavigationService => TestBed.inject(RouterOverlayNavigationService); + + beforeEach(() => { + openDialogs = []; + embed = false; + dialogMock = { afterOpened: dialogOpened, openDialogs, closeAll: vi.fn() }; + bottomSheetMock = { dismiss: vi.fn() }; + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + { provide: MatDialog, useValue: dialogMock }, + { provide: MatBottomSheet, useValue: bottomSheetMock }, + { provide: Router, useValue: { events: routerEvents } }, + { provide: EmbedModeService, useValue: { embed: () => embed } } + ] + }); + vi.restoreAllMocks(); + }); + + it('closes the open dialog on Back instead of letting the route change (#393)', () => { + const pushSpy = vi.spyOn(window.history, 'pushState'); + create(); + const dialog = openDialog(); + expect(pushSpy).toHaveBeenCalledTimes(1); + + pop(); + + expect(dialog.close).toHaveBeenCalledTimes(1); + }); + + it('closes one overlay per Back, topmost first (#393)', () => { + create(); + const first = openDialog(); + const second = openDialog(); + + pop(); + expect(second.close).toHaveBeenCalledTimes(1); + expect(first.close).not.toHaveBeenCalled(); + + pop(); + expect(first.close).toHaveBeenCalledTimes(1); + }); + + it('lets Back navigate once no overlay is left (#393)', () => { + create(); + const dialog = openDialog(); + pop(); + expect(dialog.close).toHaveBeenCalled(); + + // Nothing of ours is left on the stack, so this pop belongs to the router. + const backSpy = vi.spyOn(window.history, 'back'); + pop(); + expect(backSpy).not.toHaveBeenCalled(); + }); + + it('drops its history entry when the dialog closes by other means (#393)', () => { + // Esc, Cancel or the backdrop close the dialog without a pop. The guard entry has to go with + // it, or the next Back is swallowed closing an overlay that is no longer on screen. + const backSpy = vi.spyOn(window.history, 'back').mockImplementation(() => undefined); + create(); + const dialog = openDialog(); + + openDialogs.pop(); + dialog.closed.next(undefined); + + expect(backSpy).toHaveBeenCalledTimes(1); + }); + + it('does not push its own history entry back when Back is what closed the dialog (#393)', () => { + const backSpy = vi.spyOn(window.history, 'back').mockImplementation(() => undefined); + create(); + openDialog(); + + pop(); + + expect(backSpy).not.toHaveBeenCalled(); + }); + + it('leaves the session history alone in embed mode (#393)', () => { + // Skip runs in an iframe inside the Freeboard panel, where its history entries land in the + // host page's session history and would make the host's Back button close Skip's dialogs. + embed = true; + const pushSpy = vi.spyOn(window.history, 'pushState'); + create(); + const dialog = openDialog(); + + expect(pushSpy).not.toHaveBeenCalled(); + pop(); + expect(dialog.close).not.toHaveBeenCalled(); + }); + + it('still clears overlays when a real navigation starts', () => { + create(); + openDialog(); + + routerEvents.next(new NavigationStart(1, '/page/2', 'imperative')); + + expect(dialogMock.closeAll).toHaveBeenCalledTimes(1); + expect(bottomSheetMock.dismiss).toHaveBeenCalled(); + }); + + it('leaves the guard pop to the popstate handler rather than clearing every overlay (#393)', () => { + // The router raises NavigationStart for the guard entry's own pop — same URL, but it still + // fires, and it arrives first. Clearing there closed every overlay and unwound the guard entry + // with a second history.back(), which navigated the page after all. + const backSpy = vi.spyOn(window.history, 'back').mockImplementation(() => undefined); + create(); + openDialog(); + + routerEvents.next(new NavigationStart(1, '/page/1', 'popstate')); + + expect(dialogMock.closeAll).not.toHaveBeenCalled(); + expect(backSpy).not.toHaveBeenCalled(); + }); + + it('takes closeOnNavigation off both overlay types (#393)', () => { + // Material disposes the overlay from its own location listener, which runs before this service + // sees the pop: the overlay vanishes, and the guard entry standing behind it unwinds into a + // second Back that changes the page. Dropping this provider silently restores that. + const values = OVERLAY_BACK_GUARD_PROVIDERS.map(p => p as { provide: unknown; useValue: { closeOnNavigation: boolean } }); + expect(values.map(p => p.provide)).toEqual([MAT_DIALOG_DEFAULT_OPTIONS, MAT_BOTTOM_SHEET_DEFAULT_OPTIONS]); + expect(values.every(p => p.useValue.closeOnNavigation === false)).toBe(true); + }); + + it('dismisses a guarded bottom sheet on Back (#393)', () => { + const service = create(); + const dismissed = new Subject(); + service.guardOverlay(() => bottomSheetMock.dismiss(), dismissed); + + pop(); + + expect(bottomSheetMock.dismiss).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/app/core/services/router-overlay-navigation.service.ts b/src/app/core/services/router-overlay-navigation.service.ts index f1ad4da1..ac1abfc7 100644 --- a/src/app/core/services/router-overlay-navigation.service.ts +++ b/src/app/core/services/router-overlay-navigation.service.ts @@ -1,10 +1,40 @@ -import { DestroyRef, Injectable, inject } from '@angular/core'; +import { DestroyRef, Injectable, Provider, inject } from '@angular/core'; import { NavigationStart, Router } from '@angular/router'; -import { MatDialog } from '@angular/material/dialog'; -import { MatBottomSheet } from '@angular/material/bottom-sheet'; -import { filter } from 'rxjs/operators'; +import { MAT_DIALOG_DEFAULT_OPTIONS, MatDialog } from '@angular/material/dialog'; +import { MAT_BOTTOM_SHEET_DEFAULT_OPTIONS, MatBottomSheet } from '@angular/material/bottom-sheet'; +import { Observable, fromEvent } from 'rxjs'; +import { filter, take } from 'rxjs/operators'; import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; +import { EmbedModeService } from './embed-mode.service'; +/** + * Material's `closeOnNavigation` disposes an overlay from inside the CDK's own location listener, + * which runs before this service sees the pop — the overlay was gone, and the guard entry it stood + * behind was then unwound into a second Back that changed the page after all. Closing on navigation + * is this service's job instead: it closes one overlay for a guarded Back and all of them for a real + * route change. + */ +export const OVERLAY_BACK_GUARD_PROVIDERS: Provider[] = [ + { provide: MAT_DIALOG_DEFAULT_OPTIONS, useValue: { closeOnNavigation: false } }, + { provide: MAT_BOTTOM_SHEET_DEFAULT_OPTIONS, useValue: { closeOnNavigation: false } } +]; + +/** One blocking overlay and the history entry standing in front of it. */ +interface IGuardedOverlay { + close: () => void; +} + +/** + * Makes browser Back close the topmost blocking overlay rather than leave the page under it. + * + * Every dialog and every registered bottom sheet gets a history entry pushed in front of it at the + * URL the app is already on. Back then pops an entry whose URL is identical, so the router treats it + * as a same-URL navigation and ignores it, and the pop is free to close one overlay instead. With + * nothing of ours left on the stack, a pop is the page's own and navigates as before. + * + * An overlay dismissed by Esc, a button or the backdrop takes its entry with it, so a later Back is + * never swallowed by an entry standing in front of nothing. + */ @Injectable({ providedIn: 'root' }) @@ -12,19 +42,87 @@ export class RouterOverlayNavigationService { private readonly _router = inject(Router); private readonly _dialog = inject(MatDialog); private readonly _bottomSheet = inject(MatBottomSheet); + private readonly _embed = inject(EmbedModeService); private readonly _destroyRef = inject(DestroyRef); + private readonly _stack: IGuardedOverlay[] = []; + /** Set while a pop is closing an overlay: that entry is already gone from the history. */ + private _consumingPop = false; + /** Set while unwinding our own entry, so the pop it causes is not read as a Back press. */ + private _unwinding = false; + constructor() { this._router.events .pipe( filter((event): event is NavigationStart => event instanceof NavigationStart), takeUntilDestroyed(this._destroyRef) ) - .subscribe(() => { + .subscribe(event => { + // A pop that this service is handling closes exactly one overlay; only a genuine route + // change clears them all. The router raises NavigationStart for the guard's own pop before + // the popstate listener runs, even though the URL is unchanged, so the trigger is what + // separates a Back standing on a guard entry from a real navigation. + if (this._stack.length > 0 && event.navigationTrigger === 'popstate') return; + if (this._consumingPop) return; if (this._dialog.openDialogs.length > 0) { this._dialog.closeAll(); } this._bottomSheet.dismiss(); }); + + if (this._embed.embed()) return; + + this._dialog.afterOpened + .pipe(takeUntilDestroyed(this._destroyRef)) + .subscribe(ref => this.guardOverlay(() => ref.close(), ref.afterClosed())); + + fromEvent(window, 'popstate') + .pipe(takeUntilDestroyed(this._destroyRef)) + .subscribe(() => this.onPopState()); + } + + /** + * Put a history entry in front of an overlay this service does not open itself — a bottom sheet, + * whose service publishes no opened stream. `close` closes that overlay; `closed$` fires once it + * has gone, however it went. + */ + public guardOverlay(close: () => void, closed$: Observable): void { + if (this._embed.embed()) return; + const entry: IGuardedOverlay = { close }; + this._stack.push(entry); + // Same URL, so popping it is a same-URL navigation the router ignores. + window.history.pushState(window.history.state, '', window.location.href); + + closed$ + .pipe(take(1), takeUntilDestroyed(this._destroyRef)) + .subscribe(() => this.release(entry)); + } + + private onPopState(): void { + if (this._unwinding) { + this._unwinding = false; + return; + } + const entry = this._stack.pop(); + if (!entry) return; + + this._consumingPop = true; + try { + entry.close(); + } finally { + this._consumingPop = false; + } + } + + /** An overlay closed on its own terms; take its history entry back down with it. */ + private release(entry: IGuardedOverlay): void { + const index = this._stack.lastIndexOf(entry); + if (index === -1) return; // A pop already removed it, which is what closed the overlay. + this._stack.splice(index, 1); + // Only the topmost entry is ours to pop. A lower one is left behind rather than popping past a + // still-open overlay above it; the stale entry costs one dead Back press at most. + if (index !== this._stack.length) return; + this._unwinding = true; + window.history.back(); } } diff --git a/src/main.ts b/src/main.ts index 25f5fe4f..473df5e8 100644 --- a/src/main.ts +++ b/src/main.ts @@ -23,7 +23,7 @@ import { AuthenticationInterceptor } from './app/core/interceptors/authenticatio import { HTTP_INTERCEPTORS, withInterceptorsFromDi, provideHttpClient } from '@angular/common/http'; import { OverlayContainer } from '@angular/cdk/overlay'; import { AppOverlayContainer } from './app/core/utils/app-overlay-container'; -import { RouterOverlayNavigationService } from './app/core/services/router-overlay-navigation.service'; +import { OVERLAY_BACK_GUARD_PROVIDERS, RouterOverlayNavigationService } from './app/core/services/router-overlay-navigation.service'; if (environment.production) { enableProdMode(); @@ -104,8 +104,9 @@ bootstrapApplication(AppComponent, { const appNetInitSvc = inject(AppNetworkInitService); return appNetInitSvc.initNetworkServices(); }), - // Ensure overlays (dialogs/bottom sheets) close on route navigation, - // including standard routerLink clicks from overlay content. + // Owns what navigation does to overlays: browser Back closes the topmost one, a real route + // change closes them all. The providers hand Material's own closeOnNavigation over to it. + ...OVERLAY_BACK_GUARD_PROVIDERS, provideAppInitializer(() => { inject(RouterOverlayNavigationService); }), From 62953a9397f17d796267df0d2bb839334b7bd3fc Mon Sep 17 00:00:00 2001 From: Matti Airas Date: Sat, 15 Aug 2026 20:45:11 +0300 Subject: [PATCH 2/4] chore: return VERSION to 1.4.1 The 1.5.0 bump treated a settings relabel as a feature. It was a fix, and the cycle that PR #600 landed in was already open at 1.4.1. --- .bumpversion.cfg | 2 +- VERSION | 2 +- package.json | 2 +- src/assets/skip-dashboard-schema.json | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.bumpversion.cfg b/.bumpversion.cfg index c99e170e..1945177f 100644 --- a/.bumpversion.cfg +++ b/.bumpversion.cfg @@ -1,5 +1,5 @@ [bumpversion] -current_version = 1.5.0 +current_version = 1.4.1 commit = True tag = False diff --git a/VERSION b/VERSION index bc80560f..347f5833 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.5.0 +1.4.1 diff --git a/package.json b/package.json index 93d55f58..da861122 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@halos-org/skip", - "version": "1.5.0", + "version": "1.4.1", "publishConfig": { "access": "public" }, diff --git a/src/assets/skip-dashboard-schema.json b/src/assets/skip-dashboard-schema.json index d8ffa9f1..6d941b6a 100644 --- a/src/assets/skip-dashboard-schema.json +++ b/src/assets/skip-dashboard-schema.json @@ -581,7 +581,7 @@ "configFileVersion": 11, "configVersion": 19, "schemaVersion": 1, - "skipVersion": "1.5.0" + "skipVersion": "1.4.1" }, "widgets": [ { From 69f249bec3952c242d2b1a076c8dcfa7dca70cea Mon Sep 17 00:00:00 2001 From: Matti Airas Date: Sat, 15 Aug 2026 21:02:08 +0300 Subject: [PATCH 3/4] fix(nav): stop the widget-settings dialog opting back into closeOnNavigation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-call closeOnNavigation overrode the global default, so Material still disposed this dialog from its own location listener and the guard entry behind it unwound into a page change — the reported case, and the one the guard exists for. --- src/app/core/services/dialog.service.spec.ts | 19 +++++++++++++++++-- src/app/core/services/dialog.service.ts | 1 - 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/app/core/services/dialog.service.spec.ts b/src/app/core/services/dialog.service.spec.ts index 81b62b67..918ffdd6 100644 --- a/src/app/core/services/dialog.service.spec.ts +++ b/src/app/core/services/dialog.service.spec.ts @@ -1,16 +1,31 @@ import { TestBed } from '@angular/core/testing'; -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { MatDialog, MatDialogConfig } from '@angular/material/dialog'; import { DialogService } from './dialog.service'; describe('DialogService', () => { let service: DialogService; + let open: ReturnType; beforeEach(() => { - TestBed.configureTestingModule({}); + open = vi.fn(() => ({ afterClosed: () => ({ subscribe: () => undefined }) })); + TestBed.configureTestingModule({ + providers: [{ provide: MatDialog, useValue: { open, openDialogs: [] } }] + }); service = TestBed.inject(DialogService); }); it('should be created', () => { expect(service).toBeTruthy(); }); + + it('leaves closeOnNavigation to the Back guard (#393)', () => { + // Material disposes the overlay from its own location listener, ahead of the guard's popstate + // handler, and the guard entry left behind then unwinds into a Back that changes the page. A + // per-call `closeOnNavigation: true` overrides the global default and silently restores that. + service.openWidgetOptions({ config: {} } as Parameters[0]); + + const config = open.mock.calls[0][1] as MatDialogConfig; + expect(config.closeOnNavigation).toBeUndefined(); + }); }); diff --git a/src/app/core/services/dialog.service.ts b/src/app/core/services/dialog.service.ts index 486a27c4..ff8d0da2 100644 --- a/src/app/core/services/dialog.service.ts +++ b/src/app/core/services/dialog.service.ts @@ -108,7 +108,6 @@ export class DialogService { data: data.config, minWidth: "50vw", maxWidth: "90vw", - closeOnNavigation: true, } ); } From a02b3d9d7224a1e972f415fd6c809fbadbe1c693 Mon Sep 17 00:00:00 2001 From: Matti Airas Date: Sat, 15 Aug 2026 21:13:52 +0300 Subject: [PATCH 4/4] fix(nav): keep the dialog defaults whole and stop unwinding into a new route A second provider for MAT_DIALOG_DEFAULT_OPTIONS replaces the first rather than merging, so registering closeOnNavigation on its own dropped the app's backdrop class and focus settings. Both sets now live in one provider beside the service that needs one of them. A real navigation also closed overlays while their guard entries stood: each release then called history.back() and took the user off the route they had just asked for. The entries are dropped before anything is closed. --- .../router-overlay-navigation.service.spec.ts | 39 +++++++++++++++---- .../router-overlay-navigation.service.ts | 32 +++++++++++---- src/main.ts | 20 ++-------- 3 files changed, 61 insertions(+), 30 deletions(-) diff --git a/src/app/core/services/router-overlay-navigation.service.spec.ts b/src/app/core/services/router-overlay-navigation.service.spec.ts index 58293d2c..c51132a1 100644 --- a/src/app/core/services/router-overlay-navigation.service.spec.ts +++ b/src/app/core/services/router-overlay-navigation.service.spec.ts @@ -1,13 +1,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { Subject } from 'rxjs'; import { TestBed } from '@angular/core/testing'; -import { MatDialog, MatDialogRef } from '@angular/material/dialog'; -import { MatBottomSheet } from '@angular/material/bottom-sheet'; +import { MAT_DIALOG_DEFAULT_OPTIONS, MatDialog, MatDialogRef } from '@angular/material/dialog'; +import { MAT_BOTTOM_SHEET_DEFAULT_OPTIONS, MatBottomSheet } from '@angular/material/bottom-sheet'; import { Router, NavigationStart } from '@angular/router'; -import { MAT_DIALOG_DEFAULT_OPTIONS } from '@angular/material/dialog'; -import { MAT_BOTTOM_SHEET_DEFAULT_OPTIONS } from '@angular/material/bottom-sheet'; import { EmbedModeService } from './embed-mode.service'; -import { OVERLAY_BACK_GUARD_PROVIDERS, RouterOverlayNavigationService } from './router-overlay-navigation.service'; +import { OVERLAY_DEFAULT_OPTIONS_PROVIDERS, RouterOverlayNavigationService } from './router-overlay-navigation.service'; interface DialogStub { afterClosed: () => Subject; close: () => void } @@ -149,9 +147,36 @@ describe('RouterOverlayNavigationService', () => { // Material disposes the overlay from its own location listener, which runs before this service // sees the pop: the overlay vanishes, and the guard entry standing behind it unwinds into a // second Back that changes the page. Dropping this provider silently restores that. - const values = OVERLAY_BACK_GUARD_PROVIDERS.map(p => p as { provide: unknown; useValue: { closeOnNavigation: boolean } }); + const values = OVERLAY_DEFAULT_OPTIONS_PROVIDERS.map(p => p as { provide: unknown; useValue: Record }); expect(values.map(p => p.provide)).toEqual([MAT_DIALOG_DEFAULT_OPTIONS, MAT_BOTTOM_SHEET_DEFAULT_OPTIONS]); - expect(values.every(p => p.useValue.closeOnNavigation === false)).toBe(true); + expect(values.every(p => p.useValue['closeOnNavigation'] === false)).toBe(true); + }); + + it('keeps the app-wide dialog styling in the same provider', () => { + // A second provider for the token replaces the first rather than merging with it, so these + // options have to travel with closeOnNavigation or dialogs silently lose their backdrop. + const dialogOptions = (OVERLAY_DEFAULT_OPTIONS_PROVIDERS[0] as { useValue: Record }).useValue; + expect(dialogOptions).toMatchObject({ + hasBackdrop: true, + disableClose: false, + autoFocus: 'first-tabbable', + delayFocusTrap: true, + backdropClass: 'dialogBackdrop' + }); + }); + + it('does not unwind guard entries into the route a real navigation is entering (#393)', () => { + // closeAll() makes every open overlay report closed. An entry still on the stack would then + // release into a history.back() that takes the user off the page they just asked for. + const backSpy = vi.spyOn(window.history, 'back').mockImplementation(() => undefined); + create(); + const dialog = openDialog(); + + routerEvents.next(new NavigationStart(1, '/page/2', 'imperative')); + openDialogs.pop(); + dialog.closed.next(undefined); + + expect(backSpy).not.toHaveBeenCalled(); }); it('dismisses a guarded bottom sheet on Back (#393)', () => { diff --git a/src/app/core/services/router-overlay-navigation.service.ts b/src/app/core/services/router-overlay-navigation.service.ts index ac1abfc7..2049799d 100644 --- a/src/app/core/services/router-overlay-navigation.service.ts +++ b/src/app/core/services/router-overlay-navigation.service.ts @@ -8,14 +8,28 @@ import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; import { EmbedModeService } from './embed-mode.service'; /** - * Material's `closeOnNavigation` disposes an overlay from inside the CDK's own location listener, - * which runs before this service sees the pop — the overlay was gone, and the guard entry it stood - * behind was then unwound into a second Back that changed the page after all. Closing on navigation - * is this service's job instead: it closes one overlay for a guarded Back and all of them for a real - * route change. + * The app's overlay defaults, in one place because a provider for either token replaces it wholesale + * rather than merging — a second `MAT_DIALOG_DEFAULT_OPTIONS` provider silently drops the backdrop + * class and focus settings from the first. + * + * `closeOnNavigation` is off for both because Material implements it by disposing the overlay from + * inside the CDK's own location listener, which runs before this service sees the pop: the overlay + * was gone, and the guard entry it stood behind was then unwound into a second Back that changed the + * page after all. Closing on navigation is this service's job instead — one overlay for a guarded + * Back, all of them for a real route change. */ -export const OVERLAY_BACK_GUARD_PROVIDERS: Provider[] = [ - { provide: MAT_DIALOG_DEFAULT_OPTIONS, useValue: { closeOnNavigation: false } }, +export const OVERLAY_DEFAULT_OPTIONS_PROVIDERS: Provider[] = [ + { + provide: MAT_DIALOG_DEFAULT_OPTIONS, + useValue: { + hasBackdrop: true, + disableClose: false, + autoFocus: 'first-tabbable', + delayFocusTrap: true, + backdropClass: 'dialogBackdrop', + closeOnNavigation: false + } + }, { provide: MAT_BOTTOM_SHEET_DEFAULT_OPTIONS, useValue: { closeOnNavigation: false } } ]; @@ -64,6 +78,10 @@ export class RouterOverlayNavigationService { // separates a Back standing on a guard entry from a real navigation. if (this._stack.length > 0 && event.navigationTrigger === 'popstate') return; if (this._consumingPop) return; + // Drop the entries before closing anything. The route the user asked for is already + // committing, and an entry still on the stack when its overlay reports closed would unwind + // into a history.back() that takes them off that route again. + this._stack.length = 0; if (this._dialog.openDialogs.length > 0) { this._dialog.closeAll(); } diff --git a/src/main.ts b/src/main.ts index 473df5e8..216df597 100644 --- a/src/main.ts +++ b/src/main.ts @@ -15,7 +15,6 @@ import { SignalKConnectionService } from './app/core/services/signalk-connection import { DataService } from './app/core/services/data.service'; import { AuthenticationService } from './app/core/services/authentication.service'; import { MAT_FORM_FIELD_DEFAULT_OPTIONS } from '@angular/material/form-field'; -import { MAT_DIALOG_DEFAULT_OPTIONS } from '@angular/material/dialog'; import { BrowserModule, bootstrapApplication } from '@angular/platform-browser'; import { AppNetworkInitService } from './app/core/services/app-initNetwork.service'; import { ConnectionStateMachine } from './app/core/services/connection-state-machine.service'; @@ -23,7 +22,7 @@ import { AuthenticationInterceptor } from './app/core/interceptors/authenticatio import { HTTP_INTERCEPTORS, withInterceptorsFromDi, provideHttpClient } from '@angular/common/http'; import { OverlayContainer } from '@angular/cdk/overlay'; import { AppOverlayContainer } from './app/core/utils/app-overlay-container'; -import { OVERLAY_BACK_GUARD_PROVIDERS, RouterOverlayNavigationService } from './app/core/services/router-overlay-navigation.service'; +import { OVERLAY_DEFAULT_OPTIONS_PROVIDERS, RouterOverlayNavigationService } from './app/core/services/router-overlay-navigation.service'; if (environment.production) { enableProdMode(); @@ -42,17 +41,6 @@ bootstrapApplication(AppComponent, { useClass: AuthenticationInterceptor, multi: true, }, - // MatDialog App wide default config - { - provide: MAT_DIALOG_DEFAULT_OPTIONS, - useValue: { - hasBackdrop: true, - disableClose: false, - autoFocus: "first-tabbable", - delayFocusTrap: true, - backdropClass: "dialogBackdrop" - }, - }, { provide: MAT_FORM_FIELD_DEFAULT_OPTIONS, useValue: { @@ -104,9 +92,9 @@ bootstrapApplication(AppComponent, { const appNetInitSvc = inject(AppNetworkInitService); return appNetInitSvc.initNetworkServices(); }), - // Owns what navigation does to overlays: browser Back closes the topmost one, a real route - // change closes them all. The providers hand Material's own closeOnNavigation over to it. - ...OVERLAY_BACK_GUARD_PROVIDERS, + // Overlay defaults live with the service that depends on one of them: it owns what navigation + // does to overlays, so browser Back closes the topmost one and a real route change closes all. + ...OVERLAY_DEFAULT_OPTIONS_PROVIDERS, provideAppInitializer(() => { inject(RouterOverlayNavigationService); }),