Skip to content

[number field] Respect rounding mode on blur#4804

Open
atomiks wants to merge 18 commits into
mui:masterfrom
atomiks:codex/number-field-rounding-mode
Open

[number field] Respect rounding mode on blur#4804
atomiks wants to merge 18 commits into
mui:masterfrom
atomiks:codex/number-field-rounding-mode

Conversation

@atomiks
Copy link
Copy Markdown
Contributor

@atomiks atomiks commented May 11, 2026

Fixes #4803

Keep the committed value in sync with Intl rounding options when a number field rounds on blur.

Root cause

The blur commit path used toFixed, so it rounded with the default mode even when the display formatter used options like roundingMode: floor. Percent values also need to round at their displayed scale before converting back to the stored fraction.

Changes

  • Round committed values through the shared NumberField validation helper when explicit precision is configured.
  • Scale percent values before rounding so stored fractions match the displayed percent precision.
  • Use Intl rounding options only when roundingMode or roundingIncrement is present, preserving the existing toFixed path otherwise.
  • Add regression coverage for rounding mode, rounding increment, localized input/display cases, and percent display-scale rounding.

@atomiks atomiks added component: number field Changes related to the number field component. type: bug It doesn't behave as expected. labels May 11, 2026
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 11, 2026

commit: 4fa5370

@code-infra-dashboard
Copy link
Copy Markdown

code-infra-dashboard Bot commented May 11, 2026

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+398B(+0.09%) 🔺+171B(+0.12%)

Details of bundle changes

Performance

Total duration: 1,151.52 ms -20.95 ms(-1.8%) | Renders: 50 (+0) | Paint: 1,766.02 ms -39.04 ms(-2.2%)

Test Duration Renders
Mixed surface mount (app-like density) 87.37 ms 🔺+21.17 ms(+32.0%) 5 (+0)
Scroll Area mount (300 instances) 66.81 ms ▼-19.00 ms(-22.1%) 3 (+0)
Popover mount (300 instances) 61.90 ms ▼-18.38 ms(-22.9%) 1 (+0)
Tooltip mount (300 contained roots) 44.33 ms ▼-11.99 ms(-21.3%) 1 (+0)

8 tests within noise — details


Check out the code infra dashboard for more information about this PR.

@atomiks atomiks force-pushed the codex/number-field-rounding-mode branch from bc5b17c to 11213c4 Compare May 11, 2026 05:32
@netlify
Copy link
Copy Markdown

netlify Bot commented May 11, 2026

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit bc5b17c
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6a01693a286f0100087f0591
😎 Deploy Preview https://deploy-preview-4804--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link
Copy Markdown

netlify Bot commented May 11, 2026

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 4fa5370
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6a0c2628a8b44000085ace29
😎 Deploy Preview https://deploy-preview-4804--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@atomiks atomiks marked this pull request as ready for review May 11, 2026 09:47
Copy link
Copy Markdown
Member

@mj12albert mj12albert left a comment

Choose a reason for hiding this comment

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

Codex finding:

style: 'percent' still rounds the raw fraction, not the displayed percent.
In validate.ts:37, the new Intl path builds a plain decimal formatter. But percent inputs are parsed as fractions in parse.ts:211, and blur commits via NumberFieldInput.tsx:191. For format={{ style: 'percent', maximumFractionDigits: 2, roundingMode: 'floor' }}, 1.239% parses to 0.01239, commits as 0.01, and displays as 1%; Intl would display the parsed value as 1.23%. Consider scaling percent values before applying the helper’s fraction rounding.

@atomiks atomiks force-pushed the codex/number-field-rounding-mode branch 2 times, most recently from bfa245c to b798d64 Compare May 12, 2026 05:43
@atomiks atomiks force-pushed the codex/number-field-rounding-mode branch from b798d64 to ca1af48 Compare May 12, 2026 05:48
Copy link
Copy Markdown
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

PR Review Summary — mui/base-ui #4804

[number field] Respect rounding mode on blur — 4 files, +191/-21, fixes #4803

The PR correctly fixes the core bug (toFixed ignoring Intl roundingMode) and the percent-scale issue. Tests are well-structured. However, the refactor introduces two critical silent failures and the type-design / behavior of getMaximumFractionDigits has real correctness concerns.


