Repository: melt-ui/melt-ui
Pull request #882 (2024-01-05 16:24:54)

fix: menubar `preventScroll`
Related: #821

When using the menubar, you need to decide on a per-menubar basis whether to prevent scroll, rather than on a per-menu basis. Doing so on a per-menu basis causes significant lag and performance hits if one menu in the menubar does preventScroll and another doesn't.

A `preventScroll` prop & option has been added to the `createMenubar` builder, while we have removed the prop from the `createMenu` builder that the menubar builder returns, as it will inherit this prop from the menubar. 


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

Latest commit: 196778a53d4d3f815e2c9341b26c09bc77d96782

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/huntabyte/melt-ui/new/fix/821?filename=.changeset/lemon-shrimps-tan.md&value=---%0A%22%40melt-ui%2Fsvelte%22%3A%20patch%0A---%0A%0Afix%3A%20menubar%20%60preventScroll%60%0A)



Patch:
diff --git a/.changeset/big-walls-yell.md b/.changeset/big-walls-yell.md
new file mode 100644
index 00000000..6fa6a5f3
--- /dev/null
+++ b/.changeset/big-walls-yell.md
@@ -0,0 +1,5 @@
+---
+"@melt-ui/svelte": patch
+---
+
+Radio Group: fix keyboard navigation when disabled (closes #820)
diff --git a/.changeset/giant-buckets-bake.md b/.changeset/giant-buckets-bake.md
new file mode 100644
index 00000000..29cdea95
--- /dev/null
+++ b/.changeset/giant-buckets-bake.md
@@ -0,0 +1,5 @@
+---
+"@melt-ui/svelte": patch
+---
+
+fix: missing tooltip data-attrs
diff --git a/.changeset/lemon-shrimps-tan.md b/.changeset/lemon-shrimps-tan.md
new file mode 100644
index 00000000..7bb5e40f
--- /dev/null
+++ b/.changeset/lemon-shrimps-tan.md
@@ -0,0 +1,5 @@
+---
+"@melt-ui/svelte": minor
+---
+
+Menubar: Add `preventScroll` prop. Removes it from the returned `createMenu` builders (closes #821)
diff --git a/src/docs/data/builders/menubar.ts b/src/docs/data/builders/menubar.ts
index 868122a1..01863b5b 100644
--- a/src/docs/data/builders/menubar.ts
+++ b/src/docs/data/builders/menubar.ts
@@ -7,7 +7,7 @@ import { getMenuSchemas, getMenuTriggerDataAttrs } from './menu.js';
 import { menubarEvents } from '$lib/builders/menubar/events.js';
 import { menubarIdParts } from '$lib';
 
-const OPTION_PROPS = [PROPS.CLOSE_ON_ESCAPE, PROPS.LOOP];
+const OPTION_PROPS = [PROPS.CLOSE_ON_ESCAPE, PROPS.LOOP, PROPS.PREVENT_SCROLL];
 const BUILDER_NAME = 'menubar';
 
 const builder = builderSchema(BUILDER_NAME, {
@@ -23,9 +23,16 @@ const builder = builderSchema(BUILDER_NAME, {
 	options: OPTION_PROPS,
 });
 
+const menubarMenuBuilderProps = dropdownBuilder.props?.filter((p) => p.name !== 'preventScroll');
+const menubarMenuBuilderOptions = dropdownBuilder.options?.filter(
+	(opt) => opt.name !== 'preventScroll'
+);
+
 const menuBuilder = {
 	...dropdownBuilder,
 	title: 'createMenu',
+	props: menubarMenuBuilderProps,
+	options: menubarMenuBuilderOptions,
 };
 const {
 	menu,
diff --git a/src/docs/data/builders/tooltip.ts b/src/docs/data/builders/tooltip.ts
index cdda15c0..6afc60fb 100644
--- a/src/docs/data/builders/tooltip.ts
+++ b/src/docs/data/builders/tooltip.ts
@@ -95,6 +95,10 @@ const trigger = elementSchema('trigger', {
 const content = elementSchema('content', {
 	description: 'The tooltip content element.',
 	dataAttributes: [
+		{
+			name: 'data-state',
+			value: ATTRS.OPEN_CLOSED,
+		},
 		{
 			name: 'data-melt-tooltip-content',
 			value: ATTRS.MELT('tooltip content'),
diff --git a/src/lib/builders/menubar/create.ts b/src/lib/builders/menubar/create.ts
index 22172dbe..6da83a83 100644
--- a/src/lib/builders/menubar/create.ts
+++ b/src/lib/builders/menubar/create.ts
@@ -23,13 +23,13 @@ import {
 	removeScroll,
 	styleToString,
 	toWritableStores,
+	generateIds,
+	omit,
 } from '$lib/internal/helpers/index.js';
 import { safeOnDestroy, safeOnMount } from '$lib/internal/helpers/lifecycle.js';
 import type { MeltActionReturn } from '$lib/internal/types.js';
 import { tick } from 'svelte';
 import { get, writable } from 'svelte/store';
-import { generateIds } from '../../internal/helpers/id';
-import { omit } from '../../internal/helpers/object';
 import {
 	applyAttrsIfDisabled,
 	createMenuBuilder,
@@ -48,6 +48,7 @@ const { name } = createElHelpers<_MenuParts | 'menu'>('menubar');
 const defaults = {
 	loop: true,
 	closeOnEscape: true,
+	preventScroll: false,
 } satisfies CreateMenubarProps;
 
 export const menubarIdParts = ['menubar'] as const;
@@ -57,7 +58,7 @@ export function createMenubar(props?: CreateMenubarProps) {
 	const withDefaults = { ...defaults, ...props } satisfies CreateMenubarProps;
 
 	const options = toWritableStores(omit(withDefaults, 'ids'));
-	const { loop, closeOnEscape } = options;
+	const { loop, closeOnEscape, preventScroll } = options;
 	const activeMenu = writable<string>('');
 
 	const nextFocusable = writable<HTMLElement | null>(null);
@@ -93,7 +94,6 @@ export function createMenubar(props?: CreateMenubarProps) {
 		positioning: {
 			placement: 'bottom-start',
 		},
-		preventScroll: true,
 		arrowSize: 8,
 		dir: 'ltr',
 		loop: false,
@@ -119,7 +119,7 @@ export function createMenubar(props?: CreateMenubarProps) {
 		const { positioning, portal, forceVisible, closeOnOutsideClick, onOutsideClick } = options;
 
 		const m = createMenuBuilder({
-			rootOptions: options,
+			rootOptions: { ...options, preventScroll },
 			rootOpen,
 			rootActiveTrigger,
 			nextFocusable,
@@ -588,8 +588,8 @@ export function createMenubar(props?: CreateMenubarProps) {
 	});
 
 	const unsubs: Array<() => void> = [];
-	effect([activeMenu], ([$activeMenu]) => {
-		if (!isBrowser) return;
+	effect([activeMenu, preventScroll], ([$activeMenu, $preventScroll]) => {
+		if (!isBrowser || !$preventScroll) return;
 
 		/**
 		 * To prevent adding/removing the scroll as we cycle through
diff --git a/src/lib/builders/menubar/types.ts b/src/lib/builders/menubar/types.ts
index 9fd7b415..4e12e494 100644
--- a/src/lib/builders/menubar/types.ts
+++ b/src/lib/builders/menubar/types.ts
@@ -16,16 +16,26 @@ export type CreateMenubarProps = {
 
 	/**
 	 * Whether to close the active menu when the escape key is pressed.
+	 *
+	 * @default true
 	 */
 	closeOnEscape?: boolean;
 
+	/**
+	 * Whether to prevent scrolling the body when any menu within
+	 * the menubar is open.
+	 *
+	 * @default true
+	 */
+	preventScroll?: boolean;
+
 	/**
 	 * Optionally override the default ids we assign to the elements
 	 */
 	ids?: Partial<IdObj<MenubarIdParts>>;
 };
 
-export type CreateMenubarMenuProps = _Menu['builder'];
+export type CreateMenubarMenuProps = Omit<_Menu['builder'], 'preventScroll'>;
 export type CreateMenubarSubmenuProps = _Menu['submenu'];
 export type MenubarMenuItemProps = _Menu['item'];
 export type CreateMenuRadioGroupProps = _Menu['radioGroup'];
diff --git a/src/lib/builders/radio-group/create.ts b/src/lib/builders/radio-group/create.ts
index be95c8b5..1c693bc4 100644
--- a/src/lib/builders/radio-group/create.ts
+++ b/src/lib/builders/radio-group/create.ts
@@ -128,7 +128,7 @@ export function createRadioGroup(props?: CreateRadioGroupProps) {
 					if (!isHTMLElement(root)) return;
 
 					const items = Array.from(root.querySelectorAll(selector('item'))).filter(
-						(el): el is HTMLElement => isHTMLElement(el)
+						(el): el is HTMLElement => isHTMLElement(el) && !el.hasAttribute('data-disabled')
 					);
 					const currentIndex = items.indexOf(el);
 
diff --git a/src/lib/builders/tooltip/create.ts b/src/lib/builders/tooltip/create.ts
index 5dc622a5..e81717c5 100644
--- a/src/lib/builders/tooltip/create.ts
+++ b/src/lib/builders/tooltip/create.ts
@@ -126,12 +126,17 @@ export function createTooltip(props?: CreateTooltipProps) {
 		}
 	}
 
+	const isVisible = derived([open, forceVisible], ([$open, $forceVisible]) => {
+		return $open || $forceVisible;
+	});
+
 	const trigger = builder(name('trigger'), {
-		stores: [ids.content, ids.trigger],
-		returned: ([$contentId, $triggerId]) => {
+		stores: [ids.content, ids.trigger, open],
+		returned: ([$contentId, $triggerId, $open]) => {
 			return {
 				'aria-describedby': $contentId,
 				id: $triggerId,
+				'data-state': $open ? 'open' : 'closed',
 			};
 		},
 		action: (node: HTMLElement): MeltActionReturn<TooltipEvents['trigger']> => {
@@ -183,10 +188,6 @@ export function createTooltip(props?: CreateTooltipProps) {
 		},
 	});
 
-	const isVisible = derived([open, forceVisible], ([$open, $forceVisible]) => {
-		return $open || $forceVisible;
-	});
-
 	const content = builder(name('content'), {
 		stores: [isVisible, portal, ids.content],
 		returned: ([$isVisible, $portal, $contentId]) => {
@@ -199,6 +200,7 @@ export function createTooltip(props?: CreateTooltipProps) {
 				}),
 				id: $contentId,
 				'data-portal': $portal ? '' : undefined,
+				'data-state': $isVisible ? 'open' : 'closed',
 			};
 		},
 		action: (node: HTMLElement): MeltActionReturn<TooltipEvents['content']> => {


Test patch:
diff --git a/src/tests/radio-group/RadioGroup.spec.ts b/src/tests/radio-group/RadioGroup.spec.ts
new file mode 100644
index 00000000..095816fd
--- /dev/null
+++ b/src/tests/radio-group/RadioGroup.spec.ts
@@ -0,0 +1,87 @@
+import { render, waitFor } from '@testing-library/svelte';
+import { axe } from 'jest-axe';
+import { describe, it } from 'vitest';
+import RadioGroupTest from './RadioGroupTest.svelte';
+import userEvent from '@testing-library/user-event';
+import { testKbd as kbd } from '../utils.js';
+import type { CreateRadioGroupProps } from '$lib';
+
+const items = [
+	{
+		value: 'a',
+		disabled: false,
+	},
+	{
+		value: 'b',
+		disabled: false,
+	},
+	{
+		value: 'c',
+		disabled: false,
+	},
+	{
+		value: 'd',
+		disabled: false,
+	},
+];
+
+function setup(
+	props: CreateRadioGroupProps & { items?: { value: string; disabled: boolean }[] } = {}
+) {
+	const user = userEvent.setup();
+	return { ...render(RadioGroupTest, props), user };
+}
+
+describe('Radio Group', () => {
+	it('No accessibility violations', async () => {
+		const { container } = render(RadioGroupTest);
+		expect(await axe(container)).toHaveNoViolations();
+	});
+
+	it('doesnt require a default value', async () => {
+		const { getByTestId } = setup();
+		expect(getByTestId('value')).toHaveTextContent('undefined');
+	});
+
+	it('allows changing value by clicking on a radio button', async () => {
+		const { user, getByTestId } = setup();
+		const a = getByTestId('a');
+		const b = getByTestId('b');
+		const value = getByTestId('value');
+		await user.click(a);
+		expect(value).toHaveTextContent('a');
+		await user.click(b);
+		expect(value).toHaveTextContent('b');
+	});
+
+	it('navigates through the buttons with arrow keys', async () => {
+		const { user, getByTestId } = setup();
+
+		const els = ['a', 'b', 'c', 'd'].map((v) => getByTestId(v));
+
+		await user.click(els[0]);
+		expect(els[0]).toHaveFocus();
+		await user.keyboard(kbd.ARROW_DOWN);
+		expect(els[1]).toHaveFocus();
+		await user.keyboard(kbd.ARROW_DOWN);
+		expect(els[2]).toHaveFocus();
+		await user.keyboard(kbd.ARROW_DOWN);
+		expect(els[3]).toHaveFocus();
+		await user.keyboard(kbd.ARROW_DOWN);
+		expect(els[0]).toHaveFocus();
+	});
+
+	it('skips over disabled buttons', async () => {
+		const { user, getByTestId } = setup({
+			items: items.map((item) => ({ ...item, disabled: item.value === 'b' })),
+		});
+
+		const els = ['a', 'b', 'c', 'd'].map((v) => getByTestId(v));
+
+		await user.click(els[0]);
+		expect(els[0]).toHaveFocus();
+		await user.keyboard(kbd.ARROW_DOWN);
+		expect(els[2]).toHaveFocus();
+		expect(els[1]).not.toHaveFocus();
+	});
+});
diff --git a/src/tests/radio-group/RadioGroupTest.svelte b/src/tests/radio-group/RadioGroupTest.svelte
new file mode 100644
index 00000000..111c1785
--- /dev/null
+++ b/src/tests/radio-group/RadioGroupTest.svelte
@@ -0,0 +1,49 @@
+<script lang="ts">
+	import { createRadioGroup, melt, type CreateRadioGroupProps } from '$lib/index.js';
+	import { removeUndefined } from '../utils.js';
+
+	type $$Props = CreateRadioGroupProps & {
+		items?: Item[];
+	};
+
+	type Item = {
+		value: string;
+		disabled: boolean;
+	};
+
+	export let value: CreateRadioGroupProps['value'] = undefined;
+	export let defaultValue: CreateRadioGroupProps['defaultValue'] = undefined;
+	export let disabled: CreateRadioGroupProps['disabled'] = undefined;
+	export let onValueChange: CreateRadioGroupProps['onValueChange'] = undefined;
+	export let required: CreateRadioGroupProps['required'] = undefined;
+	export let items: Item[] = [
+		{ value: 'a', disabled: false },
+		{ value: 'b', disabled: false },
+		{ value: 'c', disabled: false },
+		{ value: 'd', disabled: false },
+	];
+
+	const {
+		elements: { root, item },
+		states: { value: localValue },
+	} = createRadioGroup({
+		value,
+		defaultValue,
+		onValueChange,
+		disabled,
+		required,
+		...removeUndefined($$restProps),
+	});
+</script>
+
+<main>
+	<div data-testid="value">{$localValue}</div>
+	<form>
+		<label id="radio-group-label" for="radio-group" data-testid="label"> Airplane mode </label>
+		<div use:melt={$root} data-testid="root">
+			{#each items as radioItem}
+				<button use:melt={$item(radioItem)} data-testid={radioItem.value}>{radioItem.value}</button>
+			{/each}
+		</div>
+	</form>
+</main>