Browser Back closes the top overlay instead of navigating

.bumpversion.cfg

@@ -1,5 +1,5 @@
 [bumpversion]
-current_version = 1.5.0
+current_version = 1.4.1
 commit = True
 tag = False
 

VERSION

@@ -1 +1 @@
-1.5.0
+1.4.1

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@halos-org/skip",
-  "version": "1.5.0",
+  "version": "1.4.1",
   "publishConfig": {
     "access": "public"
   },

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<ElementRef<HTMLElement>>('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);
         });

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: () =>

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 {

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<typeof vi.fn>;
 
   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<DialogService['openWidgetOptions']>[0]);
+
+    const config = open.mock.calls[0][1] as MatDialogConfig;
+    expect(config.closeOnNavigation).toBeUndefined();
+  });
 });

src/app/core/services/dialog.service.ts

@@ -108,7 +108,6 @@ export class DialogService {
         data: data.config,
         minWidth: "50vw",
         maxWidth: "90vw",
-        closeOnNavigation: true,
       }
     );
   }

src/app/core/services/router-overlay-navigation.service.spec.ts

@@ -0,0 +1,191 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { Subject } from 'rxjs';
+import { TestBed } from '@angular/core/testing';
+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 { EmbedModeService } from './embed-mode.service';
+import { OVERLAY_DEFAULT_OPTIONS_PROVIDERS, RouterOverlayNavigationService } from './router-overlay-navigation.service';
+
+interface DialogStub { afterClosed: () => Subject<unknown>; close: () => void }
+
+describe('RouterOverlayNavigationService', () => {
+  const routerEvents = new Subject<unknown>();
+  const dialogOpened = new Subject<MatDialogRef<unknown>>();
+  let openDialogs: DialogStub[];
+  let dialogMock: { afterOpened: Subject<MatDialogRef<unknown>>; openDialogs: DialogStub[]; closeAll: () => void };
+  let bottomSheetMock: { dismiss: ReturnType<typeof vi.fn<() => void>> };
+  let embed: boolean;
+
+  const openDialog = (): { closed: Subject<unknown>; close: ReturnType<typeof vi.fn> } => {
+    const closed = new Subject<unknown>();
+    const close = vi.fn(() => closed.next(undefined));
+    const stub: DialogStub = { afterClosed: () => closed, close };
+    openDialogs.push(stub);
+    dialogOpened.next(stub as unknown as MatDialogRef<unknown>);
+    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_DEFAULT_OPTIONS_PROVIDERS.map(p => p as { provide: unknown; useValue: Record<string, unknown> });
+    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('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<string, unknown> }).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)', () => {
+    const service = create();
+    const dismissed = new Subject<unknown>();
+    service.guardOverlay(() => bottomSheetMock.dismiss(), dismissed);
+
+    pop();
+
+    expect(bottomSheetMock.dismiss).toHaveBeenCalledTimes(1);
+  });
+});