Critical Issues (must fix before merge)

  1. NaN leak via Number(Intl.NumberFormat.format(Infinity))packages/react/src/number-field/utils/validate.ts:49
    Number.isFinite(value) guards the input, but valueToRound = value * 100 can overflow to Infinity; en-US formats that as "∞", Number(...) returns NaN, which onValueChange silently emits. The pre-PR code returned a finite-or-Infinity number — this PR turns that into a NaN leak.

  2. Uncaught RangeError on invalid roundingMode / roundingIncrementpackages/react/src/number-field/utils/validate.ts:40-49
    A user passing e.g. roundingIncrement: 3 (not in Intl's allowed set) throws on every blur. The old toFixed path silently ignored those fields. No try/catch, no Base UI Plus: warning per AGENTS.md guidelines.

  3. getMaximumFractionDigits returns wrong digit count when only minimumFractionDigits is setpackages/react/src/number-field/utils/validate.ts:13-22
    { minimumFractionDigits: 2 } resolves to max=3 (Intl picks max(min, 3)), so user typing 1.2399 rounds to 1.240, not 1.24. Worse: pre-PR code skipped rounding entirely in this branch (it required typeof maxFrac === 'number'), so the PR introduces a behavior change beyond what the description claims. The existing percent min-only test only passes because floor + that input gives the same result at 2 or 3 digits.


Important Issues (should fix)

  1. getMaximumFractionDigits conditional is misleadingpackages/react/src/number-field/utils/validate.ts:14-19
    The format?.minimumFractionDigits == null ? undefined : format branch partially fixes currency/percent defaults but only for one input shape. Either always pass format or always pass undefined — the middle ground silently introduces inconsistency.

  2. roundingMode: string and roundingIncrement: number discard standard-library invariantspackages/react/src/number-field/utils/validate.ts:6-9
    TS 6.0.3's lib.es2023.intl.d.ts already encodes precise literal unions (9 mode values, 15 increments). The local type alias is strictly weaker than what the standard provides. The real fix: bump tsconfig.base.json lib from ["es2022", "dom"] to ["es2023", "dom"] and delete the alias entirely. Node 22+ and all evergreen browsers support these at runtime.

  3. Rounding-mode test coverage only exercises 'floor'
    At minimum add 'halfEven' (1.235 → 1.24, 1.245 → 1.24, the canonical proof the path goes through Intl.NumberFormat) and a negative-value 'trunc'/'floor' divergence test (e.g. -1.239). Without these, a future regression to toFixed semantics could pass every new test.

  4. roundingIncrement blur integration test asserts display only, not committed valuepackages/react/src/number-field/input/NumberFieldInput.test.tsx:643-667
    Add one assertion on onValueChange.mock.lastCall?.[0].

  5. style: 'currency' blur path is untested
    Currency does not get the *100 scale; one blur test would lock that contract down.


Suggestions

  1. Comment wording invites rotpackages/react/src/number-field/utils/validate.ts:6
    "configured Intl types … yet" obscures that tsconfig.base.json lib: ["es2022"] is the real knob. Either bump lib (recommended) or reword to name the gating setting so it's clear when to delete the alias.

  2. Missing comments on the two trickiest lines
    isPercentWithExplicitPrecision + scale=100 trick, and the min-only ? undefined : format conditional both deserve a one-line WHY.

  3. Untested edge cases
    Negative numbers, NaN early-return, currency-style, and step+roundingMode interaction.

  4. Redundant tests
    packages/react/src/number-field/utils/validate.test.ts:37-51 packs two cases in one it(); the percent min-only path is covered twice (unit and integration).


Strengths

  • Correct percent-scale fix; style:'unit', unit:'percent' correctly excluded from *100 scaling and tested.
  • useGrouping: false + 'en-US' formatter guarantees safe Number(...) parse (no scientific notation in standard notation).
  • renderControlledNumberField helper and it.each matrix for fr-FR / ar-EG locales are high-signal, DAMP-readable tests.
  • Commit messages follow [number field] … scope convention.
  • Import path NumberFieldInput.tsx → ../utils/validate is appropriate; no layering concern.

Recommended Action

  1. Fix the critical issues first — guard the Number(formatter.format(...)) result with Number.isFinite, wrap formatter construction in try/catch with a Base UI Plus: dev warning, and decide the min-only → resolved-max semantics deliberately (probably: use format.minimumFractionDigits directly).
  2. Bump tsconfig lib to es2023 and delete the type alias.
  3. Add halfEven and negative-value rounding tests, plus an onValueChange assertion for the roundingIncrement blur test.
  4. Reword or remove the misleading "configured Intl types" comment.
  5. Re-run /pr-review-toolkit:review-pr after changes.

@atomiks
Copy link
Copy Markdown
Contributor Author

atomiks commented May 13, 2026

@flaviendelangle

  1. Guarding overflow/NaN is now handled. I don’t think we should add a try/catch or warning for invalid Intl options here though - invalid format options already throw through existing Intl.NumberFormat calls
  2. Repo-wide lib bump is out of scope of this bug fix
  3. Added new tests
  4. Reworded the comment

Some points are invalid, addressed the valid ones:

Invalid / misleading

The invalid roundingMode / roundingIncrement RangeError is not a new silent failure from this helper. Invalid Intl options already throw through existing formatting/parsing paths. Also AGENTS says Base UI: for public package Error constructors, not “Base UI Plus:” warnings.

The min-only precision claim is wrong. { minimumFractionDigits: 2 } resolving maximumFractionDigits to 3 is exactly Intl behavior. 1.2399 formats to 1.24, not meaningfully “1.240” as a number. The percent min-only test also does catch 2-vs-3 digit behavior.

The getMaximumFractionDigits conditional is not obviously wrong; it preserves old decimal-default behavior unless precision is explicit, while still resolving style-specific defaults for min-only precision.

Copy link
Copy Markdown
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

PR #4804 Re-Review — Second Pass

Repo: mui/base-ui
PR: #4804 — [number field] Respect rounding mode on blur
HEAD: db7a23065 ([number field] Harden rounding edge cases)
Fixes: #4803

The author addressed most prior findings well, but the overflow "fix" introduces a new high-severity divergence between onValueChange and onValueCommitted. I'm also retracting one of my previous critical findings — the author's rebuttal was correct.


What's resolved (acknowledged)

  • Overflow → NaN leak (prior C1): the Number.isFinite(valueToRound) guard at validate.ts:36-38 does what it says.
  • RangeError rebuttal (prior C2) — sound: parse.ts:74, NumberFieldRoot.tsx:147,269,314,699 already call getFormatter with the user's format on render/keystroke. An invalid roundingIncrement: 3 crashes on mount, not on blur. No new surface added.
  • Test gaps: halfEven (both directions), negative-value mode divergence, currency blur, and onValueChange assertion for roundingIncrement are all properly closed.
  • Comment: factually accurate now (tsconfig.base.json:5 confirms lib: ["es2022", "dom"]).

Retracted from prior review

  • Min-only-precision (prior C3) — withdraw. With {minimumFractionDigits: 2}, getMaximumFractionDigits resolves digits = 3 (Intl decimal default), but Intl display also uses max=3 for the same options. Committed numeric value matches displayed text. Author was correct.

NEW high-severity finding

Infinity leaks past the overflow guard into onValueCommitted and the visible inputNumberFieldInput.tsx:185-218

On blur with style: 'percent' + a finite-but-huge value (Number.MAX_VALUE), removeFloatingPointErrors now returns Infinity. Downstream:

  1. setValue(Infinity, …)toValidatedNumberclamp(Infinity, MIN_SAFE_INTEGER, MAX_SAFE_INTEGER) = MAX_SAFE_INTEGERonValueChange fires with MAX_SAFE_INTEGER.
  2. onValueCommitted(committed, …) is NOT re-validated (forwarded raw at NumberFieldRoot.tsx:129-134) → fires with Infinity. Two listeners observe different values.
  3. formatNumber(Infinity, locale, {style:'percent'}) returns "∞%". The input visibly displays "∞%" while the form state holds MAX_SAFE_INTEGER.
  4. parseNumber("∞%", …) returns null → next blur is a no-op. User is stuck.

The previous overflow fix moved the silent failure one level up. Recommendation: in the overflow branch at validate.ts:36-38, return a sentinel (null or the pre-blur value) that the blur path treats as "don't commit", rather than letting Infinity propagate.

Medium

  • Subnormal underflow to 0validate.ts:41. (Number.MIN_VALUE * 100).toFixed(2) === "0.00" → a non-zero input silently becomes 0. Same class as the original NaN bug; exotic in practice.
  • Currency + minimumFractionDigits is now an undocumented behavior shift. Pre-PR, getFormatter('en-US') resolved max=3 (decimal default). Post-PR, the conditional at validate.ts:17 forwards the format, so currency resolves max=2. More correct — but a 1.239 input with {style:'currency', currency:'USD', minimumFractionDigits:1} now rounds to 1.24 instead of 1.239. Worth a one-line changelog note.

Test gaps (low–medium)

  • step × roundingMode interaction untested (validate.ts:115,125,128).
  • clamp × roundingMode interaction untested.
  • Negative-value integration test (keyboard type -1.239, assert onValueChange) untested.
  • No ceil / halfExpand coverage despite the PR title claiming general rounding-mode support.

Comments (nice-to-have)

  • validate.ts:6: tie alias lifetime to a deletion trigger — "Delete once tsconfig.base.json lib includes es2023."
  • validate.ts:30-34: the isPercentWithExplicitPrecision + scale=100 trick needs a one-liner pointing at parse.ts:211-215 (percent stored as fraction; rounding must happen at display scale).
  • validate.ts:17: the conditional format?.minimumFractionDigits == null ? undefined : format reads like a typo; a one-line WHY (currency/percent default-max would otherwise clamp precision unexpectedly) would prevent a "fix" that breaks it.

Recommended action

  1. Address the Infinity leak in the blur path (high). The cleanest fix is at the helper: don't return Infinity — propagate a non-commit signal.
  2. Add a step + roundingMode test to lock the composition.
  3. Add the three one-line comments at validate.ts:6, :17, :30.
  4. Note the currency-default behavior change in the changelog.
  5. Items 2–4 can ride; item 1 is a real bug.

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label May 15, 2026
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label May 18, 2026
Copy link
Copy Markdown
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

PR Review Summary — #4804 [number field] Respect rounding mode on blur

Author: atomiks • Base: masterHead: codex/number-field-rounding-modeDiff: +448 / −26 across 6 files • Fixes: #4803

The PR replaces toFixed-based blur rounding with Intl.NumberFormat-driven rounding that honors roundingMode, roundingIncrement, roundingPriority, and significant-digit options, plus a new percent display-scale rounding step. Tests, typecheck, and lint pass.

Critical Issues

None blocking. All five agents converged on this — no correctness, security, or merge-blocker concerns.

Important Issues

  • [silent-failure] No finite-guard on committed before setValue / onValueCommittedpackages/react/src/number-field/input/NumberFieldInput.tsx:187-208. On master, Number(parsedValue.toFixed(...)) could only produce finite values. After the PR, removeFloatingPointErrors reaches into Intl.NumberFormat and the call paths broaden — but the blur handler still treats the result as definitely-finite. Defense-in-depth: if (!Number.isFinite(committed)) committed = parsedValue; immediately after the helper call.

  • [silent-failure] Runtime feature gap for roundingMode / roundingIncrement silently no-opspackages/react/src/number-field/utils/validate.ts:62-76. Older Safari/Node silently drop these option keys instead of throwing, so a user on Safari 15.3 sees rounding silently not applied while Chrome works. Consider one-time feature-detection at module load and a dev-only Base UI warning.

  • [type-design] roundingMode: string and roundingPriority: string throw away TC39 unionspackages/react/src/number-field/utils/validate.ts:7-12. The standard ES2023 lib types these as "ceil" | "floor" | "expand" | "trunc" | "halfCeil" | ... etc. Modeling the real unions costs ~3 lines and makes the shim a drop-in replacement when the lib bump lands. Today, the local type is strictly weaker than the eventual stdlib version.

  • [comments] Rot-risk: "Delete this once tsconfig.base.json includes es2023"packages/react/src/number-field/utils/validate.ts:6-7. Action-item-as-comment that nothing forces to be deleted on a future lib bump. Keep the WHY (Local augmentation for ES2023 Intl rounding fields; the repo's tsconfig lib is "es2022"), drop the action sentence (or put it in a tracking issue).

  • [tests] No end-to-end test for increment/decrement + roundingModeremoveFloatingPointErrors is now also called from toValidatedNumber, which is hit by every Arrow/scrub increment. Only a single direct unit test at packages/react/src/number-field/utils/validate.test.ts:327-339 covers this path. A regression that bypassed removeFloatingPointErrors inside the step path would slip through.

  • [tests] parseNumber Infinity change is thinly covered — only one new case (packages/react/src/number-field/utils/parse.test.ts:111-119). Add a permille overflow case to harden the !Number.isFinite swap; the pre-existing test at lines 105-109 would have passed on master too.

Suggestions

  • [type-design] Move the type into formatNumber.ts and update getFormatter's options parameter — eliminates the as NumberFormatOptionsWithRounding cast at packages/react/src/number-field/utils/validate.ts:74, and centralizes the shim so the eventual lib: es2023 deletion is one diff. Even better: bump tsconfig.base.json lib to ["es2023", "dom"] now — it would also let users actually type roundingMode on the public format prop at packages/react/src/number-field/root/NumberFieldRoot.tsx:596 (currently they can pass it, but TS errors at their call site).

  • [code-review] Magic 3 default for maximumFractionDigitspackages/react/src/number-field/utils/validate.ts:32. Matches en-US Intl default but loses style-sensitive defaults (e.g. JPY currency = 0). Pre-existing on master, so not a regression — call out as follow-up. Either name the constant or derive it from getFormatter('en-US', format).resolvedOptions() unconditionally.

  • [silent-failure] Extract the percent branch explicitlypackages/react/src/number-field/utils/validate.ts:44-47. The inner !Number.isFinite(valueToRound) check is only reachable when scale === 100 (the outer check at line 26 handles everything else). Pulling the percent path into its own branch makes the recovery path obvious and removes the need for the explanatory comment.

  • [comments] Update the stale "round the committed numeric value" commentpackages/react/src/number-field/input/NumberFieldInput.tsx:185. The helper now handles significant-digits, rounding increments, modes, and percent scaling — not just precision. Drop, or rewrite to capture the non-obvious WHY (why gated by hasExplicitPrecision).

  • [comments] Tighten "preserve the old finite value"packages/react/src/number-field/utils/validate.ts:45. There's no "new value yet" to contrast with "old"; "fall back to the unscaled value" reads cleaner.

  • [tests] Missing direct unit test for hasExplicitNumberFormatPrecision, especially maximumSignificantDigits/minimumSignificantDigits-only cases. Indirect coverage exists, but the predicate's contract is worth pinning.

  • [tests] Locale-robust display assertionsexpect(input).toHaveValue('1.23%') and '0.46%' at packages/react/src/number-field/input/NumberFieldInput.test.tsx:715-744 are hardcoded. Other tests in the file construct expectations via new Intl.NumberFormat(...) — consistency would protect against CLDR drift across Node/Chromium versions.

  • [tests] Add NaN/Infinity input tests for removeFloatingPointErrors — one-liners covering the early-return guard at line 24-26.

Strengths

  • The fix is correct and well-scoped. The blur path was using toFixed, which ignored every Intl v3 rounding option. The Intl-routed path is gated only when an option that actually needs it is present, preserving the cheap toFixed for the simple case.
  • The 0.46% boundary handling is the standout. The value * 100 pre-scrub at toFixed(digits + 6) (packages/react/src/number-field/utils/validate.ts:50-52) is exactly the right fix for binary noise breaking directional rounding modes, parameterized across floor | ceil | trunc | expand at packages/react/src/number-field/utils/validate.test.ts:81-92.
  • Two best comments (validate.ts:50-51 and validate.ts:65) are model examples of "explain WHY" — one names the underlying mechanic and gives a concrete example, the other justifies an absence (omitting style/unit/notation).
  • Splitting hasExplicitNumberFormatPrecision is the right call — reused at both the call site (gate for whether to round at all) and inside the helper (gate for percent scaling), co-located so they can't drift.
  • No CLAUDE.md / AGENTS.md violations: no as any, no new hooks (so useTimeout/useStableCallback/useIsoLayoutEffect N/A), no DOM traversal, no thrown public errors.
  • Tests use real Intl.NumberFormat end-to-end — exactly right because the entire fix is about delegating to platform rounding semantics. unit/percent vs percent, onValueChange vs onValueCommitted, and locale variants (en-US/fr-FR/ar-EG) are all distinguished.
  • Overflow safety at validate.ts:44-47 and parse.ts:217 (Infinity now returns null) is tested at validate.test.ts:169-176 and the percent-MAX_VALUE case in parse.test.ts.

Recommended Action

  1. Address before merge (low-effort hardening):

    • Add the Number.isFinite(committed) defense-in-depth guard in NumberFieldInput.tsx.
    • Tighten the roundingMode / roundingPriority types to their TC39 unions.
    • Rewrite the rot-risk comment at validate.ts:6-7 (drop the "Delete this once..." sentence).
    • Update the stale comment at NumberFieldInput.tsx:185.
  2. Consider before merge (broader, but high value):

    • Bump tsconfig.base.json lib to ["es2023", "dom"] and delete NumberFormatOptionsWithRounding entirely. This also unlocks the rounding fields on the public format prop for library consumers — without it, this PR's behavior is only triggerable via type-unsafe object literals.
  3. Follow-up (separate PR or changelog note):

    • Add increment/decrement + roundingMode end-to-end test.
    • Add direct unit tests for hasExplicitNumberFormatPrecision and NaN/Infinity inputs to removeFloatingPointErrors.
    • Mention the parseNumber Infinity behavior change (overflow → null) in the changelog for downstream consumers.

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label May 19, 2026
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: number field Changes related to the number field component. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[number field] format.roundingMode is ignored when committing rounded values

3 participants