Repository: melt-ui/melt-ui
Pull request #1133 (2024-03-29 10:16:41)

fix(focus trap): focus trap deactivates on intercepted escape
closes https://github.com/melt-ui/melt-ui/issues/1132

We should not let the focus trap package deactivate the focus trap on an escape key press because they use capture events which will fire even if an element intercepted the event. Therefore we should never let the focus trap package deactivate based on an escape key press but rather when the dialog or popover unmount which is already implemented in the destroy method of the focus trap action.

### Before - I added an escape key interceptor on the dialog content

https://github.com/melt-ui/melt-ui/assets/53095479/5dccb797-25fb-420b-afd6-13ad5272fac2

### After

https://github.com/melt-ui/melt-ui/assets/53095479/9520d91e-7ff2-46b5-af8b-f5532460261e



changeset-bot[bot]: ###  🦋  Changeset detected

Latest commit: 18b79665391fcbc24226ff2c94c1a32e22dc2c49

**The changes in this PR will be included in the next version bump.**

<details><summary>This PR includes changesets to release 1 package</summary>

  | Name            | Type  |
| --------------- | ----- |
| @melt-ui/svelte | Patch |

</details>

Not sure what this means? [Click here  to learn what changesets are](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/anatolzak/melt-ui/new/fix/focus-trap-deactivates-on-escape-when-intercepted?filename=.changeset/thirty-books-invite.md&value=---%0A%22%40melt-ui%2Fsvelte%22%3A%20patch%0A---%0A%0Afix(focus%20trap)%3A%20focus%20trap%20deactivates%20on%20escape%20that%20was%20intercepted%0A)


anatolzak: @huntabyte I resolved the conflicts :)

