From f551c0289458193b41ad26aad3e35f77938fd75e Mon Sep 17 00:00:00 2001 From: Haz Date: Tue, 25 Apr 2023 17:30:12 +0200 Subject: [PATCH 1/5] Fix FormRadio error when not providing composite store --- examples/form-radio/icon.tsx | 26 +++++++ examples/form-radio/index.tsx | 45 ++++++++++++ examples/form-radio/readme.md | 7 ++ examples/form-radio/style.css | 39 +++++++++++ examples/form-radio/test.ts | 69 +++++++++++++++++++ examples/form/index.tsx | 4 +- examples/form/style.css | 11 +-- examples/radio/icon.tsx | 2 +- packages/ariakit-core/src/utils/focus.ts | 30 +++++++- packages/ariakit-core/src/utils/store.ts | 7 ++ .../src/collection/collection-item.ts | 6 -- .../src/composite/composite-item.tsx | 26 +++---- 12 files changed, 241 insertions(+), 31 deletions(-) create mode 100644 examples/form-radio/icon.tsx create mode 100644 examples/form-radio/index.tsx create mode 100644 examples/form-radio/readme.md create mode 100644 examples/form-radio/style.css create mode 100644 examples/form-radio/test.ts diff --git a/examples/form-radio/icon.tsx b/examples/form-radio/icon.tsx new file mode 100644 index 0000000000..7bd7c139e6 --- /dev/null +++ b/examples/form-radio/icon.tsx @@ -0,0 +1,26 @@ +function getItem() { + return ( +
+
+
+
+ ); +} + +export default function Icon() { + return ( + + +
+
+
+ {getItem()} + {getItem()} + {getItem()} +
+
+
+ + + ); +} diff --git a/examples/form-radio/index.tsx b/examples/form-radio/index.tsx new file mode 100644 index 0000000000..f47f5d4f9a --- /dev/null +++ b/examples/form-radio/index.tsx @@ -0,0 +1,45 @@ +import { useId } from "react"; +import * as Ariakit from "@ariakit/react"; +import "./style.css"; + +export default function Example() { + const id = useId(); + const form = Ariakit.useFormStore({ defaultValues: { color: "" } }); + + form.useValidate(() => { + if (!form.getValue(form.names.color)) { + form.setError(form.names.color, "Please select a color."); + } + }); + + form.useSubmit(async (state) => { + alert(JSON.stringify(state.values)); + }); + + return ( + +

+ Preferences +

+ + + Favorite color + + + + + + + Submit + + ); +} diff --git a/examples/form-radio/readme.md b/examples/form-radio/readme.md new file mode 100644 index 0000000000..30b22fb0f8 --- /dev/null +++ b/examples/form-radio/readme.md @@ -0,0 +1,7 @@ +# FormRadio + +

+ Using the FormRadioGroup and FormRadio components to create a Form with custom validation that requires a user to select an option from a list of radio buttons. +

