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] Inifinite loop in the scroll button logic #13699

Closed
2 tasks done
andrewtpoe opened this issue Nov 27, 2018 · 15 comments
Closed
2 tasks done

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

andrewtpoe opened this issue Nov 27, 2018 · 15 comments
Labels
bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@andrewtpoe
Copy link

andrewtpoe commented Nov 27, 2018

  • This is not a v0.x issue.
  • 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.

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 oliviertassinari added the status: waiting for author Issue with insufficient information label Nov 27, 2018
@oliviertassinari
Copy link
Member

@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
Copy link
Author

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
Copy link
Author

@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
Copy link
Member

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

@eps1lon
Copy link
Member

eps1lon commented Nov 27, 2018

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

@andrewtpoe
Copy link
Author

andrewtpoe commented Nov 27, 2018 via email

@DekelYossef
Copy link

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
Copy link
Member

eps1lon commented Nov 28, 2018

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

@eps1lon
Copy link
Member

eps1lon commented Nov 28, 2018

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 eps1lon added bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module! and removed status: waiting for author Issue with insufficient information labels Nov 28, 2018
@eps1lon
Copy link
Member

eps1lon commented Nov 28, 2018

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 oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Nov 28, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 28, 2018

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?

@andrewtpoe
Copy link
Author

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

@eps1lon
Copy link
Member

eps1lon commented Nov 29, 2018

@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
Copy link
Member

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

@oliviertassinari oliviertassinari changed the title Tabs causes stack overflow when initial value causes scroll [Tabs] Inifinite loop in the scroll button logic Dec 3, 2018
@IssueHuntBot
Copy link

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

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! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

No branches or pull requests

5 participants