Patch:
diff --git a/.changeset/hungry-steaks-whisper.md b/.changeset/hungry-steaks-whisper.md
new file mode 100644
index 00000000..590b8a34
--- /dev/null
+++ b/.changeset/hungry-steaks-whisper.md
@@ -0,0 +1,5 @@
+---
+'@melt-ui/svelte': patch
+---
+
+Fixed bug where focus trap would deactivate on escape that was intercepted (closes #1132)
diff --git a/src/lib/builders/dialog/create.ts b/src/lib/builders/dialog/create.ts
index 18ff6642..44a1654f 100644
--- a/src/lib/builders/dialog/create.ts
+++ b/src/lib/builders/dialog/create.ts
@@ -178,12 +178,11 @@ export function createDialog(props?: CreateDialogProps) {
 			let deactivate = noop;
 
 			const destroy = executeCallbacks(
-				effect([open, closeOnEscape], ([$open, $closeOnEscape]) => {
+				effect([open], ([$open]) => {
 					if (!$open) return;
 
 					const focusTrap = createFocusTrap({
 						immediate: false,
-						escapeDeactivates: $closeOnEscape,
 						allowOutsideClick: true,
 						returnFocusOnDeactivate: false,
 						fallbackFocus: node,
diff --git a/src/lib/builders/popover/create.ts b/src/lib/builders/popover/create.ts
index 91e5f382..d48a79c3 100644
--- a/src/lib/builders/popover/create.ts
+++ b/src/lib/builders/popover/create.ts
@@ -143,7 +143,6 @@ export function createPopover(args?: CreatePopoverProps) {
 									: {
 											returnFocusOnDeactivate: false,
 											allowOutsideClick: true,
-											escapeDeactivates: $closeOnEscape,
 									  },
 								modal: {
 									shouldCloseOnInteractOutside: shouldCloseOnInteractOutside,
diff --git a/src/lib/internal/actions/focus-trap/action.ts b/src/lib/internal/actions/focus-trap/action.ts
index bb955d1f..ada4eec8 100644
--- a/src/lib/internal/actions/focus-trap/action.ts
+++ b/src/lib/internal/actions/focus-trap/action.ts
@@ -35,6 +35,7 @@ export function createFocusTrap(config: FocusTrapConfig = {}) {
 
 	const useFocusTrap = (node: HTMLElement) => {
 		trap = _createFocusTrap(node, {
+			escapeDeactivates: false,
 			clickOutsideDeactivates: false,
 			...focusTrapOptions,
 			onActivate() {
diff --git a/src/lib/internal/actions/popper/action.ts b/src/lib/internal/actions/popper/action.ts
index 1b692c76..aece77c3 100644
--- a/src/lib/internal/actions/popper/action.ts
+++ b/src/lib/internal/actions/popper/action.ts
@@ -42,7 +42,6 @@ export const usePopper = ((popperElement, args) => {
 	if (opts.focusTrap !== null) {
 		const { useFocusTrap } = createFocusTrap({
 			immediate: true,
-			escapeDeactivates: false,
 			allowOutsideClick: true,
 			returnFocusOnDeactivate: false,
 			fallbackFocus: popperElement,


Test patch:
diff --git a/src/tests/dialog/Dialog.spec.ts b/src/tests/dialog/Dialog.spec.ts
index cd75f14c..5f11acdb 100644
--- a/src/tests/dialog/Dialog.spec.ts
+++ b/src/tests/dialog/Dialog.spec.ts
@@ -4,7 +4,7 @@ import { describe, it } from 'vitest';
 import DialogTest from './DialogTest.svelte';
 import { userEvent } from '@testing-library/user-event';
 import { sleep } from '$lib/internal/helpers/index.js';
-import { testKbd as kbd, touch } from '../utils.js';
+import { assertActiveFocusTrap, testKbd as kbd, touch } from '../utils.js';
 import type { CreateDialogProps } from '$lib/index.js';
 
 function setup(props: CreateDialogProps = {}) {
@@ -186,10 +186,8 @@ describe('Dialog', () => {
 	});
 
 	it("Doesn't close on escape if child intercepts event", async () => {
-		const { getByTestId, user, content } = await open();
-
-		const input = getByTestId('input-keydown-interceptor');
-		input.focus();
+		const { getByTestId, content, user } = await open();
+		getByTestId('escape-interceptor').focus();
 		await user.keyboard(kbd.ESCAPE);
 		expect(content).toBeVisible();
 	});
@@ -229,16 +227,10 @@ describe('Dialog', () => {
 	});
 
 	it("Doesn't deactivate focus trap on escape provided `closeOnEscape` false", async () => {
-		const { getByTestId, user, content } = await open({
-			closeOnEscape: false,
-		});
-		const closer = getByTestId('floating-closer');
-
+		const { user, content } = await open({ closeOnEscape: false });
 		await user.keyboard(kbd.ESCAPE);
 		expect(content).toBeVisible();
-		expect(content).toHaveFocus();
-		await user.tab({ shift: true });
-		expect(closer).not.toHaveFocus();
+		await assertActiveFocusTrap(user, content);
 	});
 
 	it("Doesn't deactivate focus trap on outside click provided `closeOnOutsideClick` false", async () => {
@@ -268,11 +260,18 @@ describe('Dialog', () => {
 		expect(getByTestId('closeFocus')).toHaveFocus();
 	});
 
+	it("Doesn't deactivate focus trap on escape that is intercepted", async () => {
+		const { getByTestId, user, content } = await open();
+		getByTestId('escape-interceptor').focus();
+		await user.keyboard(kbd.ESCAPE);
+		expect(content).toBeVisible();
+		await assertActiveFocusTrap(user, content);
+	});
+
 	it("Doesn't deactivate focus trap on outside click that is intercepted", async () => {
-		const { getByTestId, user, trigger } = await open();
+		const { getByTestId, user, content } = await open();
 		await user.click(getByTestId('click-interceptor'));
-		await user.tab({ shift: true });
-		expect(trigger).not.toHaveFocus();
+		await assertActiveFocusTrap(user, content);
 	});
 
 	describe('Mouse Device', () => {
diff --git a/src/tests/dialog/DialogTest.svelte b/src/tests/dialog/DialogTest.svelte
index f406ab11..7b3afa7b 100644
--- a/src/tests/dialog/DialogTest.svelte
+++ b/src/tests/dialog/DialogTest.svelte
@@ -21,11 +21,12 @@
 			<h2 use:melt={$title} data-testid="title">Title</h2>
 			<p use:melt={$description} data-testid="description">Description</p>
 			<button on:click={() => open.update((p) => !p)} data-testid="toggle-open">toggle open</button>
-			<input
-				data-testid="input-keydown-interceptor"
-				type="text"
+			<button
+				data-testid="escape-interceptor"
 				on:keydown={(e) => e.key === kbd.ESCAPE && e.stopPropagation()}
-			/>
+			>
+				escape interceptor
+			</button>
 
 			<button use:melt={$close} data-testid="closer">Close</button>
 			<button use:melt={$close} data-testid="last">Close</button>
diff --git a/src/tests/popover/Popover.spec.ts b/src/tests/popover/Popover.spec.ts
index 7c25970e..ea3068f8 100644
--- a/src/tests/popover/Popover.spec.ts
+++ b/src/tests/popover/Popover.spec.ts
@@ -3,7 +3,7 @@ import { axe } from 'jest-axe';
 import PopoverTest from './PopoverTest.svelte';
 import { userEvent } from '@testing-library/user-event';
 import type { CreatePopoverProps } from '$lib/index.js';
-import { testKbd as kbd } from '../utils.js';
+import { assertActiveFocusTrap, testKbd as kbd } from '../utils.js';
 import { sleep } from '$lib/internal/helpers/index.js';
 
 function setup(props: CreatePopoverProps = {}) {
@@ -144,14 +144,18 @@ describe('Popover (Default)', () => {
 		expect(getByTestId('closeFocus')).toHaveFocus();
 	});
 
-	it("Doesn't deactivate focus trap on outside click that is intercepted", async () => {
-		const { getByTestId, user, trigger } = setup();
-		const content = getByTestId('content');
-		await user.click(trigger);
+	it("Doesn't deactivate focus trap on escape that is intercepted", async () => {
+		const { getByTestId, user, content } = await open();
+		getByTestId('escape-interceptor').focus();
+		await user.keyboard(kbd.ESCAPE);
 		expect(content).toBeVisible();
+		await assertActiveFocusTrap(user, content);
+	});
+
+	it("Doesn't deactivate focus trap on outside click that is intercepted", async () => {
+		const { getByTestId, user, content } = await open();
 		await user.click(getByTestId('click-interceptor'));
-		await user.tab({ shift: true });
 		expect(content).toBeVisible();
-		expect(trigger).not.toHaveFocus();
+		await assertActiveFocusTrap(user, content);
 	});
 });
diff --git a/src/tests/popover/PopoverTest.svelte b/src/tests/popover/PopoverTest.svelte
index 2b92d8b1..c5d9a97f 100644
--- a/src/tests/popover/PopoverTest.svelte
+++ b/src/tests/popover/PopoverTest.svelte
@@ -1,6 +1,7 @@
 <script lang="ts">
 	import { createPopover, melt, type CreatePopoverProps } from '$lib/index.js';
 	import { Settings2, X } from '$icons/index.js';
+	import { kbd } from '$lib/internal/helpers/keyboard.js';
 
 	export let openFocus: CreatePopoverProps['openFocus'] = undefined;
 	export let closeFocus: CreatePopoverProps['closeFocus'] = undefined;
@@ -49,6 +50,12 @@
 			<label for="weight">Weight</label>
 			<input type="number" id="weight" class="input" placeholder="Weight" data-testid="input4" />
 		</fieldset>
+		<button
+			on:keydown={(e) => e.key === kbd.ESCAPE && e.stopPropagation()}
+			data-testid="escape-interceptor"
+		>
+			escape interceptor
+		</button>
 		<button on:click={() => open.update((p) => !p)} data-testid="toggle-open">toggle open</button>
 	</div>
 	<button class="close" use:melt={$close} data-testid="close">
diff --git a/src/tests/utils.ts b/src/tests/utils.ts
index c516d5c3..26c254df 100644
--- a/src/tests/utils.ts
+++ b/src/tests/utils.ts
@@ -1,5 +1,6 @@
 import { kbd, removeUndefined } from '$lib/internal/helpers/index.js';
 import { fireEvent } from '@testing-library/svelte';
+import type { UserEvent } from '@testing-library/user-event';
 export { removeUndefined };
 type KbdKeys = keyof typeof kbd;
 /**
@@ -33,3 +34,19 @@ export async function touch(node: HTMLElement) {
 	await fireEvent(node, new MouseEvent('mouseup', { bubbles: true }));
 	await fireEvent(node, new MouseEvent('click', { bubbles: true }));
 }
+
+/**
+ * Simulates a specified number of tab key presses to check if the focus remains
+ * within the given element, thus determining if a focus trap is active. The `tabsPresses` parameter
+ * allows customization of how many tab key presses to simulate, with a default value of 10.
+ */
+export const assertActiveFocusTrap = async (
+	user: UserEvent,
+	element: HTMLElement,
+	tabsPresses = 10
+) => {
+	for (let i = 0; i < tabsPresses; i++) {
+		await user.tab();
+		expect(element).toContainElement(document.activeElement as HTMLElement);
+	}
+};