Skip to content

fix(react): re-attach nested inline overlay after a Suspense hide #31390

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.

Sign up for GitHub

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

Open
ptmkenny wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(react): re-attach nested inline overlay after a Suspense hide #31390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96ee3ce
fix(react): re-attach nested inline overlay after a Suspense hide
ptmkenny Aug 20, 2026
b8f454b
fix for appendChild loses position, more tests
ptmkenny Aug 21, 2026
b8878b3
trim the duplicated hide/reveal explanation
ptmkenny Aug 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
300 changes: 295 additions & 5 deletions packages/react/src/components/__tests__/createInlineOverlayComponent.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,97 @@ const IonPopover = createInlineOverlayComponent<any, any>('ion-popover', undefin

/**
* Simulate what CoreDelegate does when an overlay presents: it teleports the
* host element out of its portal parent into another in-document container
* (the running app uses ion-app; here we use any sibling).
* host element out of its portal parent into another in-document container.
* The running app uses the single `ion-app` for every overlay, so one shared
* destination is created lazily and reused - overlays that present in
* sequence end up as siblings there, in presentation order.
*/
const teleport = (el: HTMLElement) => {
const dest = document.createElement('div');
dest.id = 'teleport-destination';
document.body.appendChild(dest);
let dest = document.getElementById('teleport-destination');
if (!dest) {
dest = document.createElement('div');
dest.id = 'teleport-destination';
document.body.appendChild(dest);
}
dest.appendChild(el);
return dest;
};

/**
* A component that suspends until `resolve` is called, plus the helpers to
* drive it. Rendering `<Suspender />` inside a boundary hides that boundary's
* content - React runs `componentWillUnmount` on everything in it without
* actually unmounting - and `reveal()` brings the same instances back with
* `componentDidMount`.
*/
const createSuspender = () => {
let resolveSuspender!: () => void;
let hasResolved = false;
const suspenderPromise = new Promise<void>((resolve) => {
resolveSuspender = () => {
hasResolved = true;
resolve();
};
});

return {
Suspender: () => {
if (!hasResolved) {
throw suspenderPromise;
}
return null;
},
reveal: async () => {
await act(async () => {
resolveSuspender();
await suspenderPromise;
});
},
};
};

/**
* The ids of `#teleport-destination`'s children, in document order. Document
* order is what core's `getPresentedOverlay` reads to decide which overlay
* Escape, hardware back and the focus trap act on, so a restore that changes
* it changes which overlay the user is talking to.
*/
const teleportedOrder = () =>
Array.from(document.getElementById('teleport-destination')?.children ?? []).map((el) => el.id);

/**
* Render `children` inside a Suspense boundary alongside a sibling that can be
* made to suspend on demand. `hide()` suspends that sibling, which is what the
* reported bug hits: the overlay itself renders fine, something else in the
* boundary does not, and React hides the whole boundary - running
* `componentWillUnmount` on the overlay wrapper without unmounting it.
*/
const renderWithBoundary = (children: React.ReactNode) => {
const { Suspender, reveal } = createSuspender();

let suspend!: () => void;
const Boundary = () => {
const [isSuspended, setIsSuspended] = React.useState(false);
suspend = () => setIsSuspended(true);

return (
<React.Suspense fallback={<div>loading</div>}>
{children}
{isSuspended ? <Suspender /> : null}
</React.Suspense>
);
};

const result = render(<Boundary />);

return {
...result,
hide: () =>
act(() => {
suspend();
}),
reveal,
};
};

afterEach(() => {
Expand Down Expand Up @@ -153,3 +236,210 @@ describe('createInlineOverlayComponent: unmount cleanup', () => {
expect(document.querySelector('ion-popover')).toBeNull();
});
});

describe('createInlineOverlayComponent: hidden subtree restore', () => {
it('restores a relocated nested overlay when a Suspense boundary hides and reveals it', async () => {
/**
* React runs `componentWillUnmount` when it *hides* a subtree as well as
* when it destroys one: a Suspense boundary falling back after mount runs
* it, then runs `componentDidMount` again on the same instance when the
* boundary reveals its content. A host removed while hidden has to come
* back, since React only re-inserts nodes it removed itself. Otherwise an
* overlay that was mid-`present()` is gone for good, with no dismiss
* lifecycle ever firing.
*/
const { hide, reveal } = renderWithBoundary(
<IonModal keepContentsMounted={true}>
<IonPopover />
</IonModal>
);

const popover = document.body.querySelector('ion-popover') as HTMLElement;

// CoreDelegate teleports the host out of its `<template>` as `present()`
// starts, before the events that flip `isOpen` have fired.
const teleportDestination = teleport(popover);

// A sibling suspends, so React hides the boundary's content: the overlay
// wrapper gets componentWillUnmount without actually being unmounted.
hide();

// The boundary reveals its content again on the same instances.
await reveal();

expect(popover.isConnected).toBe(true);

@ShaneK ShaneK Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These prove the node came back. They don't show that the overlay presents, or that it can still be dismissed, and since the removal fires the overlay's disconnectedCallback, a reconnected node isn't the same as a healthy overlay. I did check the test fails against the base implementation, so it's a genuine regression test. Nothing here covers stacking order or the dismiss path after a reveal though.

Using keepContentsMounted also means the contents are already mounted, so the suspension comes from a sibling rather than the overlay's own first render, which is the trigger the issue describes. There's a home for a browser level version next to IonPopoverNested and ModalTeleport.tsx, and the react19 app supports use(). Failing that, two nested overlays here would at least pin the ordering. A nested-branch StrictMode test would be good too, since the existing one only covers the portaled branch.

@ptmkenny ptmkenny Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added several more tests; let me know if this is enough coverage.

expect(popover.parentElement).toBe(teleportDestination);
});

it('restores a hidden nested overlay to its original position in the container', async () => {
/**
* Restoring the host is not enough on its own: core's `getPresentedOverlay`
* takes the *last* match in document order, so Escape, hardware back and
* the focus trap all follow the overlay that sits last in `ion-app`.
* Anything appended there while the subtree was hidden - a Suspense
* fallback showing an `ion-loading` is the obvious one - has to stay after
* the overlay that was already presented, so the restore has to remember
* the position, not just the parent.
*/
const { hide, reveal } = renderWithBoundary(
<IonModal keepContentsMounted={true}>
<IonPopover id="nested-popover" />
</IonModal>
);

const popover = document.body.querySelector('ion-popover') as HTMLElement;
const teleportDestination = teleport(popover);

hide();

// Something else presents into the same container while the subtree is
// hidden. The restored overlay must not jump behind it.
const laterOverlay = document.createElement('ion-loading');
laterOverlay.id = 'later-overlay';
teleportDestination.appendChild(laterOverlay);

await reveal();

expect(teleportedOrder()).toEqual(['nested-popover', 'later-overlay']);
});

it('keeps the stacking order of two nested overlays across a hide', async () => {
/**
* React tears a subtree down parent-first and builds it back up
* child-first, so two nested overlays restored by appending come back in
* the opposite order. In `ion-app` that hands Escape and the focus trap to
* the wrong overlay: the menu would dismiss while the submenu opened on
* top of it stays up.
*/
const { hide, reveal } = renderWithBoundary(
<IonModal keepContentsMounted={true}>
<IonPopover id="menu-popover" keepContentsMounted={true}>
<IonPopover id="submenu-popover" />
</IonPopover>
</IonModal>
);

const menu = document.querySelector('#menu-popover') as HTMLElement;
const submenu = document.querySelector('#submenu-popover') as HTMLElement;

// Presented in order, so the submenu sits after the menu in the container.
teleport(menu);
teleport(submenu);
expect(teleportedOrder()).toEqual(['menu-popover', 'submenu-popover']);

hide();
await reveal();

expect(teleportedOrder()).toEqual(['menu-popover', 'submenu-popover']);
});

it('presents and dismisses a nested overlay after the reveal', async () => {
/**
* A reconnected node is not the same thing as a working overlay. The
* present/dismiss lifecycle has to reach the app again: the contents mount
* on `willPresent`, non-React `on*` props are re-bound, and `didDismiss`
* still unmounts the contents.
*/
const onWillPresent = jest.fn();
const onDidPresent = jest.fn();
const onDidDismiss = jest.fn();

const { hide, reveal } = renderWithBoundary(
<IonModal keepContentsMounted={true}>
<IonPopover onWillPresent={onWillPresent} onIonPopoverDidPresent={onDidPresent} onDidDismiss={onDidDismiss}>
<span data-testid="popover-content">content</span>
</IonPopover>
</IonModal>
);

const popover = document.body.querySelector('ion-popover') as HTMLElement;
teleport(popover);

hide();
await reveal();

// The overlay presents: contents mount and both present handlers fire.
act(() => {
popover.dispatchEvent(new CustomEvent('willPresent'));
popover.dispatchEvent(new CustomEvent('ionPopoverDidPresent'));
});

expect(onWillPresent).toHaveBeenCalledTimes(1);
expect(onDidPresent).toHaveBeenCalledTimes(1);
expect(document.querySelector('[data-testid="popover-content"]')).toBeTruthy();

// And it dismisses: the handler reaches the app and the contents unmount.
act(() => {
popover.dispatchEvent(new CustomEvent('didDismiss'));
});

expect(onDidDismiss).toHaveBeenCalledTimes(1);
expect(document.querySelector('[data-testid="popover-content"]')).toBeNull();
});

it('restores a nested overlay whose own contents suspended while presenting', async () => {
/**
* The reported flow. Core emits `ionMount` from the middle of `present()`,
* which is what first mounts the overlay's children, so a suspension in
* those children always lands while `present()` is in flight and the host
* has just been teleported out of its `<template>`.
*/
const { Suspender, reveal } = createSuspender();
const onDidPresent = jest.fn();

render(
<React.Suspense fallback={<div>loading</div>}>
<IonModal keepContentsMounted={true}>
<IonPopover id="nested-popover" onIonPopoverDidPresent={onDidPresent}>
<Suspender />
</IonPopover>
</IonModal>
</React.Suspense>
);

const popover = document.querySelector('#nested-popover') as HTMLElement;
const teleportDestination = teleport(popover);

// `present()`: the host is already teleported when `ionMount` mounts the
// contents, and the contents suspend as they render.
act(() => {
popover.dispatchEvent(new CustomEvent('ionMount'));
});

await reveal();

expect(popover.isConnected).toBe(true);
expect(popover.parentElement).toBe(teleportDestination);

// `present()` runs to completion against the restored host.
act(() => {
popover.dispatchEvent(new CustomEvent('ionPopoverDidPresent'));
});

expect(onDidPresent).toHaveBeenCalledTimes(1);
});

it('does not orphan a relocated nested overlay across a StrictMode mount/unmount cycle', () => {
/**
* The nested branch removes its host outright where the portaled branch
* only moves it, so the StrictMode dev cycle has to be covered on both.
* The discarded first mount must not leave an orphan behind, and the
* surviving instance must still have its host.
*/
mockComponentOnReady = (el, cb) => {
teleport(el);
cb();
};

render(
<React.StrictMode>
<IonModal keepContentsMounted={true}>
<IonPopover id="nested-popover" />
</IonModal>
</React.StrictMode>
);

expect(document.querySelectorAll('ion-popover')).toHaveLength(1);
expect(document.querySelector('ion-popover')?.isConnected).toBe(true);
});
});
33 changes: 31 additions & 2 deletions packages/react/src/components/createInlineOverlayComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
stableMergedRefs: React.RefCallback<HTMLElement>;
portalTarget: HTMLElement | null;
isUnmounted = false;
// A nested host removed in `componentWillUnmount`, with the comment left in
// its place, so `componentDidMount` can put it back where it was.
removedHost: { node: HTMLElement; anchor: Comment } | null = null;

constructor(props: InternalProps) {
super(props);
Expand Down Expand Up @@ -87,6 +90,21 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
// componentWillUnmount.
this.isUnmounted = false;

// React runs `componentWillUnmount` when it only hides a subtree and
// mounts the same instance again on the reveal, so a host removed there
// goes back at the position it came from - document order decides which
// overlay is on top. The spec covers the flow.
const { removedHost } = this;

@ShaneK ShaneK Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The detachProps call in componentWillUnmount still runs when state.isOpen, tearing down the syncEvent bindings for every non-React on* prop, and core carries on emitting on the detached node. Only onWillPresent survives that, since handleWillPresent is a class field listener that never gets removed and calls the prop directly. During the hidden window willPresent reaches the handler and didPresent doesn't. Both work again after the reveal.

So the overlay comes back, but an app that focuses an input or fires analytics off onIonModalDidPresent gets nothing for that open. I think that also explains the "never fires" observation in the issue: both do fire, on the detached node, and only onWillPresent makes it to the app.

@ptmkenny ptmkenny Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, taking a closer look at this, it seems difficult to fix without a (much?) larger refactor.

Analyzing this with Opus:

Confirmed, and it isn't limited to the nested branch. The teardown at the end of componentWillUnmount is gated only on state.isOpen, so both branches reach it: the nested one after removing its host, the portaled one after moving its host back into ion-app. A top-level overlay keeps its node and loses its bindings all the same.

What the overlay is left with for the length of the hidden window:

  • detachProps clears the syncEvent binding for every non-React on* prop, so onIonModalDidPresent, onIonModalWillDismiss and the rest are unbound on a host core is still emitting on.
  • The didDismiss listener is removed explicitly. onDidDismiss is filtered out of attachProps and only ever reaches the app through handleDidDismiss, so with that listener gone the app hears nothing at all.
  • handleWillPresent and handleIonMount are class fields that are never removed, so willPresent and ionMount still land. That is why onWillPresent is the one prop that keeps working, exactly as you said.

Core doesn't pause for any of this. present() is in flight when the boundary falls back, the enter animation finishes on the host, and didPresent is emitted into nothing. The window lasts as long as the boundary stays suspended — a cold fetch, not a frame — and everything re-binds on the reveal, so the damage is contained to that one open.

What that costs, roughly in order:

  1. An app that focuses an input or fires analytics from onIonModalDidPresent gets nothing for that open.
  2. A dismiss that lands in the window is silent. An app that clears its own isOpen in onDidDismiss never clears it, so its state says the overlay is open while core has dismissed it, and it can't be reopened without some unrelated state change.
  3. The wrapper's own state.isOpen never resets either, so the contents stay mounted after the reveal.

@

I think this is also what produced the "never fires" line in #31389: both events do fire, on a host nothing is listening to, and only onWillPresent makes it back to the app.

Put as tests, this is what fails today. Nested:

it('keeps a hidden overlay wired up to the app for the rest of that open', async () => {
  /**
   * The hide lands in the middle of `present()`, and core finishes
   * presenting either way: `didPresent` is emitted on the host while the
   * subtree is hidden. Tearing the bindings down at `componentWillUnmount`
   * would swallow it, so an app that focuses an input or fires analytics off
   * `onIonPopoverDidPresent` would get nothing for that open.
   */
  const onDidPresent = jest.fn();
  const onDidDismiss = jest.fn();

  const { hide, reveal } = renderWithBoundary(
    <IonModal keepContentsMounted={true}>
      <IonPopover onIonPopoverDidPresent={onDidPresent} onDidDismiss={onDidDismiss} />
    </IonModal>
  );

  const popover = document.body.querySelector('ion-popover') as HTMLElement;
  teleport(popover);

  // `present()` has started, so the wrapper counts the overlay as open.
  act(() => {
"comments.md" 155L, 10613B
  // `present()` has started, so the wrapper counts the overlay as open.
  act(() => {
    popover.dispatchEvent(new CustomEvent('willPresent'));
  });

  hide();
  await flushTeardown();

  // Core finishes presenting, then the overlay is dismissed - both while the
  // subtree is still hidden.
  act(() => {
    popover.dispatchEvent(new CustomEvent('ionPopoverDidPresent'));
    popover.dispatchEvent(new CustomEvent('didDismiss'));
  });

  expect(onDidPresent).toHaveBeenCalledTimes(1);
  expect(onDidDismiss).toHaveBeenCalledTimes(1);

  await reveal();
});

And the portaled branch, which keeps its node and loses its bindings anyway:

it('keeps a portaled overlay wired up to the app across a hide', async () => {
  // Top-level overlays take the other `componentWillUnmount` branch, but the
  // teardown that runs after it is the same one.
  const onDidPresent = jest.fn();
  // teardown that runs after it is the same one.
  const onDidPresent = jest.fn();

  const { hide, reveal } = renderWithBoundary(<IonModal onIonModalDidPresent={onDidPresent} />);

  const modal = document.body.querySelector('ion-modal') as HTMLElement;

  act(() => {
    modal.dispatchEvent(new CustomEvent('willPresent'));
  });

  hide();
  await flushTeardown();

  act(() => {
    modal.dispatchEvent(new CustomEvent('ionModalDidPresent'));
  });

  expect(onDidPresent).toHaveBeenCalledTimes(1);

  await reveal();
});

Do you want this fixed here or tracked on its own? Either way it comes down to what componentWillUnmount is allowed to assume: React calls it for a hide as well as for a destroy, and the teardown has to tell those apart, which touches every overlay unmount rather than only the hidden ones.

Is this on the right track? I worry that this is turning into a lot of code for what seems like it should be a more straightforward fix.

this.removedHost = null;
if (removedHost) {
const { node, anchor } = removedHost;
if (!node.isConnected && anchor.isConnected) {
anchor.replaceWith(node);
} else {
anchor.remove();
}
}

this.componentDidUpdate(this.props);

this.ref.current?.addEventListener('ionMount', this.handleIonMount);
Expand Down Expand Up @@ -149,9 +167,14 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
* Nested overlays render inline inside a `<template>`. If the host
* has been moved out of that template, React's unmount won't reach
* it, so remove it directly. A host still in its template is left
* for React to remove.
* for React to remove. A comment marks the spot, the way CoreDelegate
* marks a teleport, so a reveal can put the host back where it was.
*/
if (!(node.parentElement instanceof HTMLTemplateElement)) {
const parent = node.parentElement;
if (parent && !(parent instanceof HTMLTemplateElement)) {
const anchor = document.createComment(RESTORE_ANCHOR);
parent.insertBefore(anchor, node);
this.removedHost = { node, anchor };
node.remove();

@ShaneK ShaneK Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Calling node.remove() fires the overlay's disconnectedCallback, and the re-append doesn't put back what that tears down: the parent-removal observer, the safe-area overrides, and the focus trap cleanup when presented is true. All of it gets set up inside present(), which has already resolved by the time the host comes back.

Present a modal, remove it, then put it back: backdrop-no-scroll on body is set, drops off with the removal, and never returns. The modal is visible again over a background that scrolls.

The flow in the linked issue doesn't reach this, since the removal happens before presented flips true. I'm more worried about a boundary inside an already-open overlay re-suspending on a refetch, where presented is true and the cleanup does fire. Could you re-assert those on restore, or scope the case out in the description?

@ptmkenny ptmkenny Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm having trouble fixing this. Opus analysis suggests this can't be fixed in React:

I can't fix it from the React package. Re-appending re-runs connectedCallback, which is only prepareOverlay + triggerChanged, so the trigger click listener comes back and nothing else does: cleanupParentRemovalObserver, cleanupSafeAreaOverrides, cleanupViewTransitionListener, and cleanupRootFocusTrapAccessibility when presented is true. All of it is private instance state, and re-asserting it from the wrapper would mean a second copy of isBackdropAlwaysBlocking and the focus-trap rules living in @ionic/react.

I did try to dodge the disconnect rather than repair it — parking the host back in its marker <template> instead of removing it, so React owns the removal. That doesn't work: a DOM move between two connected parents still fires disconnectedCallback, which is what moveBefore() exists for, and that's Chrome-only.

On the backdrop-no-scroll example, I think it's narrower than stated. cleanupRootFocusTrapAccessibility only strips the class when no remaining presented overlay is locking, and for a nested overlay the parent usually is one. It bites when the parent isn't — a sheet with backdropBreakpoint > 0, showBackdrop={false}, or focusTrap={false}. Still real, just not every nested case.

So I've scoped it out, with a comment at the removal site recording what's lost and that an overlay hidden mid-present() — the case this restore exists for — isn't affected. The fix belongs in core: an overlay reconnected while presented should re-establish its present-time setup regardless of who moved it.

I'm having trouble grokking the code so I have to defer to you on this.

}
} else if (this.portalTarget && node.parentNode !== this.portalTarget) {
Expand Down Expand Up @@ -318,3 +341,9 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
};

const DELEGATE_HOST = 'ion-delegate-host';

/**
* Marks where a nested overlay host was removed from, so it can be restored to
* the same position if React was only hiding the subtree.
*/
const RESTORE_ANCHOR = 'ionic hidden overlay';
Loading
Loading