Repository: melt-ui/melt-ui
Pull request #1110 (2024-03-20 12:35:15)

fix(dialog,popover): restore focus when `open` set to `false`
closes https://github.com/melt-ui/melt-ui/issues/1109

We now restore focus to the trigger (or using the `closeFocus` prop) in an `effect` that runs when `open` is `false`. In order to not focus the trigger on initial mount when `open` is `false`, I added a parameter `skipFirstRun` to the `effect` function that optionally skips the first run of the effect.

I also added regression tests for the dialog and popover. 

Also, in the menu builder, we are already doing what I implemented with the dialog and popover, which removes the need in the menu builder to manually focus the trigger in multiple places. And I cleaned up the effect in the menu component. 

### Dialog Before
https://github.com/melt-ui/melt-ui/assets/53095479/584dd6ad-bd39-4901-9ff1-2a0e987f1a2e


### Dialog After 
https://github.com/melt-ui/melt-ui/assets/53095479/ac7ac1d1-f02e-4b2a-8f48-040438149a43

### Popover Before


https://github.com/melt-ui/melt-ui/assets/53095479/89b2dc56-e1a8-47e3-9ce3-d4c42fbe41c3

### Popover After

https://github.com/melt-ui/melt-ui/assets/53095479/fa3943a1-35d1-417b-afe9-cb4f5650b69d



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

Latest commit: 283124e48de25c84e0708563bf50d6c36094c133

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/restoring-focus-on-state-change?filename=.changeset/rich-bulldogs-unite.md&value=---%0A%22%40melt-ui%2Fsvelte%22%3A%20patch%0A---%0A%0AFix(dialog%2C%20popover)%20restore%20focus%20when%20manually%20setting%20%60open%60%20to%20%60false%60%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/8359031932)) |  | 5cc5fbef91ac170ba3431ac0908868e5957316fc |

anatolzak: @huntabyte I added an options parameter to `effect` as you requested!

