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] Scrollable tabs crashes when overriding styles in theme using slots callback #38406

Closed
2 tasks done
pvdstel opened this issue Aug 10, 2023 · 4 comments · Fixed by #38544
Closed
2 tasks done

[Tabs] Scrollable tabs crashes when overriding styles in theme using slots callback #38406

pvdstel opened this issue Aug 10, 2023 · 4 comments · Fixed by #38544
Assignees
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. package: material-ui Specific to @mui/material

Comments

@pvdstel
Copy link
Contributor

pvdstel commented Aug 10, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/mui-tabs-style-override-crash-p9wfjm-p9wfjm?file=/package.json

Downgrading the package version in the live example will "fix" the error.

Steps:

  1. Use a Tabs component, set its variant to scrollable.
  2. Create a custom theme which uses a function to override Tabs styling. This function uses the props parameter to decide which styles to apply. The ownerState.orientation field is deconstructed out of the object.

Current behavior 😯

The rendering crashes, stating that _ref.ownerState is undefined (where _ref is the name of the props parameter after downleveling). This happens because the props object does not have the ownerState prop, even though its typings suggest that it does have that prop.

Expected behavior 🤔

The props object should have the ownerState prop, as the typings describe.

Context 🔦

This appears to happen because TabsScrollbarSize is being rendered. It uses the same style overrides as the other Tabs components, but it does not get an ownerState prop. Thus, it is undefined.

Your environment 🌎

This error started appearing during routine package upgrades. In one package, it began to occur after upgrading from 5.14.2 to 5.14.4. But in others, the upgrade to 5.14.4 did not trigger the same issue, and only appeared when we forced nested dependencies to upgrade as well.

We are not completely sure, but believe that it is related to @mui/system. Downgrading that package appears to resolve the issue on the latest version of @mui/material.

"resolutions": {
    "@mui/system": "5.14.1"
}

It happens in all browsers.

npx @mui/envinfo
  System:
    OS: Windows 10 10.0.22621
  Binaries:
    Node: 18.17.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - C:\Program Files\nodejs\yarn.CMD
    npm: 9.6.7 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: Not Found
    Edge: Spartan (44.22621.1992.0), Chromium (115.0.1901.200)
  npmPackages:
    @emotion/react: 11.11.1 => 11.11.1 
    @emotion/styled: 11.11.0 => 11.11.0 
    @mui/base:  5.0.0-beta.10 
    @mui/core-downloads-tracker:  5.14.4 
    @mui/material: 5.14.4 => 5.14.4 
    @mui/private-theming:  5.14.4 
    @mui/styled-engine:  5.13.2 
    @mui/system:  5.14.4 
    @mui/types:  7.2.4 
    @mui/utils:  5.14.4 
    @types/react: 18.2.18 => 18.2.18 
    react: 18.2.0 => 18.2.0 
    react-dom: 18.2.0 => 18.2.0 
    typescript: 5.1.6 => 5.1.6 
@pvdstel pvdstel added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 10, 2023
@zannager zannager added the component: tabs This is the name of the generic UI component, not the React module! label Aug 10, 2023
@brentertz
Copy link
Contributor

I ran into this as well, reading ownerState values in the theme styleOverrides for the Tabs component. Pinning "@mui/material": "5.14.3" and adding a resolution/override for "@mui/system": "5.14.3" is my temporary workaround.

@siriwatknp
Copy link
Member

siriwatknp commented Aug 15, 2023

I propose the fix like this:

diff --git a/packages/mui-material/src/Tabs/Tabs.js b/packages/mui-material/src/Tabs/Tabs.js
index 12a9f9ba9f..7c93ba1186 100644
--- a/packages/mui-material/src/Tabs/Tabs.js
+++ b/packages/mui-material/src/Tabs/Tabs.js
@@ -213,10 +213,7 @@ const TabsIndicator = styled('span', {
   }),
 }));
 
-const TabsScrollbarSize = styled(ScrollbarSize, {
-  name: 'MuiTabs',
-  slot: 'ScrollbarSize',
-})({
+const TabsScrollbarSize = styled(ScrollbarSize)({
   overflowX: 'auto',
   overflowY: 'hidden',
   // Hide dimensionless scrollbar on macOS

Since the TabsScrollbarSize is not intended to be themable, there is no reason to add name and slot to it.

@siriwatknp siriwatknp added bug 🐛 Something doesn't work package: material-ui Specific to @mui/material good first issue Great for first contributions. Enable to learn the contribution process. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer bug 🐛 Something doesn't work labels Aug 15, 2023
@brentertz
Copy link
Contributor

The proposed solution above works fine on my end. 👍

@siriwatknp
Copy link
Member

The proposed solution above works fine on my end. 👍

Would you like to submit a PR?

brentertz added a commit to brentertz/material-ui that referenced this issue Aug 18, 2023
brentertz added a commit to brentertz/material-ui that referenced this issue Aug 18, 2023
@ZeeshanTamboli ZeeshanTamboli added the bug 🐛 Something doesn't work label Sep 11, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [Tabs] style overrides function gets incorrectly typed props object if variant='scrollable' [Tabs] Crashes for scrollable tabs when overriding styles in theme using slots callback Sep 11, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [Tabs] Crashes for scrollable tabs when overriding styles in theme using slots callback [Tabs] Scrollable tabs crashes when overriding styles in theme using slots callback Sep 11, 2023
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. package: material-ui Specific to @mui/material
Projects
None yet
6 participants