Material-ui: [Tabs] Inifinite loop in the scroll button logic

[andrewtpoe]

• [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 🤔

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 😯

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 🕹

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.
   3.
   4.

Context 🔦

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

Your Environment 🌎

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

[oliviertassinari]

@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).

[andrewtpoe]

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

[andrewtpoe]

@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.

[oliviertassinari]

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:.

[eps1lon]

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

[andrewtpoe]

In the browser.

[DekelYossef]

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!

[eps1lon]

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

[eps1lon]

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

[eps1lon]

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.

[oliviertassinari]

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.

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?

[andrewtpoe]

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

[eps1lon]

@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.

[oliviertassinari]

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

[IssueHuntBot]

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