From 0464be514040d531f9a3d686cd71b5b1003c5001 Mon Sep 17 00:00:00 2001 From: Alexander Lelidis Date: Fri, 17 Apr 2026 14:17:24 +0000 Subject: [PATCH 1/3] Checkbox: replace useLayoutEffect with ref callback for indeterminate Replace useLayoutEffect + useEffect with a ref callback pattern using useMergedRefs to set the indeterminate DOM property. This eliminates layout effects from every Checkbox render while maintaining the same synchronous timing guarantees. - Use useCallback ref to set .indeterminate on the DOM node - Merge forwarded ref with indeterminate callback via useMergedRefs - Replace imperative aria-checked useEffect with inline JSX attribute - Update tests to reflect that non-indeterminate checkboxes rely on native checked state for accessibility Closes #7764 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- packages/react/src/Checkbox/Checkbox.test.tsx | 5 ++- packages/react/src/Checkbox/Checkbox.tsx | 40 ++++++++----------- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/packages/react/src/Checkbox/Checkbox.test.tsx b/packages/react/src/Checkbox/Checkbox.test.tsx index 253baa55b83..3f3e887693a 100644 --- a/packages/react/src/Checkbox/Checkbox.test.tsx +++ b/packages/react/src/Checkbox/Checkbox.test.tsx @@ -103,11 +103,12 @@ describe('Checkbox', () => { const checkbox = getByRole('checkbox') as HTMLInputElement - expect(checkbox).toHaveAttribute('aria-checked', 'false') + // Non-indeterminate checkboxes rely on native checked state for accessibility + expect(checkbox.checked).toEqual(false) rerender() - expect(checkbox).toHaveAttribute('aria-checked', 'true') + expect(checkbox.checked).toEqual(true) rerender() diff --git a/packages/react/src/Checkbox/Checkbox.tsx b/packages/react/src/Checkbox/Checkbox.tsx index c992f840ebb..80ce8ddfe40 100644 --- a/packages/react/src/Checkbox/Checkbox.tsx +++ b/packages/react/src/Checkbox/Checkbox.tsx @@ -1,7 +1,6 @@ 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, {useContext, 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' @@ -45,7 +44,18 @@ const Checkbox = React.forwardRef( ref, // eslint-disable-next-line @typescript-eslint/no-explicit-any ): ReactElement => { - const checkboxRef = useProvidedRefOrCreate(ref as React.RefObject) + const setIndeterminate = React.useCallback( + (node: HTMLInputElement | null) => { + if (node) { + node.indeterminate = indeterminate || false + } + }, + // `checked` is intentionally included: browsers clear the indeterminate state + // when checked changes, so we need the callback to re-run to restore it. + [indeterminate, checked], + ) + const mergedRef = useMergedRefs(ref, setIndeterminate) + const checkboxGroupContext = useContext(CheckboxGroupContext) const handleOnChange: ChangeEventHandler = e => { checkboxGroupContext.onChange && checkboxGroupContext.onChange(e) @@ -54,37 +64,19 @@ const Checkbox = React.forwardRef( const inputProps = { type: 'checkbox', disabled, - ref: checkboxRef, + ref: mergedRef, checked: indeterminate ? false : checked, defaultChecked, required, ['aria-required']: required ? ('true' as const) : ('false' as const), ['aria-invalid']: validationStatus === 'error' ? ('true' as const) : ('false' as const), + ['aria-checked']: indeterminate ? ('mixed' as const) : undefined, onChange: handleOnChange, value, name: value, ...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') - } - }) - // @ts-expect-error inputProp needs a non nullable ref return }, ) From 4c2c54d1f96646a5705d83374317f29cc1cfd0ba Mon Sep 17 00:00:00 2001 From: Alexander Lelidis Date: Fri, 17 Apr 2026 14:20:34 +0000 Subject: [PATCH 2/3] Add changeset Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .changeset/checkbox-ref-callback.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/checkbox-ref-callback.md diff --git a/.changeset/checkbox-ref-callback.md b/.changeset/checkbox-ref-callback.md new file mode 100644 index 00000000000..f17830c9665 --- /dev/null +++ b/.changeset/checkbox-ref-callback.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +Checkbox: replace useLayoutEffect with ref callback for setting indeterminate state From b13ac8a4b968e59f89674835530f3d1454deafe8 Mon Sep 17 00:00:00 2001 From: Alexander Lelidis Date: Fri, 17 Apr 2026 14:24:49 +0000 Subject: [PATCH 3/3] Fix lint: add eslint-disable for intentional checked dep Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- packages/react/src/Checkbox/Checkbox.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react/src/Checkbox/Checkbox.tsx b/packages/react/src/Checkbox/Checkbox.tsx index 80ce8ddfe40..a921ac54c32 100644 --- a/packages/react/src/Checkbox/Checkbox.tsx +++ b/packages/react/src/Checkbox/Checkbox.tsx @@ -52,6 +52,7 @@ const Checkbox = React.forwardRef( }, // `checked` is intentionally included: browsers clear the indeterminate state // when checked changes, so we need the callback to re-run to restore it. + // eslint-disable-next-line react-hooks/exhaustive-deps [indeterminate, checked], ) const mergedRef = useMergedRefs(ref, setIndeterminate)