Repository navigation
ValidatedToggleControl: Fix validation timing with callback ref - #75095
Closed
ntsekouras wants to merge 3 commits into
Closed
ntsekouras wants to merge 3 commits into
ntsekouras wants to merge 3 commits into
Conversation
ntsekouras
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ntsekouras
requested review from
aduth,
gigitux,
jorgefilipecosta,
mirka and
oandregal
January 30, 2026 15:13
8 tasks done
|
Size Change: +1.46 kB (+0.05%) Total Size: 3 MB
ℹ️ View Unchanged
|
aduth
reviewed
Jan 30, 2026
aduth
left a comment
aduth
left a comment
Member
There was a problem hiding this comment.
Could we add some test coverage for the bug?
| const validityTargetRef = useRef< HTMLInputElement >( null ); | ||
| const mergedRefs = useMergeRefs( [ forwardedRef, validityTargetRef ] ); | ||
|
|
||
| const validityTargetRef = useRef< HTMLInputElement | null >( null ); |
Member
There was a problem hiding this comment.
What's the reason for this change? And similarly in the setRequiredRef argument typings below, would it ever actually be passed null, or could it be HTMLInputElement ?
Contributor
Author
There was a problem hiding this comment.
What's the reason for this change?
We need to have a reference of the node for getValidityTarget below and since we use a callback ref, we need to make validityTargetRef mutable (by adding the | null). Without it the ref is readonly.
It can be passed null, yes.
If a function is not returned by the ref callback, React will call the callback again with null as the argument when the ref gets detached.
Member
There was a problem hiding this comment.
Noting that the next sentence of that quoted documentation is "This behavior will be removed in a future version", but tracks with how it behaves today 👍 But we also have a guard with early return that would that null assignment. Not a blocker in any case.
Member
There was a problem hiding this comment.
Considering our current rate of React upgrades, the "future version", might be in far future 😅
| }, [ required ] ); | ||
| // so we need to set it manually. Using a callback ref ensures `required` is set | ||
| // synchronously when the element mounts, before any validation effects run. | ||
| const setRequiredRef = useCallback( |
Member
There was a problem hiding this comment.
Does this need to be a useCallback, or could it just be a plain function?
Member
There was a problem hiding this comment.
Does this need to be a useCallback, or could it just be a plain function?
If you don't want the ref callback to be called on every re-render, we should use useCallback.
Unless you pass the same function reference for the ref callback on every render, the callback will get temporarily cleanup and re-create during every re-render of the component.
Ref: https://react.dev/reference/react-dom/components/common#ref-callback-parameters
ntsekouras
force-pushed
the
fix-validated-toggle-group/apply-required-with-ref-callback
branch
from
February 2, 2026 09:57
58375e2 to
83ab062
Compare
|
Flaky tests detected in 83ab062. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/21585474522
|
Member
Tracing back how this component was implemented, I think this could very well be the best path forward. We may not even need to pass every prop through if we wanted to be more controlled/conservative about it, just cc @mirka |
Member
|
Right, this workaround was originally written back when we couldn't make changes to |
Member
oandregal
deleted the
fix-validated-toggle-group/apply-required-with-ref-callback
branch
February 3, 2026 16:07
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Closes: #72321
This PR implements the last item of the linked issue around DataForms layout validation.
Why?
ValidatedToggleControlwas setting the required attribute on the underlying input via a useEffect. WhenreportValidity()was called (e.g., when a card form expands), the effect hadn't run yet, so the input didn't have therequiredattribute. Withoutrequired, an unchecked checkbox is considered valid by the browser, so no invalid event fired and no error indicator/message appeared.How?
Replace the useEffect with a callback ref. The callback ref sets required synchronously when the element mounts, ensuring it's in place before any validation effects run.
Possible alternative approach
We could make
ToggleControlpass rest props down toFormToggle(similar toTextControletc..). This would allowrequiredto flow through naturally via ControlWithError's cloneElement. Should we implement that? --cc @WordPress/gutenberg-componentsTesting Instructions
?path=/story/dataviews-dataform--validation&args=layout:card-collapsible) compare trunk and this PR (npm run storybook:dev) by going toBoolean fieldsand collapse the card2 validation issuesbadge, but when opening the card again you can see both validation messages at the inputs, only in this PR (see the videos below).Before
Screen.Recording.2026-01-30.at.5.08.05.PM.mov
After
Screen.Recording.2026-01-30.at.5.06.48.PM.mov