[material-ui][Tabs] Scrollable variant fails test when clicking the tab (error reading 'offsetHeight')

nicholeuf commented 5 months ago

Steps to reproduce

Update Jun 7, 2024: There was a comment on the PR questioning whether the original live example isn't depending on https://github.com/mui/material-ui/pull/36485. Therefore, a new live example was created and linked below, which is connected to https://github.com/nicholeuf/mui-test-fixture-41388.

Original link to live example: codesandbox.io

This only seems to happen under test. See the test scenario in the link above.

Current behavior

The test fails with the following error:

    TypeError: Cannot read properties of null (reading 'offsetHeight')

      at setMeasurements (node_modules/.pnpm/@mui+material@5.15.12_@emotion+react@11.11.4_@emotion+styled@11.11.0_@types+react@18.2.48_react-dom@18.2.0_react@18.2.0/node_modules/@mui/material/node/Tabs/ScrollbarSize.js:40:47)

It fails at:

https://github.com/mui/material-ui/blob/2ef685d19ec53aafcddf15f67364a4974a86eaf2/packages/mui-material/src/Tabs/ScrollbarSize.js#L25-L27

Expected behavior

The test passes.

Context

I would like to test my application with a similar code setup.

Your environment

npx @mui/envinfo

From live example: ``` System: OS: Linux 6.1 Ubuntu 20.04.6 LTS (Focal Fossa) Binaries: Node: 20.12.1 - /home/codespace/nvm/current/bin/node npm: 10.5.0 - /home/codespace/nvm/current/bin/npm pnpm: 8.15.6 - /home/codespace/nvm/current/bin/pnpm Browsers: Chrome: Not Found npmPackages: @emotion/react: ^11.11.4 => 11.11.4 @emotion/styled: ^11.11.5 => 11.11.5 @mui/material: ^5.15.19 => 5.15.19 @mui/material-nextjs: ^5.15.11 => 5.15.11 @types/react: ^18.3.3 => 18.3.3 react: ^18.3.1 => 18.3.1 react-dom: ^18.3.1 => 18.3.1 typescript: ^5.4.5 => 5.4.5 ``` From original project where error was discovered: ``` System: OS: macOS 14.3.1 Binaries: Node: 18.19.1 - ~/.nvm/versions/node/v18.19.1/bin/node npm: 10.2.4 - ~/.nvm/versions/node/v18.19.1/bin/npm pnpm: Not Found Browsers: Chrome: 122.0.6261.111 Edge: Not Found Safari: 17.3.1 npmPackages: @emotion/react: ^11.11.4 => 11.11.4 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.38 @mui/core-downloads-tracker: 5.15.12 @mui/icons-material: ^5.15.12 => 5.15.12 @mui/material: ^5.15.12 => 5.15.12 @mui/material-nextjs: ^5.15.11 => 5.15.11 @mui/private-theming: 5.15.12 @mui/styled-engine: 5.15.11 @mui/system: 5.15.12 @mui/types: 7.2.13 @mui/utils: 5.15.12 @types/react: ^18.2.63 => 18.2.63 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^5.3.3 => 5.3.3 ```

Search keywords: tabs scrollable test

joacub commented 4 months ago

