From 3f9320ea50c422c0fa6e95ef348d21941d9c306d Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 1 Oct 2026 13:34:11 -0700 Subject: [PATCH] fix(close-watcher): only watch while a menu or overlay is open and keep the backbutton listener --- core/src/components/menu/menu.tsx | 3 +- .../menu/test/close-watcher/menu.e2e.ts | 71 ++++++ core/src/utils/config.ts | 5 +- core/src/utils/hardware-back-button.ts | 59 ++++- core/src/utils/menu-controller/index.ts | 3 +- core/src/utils/overlays.ts | 40 +++- .../utils/test/hardware-back-button.spec.ts | 219 +++++++++++++++--- 7 files changed, 355 insertions(+), 45 deletions(-) create mode 100644 core/src/components/menu/test/close-watcher/menu.e2e.ts diff --git a/core/src/components/menu/menu.tsx b/core/src/components/menu/menu.tsx index 4bf4564c40f..d9749f75b21 100644 --- a/core/src/components/menu/menu.tsx +++ b/core/src/components/menu/menu.tsx @@ -3,7 +3,7 @@ import { Build, Component, Element, Event, Host, Listen, Method, Prop, State, Wa import { getTimeGivenProgression } from '@utils/animation/cubic-bezier'; import { focusFirstDescendant, focusLastDescendant } from '@utils/focus-trap'; import { GESTURE_CONTROLLER } from '@utils/gesture'; -import { shouldUseCloseWatcher } from '@utils/hardware-back-button'; +import { shouldUseCloseWatcher, updateCloseWatcher } from '@utils/hardware-back-button'; import type { Attributes } from '@utils/helpers'; import { inheritAriaAttributes, assert, clamp, isEndSide as isEnd } from '@utils/helpers'; import { printIonError } from '@utils/logging'; @@ -726,6 +726,7 @@ export class Menu implements ComponentInterface, MenuI { // emit opened/closed events this._isOpen = isOpen; this.isAnimating = false; + updateCloseWatcher(); if (!this._isOpen) { this.blocker.unblock(); } diff --git a/core/src/components/menu/test/close-watcher/menu.e2e.ts b/core/src/components/menu/test/close-watcher/menu.e2e.ts new file mode 100644 index 00000000000..d77b89c5eeb --- /dev/null +++ b/core/src/components/menu/test/close-watcher/menu.e2e.ts @@ -0,0 +1,71 @@ +import { expect } from '@playwright/test'; +import { configs, test } from '@utils/test/playwright'; + +/** + * This behavior does not vary across modes/directions + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('menu: close watcher'), () => { + test('should close the menu on a close request and release the back button', async ({ page, skip }, testInfo) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/29648', + }); + skip.browser((browserName: string) => browserName !== 'chromium', 'Only Chromium supports the CloseWatcher API'); + + await page.setContent( + ` + + + + Menu Content + +
+ Main Content +
+ + `, + config + ); + + const menu = page.locator('ion-menu'); + const ionDidOpen = await page.spyOnEvent('ionDidOpen'); + const ionDidClose = await page.spyOnEvent('ionDidClose'); + + await menu.evaluate((el: HTMLIonMenuElement) => el.open()); + await ionDidOpen.next(); + expect(await page.evaluate(() => (window as any).liveCloseWatchers)).toBe(1); + + // Escape sends a close request to the active CloseWatcher + await page.keyboard.press('Escape'); + + await ionDidClose.next(); + await expect(menu).not.toHaveClass(/show-menu/); + expect(await page.evaluate(() => (window as any).liveCloseWatchers)).toBe(0); + }); + }); +}); diff --git a/core/src/utils/config.ts b/core/src/utils/config.ts index cca58752ee7..6a841a3fcf2 100644 --- a/core/src/utils/config.ts +++ b/core/src/utils/config.ts @@ -206,7 +206,10 @@ export interface IonicConfig { /** * @experimental * If `true`, the [CloseWatcher API](https://github.com/WICG/close-watcher) will be used to handle - * all Escape key and hardware back button presses to dismiss menus and overlays and to navigate. + * Escape key and Android back button presses that dismiss menus and overlays. It's only active + * while a menu or an overlay other than a toast is open, so back navigation works as usual + * otherwise. Hybrid apps still handle the hardware back button through the native `backbutton` + * event. * Note that the `hardwareBackButton` config option must also be `true`. */ experimentalCloseWatcher?: boolean; diff --git a/core/src/utils/hardware-back-button.ts b/core/src/utils/hardware-back-button.ts index 96da99979fb..46b4c19c5c0 100644 --- a/core/src/utils/hardware-back-button.ts +++ b/core/src/utils/hardware-back-button.ts @@ -48,6 +48,26 @@ export const blockHardwareBackButton = () => { document.addEventListener('backbutton', () => {}); }; +const closeWatcherConditions: (() => boolean)[] = []; +let syncCloseWatcher: (() => void) | undefined; + +/** + * Registers a check for whether the back button + * has something to close. The CloseWatcher is only + * active while one of these checks passes. + */ +export const addCloseWatcherCondition = (condition: () => boolean) => { + closeWatcherConditions.push(condition); +}; + +/** + * Call this whenever something the back button + * can close opens or closes. + */ +export const updateCloseWatcher = () => { + syncCloseWatcher?.(); +}; + export const startHardwareBackButton = () => { const doc = document; let busy = false; @@ -105,16 +125,32 @@ export const startHardwareBackButton = () => { }; /** - * If the CloseWatcher is defined then - * we don't want to also listen for the native - * backbutton event otherwise we may get duplicate - * events firing. + * Android WebView never sends the hardware back + * button to a CloseWatcher, so hybrid apps still + * need this. Browsers never fire backbutton. */ + doc.addEventListener('backbutton', backButtonCallback); + if (shouldUseCloseWatcher()) { let watcher: CloseWatcher | undefined; - const configureWatcher = () => { - watcher?.destroy(); + /** + * An active CloseWatcher stops the back button from + * navigating, so only keep one while there's something to close. + */ + syncCloseWatcher = () => { + const canClose = closeWatcherConditions.some((condition) => condition()); + + if (!canClose) { + watcher?.destroy(); + watcher = undefined; + return; + } + + if (watcher !== undefined) { + return; + } + watcher = new win!.CloseWatcher!(); /** @@ -122,17 +158,18 @@ export const startHardwareBackButton = () => { * the watcher gets destroyed. * As a result, we need to re-configure * the watcher so we can respond to other - * close requests. + * close requests, including after one that + * closed nothing, like a modal whose canDismiss + * returned false. */ watcher!.onclose = () => { + watcher = undefined; backButtonCallback(); - configureWatcher(); + syncCloseWatcher?.(); }; }; - configureWatcher(); - } else { - doc.addEventListener('backbutton', backButtonCallback); + syncCloseWatcher(); } }; diff --git a/core/src/utils/menu-controller/index.ts b/core/src/utils/menu-controller/index.ts index 5d170112340..6d90e958e87 100644 --- a/core/src/utils/menu-controller/index.ts +++ b/core/src/utils/menu-controller/index.ts @@ -1,6 +1,6 @@ import { doc } from '@utils/browser'; import type { BackButtonEvent } from '@utils/hardware-back-button'; -import { MENU_BACK_BUTTON_PRIORITY } from '@utils/hardware-back-button'; +import { MENU_BACK_BUTTON_PRIORITY, addCloseWatcherCondition } from '@utils/hardware-back-button'; import { printIonWarning } from '@utils/logging'; import type { MenuI, MenuControllerI } from '../../components/menu/menu-interface'; @@ -229,6 +229,7 @@ const createMenuController = (): MenuControllerI => { registerAnimation('push', menuPushAnimation); registerAnimation('overlay', menuOverlayAnimation); + addCloseWatcherCondition(() => _getOpenSync() !== undefined); doc?.addEventListener('ionBackButton', (ev: BackButtonEvent) => { const openMenu = _getOpenSync(); if (openMenu) { diff --git a/core/src/utils/overlays.ts b/core/src/utils/overlays.ts index 40bbb7b88fc..7a1dd014bd3 100644 --- a/core/src/utils/overlays.ts +++ b/core/src/utils/overlays.ts @@ -1,7 +1,7 @@ import { doc } from '@utils/browser'; import { focusFirstDescendant, focusLastDescendant, focusableQueryString } from '@utils/focus-trap'; import type { BackButtonEvent } from '@utils/hardware-back-button'; -import { shouldUseCloseWatcher } from '@utils/hardware-back-button'; +import { addCloseWatcherCondition, shouldUseCloseWatcher, updateCloseWatcher } from '@utils/hardware-back-button'; import { printIonError, printIonWarning } from '@utils/logging'; import { config } from '../global/config'; @@ -35,6 +35,12 @@ import { let lastOverlayIndex = 0; let lastId = 0; +/** + * Every overlay except toast, which doesn't trap + * focus or close on back. + */ +const NON_TOAST_OVERLAYS = 'ion-alert,ion-action-sheet,ion-loading,ion-modal,ion-popover'; + export const activeAnimations = new WeakMap(); type OverlayWithFocusTrapProps = HTMLIonOverlayElement & { @@ -186,7 +192,7 @@ const focusElementInOverlay = (hostToFocus: HTMLElement | null | undefined, over * Should NOT include: Toast */ const trapKeyboardFocus = (ev: Event, doc: Document) => { - const lastOverlay = getPresentedOverlay(doc, 'ion-alert,ion-action-sheet,ion-loading,ion-modal,ion-popover'); + const lastOverlay = getPresentedOverlay(doc, NON_TOAST_OVERLAYS); const target = ev.target as HTMLElement | null; /** @@ -383,6 +389,12 @@ const connectListeners = (doc: Document) => { true ); + /** + * Whether the overlay actually dismisses is checked at press time, + * since `backdropDismiss` can change while it's open. + */ + addCloseWatcherCondition(() => getPresentedOverlay(doc, NON_TOAST_OVERLAYS) !== undefined); + // handle back-button click doc.addEventListener('ionBackButton', (ev) => { const lastOverlay = getPresentedOverlay(doc); @@ -525,7 +537,8 @@ export const setRootAriaHidden = (hidden = false) => { * Cleans up root `aria-hidden` and `backdrop-no-scroll` when * an overlay is removed from the DOM without going through * the `dismiss()` flow (e.g., when a framework unmounts the - * overlay during a route change). + * overlay during a route change). Also releases the + * CloseWatcher if nothing is left to close. * * Should be called from an overlay's `disconnectedCallback` * when the overlay was still presented at the time of removal. @@ -535,6 +548,8 @@ export const cleanupRootFocusTrapAccessibility = () => { return; } + updateCloseWatcher(); + const remainingOverlays = getPresentedOverlays(document); const hasRemainingLocking = remainingOverlays.some((o) => locksAppRoot(o as OverlayWithFocusTrapProps)); @@ -559,7 +574,9 @@ const applyRootLock = (el: OverlayWithFocusTrapProps) => { /** * Re-applies the root lock that `cleanupRootFocusTrapAccessibility()` released. - * Call from `connectedCallback` when the overlay is still presented. + * Call from `connectedCallback` when the overlay is still presented. Also + * re-arms the CloseWatcher, which every overlay needs, so that happens + * before the `locksAppRoot` check. * * A synchronous move keeps the overlay connected, so the lock survives. A * detach with a re-insert in a later task releases it, which is the shape a @@ -570,6 +587,8 @@ export const restoreRootFocusTrapAccessibility = (overlayEl: HTMLIonOverlayEleme return; } + updateCloseWatcher(); + const el = overlayEl as OverlayWithFocusTrapProps; if (!locksAppRoot(el)) { return; @@ -828,6 +847,12 @@ export const dismiss = async ( overlay.el.remove(); + /** + * Release the back button if nothing + * else is left to close. + */ + updateCloseWatcher(); + return true; }; @@ -844,6 +869,13 @@ const overlayAnimation = async ( // Make overlay visible in case it's hidden baseEl.classList.remove('overlay-hidden'); + /** + * Removing `overlay-hidden` makes the overlay count + * as presented, so back can dismiss it while it + * animates in. + */ + updateCloseWatcher(); + const aniRoot = overlay.el; const animation = animationBuilder(aniRoot, opts); diff --git a/core/src/utils/test/hardware-back-button.spec.ts b/core/src/utils/test/hardware-back-button.spec.ts index 87f27d407cc..b195996c20d 100644 --- a/core/src/utils/test/hardware-back-button.spec.ts +++ b/core/src/utils/test/hardware-back-button.spec.ts @@ -1,7 +1,8 @@ +import { newSpecPage } from '@stencil/core/testing'; + import type { BackButtonEvent } from '../../../src/interface'; import { startHardwareBackButton } from '../hardware-back-button'; import { config } from '../../global/config'; -import { win } from '@utils/browser'; describe('Hardware Back Button', () => { beforeEach(() => startHardwareBackButton()); @@ -58,52 +59,216 @@ describe('Hardware Back Button', () => { describe('Experimental Close Watcher', () => { test('should not use the Close Watcher API when available', () => { - const mockAPI = mockCloseWatcher(); + const closeWatchers = mockCloseWatcher(); config.reset({ experimentalCloseWatcher: false }); startHardwareBackButton(); - expect(mockAPI.mock.calls).toHaveLength(0); + expect(closeWatchers.created).toBe(0); }); - test('should use the Close Watcher API when available', () => { - const mockAPI = mockCloseWatcher(); - config.reset({ experimentalCloseWatcher: true }); + test('should still handle the native back button event', async () => { + await newCloseWatcherPage(``); - startHardwareBackButton(); + const cbSpy = jest.fn(); + document.addEventListener('ionBackButton', (ev) => { + (ev as BackButtonEvent).detail.register(0, cbSpy); + }); + + dispatchBackButtonEvent(); - expect(mockAPI.mock.calls).toHaveLength(1); + expect(cbSpy).toHaveBeenCalled(); }); - test('Close Watcher should dispatch ionBackButton events', () => { - const mockAPI = mockCloseWatcher(); - config.reset({ experimentalCloseWatcher: true }); + // Fixes https://github.com/ionic-team/ionic-framework/issues/29648 + test('should not intercept the back button while nothing is open', async () => { + const { closeWatchers } = await newCloseWatcherPage(``); - startHardwareBackButton(); + expect(closeWatchers.active()).toBeUndefined(); + }); - const cbSpy = jest.fn(); - document.addEventListener('ionBackButton', cbSpy); + test('should not intercept the back button while only a toast is shown', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(``); - // Call onclose on Ionic's instance of CloseWatcher - mockAPI.getMockImplementation()!().onclose(); + await page.body.querySelector('ion-toast')!.present(); - expect(cbSpy).toHaveBeenCalled(); + expect(closeWatchers.active()).toBeUndefined(); + }); + + test('should dismiss a presented modal when the back button is pressed', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(``); + + const modal = page.body.querySelector('ion-modal')!; + await modal.present(); + + expect(closeWatchers.active()).toBeDefined(); + + const dismissed = modal.onDidDismiss(); + closeWatchers.requestClose(); + + await expect(dismissed).resolves.toEqual(expect.objectContaining({ role: 'backdrop' })); + }); + + // Fixes https://github.com/ionic-team/ionic-framework/issues/29648 + test('should stop intercepting the back button once the modal is dismissed', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(``); + + const modal = page.body.querySelector('ion-modal')!; + await modal.present(); + await modal.dismiss(); + + expect(closeWatchers.active()).toBeUndefined(); + }); + + test('should stop intercepting the back button when a presented modal is removed without dismissing', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(``); + + const modal = page.body.querySelector('ion-modal')!; + await modal.present(); + modal.remove(); + + expect(closeWatchers.active()).toBeUndefined(); + }); + + test('should keep intercepting the back button when a presented modal is moved', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(`
`); + + const modal = page.body.querySelector('ion-modal')!; + await modal.present(); + page.body.querySelector('#target')!.appendChild(modal); + + expect(closeWatchers.active()).toBeDefined(); + }); + + test('should keep intercepting the back button while a modal is still open underneath', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(` + + + `); + + const bottom = page.body.querySelector('#bottom')!; + const top = page.body.querySelector('#top')!; + await bottom.present(); + await top.present(); + + const topDismissed = top.onDidDismiss(); + closeWatchers.requestClose(); + await topDismissed; + + const bottomDismissed = bottom.onDidDismiss(); + closeWatchers.requestClose(); + + await expect(bottomDismissed).resolves.toEqual(expect.objectContaining({ role: 'backdrop' })); + expect(closeWatchers.active()).toBeUndefined(); + }); + + test('should keep intercepting the back button when the modal refuses to dismiss', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(``); + + const modal = page.body.querySelector('ion-modal')!; + let dismissAttempts = 0; + modal.canDismiss = () => { + dismissAttempts++; + return Promise.resolve(false); + }; + await modal.present(); + + closeWatchers.requestClose(); + await page.waitForChanges(); + closeWatchers.requestClose(); + await page.waitForChanges(); + + expect(dismissAttempts).toBe(2); + }); + + test('should dismiss a modal that allows backdrop dismissal only after it was presented', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(``); + + const modal = page.body.querySelector('ion-modal')!; + modal.backdropDismiss = false; + await modal.present(); + modal.backdropDismiss = true; + + const dismissed = modal.onDidDismiss(); + closeWatchers.requestClose(); + + await expect(dismissed).resolves.toEqual(expect.objectContaining({ role: 'backdrop' })); + }); + + test('should keep intercepting the back button while a toast is shown over a modal', async () => { + const { page, closeWatchers } = await newCloseWatcherPage(` + + + `); + + await page.body.querySelector('ion-modal')!.present(); + await page.body.querySelector('ion-toast')!.present(); + + expect(closeWatchers.active()).toBeDefined(); }); }); +/** + * Modules are reset first because overlays attach their back button + * listener once per module load, and every spec page gets a new document. + */ +const newCloseWatcherPage = async (html: string) => { + jest.resetModules(); + const { Modal } = await import('../../components/modal/modal'); + const { Toast } = await import('../../components/toast/toast'); + const { startHardwareBackButton } = await import('../hardware-back-button'); + const { config } = await import('../../global/config'); + + const page = await newSpecPage({ components: [Modal, Toast], html }); + + // Install the mock after newSpecPage, which resets the window + const closeWatchers = mockCloseWatcher(); + config.reset({ experimentalCloseWatcher: true }); + startHardwareBackButton(); + + return { page, closeWatchers }; +}; + +/** + * Like the browser, only the newest watcher that hasn't + * been destroyed gets close requests. + */ const mockCloseWatcher = () => { - const mockCloseWatcher = jest.fn(); - mockCloseWatcher.mockReturnValue({ - requestClose: () => null, - close: () => null, - destroy: () => null, - oncancel: () => null, - onclose: () => null, - }); - (win as any).CloseWatcher = mockCloseWatcher; + const watchers: { destroyed: boolean; onclose: (() => void) | null; destroy: () => void }[] = []; + + (window as any).CloseWatcher = class { + destroyed = false; + onclose: (() => void) | null = null; + + constructor() { + watchers.push(this); + } + + destroy() { + this.destroyed = true; + } + }; + + const active = () => watchers.filter((w) => !w.destroyed).slice(-1)[0]; - return mockCloseWatcher; + return { + get created() { + return watchers.length; + }, + active, + /** + * Presses back. Like the browser, this destroys + * the watcher before firing `close`. + */ + requestClose() { + const watcher = active(); + if (watcher) { + watcher.destroyed = true; + watcher.onclose?.(); + } + }, + }; }; const dispatchBackButtonEvent = () => {