Material-ui: Slider overflows element width

[danielzabotti]

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

Slider should not overflow its element width.

Current Behavior

Sliding to the right, the component overflows its width potentially causing styling issues

Steps to Reproduce

Link:

Preview: https://34041kkjnq.codesandbox.io/
Editor: https://codesandbox.io/s/34041kkjnq
Slide!

Context

I'm using the slider and it is causing a horizontal scrollbar to appear. The problem started in 3.0.0-alpha.22

Your Environment

| Tech | Version |
| react-dom | latest |
| react | latest |
| prop-types | latest |
| @material-ui/la | 3.0.0-alpha.22 |
| @material-ui/core | 3.3.2 |

[eps1lon]

We discussed this in #12967 specifically https://github.com/mui-org/material-ui/pull/12967#issuecomment-423853188. The issue is that adding a padding so that the outline does not overflow makes alignment harder. It's better in those cases to adjust the surrounding components so that the outline does not cover them.

[danielzabotti]

We discussed this in #12967 specifically #12967 (comment). The issue is that adding a padding so that the outline does not overflow makes alignment harder. It's better in those cases to adjust the surrounding components so that the outline does not cover them.

This is not the same issue. Please look at this recording from the provided link and observe the horizontal scrollbar behavior

[lennerd]

I found out that the thumbWrapper is causing the problem. This element is translated by percentage but at the same time has the same width as the container. So when translating the wrapper element on the x-axis the element is actually shifting outside the bounds of its container.

[eps1lon]

This is fixable if the wrapper has overflow: visible instead of auto. But I agree that this is not ideal since the component doesn't quite work in every context.

Does anybody know if we can solve this and still use transform instead of left, top etc?

[lennerd]

Maybe by using transform-origin? Not sure about it’s performance compared to transform though.

[jeffwillette]

@eps1lon your solution didn't work for me, but I was able to set overflowX: 'hidden' on my containing div to temporarily fix it. Not ideal but its a workaround for now

[eps1lon]

@deltaskelta That was targeted at the provided codesandbox. However if hidden worked then visible should as well. You probably have another wrapper div that gets the scrollbar instead.

Basically add overflow: visible to every wrapper that gets a scrollbar.

[tagdara]

I'm encountering this same problem, especially inside of a Dialog. Adding overflowX: visible to every single container all the way up the tree one at a time does not fix it. Only overflowX: hidden seems to really work, and that leads to clipping of the handle when it is in the far right position.

[lennerd]

Is performance really this important here? I mean we talk about a simple slider and one or maybe at some point more handles.

Otherwise we could simply use position left to get relative positioning to the parent.

[eps1lon]

@lennerd I'm thinking the same. I recognize that transform is faster but without a benchmark this is just premature optimization.

Maybe @Pajn has a solution since he proposed the change.

[Pajn]

EDIT: I have mixed up this issue with https://github.com/mui-org/material-ui/issues/13502
Both obviously needs to be fixed, but my rambling below is better fitted to the other one.

I don't think this is directly related to the transform but instead that I removed the max offsets https://github.com/mui-org/material-ui/pull/13325/files#diff-ce754dbddbe71df3c3d56badf6ac8d15L368

I can work around it on the bug demo with:

.SimpleSlider-slider-2 {
    padding-left: 6px;
    padding-right: 6px;
}

Note: this workaround causes the thumb to end edge-to-edge with the track and not center-to-edge so it's not a full fix.

This bug does not seem to show up on the material-ui demo nor in the app that sprung the change to begin with, I really don't know why. In my mind, as long as you does not do anything weird with pointer-events the hit test of which element was clicked on should match even if the element is outside of its parent. Obviously there are some cases when this is not true.

Is performance really this important here? I mean we talk about a simple slider and one or maybe at some point more handles.

The problem is that layout very rarely is scoped to a single element, and is instead done on the whole page. This is because so many things in CSS can change appearance outside of the parent element so for the browser to create a "layout boundary" a long list of requirements needs to be fulfilled.
If you go to an older version of the docs https://v3-0-0.material-ui.com/lab/slider/ and test the slider with the "performance tab" in Chrome dev tools and then click on the layout work you can see that it is done on the whole document Layout root #document. On a simple page like the the demo this doesn't matter much because the layout is fast. But in a big application the layout takes a lot of time.

I promised a benchmark but did not end up creating one because building the app locally in production mode did not work and in dev mode it does not make 60FPS in either of the implementations because of a lot of JS overhead. If you really want I can try to fix the build again so that I can show the difference. However, I can tell you that it previously did not meet 60 FPS and now do.

However a slow implementation is of course better than a not working implementation, I'll give it a shot but if it's not fixed soon I'm in favour of reverting my change until there is a better working version of it.

[eps1lon]

The sandbox could also by fixed by not duplicating the width. Currently it's setting the width in the root with classes and inside the root with inline styles. Removing the inline style removes the scrollbar.

[Pajn]

Hmm, I think we need a discussion of how it should behave.

