fix(number-field, slider): floating point roundoff precision bug #4263
Conversation
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Tachometer resultsChromenumber-field permalinkbasic-test
slider permalinktest-basic
Firefoxnumber-field permalinkbasic-test
slider permalinktest-basic
|
Rajdeepc
changed the title
Rajdeepc
added
bug
Rajdeepc
changed the title
Co-authored-by: Rajdeep Chandra <[email protected]>
I still see the bug outlined in the issue. We likely need to leverage |
Rajdeepc
dismissed
their stale review
Dismissing Approval due to discrepency in how it is printing in Storybook events
…dobe/spectrum-web-components into blunteshwar/slider-not-rounding-values
…dobe/spectrum-web-components into blunteshwar/slider-not-rounding-values
| @@ -495,7 +495,14 @@ export class NumberField extends TextfieldBase { | |||
| // Step shouldn't validate when 0... | |||
| if (this.step) { | |||
| const min = typeof this.min !== 'undefined' ? this.min : 0; | |||
| const moduloStep = (value - min) % this.step; | |||
| this.digitsAfterDecimal = | |||
There was a problem hiding this comment.
Can we cauterize this work to change.get('step') in one of the lifecycle callbacks? I'm not super excited about the this._lastDigitsAfterDecimal !== this.digitsAfterDecimal check, can we leverage the existing cache management mechanics?
…dobe/spectrum-web-components into blunteshwar/slider-not-rounding-values
| @@ -510,6 +512,7 @@ export class NumberField extends TextfieldBase { | |||
| value -= this.step; | |||
| } | |||
| } | |||
| value = parseFloat(this.valueFormatter.format(value)); | |||
There was a problem hiding this comment.
Is it possible to only format at the end to reduce the chances of fidelity loss?
There was a problem hiding this comment.
Nope, We need this const moduloStep = parseFloat(this.valueFormatter.format((value - min) % this.step)); format because in some cases moduloStep is evaluated as 0.00000000000017 or very close to zero , but it should be evaluated to 0, hence this formatting is required.
And we need this value = parseFloat(this.valueFormatter.format(value)); because after modulostep is evaluated the value of step is added to value and hence we again have to finally format it.
Basically the first format is to correctly calculate the moduloStep and the second format is to correctly calculate the value upto required decimal places in accordance with step.
| private _numberFormatter?: NumberFormatter; | ||
| private _numberFormatterFocused?: NumberFormatter; | ||
| private _valueFormatter!: NumberFormatter; | ||
| private digitsAfterDecimal: number = 0; |
There was a problem hiding this comment.
Do we need to keep this on the class, or can we use cache clearing to trigger the use of valueFormatter as a getter and contain this value therein?
There was a problem hiding this comment.
Yeah, we can totally do that
…dobe/spectrum-web-components into blunteshwar/slider-not-rounding-values
| const digitsAfterDecimal = | ||
| this.step != Math.floor(this.step) | ||
| ? this.step.toString().split('.')[1].length | ||
| : 0; | ||
| this._valueFormatter = new NumberFormatter( | ||
| this.languageResolver.language, | ||
| { maximumFractionDigits: digitsAfterDecimal } | ||
| ); |
There was a problem hiding this comment.
We shouldn't need to populate the cache here, if we evict it, the use of this.valueFormatter later will automatically populate it on demand.
Description
While calculating the
moduloStepinvalidateInputwe were not rounding the value due to which when the value ofmoduloStepwas expected to be 0, it was actually 1.7e-17 and causing the errorRelated issue(s)
Motivation and context
This change was required because due to wrong value calculation of
moduloStepthe value of number-field was also incorrect. Now that we are correctly calculatingmoduloStepthe value of number-field is also correct.How has this been tested?
Screenshots (if appropriate)
Before Fix
Screen.Recording.2024-04-12.at.1.11.31.PM.mov
After Fix
Screen.Recording.2024-04-12.at.1.12.34.PM.mov
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.