fix(range): round value to same number of decimal places as props to avoid floating point rounding errors #27375
Conversation
|
|
There was a problem hiding this comment.
Do you think there's value in extracting getDecimalPlaces to a util function and writing spec tests that test that as opposed to doing an e2e test? My understanding of this bug is it's less about the interaction and more about the underlying logic that caused this bug.
There was a problem hiding this comment.
I pulled the functionality out in 3537810. Let me know if you think this is too much abstraction; I couldn't think of another use case for this util off the top of my head, so I made the function pretty specific, but I could also break it up into smaller functions and move more of the logic (probably the Math.max piece) back into range.tsx.
| const clampedValue = clamp(min, value, max); | ||
|
|
||
| return roundToMaxDecimalPlaces(clampedValue, min, max, step); |
There was a problem hiding this comment.
Out of curiosity, why did you opt for this approach instead of multiplying by a scaled step (and then dividing by the scale amount)?
For example:
let value = (max - min) * ratio;
const SCALE = 100;
const SCALED_STEP = step * SCALE;
if (step > 0) {
// round to nearest multiple of step, then add min
value = Math.round(value / step) * SCALED_STEP + min;
}
return clamp(min, value / SCALE, max);Scaling the value ensures that you deal with integers to avoid JavaScript's floating point precision funkiness. As long as you scale down by the same amount you scaled up, you should get a nice neat decimal as a multiple of step.
The benefit of the above approach is that you can round and then clamp, whereas the proposed changes would do round, clamp, and then round again.
edit: Though I suppose if your step was 0.001 you'd be back to a decimal, but you could increase SCALE to a larger number like 10000.
There was a problem hiding this comment.
Because it wouldn't work for all numbers. I'd rather not set an arbitrary limit on how high or low the props can be, nor the precision they can have. For example, a SCALE of 10000 sets the bounds within 0.0001 < n < (MAX_INT / 10000), and you can't have more than 4 decimal places -- 5.12345 * 10000 = 51234.5.
There was a problem hiding this comment.
Ah yeah good point. Though I think the current approach may have some accuracy issues too since toFixed can sometimes yield funky results.
<ion-range
min="0.1"
max="0.54321876549876532"
value="0.2"
step="0.001"
>
<span slot="label">Label</span>
</ion-range>Video sample:
floating-point.mov
Is there a defined level of precision we support with ion-range?
There was a problem hiding this comment.
What's happening there is you've got a max with the same number of decimal places as the result with the rounding errors, and toFixed is rounding to as many as the max, so the error isn't trimmed. I would say that's okay, since we wouldn't be able to tell the difference between a rounding error result and something legitimate in a case like that. (While in your case, the only legitimate value with that precision is the actual max, if the step were 0.00000000000000001 instead, the two would definitely be indistinguishable.)
I think it's okay that this means we implicitly only support up to a certain level of precision, because the problem is with floating point math itself, not Ionic. If people are using precision levels that high, they're going to run into rounding error issues like this all over the place. 🤭 (Or in other words, JS itself only supports up to a certain level of precision anyway, unless you start using third party data types meant for this sort of thing. And I'd say supporting those is out of scope for this fix 😆)
There was a problem hiding this comment.
Sounds good to me! Could we add a little bit of explanation about these limitations in the code so other devs are aware?
Issue number: Resolves #21968
What is the current behavior?
When using fractional values for
min,max, orstep, it is possible for floating point rounding errors to cause unexpected values to be emitted. For example,step="0.05" min="0.1" max="1"emits a value of0.150000000004after moving one step.What is the new behavior?
Values are rounded to the max number of decimal places between the three props. Note that it isn't mathematically possible to arrive at a value with more decimal places than the props*, since addition (i.e. starting at
minand adding multiples ofstep) can't increase the precision of a number.* Unless the
valueis set manually, but in that case,ion-rangecurrently snaps to a multiple ofstepas soon as the slider is moved, resuming normal behavior.Does this introduce a breaking change?
Other information