Skip to content

fix(NumberInput): reject non-numeric characters on input#3761

Open
ReddyWish wants to merge 1 commit intomainfrom
fix/number-input-character-filtering
Open

fix(NumberInput): reject non-numeric characters on input#3761
ReddyWish wants to merge 1 commit intomainfrom
fix/number-input-character-filtering

Conversation

@ReddyWish
Copy link
Copy Markdown
Contributor

@ReddyWish ReddyWish commented Mar 25, 2026

Why

NumberInput uses type="text" (instead of type="number") to support locale-specific decimal separators (both , and .) and full control over formatting. However, there was no mechanism to prevent non-numeric characters at the input level.

The existing handleChangeextractNumber pipeline only validated after the character entered the DOM. Due to InputField maintaining its own internal localValue state independently from NumberInput's fieldValue, rejecting input in handleChange didn't reset the displayed value — InputField's useEffect that syncs from the value prop wouldn't fire because the prop value hadn't changed. This caused letters and symbols to visually persist in the input even though onChange never emitted invalid values.

onBeforeInput intercepts characters before the DOM mutation, preventing InputField from ever seeing invalid input — which is the standard approach for type="text" inputs that need character filtering (used by react-number-format, react-imask, etc.).

What

Add onBeforeInput handler to NumberInputInternal that prevents non-numeric characters from entering the input field. The handler simulates the resulting value before the DOM mutation and calls e.preventDefault() if the result wouldn't match the number format regex.

Also simplifies the handleChange logic by using the already-extracted formattedValue directly and renaming finalValueclampedValue for clarity.

Before

Screen.Recording.2026-03-25.at.16.44.04.mov

After

Screen.Recording.2026-03-25.at.16.45.11.mov

@ReddyWish ReddyWish requested a review from a team as a code owner March 25, 2026 15:46
Copilot AI review requested due to automatic review settings March 25, 2026 15:46
@github-actions github-actions bot added fix react Changes affect packages/react labels Mar 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor

✅ No New Circular Dependencies

No new circular dependencies detected. Current count: 0

@github-actions
Copy link
Copy Markdown
Contributor

📦 Alpha Package Version Published

Use pnpm i github:factorialco/f0#npm/alpha-pr-3761 to install the package

Use pnpm i github:factorialco/f0#a989ab02f511474f830a103382947cad563e3d3a to install this specific commit

@github-actions
Copy link
Copy Markdown
Contributor

🔍 Visual review for your branch is published 🔍

Here are the links to:

@github-actions
Copy link
Copy Markdown
Contributor

Coverage Report for packages/react

Status Category Percentage Covered / Total
🔵 Lines 44.11% 10391 / 23555
🔵 Statements 43.43% 10712 / 24663
🔵 Functions 36.13% 2347 / 6495
🔵 Branches 35.38% 6562 / 18545
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/react/src/experimental/Forms/Fields/NumberInput/internal.tsx 71.92% 68.75% 88.88% 73.58% 44, 53, 62, 79, 88, 90-91, 112-121
Generated in workflow #12194 for commit c14abd7 by the Vitest Coverage Report Action

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds proactive character filtering to the experimental NumberInput so invalid characters are blocked before they reach InputField’s internal localValue, reducing visual desync between displayed text and emitted numeric values.

Changes:

  • Add an onBeforeInput handler to block non-numeric inserts based on simulated post-insert value.
  • Refactor handleChange to reuse formattedValue from extractNumber and rename finalValueclampedValue.
  • Format clamped values using Intl.NumberFormat instead of re-parsing via extractNumber.

Comment on lines +73 to 85
const clampedValue = Math.max(
min ?? -Infinity,
Math.min(max ?? Infinity, parsedValue)
)

const finalExtractedData = extractNumber(finalValue.toString(), {
maxDecimals,
})
setFieldValue(finalExtractedData?.formattedValue ?? "")
if (clampedValue !== parsedValue) {
setFieldValue(formatValue(clampedValue, locale, maxDecimals))
} else {
setFieldValue(formattedValue)
}

onChange?.(finalExtractedData?.value ?? null)
onChange?.(clampedValue)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

When clamping applies, the display string is produced via Intl.NumberFormat (which rounds and uses default fraction digits when maxDecimals is undefined), but the emitted value is the raw clampedValue. This can desync what the user sees vs what onChange receives (e.g. max=1.235, maxDecimals=2 → UI shows 1.24 while onChange emits 1.235). Consider normalizing clampedValue through the same maxDecimals logic used for user input (and deriving both formatted + numeric values from that single normalization) before calling setFieldValue/onChange.

Copilot uses AI. Check for mistakes.
Comment on lines +111 to +123
const handleBeforeInput = (e: React.FormEvent<HTMLInputElement>) => {
const { data } = e.nativeEvent as InputEvent
if (!data) return

const input = e.currentTarget
const start = input.selectionStart ?? 0
const end = input.selectionEnd ?? 0
const newValue = input.value.slice(0, start) + data + input.value.slice(end)

if (!extractNumber(newValue, { maxDecimals })) {
e.preventDefault()
}
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

handleBeforeInput currently only prevents input when extractNumber(newValue) returns null (regex mismatch). extractNumber can also truncate/normalize input (e.g. extra decimals beyond maxDecimals), so those characters can still enter the DOM and then persist visually because InputField’s localValue won’t resync when the controlled value string doesn’t change. Consider also preventing inserts that would be truncated/ignored by extractNumber (while preserving the intentional partial states like a trailing decimal separator).

Copilot uses AI. Check for mistakes.
inputMode="decimal"
onChange={handleChange}
{...props}
onBeforeInput={handleBeforeInput}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

This PR introduces new user-facing behavior (character filtering via onBeforeInput), but there are no tests covering it. Since this component already has a NumberInput.test.tsx suite, add cases asserting that non-numeric characters (letters/symbols, and paste) are prevented and don’t update the displayed value or trigger onChange.

Copilot generated this review using guidance from repository custom instructions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix react Changes affect packages/react

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants