Skip to content

Commit bfff8e1

Browse files
ShaneKptmkenny
andauthored
fix(overlays): restore presented state after a DOM move (#31400)
Issue number: resolves #31389 --------- ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> Currently, `componentWillUnmount` in `createInlineOverlayComponent` removes a nested inline overlay host that `CoreDelegate` has teleported out of its `<template>`. React also runs that hook when it only *hides* a subtree, so a Suspense fallback, an Offscreen tree, or the StrictMode cycle deletes an overlay that is still presenting. React did not remove the host, so React never puts it back. Core emits `ionMount` and then goes quiet: no `willPresent`, no `didPresent`, and no dismiss lifecycle, while the React tree still believes the overlay is open. The user sees nothing at all. ## What is the new behavior? The wrapper now tells a React hide from a real destroy instead of treating both as an unmount. The marker `<template>` is the discriminator, since it stays at the JSX position and leaves the document only on a real unmount, so the teardown defers a microtask and runs only if the marker really left. A hide leaves the host exactly where it is, `present()` and `didDismiss` keep working while the subtree is hidden, and a destroy that hits while it is already hidden still cleans up even though React skips the second `componentWillUnmount` there. This addresses #31223's requirements by making a relocated portaled host move back into `portalTarget` synchronously, and the StrictMode cycle still leaves exactly one copy in the DOM. Separately, the five overlays that release the app root on disconnect (`modal`, `popover`, `alert`, `action-sheet`, `loading`) now restore it on reconnect, along with the modal's safe-area overrides and parent-removal observer, and the button gesture on `alert` and `action-sheet`. A presented overlay that gets detached and re-inserted across a task previously came back with the root lock and safe-area gone. ## Does this introduce a breaking change? - [ ] Yes - [X] No ## Other information This supersedes #31390, which fixes the same issue by recording the removed host and re-appending it on remount. That works for the reported case, but it keeps the removal, and the removal costs more than the DOM node: the disconnect releases the root lock, clears the safe-area overrides and the sheet's `--ion-modal-offset-top`, destroys the button gesture, and drops the parent-removal observer. Putting the node back restores none of that, so the smaller fix needs the core half of this PR anyway. Not removing it in the first place avoids all of that, and also covers the portaled overlays, the destroy-while-hidden case, and a dismiss landing during the hidden window, which #31390 doesn't reach. Thanks to @ptmkenny for the report, the analysis, and the original patch, which is where the diagnosis came from. Note that the core half is defence-in-depth rather than a requirement for #31389. Once the React wrapper stops removing the host, core no longer sees a detach and re-attach in the reported flow. It does fix a real pre-existing bug though, since a presented modal moved by anything else lost its safe-area, so it's worth keeping. Preview: - [Inline modal (iOS)](https://ionic-framework-git-fix-react-inline-overlay-hide-vs-unmount-ionic1.vercel.app/src/components/modal/test/inline?ionic:mode=ios) - [Modal safe-area (iOS)](https://ionic-framework-git-fix-react-inline-overlay-hide-vs-unmount-ionic1.vercel.app/src/components/modal/test/safe-area?ionic:mode=ios) ## Current dev build: ``` 9.0.1-dev.11787589191.1e183b55 ``` --------- Co-authored-by: ptmkenny <github@ptmkenny.com>
1 parent 95e2224 commit bfff8e1

17 files changed

Lines changed: 1741 additions & 137 deletions

File tree

core/src/components/action-sheet/action-sheet.tsx

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
isCancel,
1515
prepareOverlay,
1616
present,
17+
restoreRootFocusTrapAccessibility,
1718
safeCall,
1819
setOverlayId,
1920
} from '@utils/overlays';
@@ -455,6 +456,15 @@ export class ActionSheet implements ComponentInterface, OverlayInterface {
455456
connectedCallback() {
456457
prepareOverlay(this.el);
457458
this.triggerChanged();
459+
460+
// `componentDidLoad` only fires once per instance, so a reconnect has to
461+
// rebuild the gesture its disconnect destroyed.
462+
this.setupButtonActiveGesture();
463+
464+
// Re-apply the root lock if moved without dismiss() being called
465+
if (this.presented) {
466+
restoreRootFocusTrapAccessibility(this.el);
467+
}
458468
}
459469

460470
disconnectedCallback() {
@@ -478,26 +488,36 @@ export class ActionSheet implements ComponentInterface, OverlayInterface {
478488
this.buttonsChanged();
479489
}
480490

481-
componentDidLoad() {
482-
/**
483-
* Only create gesture if:
484-
* 1. A gesture does not already exist
485-
* 2. App is running in iOS mode
486-
* 3. A wrapper ref exists
487-
* 4. A group ref exists
488-
*/
491+
/**
492+
* Only create gesture if:
493+
* 1. A gesture does not already exist
494+
* 2. App is running in iOS mode
495+
* 3. A wrapper ref exists
496+
* 4. A group ref exists
497+
* 5. The host is still connected, since a reconnect can schedule this and
498+
* disconnect again before the task runs
499+
*/
500+
private setupButtonActiveGesture() {
489501
const { groupEl, wrapperEl } = this;
490-
if (!this.gesture && getIonMode(this) === 'ios' && wrapperEl && groupEl) {
491-
readTask(() => {
492-
const isScrollable = groupEl.scrollHeight > groupEl.clientHeight;
493-
if (!isScrollable) {
494-
this.gesture = createButtonActiveGesture(wrapperEl, (refEl: HTMLElement) =>
495-
refEl.classList.contains('action-sheet-button')
496-
);
497-
this.gesture.enable(true);
498-
}
499-
});
502+
if (getIonMode(this) !== 'ios' || !wrapperEl || !groupEl) {
503+
return;
500504
}
505+
readTask(() => {
506+
// Bail if a call queued ahead of this one already built the gesture
507+
// (a second would orphan the first with its listeners still bound), if
508+
// the host disconnected while this task waited, or if the group scrolls.
509+
if (this.gesture || !this.el.isConnected || groupEl.scrollHeight > groupEl.clientHeight) {
510+
return;
511+
}
512+
this.gesture = createButtonActiveGesture(wrapperEl, (refEl: HTMLElement) =>
513+
refEl.classList.contains('action-sheet-button')
514+
);
515+
this.gesture.enable(true);
516+
});
517+
}
518+
519+
componentDidLoad() {
520+
this.setupButtonActiveGesture();
501521

502522
/**
503523
* If action sheet was rendered with isOpen="true"

core/src/components/action-sheet/test/basic/action-sheet.e2e.ts

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { expect } from '@playwright/test';
2-
import { configs, test } from '@utils/test/playwright';
2+
import { configs, detachAndReattach, test } from '@utils/test/playwright';
33

44
import { ActionSheetFixture } from './fixture';
55

@@ -166,3 +166,56 @@ configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
166166
});
167167
});
168168
});
169+
170+
/**
171+
* The button gesture only exists in iOS mode, so these run there.
172+
*/
173+
configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ config, title }) => {
174+
test.describe(title('action sheet: moved while presented'), () => {
175+
test.beforeEach(async ({ page }) => {
176+
await page.goto('/src/components/action-sheet/test/basic', config);
177+
});
178+
179+
test('should keep the app root locked', async ({ page }, testInfo) => {
180+
testInfo.annotations.push({
181+
type: 'issue',
182+
description: 'https://github.com/ionic-team/ionic-framework/issues/31389',
183+
});
184+
185+
const ionActionSheetDidPresent = await page.spyOnEvent('ionActionSheetDidPresent');
186+
187+
await page.click('#basic');
188+
await ionActionSheetDidPresent.next();
189+
190+
await expect(page.locator('body')).toHaveClass(/backdrop-no-scroll/);
191+
192+
await detachAndReattach(page.locator('ion-action-sheet'));
193+
194+
await expect(page.locator('body')).toHaveClass(/backdrop-no-scroll/);
195+
});
196+
197+
test('should keep activating buttons on press', async ({ page }, testInfo) => {
198+
testInfo.annotations.push({
199+
type: 'issue',
200+
description: 'https://github.com/ionic-team/ionic-framework/issues/31389',
201+
});
202+
203+
const ionActionSheetDidPresent = await page.spyOnEvent('ionActionSheetDidPresent');
204+
205+
await page.click('#basic');
206+
await ionActionSheetDidPresent.next();
207+
208+
await detachAndReattach(page.locator('ion-action-sheet'));
209+
210+
const button = page.locator('ion-action-sheet .action-sheet-button').first();
211+
const box = (await button.boundingBox())!;
212+
213+
await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2);
214+
await page.mouse.down();
215+
216+
await expect(button).toHaveClass(/ion-activated/);
217+
218+
await page.mouse.up();
219+
});
220+
});
221+
});

core/src/components/alert/alert.tsx

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
isCancel,
1818
prepareOverlay,
1919
present,
20+
restoreRootFocusTrapAccessibility,
2021
safeCall,
2122
setOverlayId,
2223
} from '@utils/overlays';
@@ -363,10 +364,16 @@ export class Alert implements ComponentInterface, OverlayInterface {
363364
this.triggerChanged();
364365
/**
365366
* If the alert was previously connected and is being reattached, the
366-
* ResizeObserver was disconnected. componentDidLoad only fires once per
367-
* instance, so re-establish the observer here on reconnect.
367+
* `ResizeObserver` and the button gesture were torn down. `componentDidLoad`
368+
* only fires once per instance, so re-establish both here on reconnect.
368369
*/
369370
this.setupButtonGroupResizeObserver();
371+
this.setupButtonActiveGesture();
372+
373+
// Re-apply the root lock if moved without dismiss() being called
374+
if (this.presented) {
375+
restoreRootFocusTrapAccessibility(this.el);
376+
}
370377
}
371378

