[material-ui][Tabs] ScrollbarSize.setMeasurements performs a null-check for nodeRef before setting scrollbarHeight #42512

nicholeuf · 2024-06-03T19:16:13Z

Resolves #41388 by defensively checking the nodeRef.current in the setMeasurements function.

Test fixture demonstrating the error: https://github.com/nicholeuf/mui-test-fixture-41388
Live example: https://codesandbox.io/p/devbox/exciting-wiles-m75jsd?file=%2Fapp%2Fpage.test.tsx%3A41%2C1

I have followed (at least) the PR section of the contributing guide.

…function

oliviertassinari · 2024-06-04T11:21:21Z

I don't understand this fix. Looking at the code, it seems that this shouldn't be possible to happen in the first place, so that the problem is not in this file, so it shouldn't be changed. What's the root problem?

oliviertassinari

Need a test case. It might be because the reproduction isn't depending on #36485

nicholeuf · 2024-06-04T18:56:49Z

Need a test case. It might be because the reproduction isn't depending on #36485

The reproduction is depending on a version of MUI released well after #36485 was merged.

The original bug report #41388 includes multiple comments from people encountering the same error.

I will continue to work on a test case.

nicholeuf · 2024-06-07T20:00:54Z

packages/mui-material/src/Tabs/Tabs.test.js