To fill width and overflow (https://github.com/mui-org/material-ui/pull/12967#issuecomment-423853188) and supporting overflow prevention (overflow: auto on the parent) is by definition incompatible. The implementation can be fixed so that not the complete thumbWrapper overflows but the handle will overflow up to 6px and the ripple around the handle will overflow up to 24px (18px for the ripple + 6px for the handle).

Maybe a contain option or similar should be added to slider so that it is possible to make it not overflow its root element when using it in contained spaces.

Adding overflow: auto to the parent does not work very well in the old implementation either due to the mentioned problem Try it in https://v3-2-0.material-ui.com/lab/slider/ and manually adding overflow: auto However if your parent is a bit wider than the slider but not twice as wide, the old implementation did work. Something like #13616 can restore that behaviour.

[Bobgy]

I posted my comment in a duplicate issue. Cross posting here.
https://github.com/mui-org/material-ui/issues/13649#issuecomment-442049681
I wasn't aware of the discussion going on here, so some information is duplicating existing comments. However, I think my fix could cause fewer side effects than the existing PR. It doesn't need to use negative margin.

One non-ideal but easy workaround (for user) is to apply overflow-x: hidden on the scroll container, for example body. https://codesandbox.io/s/8l28mrjy8
I hit a similar issue, but as far as I can think of, whenever I don't want slider to cause a scrollbar, I don't want anything to cause a scrollbar. So I think this approach may be acceptable for an app using mui.

I tried a hack to fix the issue for slider. Not sure if that won't have side effect in other ways, so I will post my idea here first before making a PR.

HACK: overflow scroll only works for overflowing to right or bottom, if I apply a -100% translateX to thumb wrapper, although it overflowed document to the left side, it won't let document become bigger.

Here are codepens showing the idea (using the same styling approach as existing slider in mui-lab).
codepen showing problem of existing approach: https://codepen.io/Bobgy/pen/oQPXxj
codepen using the hack I just mentioned: https://codepen.io/Bobgy/pen/Krxwjo
(Note: you can change translateX of thumb-wrapper to simulate sliding the slider.)

p.s. I tried adding the fix to compiled mui code and that worked for my app, but I'm not familiar with contribution. So I want some feedback on this approach first before making any further progress.

[Fabbok1x]

Really no good solutions at all here.
This is a severe visual bug that still is unfixed.
While actually it's as simple as changing from transform to left.
Obviously benchmarks can be neglected for such a small visual operation.

[eps1lon]

This is a severe visual bug that still is unfixed.

In a package marked as alpha. It sounds like you're relying on this in production. Maybe try an earlier version?

Obviously benchmarks can be neglected for such a small visual operation.

What is a "small visual operation"? It's not obvious to me at all that this is no issue. The original PR provided plenty of material supporting this. Maybe you can add some resources that disprove this?

[charlesgiroux]

I posted my comment in a duplicate issue. Cross posting here.
#13649 (comment)
I wasn't aware of the discussion going on here, so some information is duplicating existing comments. However, I think my fix could cause fewer side effects than the existing PR. It doesn't need to use negative margin.

One non-ideal but easy workaround (for user) is to apply overflow-x: hidden on the scroll container, for example body. https://codesandbox.io/s/8l28mrjy8
I hit a similar issue, but as far as I can think of, whenever I don't want slider to cause a scrollbar, I don't want anything to cause a scrollbar. So I think this approach may be acceptable for an app using mui.
I tried a hack to fix the issue for slider. Not sure if that won't have side effect in other ways, so I will post my idea here first before making a PR.
HACK: overflow scroll only works for overflowing to right or bottom, if I apply a -100% translateX to thumb wrapper, although it overflowed document to the left side, it won't let document become bigger.
Here are codepens showing the idea (using the same styling approach as existing slider in mui-lab).
codepen showing problem of existing approach: https://codepen.io/Bobgy/pen/oQPXxj
codepen using the hack I just mentioned: https://codepen.io/Bobgy/pen/Krxwjo
(Note: you can change translateX of thumb-wrapper to simulate sliding the slider.)
p.s. I tried adding the fix to compiled mui code and that worked for my app, but I'm not familiar with contribution. So I want some feedback on this approach first before making any further progress.

+1 To me this is an acceptable workaround. I prefer applying the "overflow: hidden" as low as possible in my tree. e.g.: on a form wrapper.

[eps1lon]

@charlesgiroux While this looked promising at first it isn't easily applicable to rtl themes where this approach will cause the same issues as before. It also needs additional logic for vertical sliders.

[Bobgy]

@eps1lon good point! They are indeed constraints. I don't think that they couldn't be worked around, but I would imagine code being dirty and full of conditions.

[eps1lon]

@eps1lon good point! They are indeed constraints. I don't think that they couldn't be worked around, but I would imagine code being dirty and full of conditions.

@Bobgy It's already pretty messy as is. It would be immensely helpful if you could work on this but I understand if you don't want to since you have to consider so many cases.

One of the issues I have is that the the reported issue here is due to a seemingly arbitrary overflow: auto. It would be nice to have an actual example. Maybe there exists an easier fix on the callsite. Our documented examples work fine.

[Bobgy]

I am pretty sure I will be busy recently. I might be able to work on this a month later, but I couldn't say for sure. If anyone has other ideas or would like to implement this. Go ahead with it.

[nick-cromwell]

Was this fix included in the latest release 3.0.0-alpha.30? I'm still seeing this issue loading the slider into a google maps InfoWindow.

[joshwooding]

@nick-cromwell It was released as part of @material-ui/[email protected]

If you are still experiencing the problem please provide a reproducible example.

[nick-cromwell]

@joshwooding Thanks - I won't be upgrading @material-ui/core to 4.0.0-alpha yet so I'll be waiting until then

[darkowic]

So bad it was released as part 4 alpha... would be great if this is released for 3.x too

[MatteoGioioso]

looks like the bug it still present https://codesandbox.io/s/kr5njz435