Material-ui: [Slider] Cannot read property 'focus' of null at focusThumb

[NawarA]

Hi, I'm getting inconsistent behavior with the slider component. I'm using a slider with a min, max, and range property. Sometimes it works, sometimes it crashes with the error:

Cannot read property 'focus' of null
    at focusThumb (node_modules/material-ui/core/Slider/Slider.js:144:98)
    at eval (node_modules/material-ui/core/Slider/Slider.js:824:5
    at eval (node_modules/material-ui/core/utils/useEventCallback.js:25:29

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

Current Behavior 😯

When you click on the slider's minium range point, an error throws. Sometimes it throws, sometimes it doesnt, depending on a developer sets up the slider component. I've been investigating to see why the slider throws, and which configuration makes it throw. So far, the best I've done, is isolate the issue to the following configuration of slider:

• Developer sets up a slider with the props min, max, value, and onChange
• Developer changes the min property
• The component throws when a user clicks the slider's min ticker

It seems like if you change the min prop, you must change the value prop, otherwise the error appears.

It seems like the error is due to an out range design pattern. Perhaps your source code keeps track of the the range of HTML elements that represent the ticks, so when the user clicks the Min mark, that HTML element is now null, so your source code throws. Its an edge case in your design, and should be fixed.

A good fix would be, if the min prop changes, and if you're keeping track of the discrete HTML elements that represent marks, then you must update your internal state to account for the new min, and set the state of the component to the slider's new min mark's element. Failing to do so, would cause a null error on click...which what we're observing here.

Any who, thats the edge case and the fix, based on my observation. Please patch. Should be a simple fix to cover the missing edge case the component's state.

Expected Behavior 🤔

No error

Steps to Reproduce 🕹

Codesandbox with error:
Codesandbox with no error / with the isolated

Context 🔦

Your Environment 🌎

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.?.? |
| React | |
| Browser | |
| TypeScript | |
| etc. | |

[joshwooding]

Nice catch, this is actually due to the value being passed in is smaller than the min value. A simpler reproduction is: https://codesandbox.io/s/async-bash-qek7c?file=/src/App.js

This would be fine and is caught by the clamp but the performance shortcut means it uses the value passed in. Which makes the active index now -1 since the previousValue is not in the newValue. Hence the error when we try and get the thumb representing the index -1 :)

We could correct the if statement to:

@@ -86,7 +86,7 @@ function roundValueToStep(value, step, min) {

 function setValueIndex({ values, source, newValue, index }) {
   // Performance shortcut
-  if (values[index] === newValue) {
+  if (source[index] === newValue) {
     return source;
   }

but we could also remove the performance shortcut since I'm not sure it saves much anyway.

cc @oliviertassinari @eps1lon

[oliviertassinari]

but we could also remove the performance shortcut since I'm not sure it saves much anyway.

Remove it, try to slide, notice the lag, revert 😆.

[eps1lon]

And while we're at it:

- const output = [...values];
+ const output = values.slice();

which gets rid of over-transpiling since babel assumes any iterable.

[joshwooding]

Just so it's clear when someone picks it up:

1. Remove
   https://github.com/mui-org/material-ui/blob/4bfcbec597d24435ef0f91443b85783e795baf33/packages/material-ui/src/Slider/Slider.js#L89-L91

2. Change spread to slice.

- const output = [...values];
+ const output = values.slice();

[gautam-pahuja]

@joshwooding Can I make the changes or is there any PR already open?

[oliviertassinari]

I would be careful with switching "values" with "source". I think that we would need to make sure if works even if the values are provided in the reverse, decreasing order (I don't recall adding a test case for it).

[joshwooding]

@gautam-pahuja You can make the changes :)

@oliviertassinari I think removing it is the easiest option then.

[oliviertassinari]

I had a look at the issue. Regarding the fix, https://github.com/mui-org/material-ui/issues/20896#issuecomment-623190034 sounds great, with a new test case 👌

---

@joshwooding I have figure out what was the performance issue about. Compare these two, with the dev tool open (to slow down the runtime?), move the thumb quickly in both directions:

• "Current": https://codesandbox.io/s/silly-brown-cyrd7
• "Slower": https://codesandbox.io/s/jovial-ptolemy-754i7

By removing the optimization, we turn "Current" into "Slower". We have a test for this optimization. I have tested the two sandboxes on a high-end Android phone, the difference of performance between "Current" and "Slower" is not that important. Should we keep the logic? I don't know.

https://github.com/mui-org/material-ui/blob/15854b972b0fb37e42ecad12b0ab0c4336ac2d0f/packages/material-ui/src/Slider/Slider.js#L88-L91

[joshwooding]

@oliviertassinari Nice, definitely can reproduce :) I'm happy to keep the logic but fix the if statement.

[clementdevos]

Hi there, we're having this issue with the same inconsistency.
It's not clear to me wether this has been fixed or not, and if it's available in a release?

Cheers !

[oliviertassinari]

@clementdevos Do you want to work on the fix proposed in this thread?

[clementdevos]

Hey, i have other things on my hands right now, but i'll try and give a look.

;)

[naveensaigit]

Hi, I'm facing the same issue, but with the maximum side of the slider. It happens only if try to drag the slider. If I click in between the rail, the slider works

[oliviertassinari]

@naveensaigit Does the proposed solution in this thread solve your issue? If not, do you have a reproduction?

[naveensaigit]

I actually didn't understand the solution. What should I do to prevent the error?

[chromodome]

Like @naveensaigit i'm having the error thrown when clicking only the max value of the range. Has this been solved?

[oliviertassinari]

@chromodome Do you want to work on a pull request? Using https://github.com/mui-org/material-ui/issues/20896#issuecomment-623190034 :)?

[annabic]

Hey @oliviertassinari,
It looks like nobody has claimed this issue, so I decided to take a look after reading contribution guidelines.
I made a commit in my forked repo with the fix from #20896 (comment) and added a test case. Is this what you had in mind? :)

@@ -86,7 +86,7 @@ function roundValueToStep(value, step, min) {

 function setValueIndex({ values, source, newValue, index }) {
   // Performance shortcut
-  if (values[index] === newValue) {
+  if (source[index] === newValue) {
     return source;
   }
+++ b/packages/material-ui/src/Slider/Slider.test.js
@@ -185,6 +185,24 @@ describe('<Slider />', () => {
     });
   });

+  it('should not break when initial value is out of range', () => {
+    const { container } = render(<Slider value={[19, 41]} min={20} max={40} />);
+
+    stub(container.firstChild, 'getBoundingClientRect').callsFake(() => ({
+      width: 100,
+      height: 10,
+      bottom: 10,
+      left: 0,
+    }));
+
+    fireEvent.touchStart(
+      container.firstChild,
+      createTouches([{ identifier: 1, clientX: 100, clientY: 0 }]),
+    );
+
+    fireEvent.touchMove(document.body, createTouches([{ identifier: 1, clientX: 20, clientY: 0 }]));
+  });
+
   describe('prop: step', () => {

I had the same property error when running this test with the original if statement.

After applying the fix, all tests were passed, including the new one.

[oliviertassinari]

@mageprincess It sounds like a solid start, do you want to open a pull request on the next branch? :)

[NawarA]

@mageprincess Rock on! 🧙‍♀️ 👸