[Tabs] Fix ":first-child is potentially unsafe" error

[hbjORbj]

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

Closes #24894

Problem:

• the following console error occurs when using icon and label props together
• code sandbox: https://codesandbox.io/s/loving-wing-328em?file=/src/App.js

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

Solution:

• Error comes from the use of :first-child to select the icon and apply bottom margin to it (if and only if both label and icon props are passed)
• Stop using :first-child. Instead, wrap icon with a span element with a classname.
• code sandbox: https://codesandbox.io/s/adoring-morning-520qb?file=/src/App.js

[mui-pr-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 7f39f87

[michaldudak]

Almost there :)
Just don't hardcode the class but add it to tabClasses.ts and reference it from there.

[hbjORbj]

How's this instead? I think this is more simple.

[michaldudak]

I think I've only ever seen labels that accompany inputs. I don't think it's valid to have it standalone (see https://www.w3.org/TR/WCAG20-TECHS/H44.html). I'd go with a class instead, as I originally suggested. Best to create a new slot in useUtilityClasses, I guess.

[hbjORbj]

Sure, thanks. I made a change and updated the code sandbox.

[hbjORbj]

From running prettier.

[siriwatknp]

👍

[mnajdova]

A bit late to the party, but I was chatting now with Mateusz and he point me to this special flag that is used to disable these warnings - https://github.com/emotion-js/emotion/blob/main/packages/react/__tests__/warnings.js#L87 I wish I knew this earlier.

If we replace each of our selectors to contain this comment, we would get rid of the warnings without doing any change in the styles themself. I would prefer this, mainly because each change in the styles could break something for the developers, especially if they have some style overrides for that component. More over, here we are adding new element, which is changing the DOM tree of the components itself.

cc @michaldudak @siriwatknp @hbjORbj

[hbjORbj]

@mnajdova Yep, I think that's better. I will create a separate PR.

[michaldudak]

Even though it says warning-please-do-not-use-this-the-warning-exists-for-a-reason?

[mnajdova]

@michaldudak we have never supported zero config SSR. We have a documentation of how SSR should be configured - https://mui.com/guides/server-rendering/#mui-on-the-server so I believe it is safe to use it.

[oliviertassinari]

While we are looking at this issue, I think that it's a good opportunity to make sure that we could support #11653 too in the future without having to change the approach.

Regarding why the warning exists, looking at the source https://github.com/emotion-js/emotion/blob/eda5e687c0bc4eddcafb243a2b1028296fb45cba/packages/styled/src/base.js#L144, it seems that the motivation for emotion is in case somebody provides a child icon element to the <Tab> that is a styled(), where the first child would be a <style> element, not the actual icon element.

Regarding the No SSR configuration support. I personally think that we should discourage it. But not supporting it? I don't know, we have tried in the past, by fixing similar warnings. In #26561 people are advocating for simpler integration.

[mnajdova]

@hbjORbj you've created a PR faster than us deciding what is the best strategy 😄 If there is a real use-case for adding the wrapper, for example supporting a new scenario for the icon position, then I am all for it. My point was that it's probably not the best idea to change the structure of the component, just for us to be able to fix the warning. There are other strategies that would allow us not to change the dom rendered, like cloning an element and adding a new class name. Not saying this is better, but at least something to consider. Anyway, would be great if we can at least address the comments added in the last reviews.

[hbjORbj]

@mnajdova I agree. The sole purpose of the wrapper was to target a specific classname, so it's not the best approach. I think it's safer to clone icon and add a classname to it so that the dom rendered doesn't change.

However, I didn't fully understand the downsides of using the disabling comment you suggested (https://github.com/emotion-js/emotion/blob/main/packages/react/__tests__/warnings.js#L87). This looks the most simple solution to me.

p.s. I addressed your comments in a local branch. Will include them in a new PR.