Skip to content

Commit

Permalink
fix(overlays): prevent overlays from getting stuck open (#28069)
Browse files Browse the repository at this point in the history
Issue number: resolves #27200 

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->
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?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- 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

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Relevant links:
* #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 <[email protected]>
  • Loading branch information
mapsandapps and liamdebeasi authored Aug 31, 2023
1 parent e1fdbb3 commit 584e9d3
Show file tree
Hide file tree
Showing 9 changed files with 120 additions and 162 deletions.
28 changes: 10 additions & 18 deletions core/src/components/action-sheet/action-sheet.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<any>;
private wrapperEl?: HTMLElement;
private groupEl?: HTMLElement;
private gesture?: Gesture;
Expand Down Expand Up @@ -198,25 +199,13 @@ export class ActionSheet implements ComponentInterface, OverlayInterface {
*/
@Method()
async present(): Promise<void> {
/**
* 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();
}

/**
Expand All @@ -230,13 +219,16 @@ export class ActionSheet implements ComponentInterface, OverlayInterface {
*/
@Method()
async dismiss(data?: any, role?: string): Promise<boolean> {
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;
}

Expand Down
28 changes: 11 additions & 17 deletions core/src/components/alert/alert.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -54,7 +56,6 @@ export class Alert implements ComponentInterface, OverlayInterface {
private processedButtons: AlertButton[] = [];
private wrapperEl?: HTMLElement;
private gesture?: Gesture;
private currentTransition?: Promise<any>;

presented = false;
lastFocus?: HTMLElement;
Expand Down Expand Up @@ -373,23 +374,13 @@ export class Alert implements ComponentInterface, OverlayInterface {
*/
@Method()
async present(): Promise<void> {
/**
* 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();
}

/**
Expand All @@ -403,13 +394,16 @@ export class Alert implements ComponentInterface, OverlayInterface {
*/
@Method()
async dismiss(data?: any, role?: string): Promise<boolean> {
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;
}

Expand Down
29 changes: 10 additions & 19 deletions core/src/components/loading/loading.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<typeof setTimeout>;
private currentTransition?: Promise<any>;

presented = false;
lastFocus?: HTMLElement;
Expand Down Expand Up @@ -235,29 +236,17 @@ export class Loading implements ComponentInterface, OverlayInterface {
*/
@Method()
async present(): Promise<void> {
/**
* 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();
}

/**
Expand All @@ -271,17 +260,19 @@ export class Loading implements ComponentInterface, OverlayInterface {
*/
@Method()
async dismiss(data?: any, role?: string): Promise<boolean> {
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;
}

Expand Down
52 changes: 19 additions & 33 deletions core/src/components/modal/modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<any>;
private sheetTransition?: Promise<any>;
private isSheetModal = false;
private currentBreakpoint?: number;
Expand Down Expand Up @@ -422,24 +423,15 @@ export class Modal implements ComponentInterface, OverlayInterface {
*/
@Method()
async present(): Promise<void> {
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.
Expand Down Expand Up @@ -481,7 +473,7 @@ export class Modal implements ComponentInterface, OverlayInterface {

writeTask(() => this.el.classList.add('show-modal'));

this.currentTransition = present<ModalPresentOptions>(this, 'modalEnter', iosEnterAnimation, mdEnterAnimation, {
await present<ModalPresentOptions>(this, 'modalEnter', iosEnterAnimation, mdEnterAnimation, {
presentingEl: presentingElement,
currentBreakpoint: this.initialBreakpoint,
backdropBreakpoint: this.backdropBreakpoint,
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
}

Expand All @@ -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<ModalDismissOptions>(
const dismissed = await dismiss<ModalDismissOptions>(
this,
data,
role,
Expand All @@ -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);
Expand All @@ -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;
}

Expand Down
Loading

0 comments on commit 584e9d3

Please sign in to comment.