[number field] Add imperative control of the raw input value - #5421
[number field] Add imperative control of the raw input value#5421michaldudak wants to merge 6 commits into
Conversation
`value` holds a parsed number, so it cannot represent text that is not a number yet. Typing `-1.5` passes through `-` and `-1.`, neither of which parses, leaving no way to drive the field through those states programmatically. Add `actionsRef` with a `setInputValue` action, following the pattern `Popover.Root` already uses. The action mirrors what typing the same string does, minus the per-character gating that exists to reject keystrokes: the text is written verbatim, `value` follows it when it parses and is left alone when it does not, and an empty string clears. Marking the text unsynced is what lets an intermediate string survive the formatting sync, so the text behaves as an unsaved edit exactly as typed text does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
commit: |
michaldudak
added
component: number field
Bundle size
PerformanceTotal duration: 1,219.16 ms -28.18 ms(-2.3%) | Renders: 78 (+0) | Paint: 1,941.19 ms -6.95 ms(-0.4%)
14 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Address review feedback on the `setInputValue` action:
- Commit as soon as the text parses, instead of waiting for a blur that may
never come when the input is driven from outside the field. `keyboard`,
`wheel` and the increment/decrement buttons already commit at the point of
change; only typing defers, and typing implies focus. Clearing the text
commits too, since that also changes the value.
- Keep the action working while `disabled` or `readOnly` — it is an escape
hatch for the owner of the component, not a simulated keystroke — and say so
in the JSDoc and the docs instead of implying it mirrors typing.
Docs also now state that `value` is clamped even though the text isn't, and
describe the actual reclaim rule: blur ends the unsaved edit and re-arms the
formatting sync, so the display returns to the formatted value on the next
render, not on the next value-changing interaction.
Tests pin the commit timing, the `disabled`/`readOnly` bypass, clamping,
format-relative parsing, and the post-blur reclaim. The intermediate-string
test now uses `min={0}`, where typing `-` is rejected, so it actually covers
the gating bypass it claims to.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`setInputValue` accepted any string, so it could put the field into states a user could never type into it. It now runs the same per-character validation the typed path runs, and ignores text that fails it. The predicate moves to `isValidInputString` in the number field's parse utils and is called from both `NumberFieldInput`'s `onChange` and the action, so the two paths accept exactly the same strings by construction. Partial entries like `'-'` and `'1.'` still pass, since validation is per character rather than a parse. A rejected call is a complete no-op — unlike the typed path it doesn't mark the text dirty first, because there is no keystroke to swallow — and warns in development, since silently doing nothing gives the caller no way to tell. `disabled` and `readOnly` are still not consulted: those gate user interaction, not the component's owner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`setInputValue` branched on `isEmpty` three times, including a compound `!isEmpty && parsedValue === null` guard that had to be read twice to see it meant "valid text that isn't a number yet". Fold that into one `nextValue` whose `undefined` case means "leave `value` alone", so the guard is a single identity check. In the tests, hoist the repeated set-text click into `clickSetText()` (it appeared 20 times), drop the `label` prop of `SetInputValueApp` that nothing passed, and collapse two pairs of tests that differed only in one value into loops: the rejected-text cases and the `disabled`/`readOnly` cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
michaldudak
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a7e6da37e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // | ||
| // `disabled` and `readOnly` are still not consulted — those gate user interaction, and | ||
| // this is the component owner driving the field deliberately. | ||
| if (!isEmpty && !isValidInputString(nextInputValue, getAllowedNonNumericKeys())) { |
There was a problem hiding this comment.
When callers pass text made only of allowed characters but not a valid number-field sequence, this guard lets it through even though the input key handling would block the same state. For example, actionsRef.current.setInputValue('1.2.3') passes the per-character check, then parseNumber normalizes it to 12.3, so onValueChange and the hidden submitted value become 12.3 while the visible input shows 1.2.3; validate the whole string against the same structural constraints before applying the imperative action.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, including a detail the comment doesn't spell out: the hidden input submits 12.3 while the field shows 1.2.3, so the divergence reaches form submission.
It isn't specific to the imperative action, though. The same string through a plain change event — drag-and-drop, IME, autofill — already does this on master:
fireEvent.change(input, { target: { value: '1.2.3' } })
→ visible "1.2.3", onValueChange(12.3)
isValidInputString is the onChange predicate extracted verbatim, so the action inherits the behaviour rather than introducing it. Typing is indeed blocked, but that gate lives in onKeyDown and has never applied to non-keystroke text entry.
The cause is the "keep only the last ." normalization in parseNumber (utils/parse.ts:190-193), not the character validation being too permissive. That collapse is deliberate and tested — it's what makes a European-formatted paste like 1.234.567.89 resolve to 1234567.89 in a US-locale field (utils/parse.test.ts:193-204). 1.2.3 is the same rule meeting a string that isn't plausible in any locale.
Edited: an earlier version of this reply blamed the global decimal-separator replace. The replace is global, but it's the lastIndexOf('.') collapse that produces 12.3.
Adding a structural check inside the action alone would reintroduce exactly the typed/imperative divergence this PR set out to remove, so this is better fixed once in the shared parse/validation layer, where it also fixes the drop/IME path. Tracked separately in #5424.
| </NumberField.Root>; | ||
|
|
||
| // Puts the field into a valid intermediate state that `value` cannot express. | ||
| actionsRef.current.setInputValue('-'); |
There was a problem hiding this comment.
When users copy this tsx example into a strict TypeScript project, actionsRef.current is still typed as NumberField.Root.Actions | null from the declaration above, so calling .setInputValue() directly fails with a possibly-null ref error. Use optional chaining or a non-null assertion in the example so the new public API snippet compiles as shown.
Useful? React with 👍 / 👎.
| // formatting sync would otherwise overwrite it with the formatting of `value` on the next | ||
| // render, exactly as it does mid-typing. | ||
| allowInputSyncRef.current = false; | ||
| setInputValue(nextInputValue); |
There was a problem hiding this comment.
When onValueChange calls details.cancel() for an imperative-action, this branch has already disabled input sync and written nextInputValue into visible state before setValue can return false. A canceled actionsRef.current.setInputValue('42') over value 5 therefore leaves the textbox showing 42 while the stored/form value remains 5, unlike canceled keyboard changes that leave the display unchanged; move the visible text update after the cancellation check or roll it back when setValue is canceled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining — the imperative path already behaves the same as the typed one here:
canceled imperative → visible "42"
canceled typed → visible "42"
The comparison to canceled keyboard changes isn't the right baseline. Arrow keys never write the text directly; they reach the display through the formatting sync, so cancelling naturally leaves it untouched. For direct text entry, keeping the entered text on cancel is the existing behaviour — onChange writes the text before setValue too — and the action matches it deliberately, since the text is modelled as an unsaved edit that survives until an interaction reconciles it.
Cancelling does correctly suppress the commit added in this PR: onValueCommitted fires 0 times, because setValue returns false.
`actionsRef.current` is typed `Actions | null`, so the snippet errored when copied into a project with `strictNullChecks`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Alternative to #5412. Same problem, imperative API instead of the prop trio. Opened for comparison — these are mutually exclusive; only one should land.
Problem
valueholds a parsed number, so it can't represent text that isn't a number yet. Typing-1.5passes through-and-1., neither of which parses, and both leavevalueuntouched. Those are legal states internally — typing-already leaves the text as-withvalueatnull— but there was no way to reach them programmatically.Solution
Adds
actionsRefwith asetInputValueaction, following the patternPopover.Rootalready uses.The action mirrors what typing the same string does, minus the per-character gating that exists to reject keystrokes — the caller chose the string deliberately.
valuefollows the text when it parses, is left alone when it doesn't (so'-'can sit over an existing number without clearing it), and an empty string clears it. Changes reportreason: 'imperative-action'.Comparison with #5412
useControlledtext, authorship tracking, write dedupestate.inputValueonlyonInputValueChangeeventDetails.cancel()The imperative version is much smaller because it reuses the dirty-text machinery that already exists for typing, rather than making the text a controllable value.
The cost is that the caller can't observe or negotiate. Text set this way behaves as an unsaved edit: it survives re-renders and external
valuechanges with no callback to say so, and the field reclaims the display on the next value-changing interaction.should survive a later external value change until an interaction reconciles itpins that, including the fact that blur does not reclaim it when the text can't parse — the field goes on showing something its value contradicts until an interaction resolves it.That last part is a pre-existing quirk on
master, not introduced here. #5412 fixes it as part of its refactor; this PR leaves it in place, which is the honest cost of the smaller diff.Testing
9 new tests covering the intermediate-string cases, value coupling, the reason, and the reconciliation behavior above.
Full jsdom suite (7513) green, 400 jsdom / 430 chromium NumberField tests, forced
tsgo -bclean, plus eslint, stylelint, prettier,valelint, and an idempotentdocs:validate.🤖 Generated with Claude Code