close
The Wayback Machine - https://web.archive.org/web/20211009041903/https://github.com/mui-org/material-ui/issues/24894
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] pseudo class ":first-child" is potentially unsafe #24894

Closed
mtr1990 opened this issue Feb 13, 2021 · 15 comments
Closed

[Tabs] pseudo class ":first-child" is potentially unsafe #24894

mtr1990 opened this issue Feb 13, 2021 · 15 comments

Comments

@mtr1990
Copy link

@mtr1990 mtr1990 commented Feb 13, 2021

The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type"

an example case is: MuiTab: https://codesandbox.io/s/uzg95?file=/demo.tsx

Screen Shot 2021-02-13 at 11 43 41

@oliviertassinari oliviertassinari changed the title [MuiTab] Material v5 The pseudo class ":first-child" and ":first-of-type" [Tabs][Buttton] pseudo class ":first-child" is potentially unsafe Feb 13, 2021
@oliviertassinari oliviertassinari changed the title [Tabs][Buttton] pseudo class ":first-child" is potentially unsafe [Tabs] pseudo class ":first-child" is potentially unsafe Feb 13, 2021
@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 13, 2021

@mtr1990 Thanks for the reproduction. We didn't notice the issue in the documentation because we use compat = true, which disables this warning cc @mnajdova. However, developers might decide to not use the same configuration as the one we encourage. Related threads: #24107 (comment), emotion-js/emotion#1178.

I don't think that we should encourage developers to inline <style> inside the body. It's not server-side, low-end mobile devices friendly, and it's discouraged by the HTML spec. A no go 🚫.

One fix could be to do:

diff --git a/packages/material-ui-styled-engine/src/StyledEngineProvider/StyledEngineProvider.js b/packages/material-ui-styled-engine/src/StyledEngineProvider/StyledEngineProvider.js
index 8217655f07..530bb5242b 100644
--- a/packages/material-ui-styled-engine/src/StyledEngineProvider/StyledEngineProvider.js
+++ b/packages/material-ui-styled-engine/src/StyledEngineProvider/StyledEngineProvider.js
@@ -5,6 +5,7 @@ import createCache from '@emotion/cache';

 // Cache with option to prepend emotion's style tag
 export const cache = createCache({ key: 'css', prepend: true });
