Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(overlays): prevent overlays from getting stuck open #28069

Merged
merged 12 commits into from
Aug 31, 2023
Merged

Conversation

mapsandapps
Copy link
Contributor

@mapsandapps mapsandapps commented Aug 28, 2023

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
  • No

Other information

Relevant links:

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 by changing the Ionic version between 7.3.1 and 7.3.2-dev.11693262117.17edbf6d

@stackblitz
Copy link

stackblitz bot commented Aug 28, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label Aug 28, 2023
@mapsandapps mapsandapps changed the title WIP: Lock controller WIP: fix(overlays): prevent overlays from getting stuck open Aug 28, 2023
@mapsandapps mapsandapps changed the title WIP: fix(overlays): prevent overlays from getting stuck open fix(overlays): prevent overlays from getting stuck open Aug 28, 2023
@mapsandapps mapsandapps marked this pull request as ready for review August 29, 2023 15:04
@mapsandapps mapsandapps requested review from a team and thetaPC and removed request for a team August 29, 2023 15:05
core/src/utils/overlays.ts Outdated Show resolved Hide resolved
core/src/utils/overlays.ts Outdated Show resolved Hide resolved
core/src/components/modal/modal.tsx Outdated Show resolved Hide resolved
core/src/components/modal/modal.tsx Outdated Show resolved Hide resolved
core/src/components/popover/popover.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a couple bugs, but otherwise this looks good!

core/src/components/popover/popover.tsx Show resolved Hide resolved
core/src/components/modal/modal.tsx Show resolved Hide resolved
core/src/components/modal/modal.tsx Show resolved Hide resolved
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this!

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry one last thing - found a typo :)

core/src/utils/lock-controller.ts Outdated Show resolved Hide resolved
@mapsandapps mapsandapps added this pull request to the merge queue Aug 31, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 31, 2023
@mapsandapps mapsandapps added this pull request to the merge queue Aug 31, 2023
Merged via the queue into main with commit 584e9d3 Aug 31, 2023
@mapsandapps mapsandapps deleted the FW-4053-lock branch August 31, 2023 16:48
@adamduren
Copy link
Contributor

I am still seeing this with:

{
  "@ionic/react": "^7.5.7",
  "react": "^18.0.0",
  "react-dom": "^18.0.0",
}

Please let me know how I can help.

@adamduren
Copy link
Contributor

Here's a video of a test I created using a fresh react project made with ionic start. The test shows 10 iterations with a dismiss delay of 20ms in which the bug is reproduced 0% of the time. Changing this to 10ms however shows it can be reproduced 20% of the time. It gets worse of course if there's no delay.

This may seem like an edge case and it could be argued that IonLoading is not appropriate here but we have some async tasks that are sometimes very quick <10ms but sometimes they are much longer than which necessitates the use of IonLoading.

ion-overlay-dismiss-bug.mov

@thetaPC
Copy link
Contributor

thetaPC commented Nov 30, 2023

@adamduren Thank you for bringing this to our attention! Please open a new issue so the team can track it separately.

@hussam-001
Copy link

I am still seeing this with:

{
  "@ionic/react": "^7.5.7",
  "react": "^18.0.0",
  "react-dom": "^18.0.0",
}

Please let me know how I can help.

here is a workaround for this

import { IonLoading } from "@ionic/react";
import { useEffect, useRef } from "react";

const LoadingIndicator = ({
  isOpen,
  ...props
}: React.ComponentProps<typeof IonLoading>) => {
  const ref = useRef<HTMLIonLoadingElement>(null);

  useEffect(() => {
    ref.current?.[isOpen ? "present" : "dismiss"]();
  }, [isOpen]);

  if (!isOpen) return null;
  return <IonLoading ref={ref} {...props} />;
};

export default LoadingIndicator;

<LoadingIndicator isOpen={isLoading} />

@DunhamGitHub
Copy link

Still seing this bug...

{
  "@ionic/react": "^8.1.1",
  "react": "^18.3.1",
  "react-dom": "^18.3.1",
}
image

@DunhamGitHub
Copy link

I am still seeing this with:

{
  "@ionic/react": "^7.5.7",
  "react": "^18.0.0",
  "react-dom": "^18.0.0",
}

Please let me know how I can help.

here is a workaround for this

import { IonLoading } from "@ionic/react";
import { useEffect, useRef } from "react";

const LoadingIndicator = ({
  isOpen,
  ...props
}: React.ComponentProps<typeof IonLoading>) => {
  const ref = useRef<HTMLIonLoadingElement>(null);

  useEffect(() => {
    ref.current?.[isOpen ? "present" : "dismiss"]();
  }, [isOpen]);

  if (!isOpen) return null;
  return <IonLoading ref={ref} {...props} />;
};

export default LoadingIndicator;
<LoadingIndicator isOpen={isLoading} />

Does not show the popup dialog.

When I remove ref={ref} from the return element, then it does ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: v7 ion-loading isOpen property sometimes stuck and ignored on fast change
6 participants