IssueHunt
Bug Bounty (New!)
Browse
Sign in with GitHub
mui-org/material-ui

[Tabs] Inifinite loop in the scroll button logic #13699

andrewtpoe posted onGitHub

<!--- Provide a general summary of the issue in the Title above -->

<!-- Thank you very much for contributing to Material-UI by creating an issue! ❤️ To avoid duplicate issues we ask you to check off the following list. -->

<!-- Checked checkbox should look like this: [x] -->

  • This is not a v0.x issue. <!-- (v0.x is no longer maintained) -->
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

<!--- Describe what should happen. -->

I am rendering a tabs component with scrollButtons="auto" and scrollable={true}. The number of tabs I am rendering causes the scroll buttons to display at all times. If the initial value correlates to one of the tabs hidden on first render, the tabs component should scroll to display the correct tab and show it as selected.

Current Behavior 😯

<!--- Describe what happens instead of the expected behavior. -->

I receive the following error:

Uncaught Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
    at invariant (react-dom.development.js:57)
    at scheduleWork (react-dom.development.js:19364)
    at Object.enqueueSetState (react-dom.development.js:12789)
    at Tabs.push../node_modules/react/cjs/react.development.js.Component.setState (react.development.js:354)
    at Tabs._this.updateScrollButtonState (Tabs.js:264)
    at Tabs.componentDidUpdate (Tabs.js:309)
    at commitLifeCycles (react-dom.development.js:16737)
    at commitAllLifeCycles (react-dom.development.js:18160)
    at HTMLUnknownElement.callCallback (react-dom.development.js:147)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:196)
    at invokeGuardedCallback (react-dom.development.js:250)
    at commitRoot (react-dom.development.js:18365)
    at completeRoot (react-dom.development.js:19894)
    at performWorkOnRoot (react-dom.development.js:19817)
    at performWork (react-dom.development.js:19722)
    at performSyncWork (react-dom.development.js:19696)
    at requestWork (react-dom.development.js:19551)
    at scheduleWork (react-dom.development.js:19358)
    at Object.enqueueSetState (react-dom.development.js:12789)
    at Tabs.push../node_modules/react/cjs/react.development.js.Component.setState (react.development.js:354)
    at Tabs._this.updateScrollButtonState (Tabs.js:264)
    at Tabs.js:280
    at later (index.js:28)

Steps to Reproduce 🕹

<!--- Provide a link to a live example (you can use codesandbox.io) and an unambiguous set of steps to reproduce this bug. Include code to reproduce, if relevant (which it most likely is). This codesandbox.io template may be a good starting point: https://codesandbox.io/s/github/mui-org/material-ui/tree/master/examples/create-react-app If you're using typescript a better starting point would be https://codesandbox.io/s/github/mui-org/material-ui/tree/master/examples/create-react-app-with-typescript If YOU DO NOT take time to provide a codesandbox.io reproduction, should the COMMUNITY take time to help you? -->

Link: https://codesandbox.io/s/9oo90okxmo

This issue has happened for me with the code sandbox link, however, it is intermittent. It appears to happen more regularly with more (not necessarily related) elements rendered on screen. This makes me suspect a race condition somewhere, but as I'm not familiar with the library code I'll leave that to the experts for now.

  1. Render a Tabs component with many tabs. Include the props scrollButtons="auto" and scrollable={true}. Make sure the tabs can scroll.
  2. Set the initial value for the Tabs to a value that forces the Tabs component to scroll.

Context 🔦

<!--- What are you trying to accomplish? How has this issue affected you? Providing context helps us come up with a solution that is most useful in the real world. -->

My tabs scroll. I am providing a value on render. That value may mean the Tabs component should scroll.

Your Environment 🌎

<!--- Include as many relevant details about the environment with which you experienced the bug. If you encounter issues with typescript please include version and tsconfig. -->

Tech Version
Material-UI v3.5.1
React 16.6.3
Browser latest chrome
TypeScript no
etc.

@andrewtpoe It's not the first time we get this repport, but we couldn't reproduce it to date, if you could create a minimal reproduction example, it would help, a lot! (codesandbox can be a start).

