fix(modal): fix timing issue when rapidly closing and opening controller modal

[averyjohnston]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Build (npm run build) was run locally and any changes were pushed
• Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: Resolves #24230

If a controller modal is re-opened immediately after dismissing, the call to detachComponent that unmounts the user's modal content doesn't trigger until after the present animation (due to the await this.currentTransition line above it). This causes the modal to present, and then immediately unmount its content.

This issue only happens with controller modals; inline modals are unaffected. It also doesn't happen in core, only frameworks like Vue.

What is the new behavior?

The detachComponent call is only made for inline modals. It isn't needed for controller modals anyway, because by that point, the entire modal (including the user's element) has already been removed from the DOM.

Does this introduce a breaking change?

• Yes
• No

Other information

• Repo for testing (Vue): https://github.com/amandaesmith3/sandbox-ionic-vue/tree/FW-349
• The buttons in the header will open a controller or inline modal.
• If the browser is switched to responsive view, the X that closes the modal will be in the same spot as the rightmost header button. This lets you rapidly click to trigger the bug.
• To try the other modal type, switch the position of the two header buttons in the code, so that the one you want to try is on the right.
• For controller modals, you need at minimum 3 clicks -- present, dismiss, present. The bug happens if the present happens soon enough after the dismiss.
• For inline modals, the bug will not trigger.
• Screencast of bug: https://user-images.githubusercontent.com/90629384/145836560-5daabce8-b76c-4d48-be91-e89db29095e8.mp4
• After fix: https://user-images.githubusercontent.com/90629384/145836646-79671d76-836d-4f4e-bf9e-c576469f80e8.mp4

[sean-perkins]

Everything looks great from the reproduction app/solving the issue, nice work!

Anything we can do to solve the issue of these inserted comments not being cleaned up (they get inserted next to ion-modal in the DOM)?

<!--teleport start-->
<!--teleport end-->
<!--teleport start-->
<!--teleport end-->

I believe that's a responsibility of the framework delegate that may not be called anymore with the change for controller-based modals.

[liamdebeasi]

It looks like there is a race condition in https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/modal/modal.tsx#L495 where we the present method is allowed to proceed even though the dismiss method is not complete because we only await the animation.

I think what I was trying to do at the time was implement a locking system where only one method can be called at a time, and subsequent calls wait for the previous method to finish. Unfortunately, I only leveraged the actual transition, forgetting that there are other async calls that we need to wait for.

This patch does resolve the behavior noted in the issue, but it does not resolve the underlying race condition, so I was hoping we could resolve the race condition before we merge.

---

In terms of a different approach to fixing this, I am thinking we implement what the router-outlet component has currently: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/router-outlet/route-outlet.tsx#L240-L249

This creates a lock for a single commit call and is unlocked only when the unlock callback is called: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/router-outlet/route-outlet.tsx#L134

This would allow us to both resolve the race condition and the issue that reported this. Also, because popover uses the same currentTransition logic that the modal does, this race condition is probably there too. Happy to discuss more. Thoughts?

[averyjohnston]

The lock/unlock method didn't work; Liam and I spent a bunch of time debugging and couldn't find the source of the problem. We'll go with the original strategy for now. I've created an internal ticket to try and solve the race condition directly in the long term. Will add more details there.