+
+    // https://github.com/mui/material-ui/issues/41388
+    it('should allow a user to click a scrollable Tab without an error', async function test() {
+      function DynamicScrollableTabs() {


Analogous to DynamicScrollableTabs used in the live example.

nicholeuf · 2024-06-07T20:02:34Z

packages/mui-material/src/Tabs/Tabs.test.js

+      // Click on the Item Seven Tab
+      // In the test fixture, this fails with
+      //  TypeError: Cannot read properties of null (reading 'offsetHeight')
+      await user.click(tab7);


This test is analogous to https://github.com/nicholeuf/mui-test-fixture-41388/blob/main/app/page.test.tsx. The user click action fails in the live example test fixture and succeeds here.

nicholeuf · 2024-06-07T20:08:32Z

Need a test case. It might be because the reproduction isn't depending on #36485

The latest live example uses the latest mui release and therefore includes #36485. A test case was created analogous to the live example test case that is failing with latest mui.

nicholeuf · 2024-06-07T20:17:19Z

I don't understand this fix. Looking at the code, it seems that this shouldn't be possible to happen in the first place, so that the problem is not in this file, so it shouldn't be changed. What's the root problem?

While it doesn't appear to be possible for the nodeRef to be null, it is clearly happening in the live example. It is also happening for other people, based on the comments.

Based on the caveats https://react.dev/reference/react/useRef#caveats, could we possibly be in a situation where the ref is being referenced during rendering? The error in the live example happens when the active tab is changed by clicking another tab.

mui-bot · 2024-06-15T07:54:53Z

Netlify deploy preview

https://deploy-preview-42512--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against c22554f

ZeeshanTamboli · 2024-06-15T08:05:09Z

packages/mui-material/src/Tabs/Tabs.test.js

@@ -1438,5 +1441,93 @@ describe('<Tabs />', () => {
      expect(hasLeftScrollButton(container)).to.equal(false);
      expect(hasRightScrollButton(container)).to.equal(false);
    });
+
+    // https://github.com/mui/material-ui/issues/41388
+    it('should allow a user to click a scrollable Tab without an error', async function test() {


This test passes in next branch HEAD, which means it isn't testing what's needed. It should fail in base branch.

joacub · 2024-06-23T20:10:24Z

please merge this 👯

oliviertassinari · 2024-06-23T20:52:24Z

packages/mui-material/src/Tabs/Tabs.test.js

+          'Item Ten',
+        ];
+        return (
+          <Container maxWidth="sm">


to simplify

oliviertassinari · 2024-06-23T20:52:51Z

packages/mui-material/src/Tabs/Tabs.test.js

@@ -1,6 +1,7 @@
 import * as React from 'react';
 import { expect } from 'chai';
 import { spy } from 'sinon';
+import userEvent from '@testing-library/user-event';


We don't allow this dependency, I mean to the best of my knowledge, we trigger each event instead.

oliviertassinari · 2024-06-23T20:53:26Z

packages/mui-material/src/Tabs/Tabs.test.js

+import Box from '@mui/material/Box';
+import Container from '@mui/material/Container';


To simplify

Suggested change

import Box from '@mui/material/Box';

import Container from '@mui/material/Container';

oliviertassinari · 2024-06-23T21:39:17Z

We appreciate the work spent on this but I'm closing. We are not adding defensive checks for things that React should guarantee to be unreachable. Instead, let's focus on #41388. We need a reproduction.

joacub · 2024-06-23T21:41:05Z

I'm closing, in any case, we are not adding defensive checks for things that React should guarantee to be unreachable.

But this is not working in react 19 at all, you preferred to block people than just add one line in the meantime react do something?

oliviertassinari · 2024-06-23T21:52:16Z

@joacub If you have a reproduction, please contribute it to #41388.

We can't change the source without being able to reproduce the problems. We need to have this rule because nothing great comes from not understanding what's going on, I have never seen great work being done without it, e.g. we need to write a test case so an engineer who never worked on the codebase doesn't introduce regressions.

In this instance, something is not right, a defensive check is not missing. To be blunt, I think MUI should be here to push the envelope, getting this right should take priority over fixing things quickly and dirty. If we take a long-term lens, I think going forward with this PR would be perceived by others as: amateur hours, at least this is how I would perceive it, I wouldn't touch that library.

joacub · 2024-06-23T21:58:57Z

@joacub We can't change the source without being able to reproduce the problems. If you have a reproduction, please contribute it to #41388. React 19 seems to work: https://codesandbox.io/p/devbox/exciting-wiles-forked-td8mq5?file=%2Fpackage.json%3A26%2C39 in its simplest case.

joacub · 2024-06-23T22:01:26Z

but @oliviertassinari , bugs are every where you can not be so straight about no implementing something that solves an issue in a particular case like this, also reference can be null at some point for any reason and some mutabilities so I don't think there is a issue adding any defensive check just in case.

defensive check for nodeRef.current in ScrollbarSize.setMeasurements …

1e600df

…function

nicholeuf mentioned this pull request Jun 3, 2024

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

Closed

nicholeuf changed the title ~~ScrollbarSize.setMeasurements performs a null-check for nodeRef before setting scrollbarHeight~~ [material-ui][Tabs] ScrollbarSize.setMeasurements performs a null-check for nodeRef before setting scrollbarHeight Jun 3, 2024

oliviertassinari added component: tabs This is the name of the generic UI component, not the React module! PR: needs test The pull request can't be merged labels Jun 4, 2024

oliviertassinari requested changes Jun 4, 2024

View reviewed changes

Write a test analogous to the test in the live example that is failing

f8b96ba

nicholeuf commented Jun 7, 2024

View reviewed changes

nicholeuf requested a review from oliviertassinari June 7, 2024 20:19

Merge branch 'next' into scrollbarsize-noderef-error

c22554f

ZeeshanTamboli reviewed Jun 15, 2024

View reviewed changes

ZeeshanTamboli added package: material-ui Specific to @mui/material and removed PR: needs test The pull request can't be merged labels Jun 15, 2024

oliviertassinari reviewed Jun 23, 2024

View reviewed changes

packages/mui-material/src/Tabs/Tabs.test.js

'Item Ten',

];

return (

<Container maxWidth="sm">

Copy link

Member

oliviertassinari Jun 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to simplify

oliviertassinari reviewed Jun 23, 2024

View reviewed changes

oliviertassinari added the PR: needs test The pull request can't be merged label Jun 23, 2024

oliviertassinari closed this Jun 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[material-ui][Tabs] ScrollbarSize.setMeasurements performs a null-check for nodeRef before setting scrollbarHeight #42512

[material-ui][Tabs] ScrollbarSize.setMeasurements performs a null-check for nodeRef before setting scrollbarHeight #42512

nicholeuf commented Jun 3, 2024 •

edited by oliviertassinari

Loading

oliviertassinari commented Jun 4, 2024

oliviertassinari left a comment

nicholeuf commented Jun 4, 2024

nicholeuf Jun 7, 2024

nicholeuf Jun 7, 2024

nicholeuf commented Jun 7, 2024 •

edited

Loading

nicholeuf commented Jun 7, 2024 •

edited

Loading

mui-bot commented Jun 15, 2024 •

edited

Loading

ZeeshanTamboli Jun 15, 2024

joacub commented Jun 23, 2024

oliviertassinari Jun 23, 2024

oliviertassinari Jun 23, 2024 •

edited

Loading

oliviertassinari Jun 23, 2024 •

edited

Loading

oliviertassinari commented Jun 23, 2024 •

edited

Loading

joacub commented Jun 23, 2024

oliviertassinari commented Jun 23, 2024 •

edited

Loading

joacub commented Jun 23, 2024

joacub commented Jun 23, 2024

		import Box from '@mui/material/Box';
		import Container from '@mui/material/Container';

[material-ui][Tabs] ScrollbarSize.setMeasurements performs a null-check for nodeRef before setting scrollbarHeight #42512

[material-ui][Tabs] ScrollbarSize.setMeasurements performs a null-check for nodeRef before setting scrollbarHeight #42512

Conversation

nicholeuf commented Jun 3, 2024 • edited by oliviertassinari Loading

oliviertassinari commented Jun 4, 2024

oliviertassinari left a comment

Choose a reason for hiding this comment

nicholeuf commented Jun 4, 2024

nicholeuf Jun 7, 2024

Choose a reason for hiding this comment

nicholeuf Jun 7, 2024

Choose a reason for hiding this comment

nicholeuf commented Jun 7, 2024 • edited Loading

nicholeuf commented Jun 7, 2024 • edited Loading

mui-bot commented Jun 15, 2024 • edited Loading

Netlify deploy preview

Bundle size report

ZeeshanTamboli Jun 15, 2024

Choose a reason for hiding this comment

joacub commented Jun 23, 2024

oliviertassinari Jun 23, 2024

Choose a reason for hiding this comment

oliviertassinari Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Jun 23, 2024 • edited Loading

joacub commented Jun 23, 2024

oliviertassinari commented Jun 23, 2024 • edited Loading

joacub commented Jun 23, 2024

joacub commented Jun 23, 2024

nicholeuf commented Jun 3, 2024 •

edited by oliviertassinari

Loading

nicholeuf commented Jun 7, 2024 •

edited

Loading

nicholeuf commented Jun 7, 2024 •

edited

Loading

mui-bot commented Jun 15, 2024 •

edited

Loading

oliviertassinari Jun 23, 2024 •

edited

Loading

oliviertassinari Jun 23, 2024 •

edited

Loading

oliviertassinari commented Jun 23, 2024 •

edited

Loading

oliviertassinari commented Jun 23, 2024 •

edited

Loading