posted by oliviertassinari over 6 years ago

This is what I have, but it doesn't appear to cause the error on code sandbox. This code essentially mirrors what is causing the issue in my app though:

https://codesandbox.io/s/9oo90okxmo

posted by andrewtpoe over 6 years ago

@oliviertassinari - The codesandbox I've linked in the comment above has caused the error for me, though it appears to not happen regularly. In my app it happens all the time.

posted by andrewtpoe over 6 years ago

This is exactly the issue we had with reproducing the bug. It's gone once put in isolation, making it very hard to debug :disappointed:.

posted by oliviertassinari over 6 years ago

Does this happen in the browser or in a unit test i.e. with jsdom?

posted by eps1lon over 6 years ago

In the browser.

posted by andrewtpoe over 6 years ago

Having the same issue.. with v3.4.0 https://9oo90okxmo.codesandbox.io/ This is the same sandbox instance of @andrewtpoe, but full-screened. As you can see the error occurs. Thanks!

posted by DekelYossef over 6 years ago

@DekelYossef Perfect. That one is reproducible for me on the initial page load.

posted by eps1lon over 6 years ago

My guess: 

-if (
-  showLeftScroll !== this.state.showLeftScroll ||
-  showRightScroll !== this.state.showRightScroll
-) {
-  this.setState({ showLeftScroll, showRightScroll });
-}
+ this.setState(state => state.showLeftScroll !== showLeftScroll || state.showRightScroll !== showRightScroll ? { showLeftScroll, showRightScroll } : null )

Same issue here https://github.com/mui-org/material-ui/blob/1c8c88781c5d48915099cf4db17f255c17b052e5/packages/material-ui/src/Tabs/Tabs.js#L281-L288

posted by eps1lon over 6 years ago

Tried this and it didn't work. As it stands right now its toggling showRightScroll on every cDU. Needs further investigation. Maybe gSBU will help.

Update:

  • clientWidth of div[role="tabslist"] is changing on every cDU

Update2:

  • Fix looks promising. Could everybody who was able to reproduce it check out: https://6233p4jzpn.codesandbox.io/ The solution was to snapshot measuring and scroll properties of the tabslist and use those instead of the current ref to determine if we should update. Overall I think we might be able to simplify the component by always rendering the indicators and simply let overflow: hidden and some css translate do the rest.
posted by eps1lon over 6 years ago

Wow, nice reproduction! ❤️ This is definitely an infinit loop issue, and not the first one on the tabs I look into :). I could notice that the PrivateTabScrollButton component width isn't stable.

capture d ecran 2018-11-29 a 00 52 26

The fix should be as simple as:

export const styles = {
  /* Styles applied to the root element. */
  root: {
    color: 'inherit',
-   flex: '0 0 56px',
+   width: 56,
+   flexShrink: 0,
  },
};

/**
 * @ignore - internal component.
 */
function TabScrollButton(props) {

@eps1lon What do you think?

posted by oliviertassinari over 6 years ago

Thanks @DekelYossef! I'm not super familiar with Code Sandbox and didn't know you could do that.

posted by andrewtpoe over 6 years ago

@oliviertassinari I noticed that my fix has some undesired side-effects so I would prefer your fix. Is that component public? If so I would add a big warning that it should have a static width.

posted by eps1lon over 6 years ago

@eps1lon The component is private, I think that it's fine. Also, using width over flex-basis, should improve customizability.

posted by oliviertassinari over 6 years ago

@issuehuntfest has funded $60.00 to this issue. See it on IssueHunt

posted by IssueHuntBot over 6 years ago

Fund this Issue

$60.00
Funded
issuehunt(32,559)$60.00
Only logged in users can fund an issue

Pull requests

Recent activities

issuehuntfest funded $ 60.00 for mui-org/material-ui# 13699
over 6 years ago
Company
AboutMission
Community
SpectrumEmbed
Support
FAQTerms of usePrivacy policyCode of conductCredits
Connect
FacebookTwitter

© 2019 BoostIO, Inc.