[field] Preserve custom validity set outside the field - #5348
[field] Preserve custom validity set outside the field#5348flaviendelangle wants to merge 6 commits into
Conversation
`Form`'s submit handler sets `submitAttemptedRef` before validating, so the submit pass itself runs as an on-change validation. That branch cleared the custom validity of every input and recomputed the field as valid, discarding messages the field never set. A control can own a validity condition that no native constraint expresses (a date field whose sections spell out February 30th, for example) and surface it with `setCustomValidity`. Since `Form` renders `noValidate`, the published validity is all that blocks submission, so dropping the message let the form submit an invalid value and made the `Field.Error` disappear. Track the messages set here and clear only those, read `customError` back off the element instead of assuming it was cleared, and keep the on-change revalidation path from publishing an all-valid state over a message it doesn't own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
commit: |
Bundle size
PerformanceTotal duration: 1,051.27 ms -17.15 ms(-1.6%) | Renders: 78 (+0) | Paint: 1,638.13 ms -23.63 ms(-1.4%) No significant changes — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…e revalidation Review follow-up: - Publish the element's actual state when an external custom error survives the required-value revalidation pass, instead of leaving the stale `valueMissing` error on display. `publishAllValid` is void again and the clear-and-check lives at its single call site. - Re-derive `valid` alongside the `customError` read-back so a message set while an async `validate` was pending cannot publish `valid: true` with `customError: true`. - Drop the WeakMap entry once the hook observes its message was overwritten, so a stale entry can't wipe a foreign message from a barred element later. - Pin the previously untested paths: the revalidation bail, the `customError` read-back, the overwritten-message refusal, recovery after the external message is withdrawn, a multi-line owned message round-trip, and a foreign message on a registered group input blocking submit. - Refresh comments that still described the pre-ownership clearing behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR reviewTwo merge-blocking validity-ownership defects remain, and a third multi-input path can publish a surviving error as valid. The existing Field and CheckboxGroup suites pass in jsdom and Chromium, but focused Chromium probes reproduced all three cases. Bugs (3)1. 🔴 Normalize owned messages before comparing themLocation: if (element.willValidate && element.validationMessage !== ownMessage) {
ownedCustomValidity.delete(element);
return;
}Chromium normalizes Failure scenario: Fix: Normalize line endings before calling 2. 🔴 Restore an external error displaced by the field validatorLocation: function setOwnCustomValidity(element: HTMLInputElement, message: string) {
element.setCustomValidity(message);
ownedCustomValidity.set(element, message);
}Installing an owned error overwrites any existing control-owned custom error without retaining it. When Failure scenario: A date control sets Fix: Retain a displaced custom message when installing an owned one and restore it when that owned message is cleared, unless external code has since replaced or withdrawn it. Add the reverse-order counterpart to the new “external overwrote own” test. 3. 🟠 Recompute the representative input after clearing errorsLocation: clearCustomValidity(element, registeredInputs);
nextState.customError = element?.validity.customError ?? false;
Failure scenario: Input A contains the field validator’s error and input B contains an external error. When the validator succeeds, A is cleared while B remains custom-invalid, but Fix: After clearing—and after any awaited validation—re-run VerdictRequest changes - the normalized-message regression and displaced external errors can leave Forms permanently blocked or incorrectly submit invalid values. 🤖 Review generated with Codex |
Chromium reports `\r\n` and `\r` back as `\n`, so a multi-line owned message read as foreign on the next pass and was stranded on the control. Match ownership on normalized text. Installing an owned message over one set by other code hid a condition this hook knows nothing about, and clearing the owned message then reported the control as valid. Remember the displaced message and hand the control back to it instead. Clearing an owned message can also promote another registered input — one that kept a message of its own — to representative, so resolve it again afterwards and derive the published state from it rather than from the stale one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Applied all three in 38c8246.
All three tests fail on the previous commit in Chromium and pass with the fix. 🤖 Generated with Claude Code |
PR reviewThe ownership-tracking approach is sound and well-tested, but two verified bugs survive — both independently flagged by a second reviewer and confirmed here with failing repro tests against this branch. The Bugs (4)1. 🔴 Stale displaced entry resurrects a withdrawn foreign messageLocation: function setOwnCustomValidity(element: HTMLInputElement, message: string) {
if (element.willValidate && element.validity.customError && !hasOwnCustomValidity(element)) {
displacedCustomValidity.set(element, element.validationMessage);
}
...(Found independently by both reviewers; confirmed by failing repro.) When the hook takes ownership of an element that currently has no custom error, it neither records nor deletes a Failure scenario: Verified with a failing jsdom repro on this branch — Fix: In 2. 🟠 Revalidate fast path misses a foreign message on a non-representative group inputLocation: clearCustomValidity(element, registeredInputs);
if (!element.validity.customError) {
...
publishAllValid(false);(Found independently by both reviewers; confirmed by failing repro.) Failure scenario: Verified with a failing jsdom repro — checkbox group, Fix: After 3. 🟡 Foreign-surviving revalidate publish surfaces deferred native errors mid-typingLocation: const nextValidityData = {
value,
state: getState(element),
...The revalidate path deliberately defers native errors other than Failure scenario: Fix: Publish only the surviving custom error on this path, e.g. 4. ℹ️ Barred elements bypass displacement recording and ownership verificationLocation: For Simplifications (1)1. 🟡 Unreachable branch left behind after the fresh state re-readLocation: } else if ((!currentElement || currentElement.validity.valid) && !nextState.valid) {
nextState.valid = true;
}Before this PR, Failure scenario: Three unreachable lines that future readers must reason about (and that suggest a state divergence which can no longer occur). Fix: Delete the VerdictRequest changes — the confirmed displaced-message resurrection (finding 1) creates a sticky, submit-blocking phantom error in the exact integration this PR exists to support. 🤖 Review generated with Claude Code |
PR reviewThe latest revision fixes the three earlier findings, and CI is green. One merge-blocking ownership defect remains, plus the stale representative bug still exists in the separate Bugs (2)1. 🔴 Drop stale displaced errors before installing another owned errorLocation: function setOwnCustomValidity(element: HTMLInputElement, message: string) {
if (element.willValidate && element.validity.customError && !hasOwnCustomValidity(element)) {
displacedCustomValidity.set(element, element.validationMessage);
}
element.setCustomValidity(message);
ownedCustomValidity.set(element, normalizeValidationMessage(message));
}When outside code withdraws its error by calling If This ordering is realistic with Failure scenario: A date control sets Fix: At the beginning of Add a regression test with three commits:
2. 🟠 Re-resolve the representative in the revalidation branchLocation: clearCustomValidity(element, registeredInputs);
if (!element.validity.customError) {
publishAllValid(false);
return;
}
The later on-change success branch correctly calls Failure scenario: An on-blur CheckboxGroup validator places its error on the first hidden input. A separate control-owned error is then placed on the second input. On the next checkbox change, the first error is cleared, the second remains invalid, but Fix: Resolve the representative again immediately after VerdictRequest changes — the stale displaced-message entry can resurrect a resolved error and indefinitely block valid Form submission. 🤖 Review generated with GPT-5.6 Thinking |
Outside code can withdraw its message while the field's own error is displayed. Clearing wipes the single slot both share, so a displaced record left from an earlier pass no longer describes a live condition. Drop it when the control carries no custom error instead of restoring a withdrawn message later, which kept the field — and the surrounding Form — invalid for good. Change revalidation resolved its representative input before clearing owned messages, so a group whose remaining error sits on another input was published as valid. Resolve it again after the clear, and publish only the surviving custom error so co-occurring native errors stay deferred to the blur or submit boundary like the resolved `valueMissing` one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks both — all findings are addressed in ce28649. 🔴 Stale displaced entry resurrects a withdrawn foreign message — fixed. 🟠 Revalidate fast path misses a foreign message on a non-representative group input — fixed. The representative is re-resolved after 🟡 Foreign-surviving revalidate publish surfaces deferred native errors mid-typing — fixed. That path now publishes 🟡 Unreachable branch after the fresh state re-read — deleted. Confirmed dead: ℹ️ Barred elements bypass displacement recording and ownership verification — left as-is behaviorally, since it needs outside code writing custom validity to a barred element ( All three new tests were verified to fail against the previous revision and pass with the fix. |
PR reviewNo finding blocks merge outright, but one is close: controls that maintain external custom validity from a React effect are published one event late in both directions, so a withdrawn message visibly lingers as a stale Bugs (4)1. 🟠 Effect-driven external validity publishes one event late; withdrawn messages linger as stale field stateLocation: const currentElement = resolveRepresentativeInput();
Object.assign(
nextState,
currentElement ? getState(currentElement) : { ...DEFAULT_VALIDITY_STATE, valid: true },
);
Failure scenario: an on-change date field shows "invalid date" for February 30th; the user corrects it to the 28th on their final keystroke; the input is natively valid but the error and invalid styling stick until blur. Fix: this is inherent to pull-based ownership, and post-effect resync machinery isn't worth its bytes; instead make the contract explicit: external 2. 🟡
|
|
Addressed in #5449 |
A control can own a validity condition that no native constraint expresses — a date field whose sections spell out February 30th, a range field whose bounds are reversed — and the natural way to surface it is
setCustomValidityon the control's input. Today the field throws that away.Form's submit handler setssubmitAttemptedRefbefore callingfield.validate()on each field, soshouldValidateOnChange()is alreadytruefor the submit pass itself. That pass takes thevalidate-branch inuseFieldValidation.commit(), which callsclearCustomValidity()on every input — indiscriminately, including messages the field never set — forcescustomErrortofalse, and then recomputes the field as valid fromelement.validity. SinceFormrendersnoValidate, the publishedvalidityDatais the only thing gating submission, so the form submits an invalid value and the<Field.Error>disappears.The same clear runs on every subsequent change commit (
submitAttemptedRefis never reset), so once a form has been submitted the message can't come back.Changes
WeakMapand clear only those. The message comparison is skipped when!element.willValidate, because elements barred from constraint validation reportvalidationMessage === ''— that case is exercised by the existingCheckboxGroupdisabled-input test.nextState.customErrorback offelement.validityinstead of assuming the clear removed it.publishAllValidbail when a foreign custom error survives the clear, so the on-change revalidation path can't publish an all-valid state over a message it doesn't own either.Messages the field sets itself (from the
validateprop) are cleared exactly as before.Testing
Three tests in
FieldRoot.test.tsx. The two "keeps a message set outside the field" tests fail onmaster; "clears the message it set itself" passes both before and after, as a guard against over-fixing.Verified end to end against Base UI Plus'
DateField, which surfaces its invalid-date state this way: with this change and no Plus-side edits, entering February 30th blocks<Form>submission and keeps the error visible, in both jsdom and Chromium.🤖 Generated with Claude Code