[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

• 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 😯

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.

[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, #20896 (comment) 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 #20896 (comment) :)?

[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! 🧙‍♀️ 👸

[nour-sidaoui]

As of today, the same error happens when I click on max in certain conditions.

• min and max have the exact same values as values
• min, max and values are Numbers

@naveensaigit has said that clicking in between the sliders works and it does!

[oliviertassinari]

@nour-sidaoui Please open a new issue with a minimal live reproduction on the lastest version (v5) if we didn't fully solve the issue.

[nour-sidaoui]

@oliviertassinari
Finally found it, re-render was updating marks but not the values.
useEffect'ed' it and it worked like a charm :)

[bhagya97]

@oliviertassinari
Finally found it, re-render was updating marks but not the values.
useEffect'ed' it and it worked like a charm :)

Hey @nour-sidaoui, can you share your solution? I am still facing the same issue. Thank you.

[nour-sidaoui]

Hey @bhagya97,
What caused my issue is that my sliders were rendered on data that gets updated.
The thing to keep in mind is that this error happens when the slider'smin value is less than the range min.
Or that the slider's max value is higher than the range max.

/!\ So be sure to update all of [range, min, max] when data changes. /!\

In my context, I simply update min & max if they get updated in props:

const RangeSlider = ({ unit, min, max, setDebouncedUnits, units }) => {
// ...
// This useEffect triggers a re-render when unit's min & max are changed

useEffect(() => {
setRange([min, max])}
,[min, max])
})

)}
Hope it helps

[oliviertassinari]

If somebody has a reproduction of the error with the latest version (v5) it would be awesome. We have tried to handle it in the past in #21653.

[nour-sidaoui]

@oliviertassinari
here you go :)

https://codesandbox.io/s/material-demo-forked-85tqs
I reproduced it on min (btw if you click on the rail, it works as the value is known)

[oliviertassinari]

So it's fixed in v5: https://codesandbox.io/s/material-demo-forked-lso9p?file=/demo.js.