372379
componentWillLoad() {
@@ -394,20 +401,23 @@ export class Alert implements ComponentInterface, OverlayInterface {
394401
this.buttonGroupResizeObserver = undefined;
395402
}
396403

397-
componentDidLoad() {
398-
/**
399-
* Only create gesture if:
400-
* 1. A gesture does not already exist
401-
* 2. App is running in iOS mode
402-
* 3. A wrapper ref exists
403-
*/
404+
/**
405+
* Only create gesture if:
406+
* 1. A gesture does not already exist
407+
* 2. App is running in iOS mode
408+
* 3. A wrapper ref exists
409+
*/
410+
private setupButtonActiveGesture() {
404411
if (!this.gesture && getIonMode(this) === 'ios' && this.wrapperEl) {
405412
this.gesture = createButtonActiveGesture(this.wrapperEl, (refEl: HTMLElement) =>
406413
refEl.classList.contains('alert-button')
407414
);
408415
this.gesture.enable(true);
409416
}
417+
}
410418

419+
componentDidLoad() {
420+
this.setupButtonActiveGesture();
411421
this.setupButtonGroupResizeObserver();
412422

413423
/**

core/src/components/alert/test/basic/alert.e2e.ts

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { expect } from '@playwright/test';
22
import type { Locator } from '@playwright/test';
33
import type { E2EPage } from '@utils/test/playwright';
4-
import { configs, test } from '@utils/test/playwright';
4+
import { configs, detachAndReattach, test } from '@utils/test/playwright';
55

66
configs({ directions: ['ltr'] }).forEach(({ config, screenshot, title }) => {
77
test.describe(title('alert: basic'), () => {
@@ -203,3 +203,56 @@ class AlertFixture {
203203
await expect(this.alert).toHaveScreenshot(screenshotFn(`alert-${modifier}`));
204204
}
205205
}
206+
207+
/**
208+
* The button gesture only exists in iOS mode, so these run there.
209+
*/
210+
configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ config, title }) => {
211+
test.describe(title('alert: moved while presented'), () => {
212+
test.beforeEach(async ({ page }) => {
213+
await page.goto('/src/components/alert/test/basic', config);
214+
});
215+
216+
test('should keep the app root locked', async ({ page }, testInfo) => {
217+
testInfo.annotations.push({
218+
type: 'issue',
219+
description: 'https://github.com/ionic-team/ionic-framework/issues/31389',
220+
});
221+
222+
const ionAlertDidPresent = await page.spyOnEvent('ionAlertDidPresent');
223+
224+
await page.click('#basic');
225+
await ionAlertDidPresent.next();
226+
227+
await expect(page.locator('body')).toHaveClass(/backdrop-no-scroll/);
228+
229+
await detachAndReattach(page.locator('ion-alert'));
230+
231+
await expect(page.locator('body')).toHaveClass(/backdrop-no-scroll/);
232+
});
233+
234+
test('should keep activating buttons on press', async ({ page }, testInfo) => {
235+
testInfo.annotations.push({
236+
type: 'issue',
237+
description: 'https://github.com/ionic-team/ionic-framework/issues/31389',
238+
});
239+
240+
const ionAlertDidPresent = await page.spyOnEvent('ionAlertDidPresent');
241+
242+
await page.click('#multipleButtons');
243+
await ionAlertDidPresent.next();
244+
245+
await detachAndReattach(page.locator('ion-alert'));
246+
247+
const button = page.locator('ion-alert .alert-button').first();
248+
const box = (await button.boundingBox())!;
249+
250+
await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2);
251+
await page.mouse.down();
252+
253+
await expect(button).toHaveClass(/ion-activated/);
254+
255+
await page.mouse.up();
256+
});
257+
});
258+
});

