From 1347549164a69c1c8ffbdb57aa9d87c3daddeab8 Mon Sep 17 00:00:00 2001 From: Damian Tarnawski Date: Thu, 1 Sep 2022 16:10:33 +0000 Subject: [PATCH 1/4] fix: createButton and createFocusable now are passing props through --- packages/button/src/createButton.ts | 23 +++++------------------ packages/focus/src/createFocusable.ts | 15 +++++++-------- 2 files changed, 12 insertions(+), 26 deletions(-) diff --git a/packages/button/src/createButton.ts b/packages/button/src/createButton.ts index 45bd5d7..c8403a5 100644 --- a/packages/button/src/createButton.ts +++ b/packages/button/src/createButton.ts @@ -14,13 +14,13 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ +/* eslint-disable solid/reactivity */ import { createFocusable } from "@solid-aria/focus"; import { createPress } from "@solid-aria/interactions"; import { ElementType } from "@solid-aria/types"; -import { filterDOMProps } from "@solid-aria/utils"; import { combineProps } from "@solid-primitives/props"; -import { Accessor, createMemo, JSX, mergeProps, splitProps } from "solid-js"; +import { Accessor, JSX, mergeProps, splitProps } from "solid-js"; import { AriaButtonProps } from "./types"; @@ -82,7 +82,6 @@ export function createButton( type: "button" }; - // eslint-disable-next-line solid/reactivity props = mergeProps(defaultProps, props); const additionalButtonElementProps: JSX.ButtonHTMLAttributes = { @@ -120,12 +119,8 @@ export function createButton( } }; - const additionalProps = mergeProps( - createMemo(() => { - return props.elementType === "button" - ? additionalButtonElementProps - : additionalOtherElementProps; - }) + const additionalProps = mergeProps(() => + props.elementType === "button" ? additionalButtonElementProps : additionalOtherElementProps ); const [createPressProps] = splitProps(props, [ @@ -141,8 +136,6 @@ export function createButton( const { focusableProps } = createFocusable(props, ref); - const domProps = filterDOMProps(props, { labelable: true }); - const baseButtonProps: JSX.HTMLAttributes = { get "aria-haspopup"() { return props["aria-haspopup"]; @@ -169,13 +162,7 @@ export function createButton( } }; - const buttonProps = combineProps( - additionalProps, - focusableProps, - pressProps, - domProps, - baseButtonProps - ); + const buttonProps = combineProps(additionalProps, focusableProps, pressProps, baseButtonProps); return { buttonProps, isPressed }; } diff --git a/packages/focus/src/createFocusable.ts b/packages/focus/src/createFocusable.ts index 2749407..0a4db3c 100644 --- a/packages/focus/src/createFocusable.ts +++ b/packages/focus/src/createFocusable.ts @@ -45,11 +45,11 @@ export interface CreateFocusableProps extends CreateFocusProps, CreateKeyboardPr excludeFromTabOrder?: MaybeAccessor; } -export interface FocusableResult { +export interface FocusableResult { /** * Props to spread onto the target element. */ - focusableProps: JSX.HTMLAttributes; + focusableProps: Props & JSX.HTMLAttributes; } // TODO: add all the focus provider stuff when needed @@ -57,21 +57,20 @@ export interface FocusableResult { /** * Make an element focusable, capable of auto focus and excludable from tab order. */ -export function createFocusable( - props: CreateFocusableProps, +export function createFocusable( + props: Props, ref: Accessor -): FocusableResult { +): FocusableResult { const [autofocus, setAutofocus] = createSignal(!!access(props.autofocus)); const { focusProps } = createFocus(props); const { keyboardProps } = createKeyboard(props); - const focusableProps = { - ...combineProps(focusProps, keyboardProps), + const focusableProps = combineProps(props, focusProps, keyboardProps, { get tabIndex() { return access(props.excludeFromTabOrder) && !access(props.isDisabled) ? -1 : undefined; } - }; + }) as Props; onMount(() => { autofocus() && ref()?.focus(); From e59b6d704db5c7027b1a3aff03cca7d1410e2aeb Mon Sep 17 00:00:00 2001 From: Damian Tarnawski Date: Thu, 1 Sep 2022 16:11:13 +0000 Subject: [PATCH 2/4] test: add tests to button and focus for user props passing --- packages/button/test/createButton.test.tsx | 26 ++++++++++++++++++ packages/focus/test/createFocusable.test.ts | 30 +++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 packages/focus/test/createFocusable.test.ts diff --git a/packages/button/test/createButton.test.tsx b/packages/button/test/createButton.test.tsx index 8c88440..2e89627 100644 --- a/packages/button/test/createButton.test.tsx +++ b/packages/button/test/createButton.test.tsx @@ -15,6 +15,8 @@ * governing permissions and limitations under the License. */ +import { JSX } from "solid-js"; + import { createButton } from "../src"; import { AriaButtonProps } from "../src/types"; @@ -88,4 +90,28 @@ describe("createButton", () => { // @ts-ignore expect(buttonProps.rel).toBeUndefined(); }); + + it("user props are passed down to the returned button props", () => { + const ref = document.createElement("button"); + const onMouseMove = jest.fn(); + const props: AriaButtonProps<"button"> & JSX.IntrinsicElements["button"] = { + children: "Hello", + class: "test-class", + onMouseMove, + style: { color: "red" } + }; + + const { buttonProps } = createButton(props, () => ref); + + expect(buttonProps.children).toBe("Hello"); + expect(buttonProps.class).toBe("test-class"); + expect(buttonProps.style).toEqual({ color: "red" }); + + expect(buttonProps).toHaveProperty("onMouseMove"); + expect(typeof buttonProps.onMouseMove).toBe("function"); + + // @ts-expect-error + buttonProps.onMouseMove(); + expect(onMouseMove).toHaveBeenCalled(); + }); }); diff --git a/packages/focus/test/createFocusable.test.ts b/packages/focus/test/createFocusable.test.ts new file mode 100644 index 0000000..4a25bd5 --- /dev/null +++ b/packages/focus/test/createFocusable.test.ts @@ -0,0 +1,30 @@ +/* eslint-disable solid/reactivity */ +import { JSX } from "solid-js"; + +import { createFocusable, CreateFocusableProps } from "../src"; + +describe("createFocusable", () => { + test("user props are passed through", () => { + const ref = document.createElement("button"); + const onClick = jest.fn(); + const props: CreateFocusableProps & JSX.IntrinsicElements["button"] = { + children: "Hello", + class: "test-class", + onClick, + style: { color: "red" } + }; + + const { focusableProps } = createFocusable(props, () => ref); + + expect(focusableProps.children).toBe("Hello"); + expect(focusableProps.class).toBe("test-class"); + expect(focusableProps.style).toEqual({ color: "red" }); + + expect(focusableProps).toHaveProperty("onClick"); + expect(typeof focusableProps.onClick).toBe("function"); + + // @ts-expect-error + focusableProps.onClick(); + expect(onClick).toHaveBeenCalled(); + }); +}); From 5e0bdc07f4e087f9dc37759e34ff9c017f6ebf56 Mon Sep 17 00:00:00 2001 From: Damian Tarnawski Date: Thu, 1 Sep 2022 16:14:30 +0000 Subject: [PATCH 3/4] chore: add changeset --- .changeset/rude-melons-own.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/rude-melons-own.md diff --git a/.changeset/rude-melons-own.md b/.changeset/rude-melons-own.md new file mode 100644 index 0000000..789562c --- /dev/null +++ b/.changeset/rude-melons-own.md @@ -0,0 +1,6 @@ +--- +"@solid-aria/button": patch +"@solid-aria/focus": patch +--- + +Fix user props not being passed down to the returned props object. (#69) From c85517625e40a4150e003e812acd1a1c940ea6cd Mon Sep 17 00:00:00 2001 From: Damian Tarnawski Date: Thu, 1 Sep 2022 18:50:03 +0000 Subject: [PATCH 4/4] fix: correct handling refs in createPress and createButton --- packages/button/src/createButton.ts | 83 ++++++++------------- packages/button/src/types.ts | 30 +++++++- packages/button/test/createButton.test.tsx | 21 ++---- packages/focus/src/createFocusable.ts | 46 ++++++++---- packages/focus/test/createFocusable.test.ts | 19 ++++- packages/interactions/src/createPress.ts | 13 ++-- 6 files changed, 123 insertions(+), 89 deletions(-) diff --git a/packages/button/src/createButton.ts b/packages/button/src/createButton.ts index c8403a5..8cbb120 100644 --- a/packages/button/src/createButton.ts +++ b/packages/button/src/createButton.ts @@ -16,19 +16,25 @@ */ /* eslint-disable solid/reactivity */ -import { createFocusable } from "@solid-aria/focus"; +import { createFocusable, CreateFocusableProps } from "@solid-aria/focus"; import { createPress } from "@solid-aria/interactions"; import { ElementType } from "@solid-aria/types"; import { combineProps } from "@solid-primitives/props"; import { Accessor, JSX, mergeProps, splitProps } from "solid-js"; -import { AriaButtonProps } from "./types"; +import { AriaButtonProps, ElementOfType, PropsOfType } from "./types"; + +export interface ButtonAria { + /** + * A ref to apply onto the target element. + * Merge the given `props.ref` and all parents `PressResponder` refs. + */ + ref: (el: ElementOfType) => void; -export interface ButtonAria { /** * Props for the button element. */ - buttonProps: T; + buttonProps: Omit, "ref">; /** * Whether the button is currently pressed. @@ -36,53 +42,13 @@ export interface ButtonAria { isPressed: Accessor; } -// Order with overrides is important: 'button' should be default -export function createButton( - props: AriaButtonProps<"button">, - ref: Accessor -): ButtonAria>; - -export function createButton( - props: AriaButtonProps<"a">, - ref: Accessor -): ButtonAria>; - -export function createButton( - props: AriaButtonProps<"div">, - ref: Accessor -): ButtonAria>; - -export function createButton( - props: AriaButtonProps<"input">, - ref: Accessor -): ButtonAria>; - -export function createButton( - props: AriaButtonProps<"span">, - ref: Accessor -): ButtonAria>; - -export function createButton( - props: AriaButtonProps, - ref: Accessor -): ButtonAria>; - /** * Provides the behavior and accessibility implementation for a button component. Handles mouse, keyboard, and touch interactions, * focus behavior, and ARIA props for both native button elements and custom element types. * @param props - Props to be applied to the button. - * @param ref - A ref to a DOM element for the button. */ -export function createButton( - props: AriaButtonProps, - ref: Accessor -): ButtonAria> { - const defaultProps: AriaButtonProps<"button"> = { - elementType: "button", - type: "button" - }; - - props = mergeProps(defaultProps, props); +export function createButton(props: AriaButtonProps): ButtonAria { + props = mergeProps({ elementType: "button", type: "button" }, props) as typeof props; const additionalButtonElementProps: JSX.ButtonHTMLAttributes = { get type() { @@ -132,9 +98,12 @@ export function createButton( "preventFocusOnPress" ]); - const { pressProps, isPressed } = createPress(createPressProps); + const { pressProps, isPressed, ref: pressRef } = createPress(createPressProps); - const { focusableProps } = createFocusable(props, ref); + // createFocusable has problems with the `props` type because if you pass a component as the `elementType` prop, there is no telling if the `ref` will be a HTML Element. + const { focusableProps, ref: focusRef } = createFocusable( + props as CreateFocusableProps + ); const baseButtonProps: JSX.HTMLAttributes = { get "aria-haspopup"() { @@ -162,7 +131,19 @@ export function createButton( } }; - const buttonProps = combineProps(additionalProps, focusableProps, pressProps, baseButtonProps); - - return { buttonProps, isPressed }; + const buttonProps = combineProps( + additionalProps, + focusableProps, + pressProps, + baseButtonProps + ) as Omit, "ref">; + + return { + buttonProps, + isPressed, + ref: el => { + focusRef(el as HTMLElement); + pressRef(el as HTMLElement); + } + }; } diff --git a/packages/button/src/types.ts b/packages/button/src/types.ts index 03a9f8e..2dbd5ad 100644 --- a/packages/button/src/types.ts +++ b/packages/button/src/types.ts @@ -23,7 +23,30 @@ import { FocusableProps, PressEvents } from "@solid-aria/types"; -import { JSX } from "solid-js"; +import { Component, JSX, Ref } from "solid-js"; + +// TODO: these probably should be added to @solid-aria/types + +/** + * Infers the type of `ref` element from the `elementType` prop. + * @example + * "button" -> HTMLButtonElement + * "div" -> HTMLDivElement + * Component<{ ref: Ref }> -> HTMLInputElement + */ +export type ElementOfType = T extends keyof JSX.IntrinsicElements + ? JSX.IntrinsicElements[T] extends JSX.CustomAttributes + ? U + : never + : T extends Component<{ ref: Ref }> + ? U + : never; + +export type PropsOfType = T extends keyof JSX.IntrinsicElements + ? JSX.IntrinsicElements[T] + : T extends Component + ? Parameters[0] + : never; interface ButtonProps extends PressEvents, FocusableProps { /** @@ -38,6 +61,11 @@ interface ButtonProps extends PressEvents, FocusableProps { } export interface AriaButtonElementTypeProps { + /** + * A ref to the target element. + */ + ref?: Ref>; + /** * The HTML element or SolidJS component used to render the button, e.g. 'div', 'a', or `Link`. * @default 'button' diff --git a/packages/button/test/createButton.test.tsx b/packages/button/test/createButton.test.tsx index 2e89627..148ff54 100644 --- a/packages/button/test/createButton.test.tsx +++ b/packages/button/test/createButton.test.tsx @@ -22,17 +22,15 @@ import { AriaButtonProps } from "../src/types"; describe("createButton", () => { it("handles defaults", () => { - let ref: any; const props = {}; - const { buttonProps } = createButton(props, () => ref); + const { buttonProps } = createButton(props); expect(typeof buttonProps.onClick).toBe("function"); }); it("handles elements other than button", () => { - let ref: any; const props: AriaButtonProps<"a"> = { elementType: "a" }; - const { buttonProps } = createButton(props, () => ref); + const { buttonProps } = createButton(props); expect(buttonProps.role).toBe("button"); expect(buttonProps.tabIndex).toBe(0); @@ -43,9 +41,8 @@ describe("createButton", () => { }); it("handles elements other than button disabled", () => { - let ref: any; const props: AriaButtonProps<"a"> = { elementType: "a", isDisabled: true }; - const { buttonProps } = createButton(props, () => ref); + const { buttonProps } = createButton(props); expect(buttonProps.role).toBe("button"); expect(buttonProps.tabIndex).toBeUndefined(); @@ -56,25 +53,22 @@ describe("createButton", () => { }); it("handles the rel attribute on anchors", () => { - let ref: any; const props: AriaButtonProps<"a"> = { elementType: "a", rel: "noopener noreferrer" }; - const { buttonProps } = createButton(props, () => ref); + const { buttonProps } = createButton(props); expect(buttonProps.rel).toBe("noopener noreferrer"); }); it("handles the rel attribute as a string on anchors", () => { - let ref: any; const props: AriaButtonProps<"a"> = { elementType: "a", rel: "search" }; - const { buttonProps } = createButton(props, () => ref); + const { buttonProps } = createButton(props); expect(buttonProps.rel).toBe("search"); }); it("handles input elements", () => { - let ref: any; const props: AriaButtonProps<"input"> = { elementType: "input", isDisabled: true }; - const { buttonProps } = createButton(props, () => ref); + const { buttonProps } = createButton(props); expect(buttonProps.role).toBe("button"); expect(buttonProps.tabIndex).toBeUndefined(); @@ -92,7 +86,6 @@ describe("createButton", () => { }); it("user props are passed down to the returned button props", () => { - const ref = document.createElement("button"); const onMouseMove = jest.fn(); const props: AriaButtonProps<"button"> & JSX.IntrinsicElements["button"] = { children: "Hello", @@ -101,7 +94,7 @@ describe("createButton", () => { style: { color: "red" } }; - const { buttonProps } = createButton(props, () => ref); + const { buttonProps } = createButton(props); expect(buttonProps.children).toBe("Hello"); expect(buttonProps.class).toBe("test-class"); diff --git a/packages/focus/src/createFocusable.ts b/packages/focus/src/createFocusable.ts index 0a4db3c..b8d30a9 100644 --- a/packages/focus/src/createFocusable.ts +++ b/packages/focus/src/createFocusable.ts @@ -23,9 +23,16 @@ import { } from "@solid-aria/interactions"; import { combineProps } from "@solid-primitives/props"; import { access, MaybeAccessor } from "@solid-primitives/utils"; -import { Accessor, createSignal, JSX, onMount } from "solid-js"; +import { createSignal, JSX, onMount, Ref, splitProps } from "solid-js"; + +export interface CreateFocusableProps + extends CreateFocusProps, + CreateKeyboardProps { + /** + * A ref to the target element. + */ + ref?: Ref; -export interface CreateFocusableProps extends CreateFocusProps, CreateKeyboardProps { /** * Whether focus should be disabled. */ @@ -45,11 +52,17 @@ export interface CreateFocusableProps extends CreateFocusProps, CreateKeyboardPr excludeFromTabOrder?: MaybeAccessor; } -export interface FocusableResult { +export interface FocusableResult { + /** + * A ref to apply onto the target element. + * Merge the given `props.ref` and all parents `PressResponder` refs. + */ + ref: (el: El) => void; + /** * Props to spread onto the target element. */ - focusableProps: Props & JSX.HTMLAttributes; + focusableProps: JSX.HTMLAttributes; } // TODO: add all the focus provider stuff when needed @@ -57,25 +70,30 @@ export interface FocusableResult { /** * Make an element focusable, capable of auto focus and excludable from tab order. */ -export function createFocusable( - props: Props, - ref: Accessor -): FocusableResult { +export function createFocusable( + props: CreateFocusableProps +): FocusableResult { + // eslint-disable-next-line solid/reactivity const [autofocus, setAutofocus] = createSignal(!!access(props.autofocus)); - const { focusProps } = createFocus(props); - const { keyboardProps } = createKeyboard(props); + const propsWithoutRef = splitProps(props, ["ref"])[1]; + + const { focusProps } = createFocus(propsWithoutRef); + const { keyboardProps } = createKeyboard(propsWithoutRef); - const focusableProps = combineProps(props, focusProps, keyboardProps, { + const focusableProps = combineProps(propsWithoutRef, focusProps, keyboardProps, { get tabIndex() { return access(props.excludeFromTabOrder) && !access(props.isDisabled) ? -1 : undefined; } - }) as Props; + }); + + let target: El | undefined; + const ref = (el: El) => (props.ref as (el: El) => void)?.((target = el)); onMount(() => { - autofocus() && ref()?.focus(); + autofocus() && target?.focus(); setAutofocus(false); }); - return { focusableProps }; + return { focusableProps, ref }; } diff --git a/packages/focus/test/createFocusable.test.ts b/packages/focus/test/createFocusable.test.ts index 4a25bd5..fa51555 100644 --- a/packages/focus/test/createFocusable.test.ts +++ b/packages/focus/test/createFocusable.test.ts @@ -5,16 +5,15 @@ import { createFocusable, CreateFocusableProps } from "../src"; describe("createFocusable", () => { test("user props are passed through", () => { - const ref = document.createElement("button"); const onClick = jest.fn(); - const props: CreateFocusableProps & JSX.IntrinsicElements["button"] = { + const props: CreateFocusableProps & JSX.IntrinsicElements["button"] = { children: "Hello", class: "test-class", onClick, style: { color: "red" } }; - const { focusableProps } = createFocusable(props, () => ref); + const { focusableProps } = createFocusable(props); expect(focusableProps.children).toBe("Hello"); expect(focusableProps.class).toBe("test-class"); @@ -27,4 +26,18 @@ describe("createFocusable", () => { focusableProps.onClick(); expect(onClick).toHaveBeenCalled(); }); + + test("handles ref", () => { + const userRef = jest.fn(); + const { focusableProps, ref } = createFocusable({ ref: userRef }); + + expect("ref" in focusableProps).toBeFalsy(); + + expect(typeof ref).toBe("function"); + expect(ref).not.toBe(userRef); + expect(userRef).not.toHaveBeenCalled(); + const el = document.createElement("button"); + ref(el); + expect(userRef).toHaveBeenCalledWith(el); + }); }); diff --git a/packages/interactions/src/createPress.ts b/packages/interactions/src/createPress.ts index 92b092f..619feb6 100644 --- a/packages/interactions/src/createPress.ts +++ b/packages/interactions/src/createPress.ts @@ -14,6 +14,7 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ +/* eslint-disable solid/reactivity */ import { PointerType } from "@solid-aria/types"; import { createGlobalListeners, focusWithoutScrolling } from "@solid-aria/utils"; @@ -74,12 +75,15 @@ export function createPress(props: CreatePressProps): if (context) { context.register(); - const [, contextProps] = splitProps(context, ["register", "ref"]); + const [, contextProps] = splitProps(context, ["register"]); + // eslint-disable-next-line solid/reactivity props = combineProps(contextProps, props); } const [, domProps] = splitProps(props, [ + // ref is omited, because user should populate it himself + "ref", "onPress", "onPressChange", "onPressStart", @@ -679,12 +683,9 @@ export function createPress(props: CreatePressProps): ); return { - ref: (el: T) => { - (props.ref as ((el: T) => void) | undefined)?.(el); - context?.ref?.(el); - }, + ref: (el: T) => (props.ref as ((el: T) => void) | undefined)?.(el), isPressed, - pressProps: combineProps(domProps, pressProps) as JSX.HTMLAttributes + pressProps: combineProps(domProps, pressProps) }; }