Skip to content

Commit c3e3951

Browse files
committed
fix(overlays): honor string focusTrap config
1 parent 92db364 commit c3e3951

File tree

7 files changed

+137
-17
lines changed

7 files changed

+137
-17
lines changed

core/src/components/modal/gestures/sheet.ts

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,11 @@ export const createSheetGesture = (
9898
// Respect explicit opt-out of focus trapping/backdrop interactions
9999
// If focusTrap is false or showBackdrop is false, do not enable the backdrop or re-enable focus trap
100100
const el = baseEl as HTMLIonModalElement & { focusTrap?: boolean; showBackdrop?: boolean };
101-
if (el.focusTrap === false || el.showBackdrop === false) {
101+
const focusTrapAttr = el.getAttribute?.('focus-trap');
102+
const showBackdropAttr = el.getAttribute?.('show-backdrop');
103+
const focusTrapDisabled = el.focusTrap === false || focusTrapAttr === 'false';
104+
const backdropDisabled = el.showBackdrop === false || showBackdropAttr === 'false';
105+
if (focusTrapDisabled || backdropDisabled) {
102106
return;
103107
}
104108
baseEl.style.setProperty('pointer-events', 'auto');
@@ -241,10 +245,12 @@ export const createSheetGesture = (
241245
* ion-backdrop and .modal-wrapper always have pointer-events: auto
242246
* applied, so the modal content can still be interacted with.
243247
*/
244-
const shouldEnableBackdrop =
245-
currentBreakpoint > backdropBreakpoint &&
246-
(baseEl as HTMLIonModalElement & { focusTrap?: boolean }).focusTrap !== false &&
247-
(baseEl as HTMLIonModalElement & { showBackdrop?: boolean }).showBackdrop !== false;
248+
const modalEl = baseEl as HTMLIonModalElement & { focusTrap?: boolean; showBackdrop?: boolean };
249+
const focusTrapAttr = modalEl.getAttribute?.('focus-trap');
250+
const showBackdropAttr = modalEl.getAttribute?.('show-backdrop');
251+
const focusTrapDisabled = modalEl.focusTrap === false || focusTrapAttr === 'false';
252+
const backdropDisabled = modalEl.showBackdrop === false || showBackdropAttr === 'false';
253+
const shouldEnableBackdrop = currentBreakpoint > backdropBreakpoint && !focusTrapDisabled && !backdropDisabled;
248254
if (shouldEnableBackdrop) {
249255
enableBackdrop();
250256
} else {
@@ -591,10 +597,16 @@ export const createSheetGesture = (
591597
* Backdrop should become enabled
592598
* after the backdropBreakpoint value
593599
*/
600+
const modalEl = baseEl as HTMLIonModalElement & {
601+
focusTrap?: boolean;
602+
showBackdrop?: boolean;
603+
};
604+
const focusTrapAttr = modalEl.getAttribute?.('focus-trap');
605+
const showBackdropAttr = modalEl.getAttribute?.('show-backdrop');
606+
const focusTrapDisabled = modalEl.focusTrap === false || focusTrapAttr === 'false';
607+
const backdropDisabled = modalEl.showBackdrop === false || showBackdropAttr === 'false';
594608
const shouldEnableBackdrop =
595-
currentBreakpoint > backdropBreakpoint &&
596-
(baseEl as HTMLIonModalElement & { focusTrap?: boolean }).focusTrap !== false &&
597-
(baseEl as HTMLIonModalElement & { showBackdrop?: boolean }).showBackdrop !== false;
609+
currentBreakpoint > backdropBreakpoint && !focusTrapDisabled && !backdropDisabled;
598610
if (shouldEnableBackdrop) {
599611
enableBackdrop();
600612
} else {

core/src/components/modal/modal.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1237,6 +1237,8 @@ export class Modal implements ComponentInterface, OverlayInterface {
12371237
const isHandleCycle = handleBehavior === 'cycle';
12381238
const isSheetModalWithHandle = isSheetModal && showHandle;
12391239

1240+
const focusTrapAttr = this.el.getAttribute('focus-trap');
1241+
12401242
return (
12411243
<Host
12421244
no-router
@@ -1253,7 +1255,7 @@ export class Modal implements ComponentInterface, OverlayInterface {
12531255
[`modal-sheet`]: isSheetModal,
12541256
[`modal-no-expand-scroll`]: isSheetModal && !expandToScroll,
12551257
'overlay-hidden': true,
1256-
[FOCUS_TRAP_DISABLE_CLASS]: focusTrap === false,
1258+
[FOCUS_TRAP_DISABLE_CLASS]: focusTrap === false || focusTrapAttr === 'false',
12571259
...getClassMap(this.cssClass),
12581260
}}
12591261
onIonBackdropTap={this.onBackdropTap}

core/src/components/modal/test/basic/modal.spec.tsx

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,24 @@ describe('modal: focus trap', () => {
2828

2929
expect(modal.classList.contains(FOCUS_TRAP_DISABLE_CLASS)).toBe(true);
3030
});
31+
32+
it('should set the focus trap class when disabled via string prop', async () => {
33+
const page = await newSpecPage({
34+
components: [Modal],
35+
template: () => <ion-modal overlayIndex={1}></ion-modal>,
36+
});
37+
38+
const modal = page.body.querySelector('ion-modal')! as any;
39+
40+
// Simulate frameworks passing the boolean as a string prop, e.g. focusTrap="false" in Angular
41+
modal.focusTrap = 'false';
42+
await page.waitForChanges();
43+
44+
// This currently fails because the host class only inspects focusTrap === false
45+
// and treats the string "false" as truthy.
46+
expect(modal.classList.contains(FOCUS_TRAP_DISABLE_CLASS)).toBe(true);
47+
});
48+
3149
it('should not set the focus trap class by default', async () => {
3250
const page = await newSpecPage({
3351
components: [Modal],

core/src/components/popover/popover.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -687,6 +687,7 @@ export class Popover implements ComponentInterface, PopoverInterface {
687687
const { onLifecycle, parentPopover, dismissOnSelect, side, arrow, htmlAttributes, focusTrap } = this;
688688
const desktop = isPlatform('desktop');
689689
const enableArrow = arrow && !parentPopover;
690+
const focusTrapAttr = this.el.getAttribute('focus-trap');
690691

691692
return (
692693
<Host
@@ -704,7 +705,7 @@ export class Popover implements ComponentInterface, PopoverInterface {
704705
'overlay-hidden': true,
705706
'popover-desktop': desktop,
706707
[`popover-side-${side}`]: true,
707-
[FOCUS_TRAP_DISABLE_CLASS]: focusTrap === false,
708+
[FOCUS_TRAP_DISABLE_CLASS]: focusTrap === false || focusTrapAttr === 'false',
708709
'popover-nested': !!parentPopover,
709710
}}
710711
onIonPopoverDidPresent={onLifecycle}

core/src/components/popover/test/basic/popover.spec.tsx

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,24 @@ describe('popover: focus trap', () => {
2929

3030
expect(popover.classList.contains(FOCUS_TRAP_DISABLE_CLASS)).toBe(true);
3131
});
32+
33+
it('should set the focus trap class when disabled via string prop', async () => {
34+
const page = await newSpecPage({
35+
components: [Popover],
36+
template: () => <ion-popover overlayIndex={1}></ion-popover>,
37+
});
38+
39+
const popover = page.body.querySelector('ion-popover')! as any;
40+
41+
// Simulate frameworks passing the boolean as a string prop, e.g. focusTrap="false" in Angular
42+
popover.focusTrap = 'false';
43+
await page.waitForChanges();
44+
45+
// This currently fails because the host class only inspects focusTrap === false
46+
// and treats the string "false" as truthy.
47+
expect(popover.classList.contains(FOCUS_TRAP_DISABLE_CLASS)).toBe(true);
48+
});
49+
3250
it('should not set the focus trap class by default', async () => {
3351
const page = await newSpecPage({
3452
components: [Popover],

core/src/utils/overlays.ts

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -539,11 +539,18 @@ export const present = async <OverlayPresentOptions>(
539539
* view container subtree, skip adding aria-hidden/inert there
540540
* to avoid disabling the overlay.
541541
*/
542-
const overlayEl = overlay.el as HTMLIonOverlayElement & { focusTrap?: boolean; showBackdrop?: boolean };
543-
const shouldTrapFocus = overlayEl.tagName !== 'ION-TOAST' && overlayEl.focusTrap !== false;
542+
const overlayEl = overlay.el as HTMLIonOverlayElement & {
543+
focusTrap?: boolean;
544+
showBackdrop?: boolean;
545+
};
546+
const focusTrapAttr = overlayEl.getAttribute?.('focus-trap');
547+
const showBackdropAttr = overlayEl.getAttribute?.('show-backdrop');
548+
const focusTrapDisabled = overlayEl.focusTrap === false || focusTrapAttr === 'false';
549+
const backdropDisabled = overlayEl.showBackdrop === false || showBackdropAttr === 'false';
550+
const shouldTrapFocus = overlayEl.tagName !== 'ION-TOAST' && !focusTrapDisabled;
544551
// Only lock out root content when backdrop is active. Developers relying on showBackdrop=false
545552
// expect background interaction to remain enabled.
546-
const shouldLockRoot = shouldTrapFocus && overlayEl.showBackdrop !== false;
553+
const shouldLockRoot = shouldTrapFocus && !backdropDisabled;
547554

548555
overlay.presented = true;
549556
overlay.willPresent.emit();
@@ -681,11 +688,21 @@ export const dismiss = async <OverlayDismissOptions>(
681688
*/
682689
const overlaysLockingRoot = presentedOverlays.filter((o) => {
683690
const el = o as HTMLIonOverlayElement & { focusTrap?: boolean; showBackdrop?: boolean };
684-
return el.tagName !== 'ION-TOAST' && el.focusTrap !== false && el.showBackdrop !== false;
691+
const focusTrapAttr = el.getAttribute?.('focus-trap');
692+
const showBackdropAttr = el.getAttribute?.('show-backdrop');
693+
const focusTrapDisabled = el.focusTrap === false || focusTrapAttr === 'false';
694+
const backdropDisabled = el.showBackdrop === false || showBackdropAttr === 'false';
695+
return el.tagName !== 'ION-TOAST' && !focusTrapDisabled && !backdropDisabled;
685696
});
686-
const overlayEl = overlay.el as HTMLIonOverlayElement & { focusTrap?: boolean; showBackdrop?: boolean };
687-
const locksRoot =
688-
overlayEl.tagName !== 'ION-TOAST' && overlayEl.focusTrap !== false && overlayEl.showBackdrop !== false;
697+
const overlayEl = overlay.el as HTMLIonOverlayElement & {
698+
focusTrap?: boolean;
699+
showBackdrop?: boolean;
700+
};
701+
const focusTrapAttr = overlayEl.getAttribute?.('focus-trap');
702+
const showBackdropAttr = overlayEl.getAttribute?.('show-backdrop');
703+
const focusTrapDisabled = overlayEl.focusTrap === false || focusTrapAttr === 'false';
704+
const backdropDisabled = overlayEl.showBackdrop === false || showBackdropAttr === 'false';
705+
const locksRoot = overlayEl.tagName !== 'ION-TOAST' && !focusTrapDisabled && !backdropDisabled;
689706

690707
/**
691708
* If this is the last visible overlay that is trapping focus

core/src/utils/test/overlays/overlays-scroll-blocking.spec.ts

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,58 @@ describe('overlays: scroll blocking', () => {
5757
expect(body).not.toHaveClass('backdrop-no-scroll');
5858
});
5959

60+
it('should not block scroll when focus trapping is disabled via string prop', async () => {
61+
const page = await newSpecPage({
62+
components: [Modal],
63+
html: `
64+
<ion-modal></ion-modal>
65+
`,
66+
});
67+
68+
const modal = page.body.querySelector('ion-modal')! as any;
69+
const body = page.doc.querySelector('body')!;
70+
71+
// Simulate frameworks passing the boolean as a string prop, e.g. focusTrap="false" in Angular
72+
modal.focusTrap = 'false';
73+
await page.waitForChanges();
74+
75+
await modal.present();
76+
77+
// With focus trapping disabled, scroll should not be locked.
78+
// This currently fails because overlays.ts only checks for focusTrap !== false
79+
// and treats the string "false" as truthy.
80+
expect(body).not.toHaveClass('backdrop-no-scroll');
81+
82+
await modal.dismiss();
83+
84+
expect(body).not.toHaveClass('backdrop-no-scroll');
85+
});
86+
87+
it('should not block scroll when backdrop is disabled via string prop', async () => {
88+
const page = await newSpecPage({
89+
components: [Modal],
90+
html: `
91+
<ion-modal></ion-modal>
92+
`,
93+
});
94+
95+
const modal = page.body.querySelector('ion-modal')! as any;
96+
const body = page.doc.querySelector('body')!;
97+
98+
// Simulate frameworks passing showBackdrop="false" as a string
99+
modal.showBackdrop = 'false';
100+
await page.waitForChanges();
101+
102+
await modal.present();
103+
104+
// With backdrop disabled, scroll should not be locked.
105+
expect(body).not.toHaveClass('backdrop-no-scroll');
106+
107+
await modal.dismiss();
108+
109+
expect(body).not.toHaveClass('backdrop-no-scroll');
110+
});
111+
60112
it('should not enable scroll until last overlay is dismissed', async () => {
61113
const page = await newSpecPage({
62114
components: [Modal],

0 commit comments

Comments
 (0)