Same here on Next.js with ppr activated (https://nextjs.org/docs/app/api-reference/next-config-js/partial-prerendering).

ZeeshanTamboli commented 3 months ago

Looks like a bug.

supidupicoder2 commented 3 months ago

Same happens for me but without tests, just when trying to navigate to a specific view.

EDIT: What fixed it for me is using react and react-dom version 18.2.0 instead of 0.0.0-experimental-fb10a2c66-20240228

joacub commented 3 months ago

will be this fixed? can not update anymore nextjs , this is a simple check if the reference is undefined .....

thovden commented 3 months ago

This fails for me using the latest React 19 RC and React Compiler.

eb-revel commented 3 months ago

Also started getting this issue when rendering Tabs with React 19 / React Compiler. Monkey-patching setMeasurements inside @mui/material/Tabs/ScrollbarSize.js to the version below helps unblock development, hope this gets patched as React Compiler is a game changer for our app's performance.

  const setMeasurements = () => {
    if (nodeRef.current && scrollbarHeight.current) {
      scrollbarHeight.current = nodeRef.current.offsetHeight - nodeRef.current.clientHeight;
    }
  };

In case anyone else is blocked by this try https://www.npmjs.com/package/patch-package until this gets fixed

ZeeshanTamboli commented 3 months ago

Feel free to create a PR with a reproduction and a test.

nicholeuf commented 2 months ago

Feel free to create a PR with a reproduction and a test.

@ZeeshanTamboli I created a PR https://github.com/mui/material-ui/pull/42512 and linked it to this issue

oliviertassinari commented 2 months ago

This only seems to happen under test. See the test scenario in the link above.

From the reproduction https://github.com/nicholeuf/mui-test-fixture-41388, it seems to me that it's only that the snapshot test env isn't setup correctly. Doing this change fixes the issue:

diff --git a/utils/test-utils.tsx b/utils/test-utils.tsx
index f27a491..51c302a 100644
--- a/utils/test-utils.tsx
+++ b/utils/test-utils.tsx
@@ -17,8 +17,18 @@ const customRender = (
   options?: Omit<RenderOptions, 'wrapper'>
 ) => render(ui, { wrapper: TestWrapper, ...options });

-const customSnapshotRender = (children: ReactElement) =>
-  renderer.create(<TestWrapper>{children}</TestWrapper>);
+
+const customSnapshotRender = (children: ReactElement) => {
+  let root;
+  renderer.act(() => {
+    root = renderer.create(<TestWrapper>{children}</TestWrapper>, {
+      createNodeMock: (node) => {
+        return document.createElement(node.type as keyof HTMLElementTagNameMap);
+      },
+    });
+  });
+  return root;
+}

 export * from '@testing-library/react';
 export { customRender as render, customSnapshotRender as renderSnapshot };

See https://github.com/mui/material-ui/pull/16523/files#diff-d0f975a526b07952a6902336247e87b2de9a2d1ecd6c411c7492ac06009c8c73 for historical context.

oliviertassinari commented 2 months ago

This fails for me using the latest React 19 RC

A reproduction of the issue: https://github.com/oliviertassinari/material-ui-issue-41388 issue with React 19.0.0-rc.0 and Next.js v15.0.0-rc.0.

Looking closer, it's an issue with the ref. I have linked to #42381. #42512 is definitely the wrong fix.

Here is what happens:

Emotion adds a ref: https://github.com/emotion-js/emotion/blob/4cc565f1b7a576902b6360654afa4ce176698f76/packages/styled/src/base.js#L160
Since v19, React forwards it to the props: https://github.com/mui/material-ui/blob/2ef685d19ec53aafcddf15f67364a4974a86eaf2/packages/mui-material/src/Tabs/ScrollbarSize.js#L21
Once the ref of Emotion lands on the DOM element, it overrides the logic of the component https://github.com/mui/material-ui/blob/2ef685d19ec53aafcddf15f67364a4974a86eaf2/packages/mui-material/src/Tabs/ScrollbarSize.js#L52

To fix this, we can:

In Material UI's <ScrollbarSize> internal component:

-return <div style={styles} ref={nodeRef} {...other} />;
+return <div style={styles} {...other} ref={nodeRef} />;

but this feels like a hack

In Emotion, which would make more sense to me, in https://github.com/emotion-js/emotion/blob/4cc565f1b7a576902b6360654afa4ce176698f76/packages/styled/src/base.js#L160, only forward a ref, if one is defined.

+     if (ref) {
        newProps.ref = ref;
+     }

cc @Andarist

oliviertassinari commented 1 month ago

Issue created https://github.com/emotion-js/emotion/issues/3204.

oliviertassinari commented 1 month ago

Ok, the issue was solved in Emotion (not released yet): https://github.com/emotion-js/emotion/issues/3204

But styled-components has the same problem 🙈: https://github.com/styled-components/styled-components/issues/4331 (which we need to support for the sc engine adapter)

I think we can make the change in Material UI too, and call it a day. It will take some time for the developers to get the fix from Emotion, I'm not even sure Styled-components is maintained anymore, anyway.

-return <div style={styles} ref={nodeRef} {...other} />;
+return <div style={styles} {...other} ref={nodeRef} />;

To be noted that it only impacts this component because it doesn't use React.forwardRef.

hoangqwe159 commented 3 weeks ago

Any update on this, please?

DanFlannel commented 1 week ago

@oliviertassinari looks like they fixed this in version 11.12.0 just as a head up! Thank you for making the fix.

mui / material-ui