bug: Overlays dismiss() method can cause getTop() to return dismissed instance

[andreaslarssen]

Prerequisites

• [X] I have read the Contributing Guidelines.
• [X] I agree to follow the Code of Conduct.
• [X] I have searched for existing issues that already report this problem, without success.

Ionic Framework Version

• [ ] v4.x
• [ ] v5.x
• [X] v6.x
• [ ] Nightly

Current Behavior

I want to show a loading screen while my (Angular) app navigates. I solve this by adding and removing a loading screen (via LoadingController) on navigation start and navigation end (as well as cancel and error).

It usually works, but:

I have a parent route w/ children routes. The parent route redirects the user to a specific child (the first) each time it's loaded. That means, if the user refreshes the browser when standing on another child than the first, the parent route loads first and redirects them to the first child. Standard stuff. Let me just say that there's no indication that this is what's causing the problem, just consider it context.

This is the service method that should close the loading indicator(s):

private closeLoadingIndicators(): void {
    this.loadingController.getTop().then((loadingIndicator: HTMLIonLoadingElement | undefined) => {
      console.log('loadingIndicator:', loadingIndicator);
      console.log('undefined:', loadingIndicator === undefined);
      console.log('typeof:', typeof loadingIndicator);
      console.log('present:', loadingIndicator?.present);
      console.log('Removing loading screen');

      if (loadingIndicator) {
        loadingIndicator.dismiss().then(() => {
          this.closeLoadingIndicators();
        });
      }
    })
  }

The idea is that the DOM can have several loading indicator present, and as long as it has, remove them one by one. The output from the console.log()s are as following:

As you can see, the 'loadingIndicator' is nothing(?), but the type seems to be object. Even though the instance seems to be nothing, the if check renders true, and the app re-runs the method, causing an endless loop.

Not sure what's causing it, and the reason why I gave the context of my app routes, is because it only happens on that particular navigation (with the redirect). It does not seem to be relevant though. What does seem to be relevant, is time. When doing something like this:

from(loadingIndicator.dismiss()).pipe(
  delay(300),
  tap(() => {
    this.closeLoadingIndicators();
})).subscribe();

...everything seems to work fine. Which leads me to think that even though the LoadingController.dismiss() promise is resolved, maybe the HTML element isn't really removed from the DOM yet.

I don't know, I can't get further with this. Anyone?

Expected Behavior

The LoadingController.getTop() to return either the HTML element or undefined (like it's typed)

Steps to Reproduce

See the code reproduction url

Code Reproduction URL

https://stackblitz.com/edit/ionicon-title-bug-1wtfct?file=src/app/app.component.ts

Ionic Info

Ionic:

Ionic CLI : 6.17.0

Utility:

cordova-res : not installed globally native-run : not installed globally

System:

NodeJS : v14.17.5 npm : 7.21.1 OS : Linux 5.13

Additional Information

No response

[sean-perkins]

@andreaslarssen thanks for reporting this issue.

The reason behind this bug, is when you calling loadingController.getTop() it is currently returning the same instance of the loading component, over and over.

Our getTop() method attempts to return all non-hidden overlays, by filtering the overlays that have the class .overlay-hidden. When dismiss() is called, it waits for the animation to complete, before adding the class. This delay in adding the class, causes the class list to be modified in the next frame. When calling getTop() immediately after dismiss, the class list hasn't been modified and it returns the same element again.

As a workaround, you can requestAnimationFrame wrap this logic:

loadingIndicator.dismiss().then(() => {
  requestAnimationFrame(() => {
    this.closeLoadingIndicators();
  });
});

Likely we need to resolve dismiss with a request animation frame callback after the class is added.

[andreaslarssen]

@sean-perkins Do you know if it's the same for all controls w/ a getTop() method (modals, popovers etc.)? I haven't encountered it yet w/ those, but that don't mean it's not a bug there as well.

[sean-perkins]

@andreaslarssen in theory it applies to all overlays (alerts, loading, modal, popover, etc.). The resolution will be to the shared dismiss logic that each of the overlay implementations use.

[sean-perkins]

Hello @andreaslarssen can you test with this dev-build and let me know if you run into any issues?

6.1.14-dev.11657292469.17e64b38

Stackblitz can occasionally be difficult with recently published packages. You may need to locally clone your reproduction to install the package.

[sean-perkins]

Hello @andreaslarssen after further discovery, I do not believe this is an active bug in Ionic.

The root issue here, is you have two callers to dismiss the same overlay (recursively).

The first caller will resolve after the overlay is finished transitioning/dismissing (e.g. 300ms).

The second caller will resolve immediately, due to this check: https://github.com/ionic-team/ionic-framework/blob/abb56d22b4a81d1bc34c689de4ef7218e7503b20/core/src/utils/overlays.ts#L478

at which point, it will recursively loop until the first caller resolves. This causes the infinite loop behavior.

The tangible workaround in your implementation is to do the following:

loadingIndicator.dismiss().then(dismissed => {
  if (dismissed) {
    closeLoadingIndicators();
  }
});

This will effectively ignore the second caller and only attempt to close the remaining loading indicators once the original caller has resolved. This prevents the infinite loop.

I am going to close out this issue, as we don't see this as a bug in our implementation. I am creating a backlog task to document getTop() so that it includes language about overlays that are actively dismissing.

Thanks!

[andreaslarssen]

@sean-perkins Isn't the issue here that you can't trust the resolved dismiss() promise?

[sean-perkins]

@andreaslarssen the dismiss() method is resolving correctly. It resolves under 3 conditions:

1. The overlay dismissed (transition completed, DOM node removed) - returns true
2. The overlay you are trying to dismiss, has already been dismissed (e.g. calling the dismiss() method on the same overlay while it is transitioning. This is to prevent duplicate/incorrect transitions). - returns false
3. An error occurs within internal logic that handles dismissing.

Your application logic has multiple areas where the recursive logic for dismissing an overlay can occur, which leads to dismissing the same overlay. This causes the behavior I explained previously. The requestAnimationFrame workaround that I recommended prior, only works in your application code, as it creates a long enough delay for the first dismiss() to resolve, before the second caller is executed.

Simply updating your example to use the stateful information returned from dismiss(), resolves the application logic issue in this case.

[ionitron-bot[bot]]

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.