Patch:
diff --git a/.changeset/six-insects-joke.md b/.changeset/six-insects-joke.md
new file mode 100644
index 00000000..9d9b82c4
--- /dev/null
+++ b/.changeset/six-insects-joke.md
@@ -0,0 +1,5 @@
+---
+'@melt-ui/svelte': patch
+---
+
+Fixed a bug where manually closing a dialog or popover would not restore focus to the trigger (closes #1109)
diff --git a/src/lib/builders/dialog/create.ts b/src/lib/builders/dialog/create.ts
index 32f92ae6..9c37e8b6 100644
--- a/src/lib/builders/dialog/create.ts
+++ b/src/lib/builders/dialog/create.ts
@@ -95,10 +95,6 @@ export function createDialog(props?: CreateDialogProps) {
 
 	function handleClose() {
 		open.set(false);
-		handleFocus({
-			prop: closeFocus.get(),
-			defaultEl: activeTrigger.get(),
-		});
 	}
 
 	const trigger = makeElement(name('trigger'), {
@@ -326,6 +322,18 @@ export function createDialog(props?: CreateDialogProps) {
 		};
 	});
 
+	effect(
+		open,
+		($open) => {
+			if (!isBrowser || $open) return;
+			handleFocus({
+				prop: closeFocus.get(),
+				defaultEl: activeTrigger.get(),
+			});
+		},
+		{ skipFirstRun: true }
+	);
+
 	return {
 		ids,
 		elements: {
diff --git a/src/lib/builders/menu/create.ts b/src/lib/builders/menu/create.ts
index 8def7c5c..b8d6cada 100644
--- a/src/lib/builders/menu/create.ts
+++ b/src/lib/builders/menu/create.ts
@@ -211,10 +211,7 @@ export function createMenuBuilder(opts: _MenuBuilderOptions) {
 										}
 										return true;
 									},
-									onClose: () => {
-										rootOpen.set(false);
-										$rootActiveTrigger.focus();
-									},
+									onClose: () => rootOpen.set(false),
 									open: $isVisible,
 								},
 								portal: getPortalDestination(node, $portal),
@@ -357,11 +354,7 @@ export function createMenuBuilder(opts: _MenuBuilderOptions) {
 
 			if (closeOnEscape.get()) {
 				const escapeKeydown = useEscapeKeydown(node, {
-					handler: () => {
-						rootOpen.set(false);
-						const $rootActiveTrigger = rootActiveTrigger.get();
-						if ($rootActiveTrigger) $rootActiveTrigger.focus();
-					},
+					handler: () => rootOpen.set(false),
 				});
 				if (escapeKeydown && escapeKeydown.destroy) {
 					unsubEscapeKeydown = escapeKeydown.destroy;
@@ -1191,18 +1184,14 @@ export function createMenuBuilder(opts: _MenuBuilderOptions) {
 		}
 	});
 
-	effect([rootOpen], ([$rootOpen]) => {
-		if (!isBrowser) return;
-		if (!$rootOpen) {
-			const $rootActiveTrigger = rootActiveTrigger.get();
-			if (!$rootActiveTrigger) return;
-			const $closeFocus = closeFocus.get();
-
-			if (!$rootOpen && $rootActiveTrigger) {
-				handleFocus({ prop: $closeFocus, defaultEl: $rootActiveTrigger });
-			}
-		}
-	});
+	effect(
+		[rootOpen],
+		([$rootOpen]) => {
+			if (!isBrowser || $rootOpen) return;
+			handleFocus({ prop: closeFocus.get(), defaultEl: rootActiveTrigger.get() });
+		},
+		{ skipFirstRun: true }
+	);
 
 	effect([rootOpen, preventScroll], ([$rootOpen, $preventScroll]) => {
 		if (!isBrowser) return;
diff --git a/src/lib/builders/popover/create.ts b/src/lib/builders/popover/create.ts
index b59cd845..37a30d55 100644
--- a/src/lib/builders/popover/create.ts
+++ b/src/lib/builders/popover/create.ts
@@ -90,8 +90,6 @@ export function createPopover(args?: CreatePopoverProps) {
 
 	function handleClose() {
 		open.set(false);
-		const triggerEl = document.getElementById(ids.trigger.get());
-		handleFocus({ prop: closeFocus.get(), defaultEl: triggerEl });
 	}
 
 	const isVisible = derivedVisible({ open, activeTrigger, forceVisible });
@@ -339,6 +337,16 @@ export function createPopover(args?: CreatePopoverProps) {
 		};
 	});
 
+	effect(
+		open,
+		($open) => {
+			if (!isBrowser || $open) return;
+			const triggerEl = document.getElementById(ids.trigger.get());
+			handleFocus({ prop: closeFocus.get(), defaultEl: triggerEl });
+		},
+		{ skipFirstRun: true }
+	);
+
 	return {
 		ids,
 		elements: {
diff --git a/src/lib/internal/helpers/store/effect.ts b/src/lib/internal/helpers/store/effect.ts
index 6f0c52ef..a4666a61 100644
--- a/src/lib/internal/helpers/store/effect.ts
+++ b/src/lib/internal/helpers/store/effect.ts
@@ -3,6 +3,14 @@ import { derived } from 'svelte/store';
 import { noop } from '../index.js';
 import { safeOnDestroy } from '../lifecycle.js';
 
+type EffectOptions = {
+	/**
+	 * Whether to skip the first run
+	 * @default undefined
+	 */
+	skipFirstRun?: boolean;
+};
+
 /**
  * A utility function that creates an effect from a set of stores and a function.
  * The effect is automatically cleaned up when the component is destroyed.
@@ -10,18 +18,26 @@ import { safeOnDestroy } from '../lifecycle.js';
  * @template S - The type of the stores object
  * @param stores - The stores object to derive from
  * @param fn - The function to run when the stores change
+ * @param opts {@link EffectOptions}
  * @returns A function that can be used to unsubscribe the effect
  */
 export function effect<S extends Stores>(
 	stores: S,
-	fn: (values: StoresValues<S>) => (() => void) | void
+	fn: (values: StoresValues<S>) => (() => void) | void,
+	opts: EffectOptions = {}
 ): () => void {
+	const { skipFirstRun } = opts;
+	let isFirstRun = true;
 	let cb: (() => void) | void = undefined;
 
 	// Create a derived store that contains the stores object and an onUnsubscribe function
 	const destroy = derived(stores, (stores) => {
 		cb?.();
-		cb = fn(stores);
+		if (isFirstRun && skipFirstRun) {
+			isFirstRun = false;
+		} else {
+			cb = fn(stores);
+		}
 	}).subscribe(noop);
 
 	const unsub = () => {


Test patch:
diff --git a/src/lib/internal/helpers/tests/effect.spec.ts b/src/lib/internal/helpers/tests/effect.spec.ts
index c4c5520a..03e07966 100644
--- a/src/lib/internal/helpers/tests/effect.spec.ts
+++ b/src/lib/internal/helpers/tests/effect.spec.ts
@@ -51,4 +51,21 @@ describe('effect', () => {
 		unsub();
 		expect(calls).toBe(1);
 	});
+
+	it('Respect `skipFirstRun` prop', async () => {
+		const w = writable(1);
+		let calls = 0;
+		const unsub = effect(
+			w,
+			() => {
+				calls++;
+			},
+			{ skipFirstRun: true }
+		);
+		expect(calls).toBe(0);
+
+		w.set(2);
+		expect(calls).toBe(1);
+		unsub();
+	});
 });
diff --git a/src/tests/dialog/Dialog.spec.ts b/src/tests/dialog/Dialog.spec.ts
index 7242c9d8..e06be268 100644
--- a/src/tests/dialog/Dialog.spec.ts
+++ b/src/tests/dialog/Dialog.spec.ts
@@ -254,4 +254,18 @@ describe('Dialog', () => {
 		await user.tab({ shift: true });
 		expect(closer).not.toHaveFocus();
 	});
+
+	it('Returns focus to trigger when manually setting `open` state to false', async () => {
+		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 } = await open({
+			closeFocus: () => document.getElementById('closeFocus'),
+		});
+		await user.click(getByTestId('toggle-open'));
+		expect(getByTestId('closeFocus')).toHaveFocus();
+	});
 });
diff --git a/src/tests/dialog/DialogTest.svelte b/src/tests/dialog/DialogTest.svelte
index 60d8e2ea..16218385 100644
--- a/src/tests/dialog/DialogTest.svelte
+++ b/src/tests/dialog/DialogTest.svelte
@@ -20,6 +20,7 @@
 		<div use:melt={$content} data-testid="content">
 			<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"
diff --git a/src/tests/popover/Popover.spec.ts b/src/tests/popover/Popover.spec.ts
index 7d1f5664..7c15d4bf 100644
--- a/src/tests/popover/Popover.spec.ts
+++ b/src/tests/popover/Popover.spec.ts
@@ -164,4 +164,28 @@ describe('Popover (Default)', () => {
 		await user.tab();
 		expect(getByTestId('closeFocus')).not.toHaveFocus();
 	});
+
+	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();
+		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({
+			closeFocus: () => document.getElementById('closeFocus'),
+		});
+		const content = getByTestId('content');
+
+		expect(trigger).not.toHaveFocus();
+		await user.click(trigger);
+		expect(content).toBeVisible();
+		await user.click(getByTestId('toggle-open'));
+		expect(getByTestId('closeFocus')).toHaveFocus();
+	});
 });
diff --git a/src/tests/popover/PopoverTest.svelte b/src/tests/popover/PopoverTest.svelte
index 0b85c0a8..f8b912c4 100644
--- a/src/tests/popover/PopoverTest.svelte
+++ b/src/tests/popover/PopoverTest.svelte
@@ -8,6 +8,7 @@
 
 	const {
 		elements: { trigger, content, arrow, close },
+		states: { open },
 	} = createPopover({
 		openFocus,
 		closeFocus,
@@ -48,6 +49,7 @@
 			<label for="weight">Weight</label>
 			<input type="number" id="weight" class="input" placeholder="Weight" data-testid="input4" />
 		</fieldset>
+		<button on:click={() => open.update((p) => !p)} data-testid="toggle-open">toggle open</button>
 	</div>
 	<button class="close" use:melt={$close} data-testid="close">
 		<X class="h-4 w-4 " />