Checkbox: replace useLayoutEffect with ref callback for indeterminate#7765
Checkbox: replace useLayoutEffect with ref callback for indeterminate#7765
Conversation
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>
🦋 Changeset detectedLatest commit: b13ac8a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
github-actions
bot
added
the
integration-tests: recommended
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes per-render React effects from Checkbox by using a ref callback to set the DOM indeterminate property during commit and by making aria-checked declarative for the indeterminate case.
Changes:
- Replaced
useLayoutEffectindeterminate synchronization with auseCallbackref callback merged viauseMergedRefs. - Replaced the imperative
aria-checkeduseEffectwith a declarativearia-checked={indeterminate ? 'mixed' : undefined}. - Updated the related unit test and added a changeset entry.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Checkbox/Checkbox.tsx | Removes layout/effects by using a merged ref callback for indeterminate and declarative aria-checked for indeterminate. |
| packages/react/src/Checkbox/Checkbox.test.tsx | Updates assertions to align with relying on native checked semantics when not indeterminate. |
| .changeset/checkbox-ref-callback.md | Documents the patch-level change in a changeset. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
| ['aria-checked']: indeterminate ? ('mixed' as const) : undefined, | ||
| onChange: handleOnChange, | ||
| value, | ||
| name: value, | ||
| ...rest, |
There was a problem hiding this comment.
...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.
| }, | ||
| // `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 |
There was a problem hiding this comment.
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.
| // eslint-disable-next-line react-hooks/exhaustive-deps |
| it('renders an aria-checked attribute correctly', () => { | ||
| const handleChange = vi.fn() | ||
| const {getByRole, rerender} = render(<Checkbox checked={false} onChange={handleChange} />) | ||
|
|
||
| const checkbox = getByRole('checkbox') as HTMLInputElement |
There was a problem hiding this comment.
This test name no longer matches what’s being asserted (it now checks checkbox.checked rather than aria-checked for the non-indeterminate cases). Consider renaming the test and/or explicitly asserting that aria-checked is not present when indeterminate is false, so the declarative behavior is actually covered.
Summary
Replaces
useLayoutEffectanduseEffectin theCheckboxcomponent with a ref callback pattern, eliminating layout effects from every Checkbox render.Changes
useLayoutEffect→ ref callback: UsesuseCallback+useMergedRefsto set.indeterminateon the DOM node synchronously during commit (same timing guarantees, no registered effect)useEffect→ inline JSX: Replaces the imperativearia-checkedeffect with a declarativearia-checked={indeterminate ? 'mixed' : undefined}attribute. Non-indeterminate checkboxes rely on native checked state for accessibility.useEffect,useLayoutEffect,useProvidedRefOrCreateMotivation
Layout effects run synchronously after DOM mutations and block painting. While individually cheap, they add up in pages rendering many checkboxes (e.g., list views with bulk selection). This change removes one layout effect and one regular effect per Checkbox instance.
Testing
aria-checkedtest to reflect that non-indeterminate checkboxes use native checked stateCloses #7764