zillow / react-slider

Accessible, CSS agnostic, slider component for React.
https://zillow.github.io/react-slider
MIT License
891 stars 232 forks source link

Floating step - different results with click and keyboard #179

Closed viters closed 4 years ago

viters commented 4 years ago

Description

When I set step to floating value, like 0.1 - using slider with mouse is okay and everything works as expected.

When I use keyboard to change slider value, there are floating point issues:

image

The issue is probably because of floating point operations at lines: https://github.com/zillow/react-slider/blob/master/src/components/ReactSlider/ReactSlider.jsx#L730 https://github.com/zillow/react-slider/blob/master/src/components/ReactSlider/ReactSlider.jsx#L736

I could suggest solution like this:

[...]
const sanitizeFloatValue = (a) => Number.isInteger(a) ? a : parseFloat(a.toFixed(10));
[...]

const newValue = sanitizeFloatValue(oldValue + step);

image

@stonebk If you think this solution is acceptable, I will make PR.

CodeSandbox

https://stackblitz.com/edit/react-oeujyh

stonebk commented 4 years ago

@viters thanks for the detailed issue report!

I don't think I would add this kind of sanitization logic to the component itself because it is the result of using JavaScript floats and not really the fault of the slider logic.

Perhaps we can leverage our onBeforeChange lifecycle method as a place to run sanitization logic and return a different value.

viters commented 4 years ago

I don't think I would add this kind of sanitization logic to the component itself because it is the result of using JavaScript floats and not really the fault of the slider logic.

@stonebk I think libraries do fight with JS sometimes to hide this fight from users.

There may be better solution, but I would not require users to handle this difference themselves. In other words, I think slider should produce the same results, regardless of interaction type (mouse, touch, keyboard). Moreover, I think it's a nice idea to imitate native controls - and native range slider does handle floats itself (https://stackblitz.com/edit/typescript-9gazv2?file=index.html).

Maybe there is a way to unify how newValue is calculated for mouse and keyboard?

viters commented 4 years ago

One example of handling this situation is presented by rc-slider: https://github.com/react-component/slider/blob/master/src/utils.ts Especially ensureValuePrecision.

This is proper solution I think, setting the target precision based on step from props and ensuring the output value has the same precision. As as library user I would not expect receiving e.g. 0.1001 value when I have step of 0.1.

Sorry for open/close missclick.

stonebk commented 4 years ago

Yeah, the more I think about it, it makes sense that we resolve the issue since we are the ones doing the addition/subtraction.