ionic-framework icon indicating copy to clipboard operation
ionic-framework copied to clipboard

bug: conditionally rendered modal via route doesn't close when navigating away

Open corysmc opened this issue 1 year ago • 9 comments

Prerequisites

Ionic Framework Version

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

Current Behavior

After updating to ionic 6, Conditionally rendered ion-modals are not dismissing after changing routes. There's a fix in route for an error that happened when the modal was removed from the dom https://github.com/ionic-team/ionic-framework/issues/25590 - but now the modal won't dismiss when navigating away, and the animations aren't happening - and the modal won't dismiss after changing routes.

A route based conditionally rendered IonModal worked in ionic v5 - and I've been told this is a common pattern in a react app, so I would expect this to work out of the box.

Video:

Here's a video of the simple demo I put together based off of a fork from the ionic-react-conference app https://user-images.githubusercontent.com/6452188/185195963-06ad49bf-0533-41ba-9f6e-0cb01326eff9.mov

Expected Behavior

When conditionally rendering an IonModal in React I would expect to be able to show/hide an ion modal based on a matched route. Ideally it would also animate in/out when mounting and unmounting (see video for hack workaround to get this working)

Steps to Reproduce

  1. Install the dev build that fixes the original issue "Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node."
  2. Add a Modal component that has prop isOpen={true}
  3. Add a Switch & Route to the component
  4. Inside the Modal Component change the route
  5. Route to main component ("/support"), click on "New Support Ticket", then click one of the back buttons
  6. ⚠️ Notice the modal doesn't close (see video above)

Step 1 yarn add @ionic/[email protected] and yarn add @ionic/[email protected] or npm i @ionic/[email protected] and npm i @ionic/[email protected]

Step 2 The SupportModal component above looks something like this: <IonModal isOpen>...

Step 3

...
<IonContent>
  <IonButton href="/support/new">New Support Ticket</IonButton>
</IonContent>
<Switch>
  <Route path="/support/new" exact component={SupportModal} />
</Switch>
...

Step 4 Add this to the SupportModal <IonButton href="/support">Go Back</IonButton>

Full code here: https://github.com/corysmc/ionic-react-conference-app

Code Reproduction URL

https://github.com/corysmc/ionic-react-conference-app

Ionic Info

Ionic:

Ionic CLI : 6.19.0 Ionic Framework : @ionic/react 6.2.3-dev.11660337759.18ea0f7e

Capacitor:

Capacitor CLI : 1.3.0 @capacitor/android : not installed @capacitor/core : 1.3.0 @capacitor/ios : not installed

Utility:

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

System:

NodeJS : v14.18.2 npm : 6.14.15 OS : macOS Monterey

Additional Information

Related issues https://github.com/ionic-team/ionic-framework/issues/25590 https://github.com/ionic-team/ionic-framework/issues/24887

corysmc avatar Aug 17 '22 17:08 corysmc

Thanks for the issue. Are you able to provide a repo that contains only the minimum amount of code required to reproduce this issue? I am getting import errors when I try to run your app, possibly due to outdated dependencies.

liamdebeasi avatar Aug 17 '22 18:08 liamdebeasi

Hmm that's interesting , maybe because I was using yarn instead of npm. I was able to reproduce again by cloning to my other computer, running npm install and npm start - It did change my package-lock file - so I've committed those changes. Can you try again @liamdebeasi ? Thanks

corysmc avatar Aug 17 '22 20:08 corysmc

@liamdebeasi - here's an even smaller repo: https://github.com/corysmc/test-route-modal

corysmc avatar Aug 17 '22 20:08 corysmc

Thanks. While coupling modals with routing is an anti-pattern and not something we encourage, conditionally rendering the modal should remove the underlying Web Component. Let me take a closer look and see why this is happening.

liamdebeasi avatar Aug 17 '22 22:08 liamdebeasi

Thanks @liamdebeasi - I really appreciate it! I totally agree about the anti-pattern. I'll see if we can make the change internally to use modals properly.

For reference here's another related issue https://github.com/ionic-team/ionic-framework/issues/23567

corysmc avatar Aug 18 '22 18:08 corysmc

Hey @corysmc can you test with this dev-build and let me know if you run into any odd quirks?

npm i @ionic/[email protected] @ionic/[email protected]

I tested with the test-route-modal repository and it appears to dismiss correctly on route actions.

sean-perkins avatar Aug 25 '22 20:08 sean-perkins

@ionic/[email protected] works for me too. The modal unmounted correctly when navigating to another page

IhorHolovatsky avatar Aug 31 '22 20:08 IhorHolovatsky

Any status update on this fix?

puopg avatar Sep 09 '22 14:09 puopg

@puopg this is still an active bug in discovery/development.

The issue of dismissing the overlay on route navigation has been resolved in the dev-build, but is also dependent on another bug: #25590 that requires additional effort.

We have multiple strategies of presenting and controlling the rendering of overlays (isOpen, trigger, controllers, keepContentsMounted, etc.). The challenge that I am currently working through is solving this bug in a way that does not regress other presenting strategies. Moving the rendering of the overlay to a React Portal solves the pain points of both issues for isOpen usages, but regresses trigger implementations.

I will share an update on both issues once I validate a strategy that works and is well tested. I will also likely share a new dev-build when that occurs.

Thanks!

sean-perkins avatar Sep 09 '22 18:09 sean-perkins