From 7b551fd54b9e16a2538e5b82a13d72b3007fa045 Mon Sep 17 00:00:00 2001
From: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
Date: Tue, 5 Sep 2023 17:04:27 -0400
Subject: [PATCH] fix(react): overlay content is shown with hook  (#28109)

Issue number: resolves #28102

---------

<!-- 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. -->

When one modal is added and another modal is removed, the modal that is
removed does not account for the newly added modal when updating the
overlay context in React. As a result, the inner contents of the newly
added modal is not mounted.

We originally tried to fix this in
https://github.com/ionic-team/ionic-framework/pull/24553, but the fix
was not complete. While storing the latest information in a React ref
was correct, the way we updated the ref was done in a way such that data
was still stale.

In particular, the `overlaysRef` is updated whenever `IonOverlayManager`
is re-rendered. State updates are batched, so updating the state twice
in quick succession does not necessarily result in 2 separate renders.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- We need to make sure the ref is updated synchronously before any
render so that `addOverlay` and `removeOverlay` always have access to
the latest data.
- Added a test

## 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. -->

Dev build: `7.3.3-dev.11693592339.18e000af`

---------

Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
Co-authored-by: Amanda Johnston <90629384+amandaejohnston@users.noreply.github.com>
---
 .../src/components/IonOverlayManager.tsx      | 39 ++++++++++++++++++-
 .../src/pages/overlay-hooks/ModalHook.tsx     | 29 ++++++++++++++
 .../e2e/specs/overlay-hooks/useIonModal.cy.ts | 12 ++++++
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/packages/react/src/components/IonOverlayManager.tsx b/packages/react/src/components/IonOverlayManager.tsx
index 60e7ef1f92..0c07ea7823 100644
--- a/packages/react/src/components/IonOverlayManager.tsx
+++ b/packages/react/src/components/IonOverlayManager.tsx
@@ -40,7 +40,6 @@ export const IonOverlayManager: React.FC<IonOverlayManagerProps> = ({ onAddOverl
    */
   const [overlays, setOverlays] = useState<OverlaysList>({});
   const overlaysRef = useRef<OverlaysList>({});
-  overlaysRef.current = overlays;
 
   useEffect(() => {
     /* Setup the callbacks that get called from <IonApp /> */
@@ -51,12 +50,50 @@ export const IonOverlayManager: React.FC<IonOverlayManagerProps> = ({ onAddOverl
   const addOverlay = (id: string, component: ReactComponentOrElement, containerElement: HTMLDivElement) => {
     const newOverlays = { ...overlaysRef.current };
     newOverlays[id] = { component, containerElement };
+
+    /**
+     * In order for this function to use the latest data
+     * we need to update the ref synchronously.
+     * However, updating a ref does not cause a re-render
+     * which is why we still update the state.
+     *
+     * Note that updating the ref in the body
+     * of IonOverlayManager is not sufficient
+     * because that relies on overlaysRef being
+     * updated as part of React's render loop. State updates
+     * are batched, so updating the state twice in quick succession does
+     * not necessarily result in 2 separate render calls.
+     * This means if two modals are added one after the other,
+     * the first modal will not have been added to
+     * overlaysRef since React has not re-rendered yet.
+     * More info: https://react.dev/reference/react/useState#setstate-caveats
+     */
+    overlaysRef.current = newOverlays;
     setOverlays(newOverlays);
   };
 
   const removeOverlay = (id: string) => {
     const newOverlays = { ...overlaysRef.current };
     delete newOverlays[id];
+
+    /**
+     * In order for this function to use the latest data
+     * we need to update the ref synchronously.
+     * However, updating a ref does not cause a re-render
+     * which is why we still update the state.
+     *
+     * Note that updating the ref in the body
+     * of IonOverlayManager is not sufficient
+     * because that relies on overlaysRef being
+     * updated as part of React's render loop. State updates
+     * are batched, so updating the state twice in quick succession does
+     * not necessarily result in 2 separate render calls.
+     * This means if two modals are added one after the other,
+     * the first modal will not have been added to
+     * overlaysRef since React has not re-rendered yet.
+     * More info: https://react.dev/reference/react/useState#setstate-caveats
+     */
+    overlaysRef.current = newOverlays;
     setOverlays(newOverlays);
   };
 
diff --git a/packages/react/test/base/src/pages/overlay-hooks/ModalHook.tsx b/packages/react/test/base/src/pages/overlay-hooks/ModalHook.tsx
index 9712f5a226..fe3dfd0c28 100644
--- a/packages/react/test/base/src/pages/overlay-hooks/ModalHook.tsx
+++ b/packages/react/test/base/src/pages/overlay-hooks/ModalHook.tsx
@@ -30,6 +30,8 @@ const Body: React.FC<{
       <IonButton expand="block" onClick={() => onDismiss({ test: true }, 'close')}>
         Close
       </IonButton>
+
+      <button onClick={onDismiss} id="show-secondary-modal">Show Secondary Modal</button>
     </IonContent>
   </IonPage>
 );
@@ -83,6 +85,14 @@ const ModalHook: React.FC = () => {
 
   const [presentModalWithContext] = useIonModal(ModalWithContext);
 
+  const [presentSecondaryModal] = useIonModal(ModalSecondary);
+  const [presentRootModal, dismissRootModal] = useIonModal(Body, {
+    onDismiss: () => {
+      dismissRootModal();
+      presentSecondaryModal();
+    }
+  });
+
   return (
     <MyContext.Provider value={{ value: 'overriden value' }}>
       <IonPage>
@@ -134,6 +144,16 @@ const ModalHook: React.FC = () => {
             Show Modal with Context
           </IonButton>
 
+          <IonButton
+            expand="block"
+            onClick={() => {
+              presentRootModal()
+            }}
+            id="show-root-modal"
+          >
+            Show Root Modal
+          </IonButton>
+
           <div>Count: {count}</div>
           <div>Dismissed with role: {dismissedRole}</div>
           <div>Data: {dismissedData && JSON.stringify(dismissedData)}</div>
@@ -143,6 +163,15 @@ const ModalHook: React.FC = () => {
   );
 };
 
+const ModalSecondary: React.FC = () => {
+  return (
+    <div className="ion-padding">
+      <h1>Secondary Modal</h1>
+      <p>This text should be visible</p>
+    </div>
+  )
+}
+
 const MyContext = React.createContext({
   value: 'default value',
 });
diff --git a/packages/react/test/base/tests/e2e/specs/overlay-hooks/useIonModal.cy.ts b/packages/react/test/base/tests/e2e/specs/overlay-hooks/useIonModal.cy.ts
index 64e904061c..12835ee7c7 100644
--- a/packages/react/test/base/tests/e2e/specs/overlay-hooks/useIonModal.cy.ts
+++ b/packages/react/test/base/tests/e2e/specs/overlay-hooks/useIonModal.cy.ts
@@ -70,4 +70,16 @@ describe('useIonModal', () => {
     //verify context value is overriden value
     cy.get('div').contains('overriden value')
   });
+
+  it('should render nested modal when modals are added and removed at the same time', () => {
+    cy.get('#show-root-modal').click();
+
+    cy.get('ion-modal').should('have.length', 1);
+
+    cy.get('ion-modal #show-secondary-modal').click();
+
+    cy.get('ion-modal').should('have.length', 1);
+
+    cy.get('ion-modal').contains('This text should be visible');
+  });
 });