+ +Example diff --git a/examples/form-radio/style.css b/examples/form-radio/style.css new file mode 100644 index 0000000000..e9dd15af35 --- /dev/null +++ b/examples/form-radio/style.css @@ -0,0 +1,39 @@ +@import url("../form/style.css"); + +.radio-group { + @apply + flex + flex-col + items-start + gap-2 + ; +} + +.radio-group-label { + @apply + text-sm + font-semibold + text-black/70 + dark:text-white/70 + ; +} + +.radio-label { + @apply + flex + items-center + gap-2 + ; +} + +.button { + @apply + flex-none + ; +} + +.error { + @apply + w-full + ; +} diff --git a/examples/form-radio/test.ts b/examples/form-radio/test.ts new file mode 100644 index 0000000000..877773d679 --- /dev/null +++ b/examples/form-radio/test.ts @@ -0,0 +1,69 @@ +import { click, getByRole, press, queryByText } from "@ariakit/test"; + +const getSubmit = () => getByRole("button", { name: "Submit" }); +const getRadio = (name: string) => getByRole("radio", { name }); +const getError = () => queryByText("Please select a color."); + +const spyOnAlert = () => vi.spyOn(window, "alert").mockImplementation(() => {}); + +let alert = spyOnAlert(); + +beforeEach(() => { + alert = spyOnAlert(); + return () => alert.mockClear(); +}); + +test("focus on the first radio button by tabbing", async () => { + expect(getRadio("Red")).not.toHaveFocus(); + expect(getRadio("Green")).not.toHaveFocus(); + expect(getRadio("Blue")).not.toHaveFocus(); + await press.Tab(); + expect(getRadio("Red")).toHaveFocus(); +}); + +test("show error on blur", async () => { + await press.Tab(); + expect(getError()).not.toBeInTheDocument(); + await press.Tab(); + expect(getSubmit()).toHaveFocus(); + expect(getError()).toBeInTheDocument(); +}); + +test("show error on submit", async () => { + expect(getError()).not.toBeInTheDocument(); + await click(getSubmit()); + expect(getError()).toBeInTheDocument(); +}); + +test("focus on radio with error on submit", async () => { + await click(getSubmit()); + expect(getRadio("Red")).toHaveFocus(); +}); + +test("fix error on change", async () => { + await press.Tab(); + expect(getRadio("Red")).toHaveFocus(); + await press.Tab(); + expect(getSubmit()).toHaveFocus(); + await press.ShiftTab(); + expect(getRadio("Blue")).toHaveFocus(); + expect(getError()).toBeInTheDocument(); + await press.Space(); + expect(getError()).not.toBeInTheDocument(); +}); + +test("submit form", async () => { + await click(getRadio("Green")); + await click(getSubmit()); + expect(alert).toHaveBeenCalledWith(JSON.stringify({ color: "green" })); +}); + +test("reset form on submit", async () => { + await press.Tab(); + await press.Space(); + await press.Tab(); + await press.Enter(); + expect(getRadio("Red")).not.toBeChecked(); + expect(getRadio("Green")).not.toBeChecked(); + expect(getRadio("Blue")).not.toBeChecked(); +}); diff --git a/examples/form/index.tsx b/examples/form/index.tsx index 69dd777abe..d1487ece38 100644 --- a/examples/form/index.tsx +++ b/examples/form/index.tsx @@ -4,8 +4,8 @@ import "./style.css"; export default function Example() { const form = Ariakit.useFormStore({ defaultValues: { name: "", email: "" } }); - form.useSubmit(async () => { - alert(JSON.stringify(form.getState().values)); + form.useSubmit(async (state) => { + alert(JSON.stringify(state.values)); }); return ( diff --git a/examples/form/style.css b/examples/form/style.css index 4376865466..e59deb0a9d 100644 --- a/examples/form/style.css +++ b/examples/form/style.css @@ -57,14 +57,9 @@ ; } -.error:empty { - @apply - absolute - ; -} - -.error:not(:empty) { +.error { @apply + empty:hidden w-fit rounded-md py-2 @@ -94,7 +89,7 @@ ; } -.button.reset { +.reset { @apply font-semibold text-black/70 diff --git a/examples/radio/icon.tsx b/examples/radio/icon.tsx index c770e06377..560693d2d4 100644 --- a/examples/radio/icon.tsx +++ b/examples/radio/icon.tsx @@ -1,7 +1,7 @@ function getItem(filled = false) { return (
-
+
{filled && (
)} diff --git a/packages/ariakit-core/src/utils/focus.ts b/packages/ariakit-core/src/utils/focus.ts index 61f95a6684..1cf6710525 100644 --- a/packages/ariakit-core/src/utils/focus.ts +++ b/packages/ariakit-core/src/utils/focus.ts @@ -38,8 +38,34 @@ export function isFocusable(element: Element): element is HTMLElement { * isTabbable(document.querySelector("input[hidden]")); // false * isTabbable(document.querySelector("input:disabled")); // false */ -export function isTabbable(element: Element): element is HTMLElement { - return isFocusable(element) && !hasNegativeTabIndex(element); +export function isTabbable( + element: Element | HTMLElement | HTMLInputElement +): element is HTMLElement { + if (!isFocusable(element)) return false; + if (hasNegativeTabIndex(element)) return false; + // If the element is a radio button in a form, we must take roving tabindex + // into account. + if (!("form" in element)) return true; + if (!element.form) return true; + if (element.checked) return true; + if (element.type !== "radio") return true; + // If the radio button is not part of a radio group, it's tabbable. + const radioGroup = element.form.elements.namedItem(element.name); + if (!radioGroup) return true; + if (!("length" in radioGroup)) return true; + // If we are in a radio group, we must check if the active element is part of + // the same group, which would make all the other radio buttons in the group + // non-tabbable. + const activeElement = getActiveElement(element) as + | HTMLElement + | HTMLInputElement + | null; + if (!activeElement) return true; + if (activeElement === element) return true; + if (!("form" in activeElement)) return true; + if (activeElement.form !== element.form) return true; + if (activeElement.name !== element.name) return true; + return false; } /** diff --git a/packages/ariakit-core/src/utils/store.ts b/packages/ariakit-core/src/utils/store.ts index 6d4e0884d1..ba71053795 100644 --- a/packages/ariakit-core/src/utils/store.ts +++ b/packages/ariakit-core/src/utils/store.ts @@ -242,35 +242,42 @@ export interface Store { setState(key: K, value: SetStateAction): void; /** * Register a callback function that's called when the store is initialized. + * @deprecated This is experimental and may be removed in the future. */ setup: (callback: () => void | (() => void)) => () => void; /** * Function that should be called when the store is initialized. + * @deprecated This is experimental and may be removed in the future. */ init: () => () => void; /** * Registers a listener function that's called immediately and synchronously * whenever the store state changes. + * @deprecated This is experimental and may be removed in the future. */ sync: Sync; /** * Registers a listener function that's called after state changes in the * store. + * @deprecated This is experimental and may be removed in the future. */ subscribe: Sync; /** * Registers a listener function that's called immediately and after a batch * of state changes in the store. + * @deprecated This is experimental and may be removed in the future. */ syncBatch: Sync; /** * Creates a new store with a subset of the current store state and keeps them * in sync. + * @deprecated This is experimental and may be removed in the future. */ pick>(...keys: K): Store>; /** * Creates a new store with a subset of the current store state and keeps them * in sync. + * @deprecated This is experimental and may be removed in the future. */ omit>(...keys: K): Store>; } diff --git a/packages/ariakit-react-core/src/collection/collection-item.ts b/packages/ariakit-react-core/src/collection/collection-item.ts index 91be51bbbd..2c5cb0f756 100644 --- a/packages/ariakit-react-core/src/collection/collection-item.ts +++ b/packages/ariakit-react-core/src/collection/collection-item.ts @@ -26,12 +26,6 @@ export const useCollectionItem = createHook( const context = useContext(CollectionContext); store = store || context; - invariant( - store, - process.env.NODE_ENV !== "production" && - "CollectionItem must be wrapped in a Collection component" - ); - const id = useId(props.id); const unrenderItem = useRef<() => void>(); diff --git a/packages/ariakit-react-core/src/composite/composite-item.tsx b/packages/ariakit-react-core/src/composite/composite-item.tsx index a56b0af221..da223f7771 100644 --- a/packages/ariakit-react-core/src/composite/composite-item.tsx +++ b/packages/ariakit-react-core/src/composite/composite-item.tsx @@ -11,7 +11,6 @@ import { isTextField, } from "@ariakit/core/utils/dom"; import { isPortalEvent, isSelfTarget } from "@ariakit/core/utils/events"; -import { invariant } from "@ariakit/core/utils/misc"; import type { BooleanOrCallback } from "@ariakit/core/utils/types"; import type { CollectionItemOptions } from "../collection/collection-item.js"; import { useCollectionItem } from "../collection/collection-item.js"; @@ -25,6 +24,7 @@ import { useSafeLayoutEffect, useWrapElement, } from "../utils/hooks.js"; +import { useStoreState } from "../utils/store.js"; import { createElement, createHook, @@ -170,16 +170,16 @@ export const useCompositeItem = createHook( const context = useContext(CompositeContext); store = store || context; - invariant( - store, - process.env.NODE_ENV !== "production" && - "CompositeItem must be wrapped in a Composite component" - ); + // invariant( + // store, + // process.env.NODE_ENV !== "production" && + // "CompositeItem must be wrapped in a Composite component" + // ); const id = useId(props.id); const ref = useRef(null); const row = useContext(CompositeRowContext); - const rowId = store.useState((state) => { + const rowId = useStoreState(store, (state) => { if (rowIdProp) return rowIdProp; if (!row?.baseElement) return; if (row.baseElement !== state.baseElement) return; @@ -316,7 +316,8 @@ export const useCompositeItem = createHook( } }); - const baseElement = store.useState( + const baseElement = useStoreState( + store, (state) => state.baseElement || undefined ); @@ -335,9 +336,9 @@ export const useCompositeItem = createHook( [providerValue] ); - const isActiveItem = store.useState((state) => state.activeId === id); + const isActiveItem = useStoreState(store, (state) => state.activeId === id); const role = useRole(ref, props); - const virtualFocus = store.useState("virtualFocus"); + const virtualFocus = useStoreState(store, "virtualFocus"); let ariaSelected: boolean | undefined; if (isActiveItem) { @@ -354,7 +355,8 @@ export const useCompositeItem = createHook( } } - const shouldTabIndex = store.useState( + const shouldTabIndex = useStoreState( + store, (state) => !state.virtualFocus && isActiveItem ); @@ -364,7 +366,7 @@ export const useCompositeItem = createHook( "data-active-item": isActiveItem ? "" : undefined, ...props, ref: useForkRef(ref, props.ref), - tabIndex: shouldTabIndex ? props.tabIndex : -1, + tabIndex: shouldTabIndex !== false ? props.tabIndex : -1, onFocus, onBlurCapture, onKeyDown, From 65cd88f952999d4ea40196dff38f0500416bb8d2 Mon Sep 17 00:00:00 2001 From: Haz Date: Tue, 25 Apr 2023 17:41:52 +0200 Subject: [PATCH 2/5] Support useId on React17 --- packages/ariakit-core/src/utils/store.ts | 7 ------- .../src/collection/collection-item.ts | 2 +- vitest.setup.ts | 10 +++++++++- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/ariakit-core/src/utils/store.ts b/packages/ariakit-core/src/utils/store.ts index ba71053795..6d4e0884d1 100644 --- a/packages/ariakit-core/src/utils/store.ts +++ b/packages/ariakit-core/src/utils/store.ts @@ -242,42 +242,35 @@ export interface Store { setState(key: K, value: SetStateAction): void; /** * Register a callback function that's called when the store is initialized. - * @deprecated This is experimental and may be removed in the future. */ setup: (callback: () => void | (() => void)) => () => void; /** * Function that should be called when the store is initialized. - * @deprecated This is experimental and may be removed in the future. */ init: () => () => void; /** * Registers a listener function that's called immediately and synchronously * whenever the store state changes. - * @deprecated This is experimental and may be removed in the future. */ sync: Sync; /** * Registers a listener function that's called after state changes in the * store. - * @deprecated This is experimental and may be removed in the future. */ subscribe: Sync; /** * Registers a listener function that's called immediately and after a batch * of state changes in the store. - * @deprecated This is experimental and may be removed in the future. */ syncBatch: Sync; /** * Creates a new store with a subset of the current store state and keeps them * in sync. - * @deprecated This is experimental and may be removed in the future. */ pick>(...keys: K): Store>; /** * Creates a new store with a subset of the current store state and keeps them * in sync. - * @deprecated This is experimental and may be removed in the future. */ omit>(...keys: K): Store>; } diff --git a/packages/ariakit-react-core/src/collection/collection-item.ts b/packages/ariakit-react-core/src/collection/collection-item.ts index 2c5cb0f756..a5f6571022 100644 --- a/packages/ariakit-react-core/src/collection/collection-item.ts +++ b/packages/ariakit-react-core/src/collection/collection-item.ts @@ -1,7 +1,7 @@ import type { RefCallback } from "react"; import { useCallback, useContext, useRef } from "react"; import type { CollectionStoreItem } from "@ariakit/core/collection/collection-store"; -import { identity, invariant } from "@ariakit/core/utils/misc"; +import { identity } from "@ariakit/core/utils/misc"; import { useForkRef, useId } from "../utils/hooks.js"; import { createComponent, createElement, createHook } from "../utils/system.js"; import type { As, Options, Props } from "../utils/types.js"; diff --git a/vitest.setup.ts b/vitest.setup.ts index e1dc32d79a..4667646b08 100644 --- a/vitest.setup.ts +++ b/vitest.setup.ts @@ -58,9 +58,17 @@ if (version.startsWith("17")) { const actual = await vi.importActual("react"); return { ...actual, + useInsertionEffect: undefined, useDeferredValue: (v: T) => v, useTransition: () => [false, (v: () => any) => v()], - useInsertionEffect: undefined, + useId: () => { + const [id, setId] = actual.useState(); + actual.useLayoutEffect(() => { + const random = Math.random().toString(36).substr(2, 6); + setId(`id-${random}`); + }, []); + return id; + }, }; }); } From 6045371906dca48a9959df6bb388ce12a6d03b42 Mon Sep 17 00:00:00 2001 From: Haz Date: Tue, 25 Apr 2023 17:59:12 +0200 Subject: [PATCH 3/5] Fix React17 mocks --- vitest.setup.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/vitest.setup.ts b/vitest.setup.ts index 4667646b08..b10adacdd1 100644 --- a/vitest.setup.ts +++ b/vitest.setup.ts @@ -56,9 +56,7 @@ if (version.startsWith("17")) { vi.mock("react", async () => { // eslint-disable-next-line @typescript-eslint/consistent-type-imports const actual = await vi.importActual("react"); - return { - ...actual, - useInsertionEffect: undefined, + const mocks = { useDeferredValue: (v: T) => v, useTransition: () => [false, (v: () => any) => v()], useId: () => { @@ -70,6 +68,7 @@ if (version.startsWith("17")) { return id; }, }; + return { ...mocks, ...actual }; }); } From 13e4c13fcf7f09c7d5e0a8c01d20fe4263333440 Mon Sep 17 00:00:00 2001 From: Haz Date: Tue, 25 Apr 2023 19:19:10 +0200 Subject: [PATCH 4/5] Add changesets --- .changeset/large-goats-kneel.md | 6 ++++++ .changeset/lemon-comics-compare.md | 5 +++++ .../ariakit-react-core/src/composite/composite-item.tsx | 6 ------ 3 files changed, 11 insertions(+), 6 deletions(-) create mode 100644 .changeset/large-goats-kneel.md create mode 100644 .changeset/lemon-comics-compare.md diff --git a/.changeset/large-goats-kneel.md b/.changeset/large-goats-kneel.md new file mode 100644 index 0000000000..ad61765488 --- /dev/null +++ b/.changeset/large-goats-kneel.md @@ -0,0 +1,6 @@ +--- +"@ariakit/react-core": patch +"@ariakit/react": patch +--- + +Fixed `FormRadio` error when not explicitly providing the composite store. ([#2313](https://github.com/ariakit/ariakit/pull/2313)) diff --git a/.changeset/lemon-comics-compare.md b/.changeset/lemon-comics-compare.md new file mode 100644 index 0000000000..173d50f878 --- /dev/null +++ b/.changeset/lemon-comics-compare.md @@ -0,0 +1,5 @@ +--- +"@ariakit/core": patch +--- + +Added support for native radio buttons within forms, that work with roving tabindex, to all `focus` utilities. ([#2313](https://github.com/ariakit/ariakit/pull/2313)) diff --git a/packages/ariakit-react-core/src/composite/composite-item.tsx b/packages/ariakit-react-core/src/composite/composite-item.tsx index da223f7771..e281b42036 100644 --- a/packages/ariakit-react-core/src/composite/composite-item.tsx +++ b/packages/ariakit-react-core/src/composite/composite-item.tsx @@ -170,12 +170,6 @@ export const useCompositeItem = createHook( const context = useContext(CompositeContext); store = store || context; - // invariant( - // store, - // process.env.NODE_ENV !== "production" && - // "CompositeItem must be wrapped in a Composite component" - // ); - const id = useId(props.id); const ref = useRef(null); const row = useContext(CompositeRowContext); From 157db62d4be76cddddfdd3e2be50fb681871c462 Mon Sep 17 00:00:00 2001 From: Haz Date: Tue, 25 Apr 2023 19:27:44 +0200 Subject: [PATCH 5/5] Update icon --- examples/form-radio/icon.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/examples/form-radio/icon.tsx b/examples/form-radio/icon.tsx index 7bd7c139e6..ae6511a6c1 100644 --- a/examples/form-radio/icon.tsx +++ b/examples/form-radio/icon.tsx @@ -1,8 +1,8 @@ -function getItem() { +function getItem(textWidth = "w-10") { return (
-
+
); } @@ -11,12 +11,12 @@ export default function Icon() { return ( -
+
- {getItem()} - {getItem()} - {getItem()} + {getItem("w-10")} + {getItem("w-8")} + {getItem("w-14")}