From beaceba54a06c748b518b0507a46c31ce1368025 Mon Sep 17 00:00:00 2001 From: Adam Dierkens Date: Mon, 6 Jul 2026 12:59:10 -0400 Subject: [PATCH] refactor(Checkbox): set indeterminate via ref callback instead of layout effect Replace the useLayoutEffect that imperatively set the DOM `.indeterminate` property (and the useEffect that set `aria-checked`) with a single ref callback merged into the forwarded ref via useMergedRefs. The callback runs synchronously during commit, preserving the previous timing and behavior while removing two effects per Checkbox instance. Internal-only refactor; no public API change. Closes #7764 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../checkbox-indeterminate-ref-callback.md | 5 ++ packages/react/src/Checkbox/Checkbox.tsx | 59 +++++++++++-------- 2 files changed, 40 insertions(+), 24 deletions(-) create mode 100644 .changeset/checkbox-indeterminate-ref-callback.md diff --git a/.changeset/checkbox-indeterminate-ref-callback.md b/.changeset/checkbox-indeterminate-ref-callback.md new file mode 100644 index 00000000000..7f38a4e56a1 --- /dev/null +++ b/.changeset/checkbox-indeterminate-ref-callback.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Checkbox: Improve rendering performance by setting the indeterminate state with a ref callback instead of a layout effect diff --git a/packages/react/src/Checkbox/Checkbox.tsx b/packages/react/src/Checkbox/Checkbox.tsx index 6e54d07cfdd..57a2fd63ccf 100644 --- a/packages/react/src/Checkbox/Checkbox.tsx +++ b/packages/react/src/Checkbox/Checkbox.tsx @@ -1,7 +1,13 @@ import {clsx} from 'clsx' -import {useProvidedRefOrCreate} from '../hooks' -import React, {useContext, useEffect, type ChangeEventHandler, type InputHTMLAttributes, type ReactElement} from 'react' -import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' +import {useMergedRefs} from '../hooks' +import React, { + useCallback, + useContext, + useRef, + type ChangeEventHandler, + type InputHTMLAttributes, + type ReactElement, +} from 'react' import type {FormValidationStatus} from '../utils/types/FormValidationStatus' import {CheckboxGroupContext} from '../CheckboxGroup/CheckboxGroupContext' import classes from './Checkbox.module.css' @@ -58,7 +64,7 @@ const Checkbox = React.forwardRef( ref, // eslint-disable-next-line @typescript-eslint/no-explicit-any ): ReactElement => { - const checkboxRef = useProvidedRefOrCreate(ref as React.RefObject) + const checkboxRef = useRef(null) const checkboxGroupContext = useContext(CheckboxGroupContext) const handleOnChange: ChangeEventHandler = e => { checkboxGroupContext.onChange && checkboxGroupContext.onChange(e) @@ -69,10 +75,34 @@ const Checkbox = React.forwardRef( checkboxRef.current.setAttribute('aria-checked', 'mixed') } } + + // Ref callback that runs synchronously during commit (same timing as + // `useLayoutEffect`) to imperatively set the DOM `.indeterminate` property, + // which has no HTML attribute equivalent, along with the matching + // `aria-checked` value. This avoids a layout effect per Checkbox instance. + const setIndeterminate = useCallback( + (node: HTMLInputElement | null) => { + if (node) { + node.indeterminate = indeterminate || false + if (indeterminate) { + node.setAttribute('aria-checked', 'mixed') + } else { + node.setAttribute('aria-checked', node.checked ? 'true' : 'false') + } + } + }, + // `checked` is intentionally included: the callback reads `node.checked` (to also + // cover uncontrolled inputs), so it must re-run to refresh `aria-checked` when the + // controlled `checked` prop changes. eslint can't see this indirect dependency. + // eslint-disable-next-line react-hooks/exhaustive-deps + [indeterminate, checked], + ) + const mergedRef = useMergedRefs(ref, useMergedRefs(checkboxRef, setIndeterminate)) + const inputProps = { type: 'checkbox', disabled, - ref: checkboxRef, + ref: mergedRef, checked: indeterminate ? false : checked, defaultChecked, required, @@ -84,26 +114,7 @@ const Checkbox = React.forwardRef( ...rest, } - useLayoutEffect(() => { - if (checkboxRef.current) { - checkboxRef.current.indeterminate = indeterminate || false - } - }, [indeterminate, checked, checkboxRef]) - - useEffect(() => { - const {current: checkbox} = checkboxRef - if (!checkbox) { - return - } - - if (indeterminate) { - checkbox.setAttribute('aria-checked', 'mixed') - } else { - checkbox.setAttribute('aria-checked', checkbox.checked ? 'true' : 'false') - } - }) return ( - // @ts-expect-error inputProp needs a non nullable ref