Skip to content

fix(InputNumber): fix decimal rounding on spin#8392

Merged
melloware merged 1 commit intoprimefaces:masterfrom
selfplusplus2:master
Nov 14, 2025
Merged

fix(InputNumber): fix decimal rounding on spin#8392
melloware merged 1 commit intoprimefaces:masterfrom
selfplusplus2:master

Conversation

@selfplusplus2
Copy link
Contributor

Fixes #8382

@github-actions
Copy link

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@melloware
Copy link
Member

GitHub Copilot Chat Assistant

Quick summary

  • The new implementation is simpler than the old (it auto-computes precision from the operands), but it has a few pitfalls:
    • It fails for numbers printed in exponential notation (e.g. 1e-7) because toString().split('.') yields no decimal part.
    • It doesn't guard against string inputs, NaN, Infinity.
    • Math.pow(10, decimals) can exceed safe integers for large decimal counts (and large scale factors can overflow Number.MAX_SAFE_INTEGER).
    • Rounding via Math.round((base + increment) * precision) can still pick up floating errors before scaling; it's safer to scale each operand or the sum before rounding.
    • For financial/critical precision you should use a decimal library (decimal.js / Big.js / BigNumber).

Problems I found in current diff

  • Exponential notation: (0.0000001).toString() === "1e-7" -> baseDec becomes 0, but you actually need 7 decimals.
  • Input types: if base or increment are strings (common in UI code) you need to coerce to Number and handle invalid values.
  • Very large decimals: Math.pow(10, n) with n > ~15 can cause factor > 2^53 and integer math becomes unsafe.
  • Edge cases (NaN, Infinity) are not handled.

Suggested robust implementation

  • Count decimals correctly including exponential notation.
  • Cap decimals to a safe limit (e.g. 15) to avoid exceeding Number.MAX_SAFE_INTEGER.
  • Use scaled integer addition and rounding: Math.round(base * factor + increment * factor) / factor
  • Return NaN when inputs are not finite numbers, or fallback gracefully.

Example implementation (replace your function with this)
function countDecimals(value) {
if (!isFinite(value)) return 0;
const s = String(value);
// handle exponential notation
if (s.indexOf('e') >= 0) {
const [mantissa, expStr] = s.split('e');
const exp = parseInt(expStr, 10);
const dec = (mantissa.split('.')[1] || '').length;
return Math.max(0, dec - exp);
}
return (s.split('.')[1] || '').length;
}

const addWithPrecision = (base, increment) => {
base = Number(base);
increment = Number(increment);
if (!isFinite(base) || !isFinite(increment)) return NaN;

const baseDec = countDecimals(base);
const incDec = countDecimals(increment);

// cap decimals to avoid factor > Number.MAX_SAFE_INTEGER
const decimals = Math.min(Math.max(baseDec, incDec), 15);
const factor = Math.pow(10, decimals);

// If scaling would overflow, fall back to simple addition with an approximate rounding
const maxSafe = Number.MAX_SAFE_INTEGER;
if (Math.abs(base) * factor > maxSafe || Math.abs(increment) * factor > maxSafe) {
const sum = base + increment;
// round to a reasonable number of digits (15)
const fallbackFactor = Math.pow(10, 15);
return Math.round(sum * fallbackFactor) / fallbackFactor;
}

return Math.round(base * factor + increment * factor) / factor;
};

Behavior examples you can use to test

  • addWithPrecision(0.1, 0.2) -> 0.3
  • addWithPrecision(1e-7, 1e-7) -> 2e-7
  • addWithPrecision('1.234', 0.001) -> 1.235
  • addWithPrecision(NaN, 1) -> NaN
  • addWithPrecision(Infinity, 1) -> NaN

Recommendations

  • Add unit tests covering:
    • small decimals (0.1 + 0.2)
    • exponential inputs (1e-7)
    • string inputs
    • very large/safe boundary cases
  • If this function is used for money or other critical decimal arithmetic, switch to a decimal/BigNumber library (decimal.js, big.js, bignumber.js) to avoid all floating-point pitfalls.
  • Keep the decimal cap (15) documented so future maintainers understand the safety tradeoff.

If you want, I can:

  • open a PR with this improved implementation and tests, or
  • adapt the implementation to use a specific decimal library if you prefer. Which would you like?

@github-actions
Copy link

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@selfplusplus2
Copy link
Contributor Author

I have amended to factor in exponential numbers and added some tests

@melloware
Copy link
Member

please run npm run format

@melloware melloware added the Type: Bug Issue contains a defect related to a specific component. label Nov 14, 2025
@github-actions
Copy link

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@selfplusplus2
Copy link
Contributor Author

Formatting updated

@melloware
Copy link
Member

ok npm run lint next please

@github-actions
Copy link

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@selfplusplus2
Copy link
Contributor Author

Done

@melloware
Copy link
Member

weird now its saying the format is wrong again?

Amended to factor in exponential numbers and added tests with  formatting and linting - take 2
@github-actions
Copy link

Thanks a lot for your contribution! But, PR does not seem to be linked to any issues. Please manually link to an issue or mention it in the description using #<issue_id>.

@selfplusplus2
Copy link
Contributor Author

It doesn't like my editor white space settings 🤣🤦. I've re-run format and lint.

@melloware melloware merged commit 5533a98 into primefaces:master Nov 14, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug Issue contains a defect related to a specific component.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InputNumber step rounding to one decimal place

2 participants

Comments