Repository: melt-ui/melt-ui
Pull request #1094 (2024-03-14 18:31:47)

fix(dialog,popover):focus trap `closeOnOutsideClick: false` bug
closes https://github.com/melt-ui/melt-ui/issues/1084

Focus trap should not deactivate on click outside if the prop `closeOnOutsideClick: false` is passed. To make sure that this issue https://github.com/melt-ui/melt-ui/issues/452 doesn't occur, we need to allow outside clicks in the focus trap--this doesn't affect the content getting closes.

### Popover Before with `closeOnOutsideClick: false`

https://github.com/melt-ui/melt-ui/assets/53095479/f4e9d207-dc24-455a-93ed-1c0cee4baa9f



### Popover After

https://github.com/melt-ui/melt-ui/assets/53095479/3bc5c44d-9810-484b-8456-792eb9754fa5


### Dialog Before with `closeOnOutsideClick: false`

https://github.com/melt-ui/melt-ui/assets/53095479/55c63c71-d823-4d4f-bc7f-6ea719cf412b


### Dialog After

https://github.com/melt-ui/melt-ui/assets/53095479/d85f6682-aa51-4efb-a1c6-40b831e96471



changeset-bot[bot]: ###  ⚠️  No Changeset found

Latest commit: 95b3bed2a89f19d3b17f19c6cff5de8c366b2363

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. **If these changes should result in a version bump, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/main/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/anatolzak/melt-ui/new/fix/focus-trap-deactivates-on-click-outside?filename=.changeset/gold-eggs-sing.md&value=---%0A%22%40melt-ui%2Fsvelte%22%3A%20patch%0A---%0A%0Afix(dialog%2C%20popover)%3A%20focus%20trap%20deactivates%20on%20click%20outside%20provided%20%60closeOnOutsideClick%3A%20false%60%0A)


TGlide: Merge conflicts :)
anatolzak: just did @TGlide  :)
huntabyte: Do you think you could add a regression test? One that fails without the changes in this PR but passes with it.
anatolzak: @huntabyte I tried to do it and gave up lol but I will get it done!
huntabyte: > @huntabyte I tried to do it and gave up lol but I will get it done!

Ah yeah focus states can be a bit of a PITA - have to slap a bunch of `sleep`s and/or `waitFor`s in there but don't beat yourself up about it if you already gave it a fair shot. I can verify from the videos, it just seems like something easy to regress on without a test.
anatolzak: @huntabyte added the tests! 
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/8309314171)) |  | a81684f7fe900e376a1927d5ce453d58b55eeb93 |


Patch:
diff --git a/.changeset/fifty-pigs-promise.md b/.changeset/fifty-pigs-promise.md
new file mode 100644
index 00000000..c749be3b
--- /dev/null
+++ b/.changeset/fifty-pigs-promise.md
@@ -0,0 +1,5 @@
+---
+'@melt-ui/svelte': patch
+---
+
+Fixed bug where focus trap deactivates in dialog and popover when clicking outside provided `closeOnOutsideClick: false` (closes #1084)
diff --git a/src/lib/builders/dialog/create.ts b/src/lib/builders/dialog/create.ts
index 0d85213d..a4271407 100644
--- a/src/lib/builders/dialog/create.ts
+++ b/src/lib/builders/dialog/create.ts
@@ -186,26 +186,30 @@ export function createDialog(props?: CreateDialogProps) {
 			let deactivate = noop;
 
 			const destroy = executeCallbacks(
-				effect([open, closeOnEscape], ([$open, $closeOnEscape]) => {
-					if (!$open) return;
-
-					const focusTrap = createFocusTrap({
-						immediate: false,
-						escapeDeactivates: $closeOnEscape,
-						clickOutsideDeactivates: 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, 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([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 c8f077b4..e1a51f29 100644
--- a/src/lib/builders/popover/create.ts
+++ b/src/lib/builders/popover/create.ts
@@ -146,7 +146,8 @@ export function createPopover(args?: CreatePopoverProps) {
 									? null
 									: {
 											returnFocusOnDeactivate: false,
-											clickOutsideDeactivates: true,
+											clickOutsideDeactivates: $closeOnOutsideClick,
+											allowOutsideClick: true,
 											escapeDeactivates: $closeOnEscape,
 									  },
 								modal: {


Test patch:
diff --git a/src/tests/dialog/Dialog.spec.ts b/src/tests/dialog/Dialog.spec.ts
index c9dedae1..1e23d027 100644
--- a/src/tests/dialog/Dialog.spec.ts
+++ b/src/tests/dialog/Dialog.spec.ts
@@ -277,4 +277,19 @@ describe('Dialog', () => {
 		await user.pointer({ target: overlay, offset: 2, keys: '[/MouseLeft]' });
 		expect(content).not.toBeVisible();
 	});
+
+	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();
+		await user.click(getByTestId('overlay'));
+		expect(content).toBeVisible();
+		expect(getByTestId('content')).toHaveFocus();
+		await user.tab({ shift: true });
+		expect(getByTestId('floating-closer')).not.toHaveFocus();
+	});
 });
diff --git a/src/tests/popover/Popover.spec.ts b/src/tests/popover/Popover.spec.ts
index 676c4f24..b091a154 100644
--- a/src/tests/popover/Popover.spec.ts
+++ b/src/tests/popover/Popover.spec.ts
@@ -135,4 +135,18 @@ describe('Popover (Default)', () => {
 		expect(trigger.id).toBe(ids.trigger);
 		expect(content.id).toBe(ids.content);
 	});
+
+	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();
+		await user.click(getByTestId('outside'));
+		expect(content).toBeVisible();
+		await user.tab();
+		expect(getByTestId('closeFocus')).not.toHaveFocus();
+	});
 });