Slider work wrong when we config max%step !== 0

[hoannguyen02]

Slider work wrong when we config max%step !== 0

• [x ] The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

If we configuration slider component like this, the step works wrong way

      <Slider
        defaultValue={150}
        step={100}
        max={750}
        min={150}
      />

Expected Behavior 🤔

Expected Step work right way

Steps to Reproduce 🕹

Change step to 50/150/250, it must be max%step === 0 , it will works as expected

      <Slider
        defaultValue={150}
        step={50}
        max={750}
        min={150}
      />

Tech	Version
Material-UI	v4.6.1
React	16.12.0

[oliviertassinari]

@nguyetem Thank you for the bug report, I can confirm it. It doesn't behave correctly. At least if we compare it with jQuery UI: https://codepen.io/oliviertassinari/pen/XWWoZzL?editors=1000 or if we set the marks prop and we expect each mark to correspond to a value the slider can reach. I would propose the following diff:

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index ad7f144e1..49afa013b 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -83,8 +83,8 @@ function getDecimalPrecision(num) {
   return decimalPart ? decimalPart.length : 0;
 }

-function roundValueToStep(value, step) {
-  const nearest = Math.round(value / step) * step;
+function roundValueToStep(value, step, min) {
+  const nearest = Math.round((value - min) / step) * step + min;
   return Number(nearest.toFixed(getDecimalPrecision(step)));
 }

@@ -492,7 +492,7 @@ const Slider = React.forwardRef(function Slider(props, ref) {
     event.preventDefault();

     if (step) {
-      newValue = roundValueToStep(newValue, step);
+      newValue = roundValueToStep(newValue, step, min);
     }

     newValue = clamp(newValue, min, max);
@@ -547,7 +547,7 @@ const Slider = React.forwardRef(function Slider(props, ref) {
       let newValue;
       newValue = percentToValue(percent, min, max);
       if (step) {
-        newValue = roundValueToStep(newValue, step);
+        newValue = roundValueToStep(newValue, step, min);
       } else {
         const marksValues = marks.map(mark => mark.value);
         const closestIndex = findClosest(marksValues, newValue);
@@ -946,6 +946,9 @@ Slider.propTypes = {
   orientation: PropTypes.oneOf(['horizontal', 'vertical']),
   /**
    * The granularity with which the slider can step through values. (A "discrete" slider.)
+   * The `min` prop serves as the origin for the valid values.
+   * We recommend (max - min) to be evenly divisible by the step.
+   *
    * When step is `null`, the thumb can only be slid onto marks provided with the `marks` prop.
    */
   step: PropTypes.number,
diff --git a/packages/material-ui/src/Slider/Slider.test.js b/packages/material-ui/src/Slider/Slider.test.js
index d33596eea..13ebf199f 100644
--- a/packages/material-ui/src/Slider/Slider.test.js
+++ b/packages/material-ui/src/Slider/Slider.test.js
@@ -306,22 +306,37 @@ describe('<Slider />', () => {
       key: 'ArrowRight',
     };

+    it('should use min as the step origin', () => {
+      const { getByRole } = render(<Slider defaultValue={150} step={100} max={750} min={150} />);
+      const thumb = getByRole('slider');
+      thumb.focus();
+
+      fireEvent.keyDown(document.activeElement, moveRightEvent);
+      expect(thumb).to.have.attribute('aria-valuenow', '250');
+
+      fireEvent.keyDown(document.activeElement, moveLeftEvent);
+      expect(thumb).to.have.attribute('aria-valuenow', '150');
+    });
+
     it('should reach right edge value', () => {
       const { getByRole } = render(<Slider defaultValue={90} min={6} max={108} step={10} />);
       const thumb = getByRole('slider');
       thumb.focus();

       fireEvent.keyDown(document.activeElement, moveRightEvent);
-      expect(thumb).to.have.attribute('aria-valuenow', '100');
+      expect(thumb).to.have.attribute('aria-valuenow', '96');
+
+      fireEvent.keyDown(document.activeElement, moveRightEvent);
+      expect(thumb).to.have.attribute('aria-valuenow', '106');

       fireEvent.keyDown(document.activeElement, moveRightEvent);
       expect(thumb).to.have.attribute('aria-valuenow', '108');

       fireEvent.keyDown(document.activeElement, moveLeftEvent);
-      expect(thumb).to.have.attribute('aria-valuenow', '100');
+      expect(thumb).to.have.attribute('aria-valuenow', '96');

       fireEvent.keyDown(document.activeElement, moveLeftEvent);
-      expect(thumb).to.have.attribute('aria-valuenow', '90');
+      expect(thumb).to.have.attribute('aria-valuenow', '86');
     });

     it('should reach left edge value', () => {
@@ -329,17 +344,14 @@ describe('<Slider />', () => {
       const thumb = getByRole('slider');
       thumb.focus();

-      fireEvent.keyDown(document.activeElement, moveLeftEvent);
-      expect(thumb).to.have.attribute('aria-valuenow', '10');
-
       fireEvent.keyDown(document.activeElement, moveLeftEvent);
       expect(thumb).to.have.attribute('aria-valuenow', '6');

       fireEvent.keyDown(document.activeElement, moveRightEvent);
-      expect(thumb).to.have.attribute('aria-valuenow', '20');
+      expect(thumb).to.have.attribute('aria-valuenow', '16');

       fireEvent.keyDown(document.activeElement, moveRightEvent);
-      expect(thumb).to.have.attribute('aria-valuenow', '30');
+      expect(thumb).to.have.attribute('aria-valuenow', '26');
     });

     it('should round value to step precision', () => {

What do you think about it? Do you want to submit a pull request? :)

[julie-volkova]

Hey, i've got a chance to work on this. What do you think?

[hoannguyen02]

Hey, i've got a chance to work on this. What do you think?

Hi there!
Sorry for late reply and many thanks for the pull request

[mumac]

Hi,
I faced error, when put to "min" and "max" props a number value as string.

In this case got the incorrect calculation number + string and an attempt to call .toFixed() for string.

Maybe to avoid such errors need to add a type check for "min" and "max" props with console warning.

[oliviertassinari]

Maybe to avoid such errors need to add a type check for "min" and "max" props with console warning.

@mumac It already does. The component asks for a number.