Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/checkbox-ref-callback.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

Checkbox: replace useLayoutEffect with ref callback for setting indeterminate state
5 changes: 3 additions & 2 deletions packages/react/src/Checkbox/Checkbox.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(<Checkbox checked={true} onChange={handleChange} />)

expect(checkbox).toHaveAttribute('aria-checked', 'true')
expect(checkbox.checked).toEqual(true)

rerender(<Checkbox indeterminate checked onChange={handleChange} />)

Expand Down
41 changes: 17 additions & 24 deletions packages/react/src/Checkbox/Checkbox.tsx
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -45,7 +44,19 @@ const Checkbox = React.forwardRef<HTMLInputElement, CheckboxProps>(
ref,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
): ReactElement<any> => {
const checkboxRef = useProvidedRefOrCreate(ref as React.RefObject<HTMLInputElement>)
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.
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The eslint-disable-next-line react-hooks/exhaustive-deps looks unnecessary here because the dependency list is already explicit and complete. Keeping this suppression can hide real missing-dependency issues in future edits; consider removing the disable and leaving the explanatory comment about checked triggering a rerun.

Suggested change
// eslint-disable-next-line react-hooks/exhaustive-deps

Copilot uses AI. Check for mistakes.
[indeterminate, checked],
)
const mergedRef = useMergedRefs(ref, setIndeterminate)

const checkboxGroupContext = useContext(CheckboxGroupContext)
const handleOnChange: ChangeEventHandler<HTMLInputElement> = e => {
checkboxGroupContext.onChange && checkboxGroupContext.onChange(e)
Expand All @@ -54,37 +65,19 @@ const Checkbox = React.forwardRef<HTMLInputElement, CheckboxProps>(
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,
Comment on lines +74 to 78
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...rest is spread after the computed props, so a consumer-provided aria-checked (and, in React 19 where ref is a normal prop, potentially ref) can override the component-controlled values. That’s a behavior change vs the previous effect-based implementation and can break the required aria-checked="mixed" semantics for indeterminate checkboxes (or bypass setIndeterminate if ref is overridden). Consider spreading rest earlier (or explicitly omitting/overriding aria-checked/ref) so the indeterminate accessibility and ref callback behavior can’t be overridden unintentionally.

Copilot uses AI. Check for mistakes.
}

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 <input {...inputProps} className={clsx(className, sharedClasses.Input, classes.Checkbox)} />
},
)
Expand Down
Loading