+cache.compat = true;

 export default function StyledEngineProvider(props) {
   const { injectFirst, children } = props;
diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index 11a700c67b..f2cd9ad22e 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -57,17 +57,17 @@ const useUtilityClasses = (styleProps) => {

 const commonIconStyles = (styleProps) => ({
   ...(styleProps.size === 'small' && {
-    '& > *:nth-of-type(1)': {
+    '& > *:first-child': {
       fontSize: 18,
     },
   }),
   ...(styleProps.size === 'medium' && {
-    '& > *:nth-of-type(1)': {
+    '& > *:first-child': {
       fontSize: 20,
     },
   }),
   ...(styleProps.size === 'large' && {
-    '& > *:nth-of-type(1)': {
+    '& > *:first-child': {
       fontSize: 22,
     },
   }),

But then, the components would come with the warning by default. So, developers would need to provide a custom cache systematically. We can't do that.

Another possible workaround is:

      [`& .${tabClasses.wrapper} > *:first-of-type`]: {
        marginBottom: 6,
      },
      [`& .${tabClasses.wrapper} > * + *`]: {
        marginBottom: 0,
      },

But developers will have a hard time customizing it, so a go no too. The least-worse option I can think of is:

diff --git a/packages/material-ui/src/Tab/Tab.js b/packages/material-ui/src/Tab/Tab.js
index 7abf5edccc..22c2207b26 100644
--- a/packages/material-ui/src/Tab/Tab.js
+++ b/packages/material-ui/src/Tab/Tab.js
@@ -69,8 +69,8 @@ const TabRoot = experimentalStyled(
     styleProps.label && {
       minHeight: 72,
       paddingTop: 9,
-      [`& .${tabClasses.wrapper} > *:first-child`]: {
-        marginBottom: 6,
+      [`& .${tabClasses.wrapper} > * + *`]: {
+        marginTop: 6,
       },
     }),
   /* Styles applied to the root element if the parent [`Tabs`](/api/tabs/) has `textColor="inherit"`. */
@@ -145,7 +145,7 @@ const Tab = React.forwardRef(function Tab(inProps, ref) {
     icon,
     // eslint-disable-next-line react/prop-types
     indicator,
-    label,
+    label: labelProp,
     onChange,
     onClick,
     onFocus,
@@ -166,7 +166,7 @@ const Tab = React.forwardRef(function Tab(inProps, ref) {
     disableFocusRipple,
     selected,
     icon: !!icon,
-    label: !!label,
+    label: !!labelProp,
     fullWidth,
     textColor,
     wrapped,
@@ -194,6 +194,8 @@ const Tab = React.forwardRef(function Tab(inProps, ref) {
     }
   };

+  const label = typeof labelProp === 'string' && !!icon ? <span>{labelProp}</span> : labelProp;

forcing a wrapper around the label when a string for * + * to be able to take effect.

@mark-night
Copy link

@mark-night mark-night commented Apr 22, 2021

Is there a way to temporarily omit/disable this error in browser console? I'm sure it'll be fixed in some future release, but seeing them keep being thrown in console is really annoying.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 22, 2021

@mark-night You would need to ask the team of emotion. I was actually eager to turn off these options for the whole Material-UI project as we don't encourage to inline style in the body, but Gatsby doesn't provide the necessary API to render the style in head so I didn't push forward.

This issue includes a possible solution.

@mark-night
Copy link

@mark-night mark-night commented Apr 22, 2021

@oliviertassinari I see the source of issue is from emotion or even more teams. I believe each package chooses a certain way for itself for a reason, thus this could take a long time even if it will be changed.
Meanwhile, I do see your solutions to the issue and they all seem to be simple and clear and more than enough for my need. However, they all need to modify the Material-ui source code, how do I take the advantage of your solution fix without forking the project which kicks me off future updates? Or, can I simply modify the source code in node_modules in my project? Will this cause issue for future updates? Thanks.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 22, 2021

@mark-night Follow our /examples

@mark-night
Copy link

@mark-night mark-night commented Apr 22, 2021

@oliviertassinari I hate to ask this stupid question, but what did you mean by examples? If you were referring to the examples folder under source root, it looks like they are just examples for how to install and setup material-ui with different tools. I'm not having any issue installing and setup material-ui in my project at all.

Your solutions were very clear and make sense, but how do I apply the solution? Do I just apply the change to my local install (via npm) in local node_modules/ ? Then wouldn't this change be overridden by next update or npm install once deployed? Kinda confused.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 22, 2021

I meant that if you dig deep, you will find a solution in the /examples folder, but it's not important, what matters is that somebody champions this issue and gets a fix merged on HEAD.

@mark-night
Copy link

@mark-night mark-night commented Apr 23, 2021

you will find a solution in the /examples folder

Confused, isn't the solution right here above, e.g. to modify Tab.js in the source code? My question was how to make the changes being made survive from future update (if it's still necessary for the update).

After some googling, was able to find this patch-package, good enough for now.

Thanks.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 23, 2021

@mark-night The solution I had in mind is related to the compat mode of emotion. But patch-package sounds good as well. If you want to contribute a fix, we would be happy to review a PR.

@mark-night
Copy link

@mark-night mark-night commented Apr 23, 2021

I finally see what you meant. :) I thought cache is only for ssr and I'm not familiar with emotion neither, didn't think down that path at all. I'll check out what emotion compact really does later.

As I'm in progress of making a pure front-end ui, your other solution of modifying Tab.js is simple and good enough, thus I went with that one.

If you want to contribute a fix

I was just copying your code. :)

@iamdevelopergirl
Copy link

@iamdevelopergirl iamdevelopergirl commented Aug 26, 2021

Can I work on the above comments and create a PR?

@kkaplita
Copy link

@kkaplita kkaplita commented Sep 3, 2021

Can I work on the above comments and create a PR?

I would like to see it when it is ready :)

@hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Sep 30, 2021

@iamdevelopergirl Do you have updates on this?

@jonoman55
Copy link

@jonoman55 jonoman55 commented Oct 1, 2021

Does anyone know when this will be fixed? I have @mui/material": "^5.0.0" installed and I am receiving this error while using icons in Tabs. Thanks!

@iamdevelopergirl
Copy link

@iamdevelopergirl iamdevelopergirl commented Oct 1, 2021

Hi, Apologies, I did not get time to work on it. Can someone else have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants