From 584e9d3be220343451c2d4b9bf90658ecd530de1 Mon Sep 17 00:00:00 2001 From: Shawn Taylor Date: Thu, 31 Aug 2023 12:30:43 -0400 Subject: [PATCH] fix(overlays): prevent overlays from getting stuck open (#28069) Issue number: resolves #27200 --------- ## What is the current behavior? A bug occurs when you click twice quickly to open an overlay with a small timeout. In some cases, the overlay will present, dismiss, present, then not dismiss the second time, getting stuck open. You can reproduce manually this by grabbing the test HTML included in this PR and putting it in a branch that doesn't include a fix. ## What is the new behavior? - When an overlay with a short timeout is triggered twice quickly, it will open-close-open-close. - The behavior is the same for all overlay components ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information Relevant links: * https://github.com/ionic-team/ionic-framework/issues/27200 * https://ionic-cloud.atlassian.net/browse/FW-4374 * https://ionic-cloud.atlassian.net/browse/FW-4053 I'm not sure how to write an automated test for this bug due to the short timeout required. You can manually test the fix in [this Stackblitz](https://stackblitz.com/edit/g1kjci?file=package.json) by changing the Ionic version between 7.3.1 and 7.3.2-dev.11693262117.17edbf6d --------- Co-authored-by: Liam DeBeasi --- .../components/action-sheet/action-sheet.tsx | 28 ++++------ core/src/components/alert/alert.tsx | 28 ++++------ core/src/components/loading/loading.tsx | 29 ++++------- core/src/components/modal/modal.tsx | 52 +++++++------------ core/src/components/picker/picker.tsx | 30 ++++------- core/src/components/popover/popover.tsx | 46 +++++----------- .../router-outlet/router-outlet.tsx | 1 + core/src/components/toast/toast.tsx | 35 ++++--------- core/src/utils/lock-controller.ts | 35 +++++++++++++ 9 files changed, 121 insertions(+), 163 deletions(-) create mode 100644 core/src/utils/lock-controller.ts diff --git a/core/src/components/action-sheet/action-sheet.tsx b/core/src/components/action-sheet/action-sheet.tsx index 01fcc34752..813e38c7f9 100644 --- a/core/src/components/action-sheet/action-sheet.tsx +++ b/core/src/components/action-sheet/action-sheet.tsx @@ -3,6 +3,7 @@ import { Watch, Component, Element, Event, Host, Method, Prop, h, readTask } fro import type { Gesture } from '@utils/gesture'; import { createButtonActiveGesture } from '@utils/gesture/button-active'; import { raf } from '@utils/helpers'; +import { createLockController } from '@utils/lock-controller'; import { BACKDROP, createDelegateController, @@ -40,8 +41,8 @@ import { mdLeaveAnimation } from './animations/md.leave'; }) export class ActionSheet implements ComponentInterface, OverlayInterface { private readonly delegateController = createDelegateController(this); + private readonly lockController = createLockController(); private readonly triggerController = createTriggerController(); - private currentTransition?: Promise; private wrapperEl?: HTMLElement; private groupEl?: HTMLElement; private gesture?: Gesture; @@ -198,25 +199,13 @@ export class ActionSheet implements ComponentInterface, OverlayInterface { */ @Method() async present(): Promise { - /** - * When using an inline action sheet - * and dismissing a action sheet it is possible to - * quickly present the action sheet while it is - * dismissing. We need to await any current - * transition to allow the dismiss to finish - * before presenting again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; - } + const unlock = await this.lockController.lock(); await this.delegateController.attachViewToDom(); - this.currentTransition = present(this, 'actionSheetEnter', iosEnterAnimation, mdEnterAnimation); + await present(this, 'actionSheetEnter', iosEnterAnimation, mdEnterAnimation); - await this.currentTransition; - - this.currentTransition = undefined; + unlock(); } /** @@ -230,13 +219,16 @@ export class ActionSheet implements ComponentInterface, OverlayInterface { */ @Method() async dismiss(data?: any, role?: string): Promise { - this.currentTransition = dismiss(this, data, role, 'actionSheetLeave', iosLeaveAnimation, mdLeaveAnimation); - const dismissed = await this.currentTransition; + const unlock = await this.lockController.lock(); + + const dismissed = await dismiss(this, data, role, 'actionSheetLeave', iosLeaveAnimation, mdLeaveAnimation); if (dismissed) { this.delegateController.removeViewFromDom(); } + unlock(); + return dismissed; } diff --git a/core/src/components/alert/alert.tsx b/core/src/components/alert/alert.tsx index 6e5c48d605..b8b7de6b8f 100644 --- a/core/src/components/alert/alert.tsx +++ b/core/src/components/alert/alert.tsx @@ -4,6 +4,7 @@ import { ENABLE_HTML_CONTENT_DEFAULT } from '@utils/config'; import type { Gesture } from '@utils/gesture'; import { createButtonActiveGesture } from '@utils/gesture/button-active'; import { raf } from '@utils/helpers'; +import { createLockController } from '@utils/lock-controller'; import { createDelegateController, createTriggerController, @@ -46,6 +47,7 @@ import { mdLeaveAnimation } from './animations/md.leave'; }) export class Alert implements ComponentInterface, OverlayInterface { private readonly delegateController = createDelegateController(this); + private readonly lockController = createLockController(); private readonly triggerController = createTriggerController(); private customHTMLEnabled = config.get('innerHTMLTemplatesEnabled', ENABLE_HTML_CONTENT_DEFAULT); private activeId?: string; @@ -54,7 +56,6 @@ export class Alert implements ComponentInterface, OverlayInterface { private processedButtons: AlertButton[] = []; private wrapperEl?: HTMLElement; private gesture?: Gesture; - private currentTransition?: Promise; presented = false; lastFocus?: HTMLElement; @@ -373,23 +374,13 @@ export class Alert implements ComponentInterface, OverlayInterface { */ @Method() async present(): Promise { - /** - * When using an inline alert - * and dismissing an alert it is possible to - * quickly present the alert while it is - * dismissing. We need to await any current - * transition to allow the dismiss to finish - * before presenting again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; - } + const unlock = await this.lockController.lock(); await this.delegateController.attachViewToDom(); - this.currentTransition = present(this, 'alertEnter', iosEnterAnimation, mdEnterAnimation); - await this.currentTransition; - this.currentTransition = undefined; + await present(this, 'alertEnter', iosEnterAnimation, mdEnterAnimation); + + unlock(); } /** @@ -403,13 +394,16 @@ export class Alert implements ComponentInterface, OverlayInterface { */ @Method() async dismiss(data?: any, role?: string): Promise { - this.currentTransition = dismiss(this, data, role, 'alertLeave', iosLeaveAnimation, mdLeaveAnimation); - const dismissed = await this.currentTransition; + const unlock = await this.lockController.lock(); + + const dismissed = await dismiss(this, data, role, 'alertLeave', iosLeaveAnimation, mdLeaveAnimation); if (dismissed) { this.delegateController.removeViewFromDom(); } + unlock(); + return dismissed; } diff --git a/core/src/components/loading/loading.tsx b/core/src/components/loading/loading.tsx index 46fd93da0c..0892b082d1 100644 --- a/core/src/components/loading/loading.tsx +++ b/core/src/components/loading/loading.tsx @@ -2,6 +2,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { Watch, Component, Element, Event, Host, Method, Prop, h } from '@stencil/core'; import { ENABLE_HTML_CONTENT_DEFAULT } from '@utils/config'; import { raf } from '@utils/helpers'; +import { createLockController } from '@utils/lock-controller'; import { BACKDROP, dismiss, @@ -42,10 +43,10 @@ import { mdLeaveAnimation } from './animations/md.leave'; }) export class Loading implements ComponentInterface, OverlayInterface { private readonly delegateController = createDelegateController(this); + private readonly lockController = createLockController(); private readonly triggerController = createTriggerController(); private customHTMLEnabled = config.get('innerHTMLTemplatesEnabled', ENABLE_HTML_CONTENT_DEFAULT); private durationTimeout?: ReturnType; - private currentTransition?: Promise; presented = false; lastFocus?: HTMLElement; @@ -235,29 +236,17 @@ export class Loading implements ComponentInterface, OverlayInterface { */ @Method() async present(): Promise { - /** - * When using an inline loading indicator - * and dismissing a loading indicator it is possible to - * quickly present the loading indicator while it is - * dismissing. We need to await any current - * transition to allow the dismiss to finish - * before presenting again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; - } + const unlock = await this.lockController.lock(); await this.delegateController.attachViewToDom(); - this.currentTransition = present(this, 'loadingEnter', iosEnterAnimation, mdEnterAnimation); - - await this.currentTransition; + await present(this, 'loadingEnter', iosEnterAnimation, mdEnterAnimation); if (this.duration > 0) { this.durationTimeout = setTimeout(() => this.dismiss(), this.duration + 10); } - this.currentTransition = undefined; + unlock(); } /** @@ -271,17 +260,19 @@ export class Loading implements ComponentInterface, OverlayInterface { */ @Method() async dismiss(data?: any, role?: string): Promise { + const unlock = await this.lockController.lock(); + if (this.durationTimeout) { clearTimeout(this.durationTimeout); } - this.currentTransition = dismiss(this, data, role, 'loadingLeave', iosLeaveAnimation, mdLeaveAnimation); - - const dismissed = await this.currentTransition; + const dismissed = await dismiss(this, data, role, 'loadingLeave', iosLeaveAnimation, mdLeaveAnimation); if (dismissed) { this.delegateController.removeViewFromDom(); } + unlock(); + return dismissed; } diff --git a/core/src/components/modal/modal.tsx b/core/src/components/modal/modal.tsx index 7d877fe39a..0b4a0d3799 100644 --- a/core/src/components/modal/modal.tsx +++ b/core/src/components/modal/modal.tsx @@ -4,6 +4,7 @@ import { findIonContent, printIonContentErrorMsg } from '@utils/content'; import { CoreDelegate, attachComponent, detachComponent } from '@utils/framework-delegate'; import { raf, inheritAttributes, hasLazyBuild } from '@utils/helpers'; import type { Attributes } from '@utils/helpers'; +import { createLockController } from '@utils/lock-controller'; import { printIonWarning } from '@utils/logging'; import { Style as StatusBarStyle, StatusBar } from '@utils/native/status-bar'; import { @@ -64,10 +65,10 @@ import { setCardStatusBarDark, setCardStatusBarDefault } from './utils'; shadow: true, }) export class Modal implements ComponentInterface, OverlayInterface { + private readonly lockController = createLockController(); private readonly triggerController = createTriggerController(); private gesture?: Gesture; private coreDelegate: FrameworkDelegate = CoreDelegate(); - private currentTransition?: Promise; private sheetTransition?: Promise; private isSheetModal = false; private currentBreakpoint?: number; @@ -422,24 +423,15 @@ export class Modal implements ComponentInterface, OverlayInterface { */ @Method() async present(): Promise { + const unlock = await this.lockController.lock(); + if (this.presented) { + unlock(); return; } const { presentingElement, el } = this; - /** - * When using an inline modal - * and dismissing a modal it is possible to - * quickly present the modal while it is - * dismissing. We need to await any current - * transition to allow the dismiss to finish - * before presenting again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; - } - /** * If the modal is presented multiple times (inline modals), we * need to reset the current breakpoint to the initial breakpoint. @@ -481,7 +473,7 @@ export class Modal implements ComponentInterface, OverlayInterface { writeTask(() => this.el.classList.add('show-modal')); - this.currentTransition = present(this, 'modalEnter', iosEnterAnimation, mdEnterAnimation, { + await present(this, 'modalEnter', iosEnterAnimation, mdEnterAnimation, { presentingEl: presentingElement, currentBreakpoint: this.initialBreakpoint, backdropBreakpoint: this.backdropBreakpoint, @@ -532,15 +524,13 @@ export class Modal implements ComponentInterface, OverlayInterface { setCardStatusBarDark(); } - await this.currentTransition; - if (this.isSheetModal) { this.initSheetGesture(); } else if (hasCardModal) { this.initSwipeToClose(); } - this.currentTransition = undefined; + unlock(); } private initSwipeToClose() { @@ -656,12 +646,20 @@ export class Modal implements ComponentInterface, OverlayInterface { return false; } + /** + * Because the canDismiss check below is async, + * we need to claim a lock before the check happens, + * in case the dismiss transition does run. + */ + const unlock = await this.lockController.lock(); + /** * If a canDismiss handler is responsible * for calling the dismiss method, we should * not run the canDismiss check again. */ if (role !== 'handler' && !(await this.checkCanDismiss(data, role))) { + unlock(); return false; } @@ -683,21 +681,9 @@ export class Modal implements ComponentInterface, OverlayInterface { this.keyboardOpenCallback = undefined; } - /** - * When using an inline modal - * and presenting a modal it is possible to - * quickly dismiss the modal while it is - * presenting. We need to await any current - * transition to allow the present to finish - * before dismissing again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; - } - const enteringAnimation = activeAnimations.get(this) || []; - this.currentTransition = dismiss( + const dismissed = await dismiss( this, data, role, @@ -711,8 +697,6 @@ export class Modal implements ComponentInterface, OverlayInterface { } ); - const dismissed = await this.currentTransition; - if (dismissed) { const { delegate } = this.getDelegate(); await detachComponent(delegate, this.usersElement); @@ -729,8 +713,10 @@ export class Modal implements ComponentInterface, OverlayInterface { enteringAnimation.forEach((ani) => ani.destroy()); } this.currentBreakpoint = undefined; - this.currentTransition = undefined; this.animation = undefined; + + unlock(); + return dismissed; } diff --git a/core/src/components/picker/picker.tsx b/core/src/components/picker/picker.tsx index 9f1f587086..19d03d2f8b 100644 --- a/core/src/components/picker/picker.tsx +++ b/core/src/components/picker/picker.tsx @@ -1,6 +1,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { Component, Element, Event, Host, Method, Prop, State, Watch, h } from '@stencil/core'; import { raf } from '@utils/helpers'; +import { createLockController } from '@utils/lock-controller'; import { createDelegateController, createTriggerController, @@ -38,10 +39,10 @@ import type { PickerButton, PickerColumn } from './picker-interface'; }) export class Picker implements ComponentInterface, OverlayInterface { private readonly delegateController = createDelegateController(this); + private readonly lockController = createLockController(); private readonly triggerController = createTriggerController(); private durationTimeout?: ReturnType; - private currentTransition?: Promise; lastFocus?: HTMLElement; @Element() el!: HTMLIonPickerElement; @@ -215,29 +216,17 @@ export class Picker implements ComponentInterface, OverlayInterface { */ @Method() async present(): Promise { - /** - * When using an inline picker - * and dismissing an picker it is possible to - * quickly present the picker while it is - * dismissing. We need to await any current - * transition to allow the dismiss to finish - * before presenting again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; - } + const unlock = await this.lockController.lock(); await this.delegateController.attachViewToDom(); - this.currentTransition = present(this, 'pickerEnter', iosEnterAnimation, iosEnterAnimation, undefined); - - await this.currentTransition; - - this.currentTransition = undefined; + await present(this, 'pickerEnter', iosEnterAnimation, iosEnterAnimation, undefined); if (this.duration > 0) { this.durationTimeout = setTimeout(() => this.dismiss(), this.duration); } + + unlock(); } /** @@ -251,16 +240,19 @@ export class Picker implements ComponentInterface, OverlayInterface { */ @Method() async dismiss(data?: any, role?: string): Promise { + const unlock = await this.lockController.lock(); + if (this.durationTimeout) { clearTimeout(this.durationTimeout); } - this.currentTransition = dismiss(this, data, role, 'pickerLeave', iosLeaveAnimation, iosLeaveAnimation); - const dismissed = await this.currentTransition; + const dismissed = await dismiss(this, data, role, 'pickerLeave', iosLeaveAnimation, iosLeaveAnimation); if (dismissed) { this.delegateController.removeViewFromDom(); } + unlock(); + return dismissed; } diff --git a/core/src/components/popover/popover.tsx b/core/src/components/popover/popover.tsx index 476b150d74..75e16ad70d 100644 --- a/core/src/components/popover/popover.tsx +++ b/core/src/components/popover/popover.tsx @@ -2,6 +2,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { Component, Element, Event, Host, Method, Prop, State, Watch, h } from '@stencil/core'; import { CoreDelegate, attachComponent, detachComponent } from '@utils/framework-delegate'; import { addEventListener, raf, hasLazyBuild } from '@utils/helpers'; +import { createLockController } from '@utils/lock-controller'; import { printIonWarning } from '@utils/logging'; import { BACKDROP, @@ -58,7 +59,7 @@ export class Popover implements ComponentInterface, PopoverInterface { private triggerEl?: HTMLElement | null; private parentPopover: HTMLIonPopoverElement | null = null; private coreDelegate: FrameworkDelegate = CoreDelegate(); - private currentTransition?: Promise; + private readonly lockController = createLockController(); private destroyTriggerInteraction?: () => void; private destroyKeyboardInteraction?: () => void; private destroyDismissInteraction?: () => void; @@ -430,20 +431,11 @@ export class Popover implements ComponentInterface, PopoverInterface { */ @Method() async present(event?: MouseEvent | TouchEvent | PointerEvent | CustomEvent): Promise { - if (this.presented) { - return; - } + const unlock = await this.lockController.lock(); - /** - * When using an inline popover - * and dismissing a popover it is possible to - * quickly present the popover while it is - * dismissing. We need to await any current - * transition to allow the dismiss to finish - * before presenting again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; + if (this.presented) { + unlock(); + return; } const { el } = this; @@ -493,7 +485,7 @@ export class Popover implements ComponentInterface, PopoverInterface { await waitForMount(); } - this.currentTransition = present(this, 'popoverEnter', iosEnterAnimation, mdEnterAnimation, { + await present(this, 'popoverEnter', iosEnterAnimation, mdEnterAnimation, { event: event || this.event, size: this.size, trigger: this.triggerEl, @@ -502,10 +494,6 @@ export class Popover implements ComponentInterface, PopoverInterface { align: this.alignment, }); - await this.currentTransition; - - this.currentTransition = undefined; - /** * If popover is nested and was * presented using the "Right" arrow key, @@ -515,6 +503,8 @@ export class Popover implements ComponentInterface, PopoverInterface { if (this.focusDescendantOnPresent) { focusFirstDescendant(this.el, this.el); } + + unlock(); } /** @@ -527,24 +517,14 @@ export class Popover implements ComponentInterface, PopoverInterface { */ @Method() async dismiss(data?: any, role?: string, dismissParentPopover = true): Promise { - /** - * When using an inline popover - * and presenting a popover it is possible to - * quickly dismiss the popover while it is - * presenting. We need to await any current - * transition to allow the present to finish - * before dismissing again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; - } + const unlock = await this.lockController.lock(); const { destroyKeyboardInteraction, destroyDismissInteraction } = this; if (dismissParentPopover && this.parentPopover) { this.parentPopover.dismiss(data, role, dismissParentPopover); } - this.currentTransition = dismiss( + const shouldDismiss = await dismiss( this, data, role, @@ -553,7 +533,7 @@ export class Popover implements ComponentInterface, PopoverInterface { mdLeaveAnimation, this.event ); - const shouldDismiss = await this.currentTransition; + if (shouldDismiss) { if (destroyKeyboardInteraction) { destroyKeyboardInteraction(); @@ -573,7 +553,7 @@ export class Popover implements ComponentInterface, PopoverInterface { await detachComponent(delegate, this.usersElement); } - this.currentTransition = undefined; + unlock(); return shouldDismiss; } diff --git a/core/src/components/router-outlet/router-outlet.tsx b/core/src/components/router-outlet/router-outlet.tsx index 177c0b8b68..dbdd8763ed 100644 --- a/core/src/components/router-outlet/router-outlet.tsx +++ b/core/src/components/router-outlet/router-outlet.tsx @@ -285,6 +285,7 @@ export class RouterOutlet implements ComponentInterface, NavOutlet { return true; } + // TODO: FW-5048 - Remove this code in favor of using lock controller from utils private async lock() { const p = this.waitPromise; let resolve!: () => void; diff --git a/core/src/components/toast/toast.tsx b/core/src/components/toast/toast.tsx index fcc234ae85..3137e73d54 100644 --- a/core/src/components/toast/toast.tsx +++ b/core/src/components/toast/toast.tsx @@ -2,6 +2,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { State, Watch, Component, Element, Event, h, Host, Method, Prop } from '@stencil/core'; import { ENABLE_HTML_CONTENT_DEFAULT } from '@utils/config'; import { raf } from '@utils/helpers'; +import { createLockController } from '@utils/lock-controller'; import { printIonWarning } from '@utils/logging'; import { createDelegateController, @@ -51,8 +52,8 @@ import type { ToastButton, ToastPosition, ToastLayout } from './toast-interface' }) export class Toast implements ComponentInterface, OverlayInterface { private readonly delegateController = createDelegateController(this); + private readonly lockController = createLockController(); private readonly triggerController = createTriggerController(); - private currentTransition?: Promise; private customHTMLEnabled = config.get('innerHTMLTemplatesEnabled', ENABLE_HTML_CONTENT_DEFAULT); private durationTimeout?: ReturnType; @@ -270,28 +271,11 @@ export class Toast implements ComponentInterface, OverlayInterface { */ @Method() async present(): Promise { - /** - * When using an inline toast - * and dismissing a toast it is possible to - * quickly present the toast while it is - * dismissing. We need to await any current - * transition to allow the dismiss to finish - * before presenting again. - */ - if (this.currentTransition !== undefined) { - await this.currentTransition; - } + const unlock = await this.lockController.lock(); await this.delegateController.attachViewToDom(); - this.currentTransition = present( - this, - 'toastEnter', - iosEnterAnimation, - mdEnterAnimation, - this.position - ); - await this.currentTransition; + await present(this, 'toastEnter', iosEnterAnimation, mdEnterAnimation, this.position); /** * Content is revealed to screen readers after @@ -300,11 +284,11 @@ export class Toast implements ComponentInterface, OverlayInterface { */ this.revealContentToScreenReader = true; - this.currentTransition = undefined; - if (this.duration > 0) { this.durationTimeout = setTimeout(() => this.dismiss(undefined, 'timeout'), this.duration); } + + unlock(); } /** @@ -318,11 +302,13 @@ export class Toast implements ComponentInterface, OverlayInterface { */ @Method() async dismiss(data?: any, role?: string): Promise { + const unlock = await this.lockController.lock(); + if (this.durationTimeout) { clearTimeout(this.durationTimeout); } - this.currentTransition = dismiss( + const dismissed = await dismiss( this, data, role, @@ -331,13 +317,14 @@ export class Toast implements ComponentInterface, OverlayInterface { mdLeaveAnimation, this.position ); - const dismissed = await this.currentTransition; if (dismissed) { this.delegateController.removeViewFromDom(); this.revealContentToScreenReader = false; } + unlock(); + return dismissed; } diff --git a/core/src/utils/lock-controller.ts b/core/src/utils/lock-controller.ts new file mode 100644 index 0000000000..a8dec006b3 --- /dev/null +++ b/core/src/utils/lock-controller.ts @@ -0,0 +1,35 @@ +/** + * Creates a lock controller. + * + * Claiming a lock means that nothing else can acquire the lock until it is released. + * This can momentarily prevent execution of code that needs to wait for the earlier code to finish. + * For example, this can be used to prevent multiple transitions from occurring at the same time. + */ +export const createLockController = () => { + let waitPromise: Promise; + + /** + * When lock() is called, the lock is claimed. + * Once a lock has been claimed, it cannot be claimed again until it is released. + * When this function gets resolved, the lock is released, allowing it to be claimed again. + * + * @example ```tsx + * const unlock = await this.lockController.lock(); + * // do other stuff + * unlock(); + * ``` + */ + const lock = async () => { + const p = waitPromise; + let resolve!: () => void; + waitPromise = new Promise((r) => (resolve = r)); + if (p !== undefined) { + await p; + } + return resolve; + }; + + return { + lock, + }; +};