material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

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

Open nicholeuf opened this issue 11 months ago • 10 comments

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.

Screenshot 2024-06-07 at 3 40 13 PM

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/@[email protected]_@[email protected]_@[email protected]_@[email protected][email protected][email protected]/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

nicholeuf avatar Mar 07 '24 03:03 nicholeuf

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

joacub avatar Apr 02 '24 22:04 joacub

Looks like a bug.

ZeeshanTamboli avatar Apr 29 '24 13:04 ZeeshanTamboli

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

supidupicoder2 avatar Apr 30 '24 05:04 supidupicoder2

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

joacub avatar May 09 '24 17:05 joacub

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

thovden avatar May 17 '24 09:05 thovden

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

eb-revel avatar May 20 '24 08:05 eb-revel

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

ZeeshanTamboli avatar May 20 '24 13:05 ZeeshanTamboli

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

nicholeuf avatar Jun 03 '24 19:06 nicholeuf

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 avatar Jun 23 '24 21:06 oliviertassinari

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.

SCR-20240624-bfpp

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:

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

  1. 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 avatar Jun 23 '24 22:06 oliviertassinari

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

oliviertassinari avatar Jul 02 '24 23:07 oliviertassinari

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.

oliviertassinari avatar Jul 09 '24 21:07 oliviertassinari

Any update on this, please?

hoangqwe159 avatar Jul 30 '24 06:07 hoangqwe159

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

DanFlannel avatar Aug 14 '24 00:08 DanFlannel