[Tabs] Are not working with React.Suspense

[HaNdTriX]

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

Tabs should mount correctly when using <React.Suspense>

Current Behavior 😯

Tabs do not render correctly when using <React.Suspense>.

• scrollbars are visible
• <TabIndicator /> is missing

Steps to Reproduce 🕹

1. Open https://codesandbox.io/s/p5186w26nm

Context 🔦

I believe the issue here is that componentDidMount is called before the Component is visible.
(<React.Suspense> renders the Tabs to a hidden div).
This causes all layout methods (updateIndicatorState, updateScrollButtonState) to not work properly.

Your Environment 🌎

Tech	Version
Material-UI	v3.8.1
React	v16.7.0-alpha.2

[HaNdTriX]

I believe this issue affects all layout methods used in componentDidMount and @material-ui (not only Tabs).

[joshwooding]

This is due to how the scrollbar size is calculated
https://github.com/mui-org/material-ui/blob/0d8e8678bc3eb7f832238b5b8e4c584450680882/packages/material-ui/src/Tabs/ScrollbarSize.js#L57

when an element isn't visible it's offsetHeight and clientHeight are 0 which breaks the calculation as it's listening on window resizing

[HaNdTriX]

@joshwooding I thought so. We might need another lifecycle hook to start these calculations as soon as this component is visible.

[oliviertassinari]

Oh boy! React Suspense, what have you done! 😆
Regarding the scrollbar height issue, one possible workaround is to use the Portal component:

--- a/packages/material-ui/src/Tabs/ScrollbarSize.js
+++ b/packages/material-ui/src/Tabs/ScrollbarSize.js
@@ -2,6 +2,7 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import EventListener from 'react-event-listener';
 import debounce from 'debounce'; // < 1kb payload overhead when lodash/debounce is > 3kb.
+import Portal from '../Portal';

 const styles = {
   width: 90,
@@ -34,15 +35,15 @@ class ScrollbarSize extends React.Component {
     }
   }

-  componentDidMount() {
-    this.setMeasurements();
-    this.props.onChange(this.scrollbarHeight);
-  }
-
   componentWillUnmount() {
     this.handleResize.clear();
   }

+  handleRendered = () => {
+    this.setMeasurements();
+    this.props.onChange(this.scrollbarHeight);
+  };
-
   componentWillUnmount() {
     this.handleResize.clear();
   }

+  handleRendered = () => {
+    this.setMeasurements();
+    this.props.onChange(this.scrollbarHeight);
+  };
+
   handleRef = ref => {
     this.nodeRef = ref;
   };
@@ -61,7 +62,9 @@ class ScrollbarSize extends React.Component {
     return (
       <React.Fragment>
         <EventListener target="window" onResize={this.handleResize} />
-        <div style={styles} ref={this.handleRef} />
+        <Portal onRendered={this.handleRendered}>
+          <div style={styles} ref={this.handleRef} />
+        </Portal>
       </React.Fragment>
     );
   }

Now, regarding the React lifecycles, I can't find anything we can leverage to trigger the tab indicator and scrollbar button display. I think that we should open an issue on React side. I doubt we are the only one with the problem. I think that React should provide an API to notify the components that the display: none style was removed.

[oliviertassinari]

I have opened an issue on React side: facebook/react#14536. The Portal is really a dirty hack that only solves 1/3 of the issue.

[oliviertassinari]

@joshwooding has found in #14181 that we can potentially use the IntersectionObserver API.

[eps1lon]

@joshwooding has found in #14181 that we can potentially use the IntersectionObserver API.

I like this API but https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API#Browser_compatibility

[eps1lon]

Disclaimer: Don't use this in production.

Fixed demo: https://codesandbox.io/s/6znx30q23w

There are two heuristics for determining if a tree is suspended:

1. The fallback is not unmounted yet i.e. the tree is suspended by default and only "unsuspend" in cWU
2. A DOM node in the tree without inline styles changes its style (checked via MutationObserver

The first point will not work if no fallback is rendered i.e. maxDuration is not reached. The second point is all over the place since I tried to make it as memory safe as possible. It's also quite wasteful since I simply remount the Tabs once I think the tree is not supsended.

[joshwooding]

I was looking at MutationObserver the only thing I don't like about is that it uses polling and although the fix won't be bad for Tabs with Textarea I suspect the performance implications to be greater

[eps1lon]

I was looking at MutationObserver the only thing I don't like about is that it uses polling and although the fix won't be bad for Tabs with Textarea I suspect the performance implications to be greater

None of this (including react) is production ready so I think you're getting ahead of yourself if you're already thinking about performance.

[joshwooding]

I was looking at MutationObserver the only thing I don't like about is that it uses polling and although the fix won't be bad for Tabs with Textarea I suspect the performance implications to be greater

None of this (including react) is production ready so I think you're getting ahead of yourself if you're already thinking about performance.

I do realise the code isn’t production ready. I was just adding my thoughts and just listing the potential downsides I had read about. 🙂

[eps1lon]

[...] I had read about.

Speaking about it: Do you have sources for this? Quick google search did not yield anything beyond specific problems on stackoverflow about bad performance of MutationObserver. Did not find anything about polling.

[Jessidhia]

The entire point of using IntersectionObserver or MutationObserver is to eliminate polling. Polling might only be needed if you're having to polyfill it.

[joshwooding]

@eps1lon I can’t remember where I read it now. Maybe I misread something

[HaNdTriX]

🤫 You can also use CSS animation events to detect the visibility of a Dom node. Nevertheless I believe this issue needs to be fixed by react core.

[Eldraed]

Hello,
Just for information, i'm also having trouble with the "outlined" variant of TextField due to this portion of code :
https://github.com/mui-org/material-ui/blob/41920170e5d631ec3d1ac686b7bdf0e31befc3e6/packages/material-ui/src/TextField/TextField.js#L97-L103
Like Tabs, width is calculated during the fallback of my Suspense Loader, so width = 0px !

In result, the line pass under the label :

[eps1lon]

@Eldraed This is a common issue with Suspense in sync mode. React will still mount trees in an inconsistent state and just display: none; the components that are not suspended.

What you can do for now is remount the TextField if your placeholder unmounts with a key that changes depending on the mount state of your placeholder.

If you can you could also put the suspense tree inside React.unstable_ConcurrentMode which doesn't mount inconsistent trees.

See facebook/react#14536 for more information.

[Eldraed]

Thanks for the reply @eps1lon

I tested Concurrent Mode before posting earlier, i didn't mention it.
It works well, label is good but Concurrent mode comes with Strict mode... I was looking on using Strict mode weeks ago but some libraries doesn't works well with it for now (like Apollo). So i left that aside for now, and was hoping to get updates soon on it.

So, I'll rather look at your first option :)

[gaearon]

This is fixed with React 18 alpha.

17: https://codesandbox.io/s/material-ui-tabs-suspense-bug-forked-4n89l?file=/src/index.js
18 alpha with createRoot: https://codesandbox.io/s/material-ui-tabs-suspense-bug-forked-lw409?file=/src/index.js

Concurrent mode comes with Strict mode

Not anymore! (Also, it's not a "mode" anymore either.)

[eps1lon]

Closing since the underlying React issue has been closed as well.