core/src/components/loading/loading.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
eventMethod,
1313
prepareOverlay,
1414
present,
15+
restoreRootFocusTrapAccessibility,
1516
setOverlayId,
1617
} from '@utils/overlays';
1718
import { sanitizeDOMString } from '@utils/sanitization';
@@ -208,6 +209,11 @@ export class Loading implements ComponentInterface, OverlayInterface {
208209
connectedCallback() {
209210
prepareOverlay(this.el);
210211
this.triggerChanged();
212+
213+
// Re-apply the root lock if moved without dismiss() being called
214+
if (this.presented) {
215+
restoreRootFocusTrapAccessibility(this.el);
216+
}
211217
}
212218

213219
componentWillLoad() {

core/src/components/loading/test/basic/loading.e2e.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { expect } from '@playwright/test';
22
import type { E2EPage, ScreenshotFn } from '@utils/test/playwright';
3-
import { configs, test } from '@utils/test/playwright';
3+
import { configs, detachAndReattach, test } from '@utils/test/playwright';
44

55
const runVisualTest = async (page: E2EPage, selector: string, screenshot: ScreenshotFn, screenshotModifier: string) => {
66
const ionLoadingDidPresent = await page.spyOnEvent('ionLoadingDidPresent');
@@ -100,3 +100,30 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, c
100100
});
101101
});
102102
});
103+
104+
/**
105+
* This behavior does not vary across modes/directions.
106+
*/
107+
configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ config, title }) => {
108+
test.describe(title('loading: moved while presented'), () => {
109+
test('should keep the app root locked', async ({ page }, testInfo) => {
110+
testInfo.annotations.push({
111+
type: 'issue',
112+
description: 'https://github.com/ionic-team/ionic-framework/issues/31389',
113+
});
114+
115+
await page.goto('/src/components/loading/test/basic', config);
116+
const ionLoadingDidPresent = await page.spyOnEvent('ionLoadingDidPresent');
117+
118+
// This one carries no duration, so it can't auto-dismiss mid-move.
119+
await page.click('#backdrop-loading');
120+
await ionLoadingDidPresent.next();
121+
122+
await expect(page.locator('body')).toHaveClass(/backdrop-no-scroll/);
123+
124+
await detachAndReattach(page.locator('ion-loading'));
125+
126+
await expect(page.locator('body')).toHaveClass(/backdrop-no-scroll/);
127+
});
128+
});
129+
});

0 commit comments

Comments
 (0)