Repository: melt-ui/melt-ui
Pull request #1135 (2024-03-29 10:59:39)

Fix(focus trap): trap deactivation intercepted interact outside
closes https://github.com/melt-ui/melt-ui/issues/1134

We should not let the focus trap package deactivate the focus trap on an outside interaction 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 outside interaction but rather when the dialog or popover unmount which is already implemented in the destroy method of the focus trap action.

### Before

https://github.com/melt-ui/melt-ui/assets/53095479/3445b6eb-df44-4569-9ccc-a08d733b3fb6


### After


https://github.com/melt-ui/melt-ui/assets/53095479/6cb1fe7d-019c-4fde-b7c8-d71befb704ac



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

Latest commit: 72d0fcc682f8cbd862b9b8d96146800877b20eb1

**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-intercepted-interact-outside?filename=.changeset/great-frogs-flow.md&value=---%0A%22%40melt-ui%2Fsvelte%22%3A%20patch%0A---%0A%0AFix(focus%20trap)%20focus%20trap%20deactivates%20on%20an%20intercepted%20interact%20outside%0A)


github-actions[bot]: <!-- refined-cf-pages-action:deployment-summary:melt-ui -->

###### built with [Refined Cloudflare Pages Action](https://github.com/AdrianGonz97/refined-cf-pages-action)
## ⚡ Cloudflare Pages Deployment
| Name | Status | Preview | Last Commit |
| :--- | :----- | :------ | :---------- |
| **melt-ui** | 🔨 Building ([View Log](https://github.com/melt-ui/melt-ui/actions/runs/8480209130)) |  | 72d0fcc682f8cbd862b9b8d96146800877b20eb1 |


Patch:
diff --git a/.changeset/good-pants-crash.md b/.changeset/good-pants-crash.md
new file mode 100644
index 00000000..0be0476a
--- /dev/null
+++ b/.changeset/good-pants-crash.md
@@ -0,0 +1,5 @@
+---
+'@melt-ui/svelte': patch
+---
+
+Fixed bug where focus trap would deactivate on an intercepted outside interaction (closes #1134)
diff --git a/src/lib/builders/dialog/create.ts b/src/lib/builders/dialog/create.ts
index 9c37e8b6..18ff6642 100644
--- a/src/lib/builders/dialog/create.ts
+++ b/src/lib/builders/dialog/create.ts
@@ -178,30 +178,26 @@ export function createDialog(props?: CreateDialogProps) {
 			let deactivate = noop;
 
 			const destroy = executeCallbacks(
-				effect(
-					[open, closeOnOutsideClick, closeOnEscape],
-					([$open, $closeOnOutsideClick, $closeOnEscape]) => {
-						if (!$open) return;
-
-						const focusTrap = createFocusTrap({
-							immediate: false,
-							escapeDeactivates: $closeOnEscape,
-							clickOutsideDeactivates: $closeOnOutsideClick,
-							allowOutsideClick: true,
-							returnFocusOnDeactivate: false,
-							fallbackFocus: node,
-						});
-
-						activate = focusTrap.activate;
-						deactivate = focusTrap.deactivate;
-						const ac = focusTrap.useFocusTrap(node);
-						if (ac && ac.destroy) {
-							return ac.destroy;
-						} else {
-							return focusTrap.deactivate;
-						}
+				effect([open, closeOnEscape], ([$open, $closeOnEscape]) => {
+					if (!$open) return;
+
+					const focusTrap = createFocusTrap({
+						immediate: false,
+						escapeDeactivates: $closeOnEscape,
+						allowOutsideClick: true,
+						returnFocusOnDeactivate: false,
+						fallbackFocus: node,
+					});
+
+					activate = focusTrap.activate;
+					deactivate = focusTrap.deactivate;
+					const ac = focusTrap.useFocusTrap(node);
+					if (ac && ac.destroy) {
+						return ac.destroy;
+					} else {
+						return focusTrap.deactivate;
 					}
-				),
+				}),
 				effect([closeOnOutsideClick, open], ([$closeOnOutsideClick, $open]) => {
 					return useModal(node, {
 						open: $open,
diff --git a/src/lib/builders/popover/create.ts b/src/lib/builders/popover/create.ts
index 37a30d55..91e5f382 100644
--- a/src/lib/builders/popover/create.ts
+++ b/src/lib/builders/popover/create.ts
@@ -142,7 +142,6 @@ export function createPopover(args?: CreatePopoverProps) {
 									? null
 									: {
 											returnFocusOnDeactivate: false,
-											clickOutsideDeactivates: $closeOnOutsideClick,
 											allowOutsideClick: true,
 											escapeDeactivates: $closeOnEscape,
 									  },
diff --git a/src/lib/internal/actions/focus-trap/action.ts b/src/lib/internal/actions/focus-trap/action.ts
index 8cd1e7f2..bb955d1f 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, {
+			clickOutsideDeactivates: false,
 			...focusTrapOptions,
 			onActivate() {
 				hasFocus.set(true);
diff --git a/vite.config.ts b/vite.config.ts
index 7df879ab..2186b0ef 100644
--- a/vite.config.ts
+++ b/vite.config.ts
@@ -25,5 +25,6 @@ export default defineConfig({
 			inline: ['clsx'],
 		},
 		retry: 5,
+		allowOnly: !process.env.CI,
 	},
 });


Test patch:
diff --git a/src/tests/dialog/Dialog.spec.ts b/src/tests/dialog/Dialog.spec.ts
index 3103637e..cd75f14c 100644
--- a/src/tests/dialog/Dialog.spec.ts
+++ b/src/tests/dialog/Dialog.spec.ts
@@ -211,9 +211,8 @@ describe('Dialog', () => {
 		expect(description.id).toBe(ids.description);
 	});
 
-	it("Doesn't close on pointerup if the previous pointerdown didn't occur inside the dialog", async () => {
+	it("Doesn't close on pointerup if the previous pointerdown occurred inside the dialog", async () => {
 		const { user, overlay, content } = await open();
-
 		expect(overlay).toBeVisible();
 		await user.pointer({ target: content, offset: 2, keys: '[MouseLeft>]' });
 		await user.pointer({ target: overlay, offset: 2, keys: '[/MouseLeft]' });
@@ -269,6 +268,13 @@ describe('Dialog', () => {
 		expect(getByTestId('closeFocus')).toHaveFocus();
 	});
 
+	it("Doesn't deactivate focus trap on outside click that is intercepted", async () => {
+		const { getByTestId, user, trigger } = await open();
+		await user.click(getByTestId('click-interceptor'));
+		await user.tab({ shift: true });
+		expect(trigger).not.toHaveFocus();
+	});
+
 	describe('Mouse Device', () => {
 		it("Doesn't close when interacting outside with click interceptor", async () => {
 			const { getByTestId, user, content, overlay } = await open();
diff --git a/src/tests/popover/Popover.spec.ts b/src/tests/popover/Popover.spec.ts
index 7c15d4bf..7c25970e 100644
--- a/src/tests/popover/Popover.spec.ts
+++ b/src/tests/popover/Popover.spec.ts
@@ -19,6 +19,15 @@ function setup(props: CreatePopoverProps = {}) {
 	};
 }
 
+const open = async (props: CreatePopoverProps = {}) => {
+	const returned = setup(props);
+	const { user, trigger, content } = returned;
+	expect(content).not.toBeVisible();
+	await user.click(trigger);
+	expect(content).toBeVisible();
+	return returned;
+};
+
 describe('Popover (Default)', () => {
 	test('No accessibility violations', async () => {
 		const { container } = render(PopoverTest);
@@ -27,31 +36,19 @@ describe('Popover (Default)', () => {
 	});
 
 	test('Opens on click', async () => {
-		const { content, trigger, user } = setup();
-
-		expect(content).not.toBeVisible();
-		await user.click(trigger);
-		await waitFor(() => expect(content).toBeVisible());
+		await open();
 	});
 
 	test('Closes on escape', async () => {
-		const { content, trigger, user } = setup();
-
-		expect(content).not.toBeVisible();
-		await user.click(trigger);
-		await waitFor(() => expect(content).toBeVisible());
+		const { content, user } = await open();
 		await user.keyboard(kbd.ESCAPE);
-		await waitFor(() => expect(content).not.toBeVisible());
+		expect(content).not.toBeVisible();
 	});
 
 	test('Closes when click outside content', async () => {
-		const { content, trigger, user, getByTestId } = setup();
-
-		expect(content).not.toBeVisible();
-		await user.click(trigger);
-		await waitFor(() => expect(content).toBeVisible());
+		const { content, user, getByTestId } = await open();
 		await user.click(content);
-		await waitFor(() => expect(content).toBeVisible());
+		expect(content).toBeVisible();
 		const outside = getByTestId('outside');
 		await sleep(100);
 		expect(outside).toBeVisible();
@@ -61,15 +58,9 @@ describe('Popover (Default)', () => {
 	});
 
 	test('doesnt close when preventDefault called in `onOutsideClick`', async () => {
-		const { content, trigger, user, getByTestId } = setup({
-			onOutsideClick: (e) => {
-				e.preventDefault();
-			},
+		const { content, user, getByTestId } = await open({
+			onOutsideClick: (e) => e.preventDefault(),
 		});
-
-		expect(content).not.toBeVisible();
-		await user.click(trigger);
-		await waitFor(() => expect(content).toBeVisible());
 		await user.click(content);
 		await waitFor(() => expect(content).toBeVisible());
 		const outside = getByTestId('outside');
@@ -82,38 +73,24 @@ describe('Popover (Default)', () => {
 	});
 
 	test('Toggles open state on trigger click', async () => {
-		const { content, trigger, user } = setup();
-
-		expect(content).not.toBeVisible();
-		await user.click(trigger);
-		await waitFor(() => expect(content).toBeVisible());
+		const { content, trigger, user } = await open();
 		await user.click(trigger);
 		await waitFor(() => expect(content).not.toBeVisible());
 	});
 
 	test('Respects `openFocus` prop', async () => {
-		const { content, trigger, user, getByTestId } = setup({
-			openFocus: () => {
-				return document.getElementById('openFocus');
-			},
+		const { getByTestId } = await open({
+			openFocus: () => document.getElementById('openFocus'),
 		});
-
-		await user.click(trigger);
-		await waitFor(() => expect(content).toBeVisible());
 		const openFocus = getByTestId('openFocus');
 		await sleep(1);
 		expect(openFocus).toHaveFocus();
 	});
 
 	test('Respects `closeFocus` prop', async () => {
-		const { content, trigger, user, getByTestId } = setup({
-			closeFocus: () => {
-				return document.getElementById('closeFocus');
-			},
+		const { content, user, getByTestId } = await open({
+			closeFocus: () => document.getElementById('closeFocus'),
 		});
-
-		await user.click(trigger);
-		await waitFor(() => expect(content).toBeVisible());
 		await user.keyboard(kbd.ESCAPE);
 		await waitFor(() => expect(content).not.toBeVisible());
 		const closeFocus = getByTestId('closeFocus');
@@ -137,13 +114,7 @@ describe('Popover (Default)', () => {
 	});
 
 	it("Doesn't deactivate focus trap on escape provided `closeOnEscape` false", async () => {
-		const { getByTestId, user, trigger } = setup({
-			closeOnEscape: false,
-		});
-		const content = getByTestId('content');
-		expect(content).not.toBeVisible();
-		await user.click(trigger);
-		expect(content).toBeVisible();
+		const { getByTestId, user, content } = await open({ closeOnEscape: false });
 		await user.keyboard(kbd.ESCAPE);
 		expect(content).toBeVisible();
 		expect(getByTestId('content')).toHaveFocus();
@@ -152,13 +123,7 @@ describe('Popover (Default)', () => {
 	});
 
 	it("Doesn't deactivate focus trap on outside click provided `closeOnOutsideClick` false", async () => {
-		const { getByTestId, user, trigger } = setup({
-			closeOnOutsideClick: false,
-		});
-		const content = getByTestId('content');
-		expect(content).not.toBeVisible();
-		await user.click(trigger);
-		expect(content).toBeVisible();
+		const { getByTestId, user, content } = await open({ closeOnOutsideClick: false });
 		await user.click(getByTestId('outside'));
 		expect(content).toBeVisible();
 		await user.tab();
@@ -166,26 +131,27 @@ describe('Popover (Default)', () => {
 	});
 
 	it('Returns focus to trigger when manually setting `open` state to false', async () => {
-		const { getByTestId, user, trigger } = setup();
-		const content = getByTestId('content');
-
-		expect(trigger).not.toHaveFocus();
-		await user.click(trigger);
-		expect(content).toBeVisible();
+		const { getByTestId, user, trigger } = await open();
 		await user.click(getByTestId('toggle-open'));
 		expect(trigger).toHaveFocus();
 	});
 
 	it('Respects the `closeFocus` prop when manually setting `open` state to false', async () => {
-		const { getByTestId, user, trigger } = setup({
+		const { getByTestId, user } = await open({
 			closeFocus: () => document.getElementById('closeFocus'),
 		});
-		const content = getByTestId('content');
+		await user.click(getByTestId('toggle-open'));
+		expect(getByTestId('closeFocus')).toHaveFocus();
+	});
 
-		expect(trigger).not.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);
 		expect(content).toBeVisible();
-		await user.click(getByTestId('toggle-open'));
-		expect(getByTestId('closeFocus')).toHaveFocus();
+		await user.click(getByTestId('click-interceptor'));
+		await user.tab({ shift: true });
+		expect(content).toBeVisible();
+		expect(trigger).not.toHaveFocus();
 	});
 });
diff --git a/src/tests/popover/PopoverTest.svelte b/src/tests/popover/PopoverTest.svelte
index f8b912c4..2b92d8b1 100644
--- a/src/tests/popover/PopoverTest.svelte
+++ b/src/tests/popover/PopoverTest.svelte
@@ -57,6 +57,7 @@
 	<button data-testid="openFocus" id="openFocus"> focus me on open </button>
 </div>
 <div data-testid="outside" />
+<button on:click|stopPropagation data-testid="click-interceptor">click interceptor</button>
 
 <style lang="postcss">
 	fieldset {