Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tabs] Fix an infinite loop #14664

Merged
merged 2 commits into from
Mar 13, 2019

Conversation

caroe233
Copy link
Contributor

@caroe233 caroe233 commented Feb 25, 2019

I want the area for the scroll buttons of tabs to disappear when no scroll button should be displayed. Therefore I used CSS Override and set the width (or in this case the flexBasis) to 0:

MuiTabs: {
  scrollButtons: {
    flexBasis: 0
  }
}

This works fine most of the times, but in Edge with "weird" browser zoom factors, such as 105% it leads to an infinite loop. (I have not been able to reproduce this behavior with the standard zoom factors that are available in chrome...)

The code realizes that it needs to enable the left scroll button because the scrollWidth is bigger than the clientWidth. It then comes to the code and should realize that the scrollWidth is no longer bigger than (clientWidth + scrollLeft), but for some reason there seems to be a rounding issue for the scrollWidth property which leads to the scrollWidth property being 1px bigger than the (clientWidth + scrollLeft) property.

I fixed this by adding a "+ 1" to the condition to ignore rounding issues.

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 25, 2019

Details of bundle changes.

Comparing: 2f6a982...04261e2

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 362,768 362,772 92,403 92,403
@material-ui/core/Paper 0.00% 0.00% 68,878 68,878 20,172 20,172
@material-ui/core/Paper.esm 0.00% 0.00% 63,340 63,340 19,264 19,264
@material-ui/core/Popper 0.00% 0.00% 30,458 30,458 10,566 10,566
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,284 17,284 5,703 5,703
@material-ui/core/useMediaQuery 0.00% 0.00% 2,471 2,471 1,036 1,036
@material-ui/lab 0.00% 0.00% 170,726 170,726 50,110 50,110
@material-ui/styles 0.00% 0.00% 57,220 57,220 16,216 16,216
@material-ui/system 0.00% 0.00% 17,041 17,041 4,489 4,489
Button 0.00% 0.00% 91,218 91,218 26,957 26,957
Modal 0.00% 0.00% 90,263 90,263 26,728 26,728
colorManipulator 0.00% 0.00% 3,232 3,232 1,298 1,298
docs.landing 0.00% 0.00% 51,908 51,908 11,302 11,302
docs.main 0.00% 0.00% 648,034 648,034 201,660 201,660
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 311,223 311,227 85,394 85,396

Generated by 🚫 dangerJS against 04261e2

@oliviertassinari
Copy link
Member

@caroe233 Thank you for taking the time to submit a pull request. However, we need a reproduction example before considering any changes. Could you provide one? Thank you!

@caroe233
Copy link
Contributor Author

caroe233 commented Mar 1, 2019

Hi @oliviertassinari ,

this is the sandbox: https://codesandbox.io/s/13mwv6llxl?fontsize=14

For reproducing the error, open the link in EDGE, open the preview in a new window (or use the following link going to the separated preview: https://13mwv6llxl.codesandbox.io/

You should see four tabs.

Then use your mousewheel to change the browser zoom to 105% and EDGE will crash.

Hope that's what you were asking for :)

@oliviertassinari oliviertassinari added the component: tabs This is the name of the generic UI component, not the React module! label Mar 13, 2019
@oliviertassinari oliviertassinari changed the title [Tabs] No infinite loop for tabs [Tabs] Fix an infinite loop Mar 13, 2019
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Mar 13, 2019
@oliviertassinari
Copy link
Member

@caroe233 Thank you! an extra pixel won't harm :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants