[Typography] Apply font properties to typography inherit variant

[oyar99]

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

Ensure that Typography components actually inherit font properties when inherit variant is used.
Since the Link component is built on top of the Typography component, this also fixes #32942.

const theme = createTheme({
  typography: {
    fontFamily: "Inconsolata, Arial"
  }
});

Before:

<ThemeProvider theme={theme}>
    <Typography variant="body1">
      <Link>This is good (it's Inconsolata)</Link>
      <br />
      <Link component="button">This is not good (it's Arial)</Link>
    </Typography>
</ThemeProvider>

Now:

<ThemeProvider theme={theme}>
    <Typography variant="body1">
      <Link>This is good (it's Inconsolata)</Link>
      <br />
      <Link component="button">This is good too (it's Inconsolata)</Link>
    </Typography>
</ThemeProvider>

Fixes #32942

[michaldudak]

Looks good, thanks!

[mui-bot]

Netlify deploy preview

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

@material-ui/core: parsed: +0.10% , gzip: +0.09%
@material-ui/lab: parsed: +0.19% , gzip: +0.16%
@material-ui/system: parsed: +0.54% , gzip: +0.48%
@mui/material-next: parsed: +0.37% , gzip: +0.32%
@mui/joy: parsed: +0.10% , gzip: +0.11%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 8b6f44a

[michaldudak]

There's a unit test failing. Would you mind taking a look at it?

[michaldudak]

The visual regression tests detected another issue. The sx prop doesn't override the CSS properties from the variant. Compare https://mui.com/material-ui/react-tree-view/#gmail-clone with https://deploy-preview-33621--material-ui.netlify.app/material-ui/react-tree-view/#gmail-clone.

This has worked fine before, so I wonder if it's related to the latest changes to Emotion support in the System (cc @garronej, @mnajdova).

[mnajdova]

@michaldudak I don't think the problem is with the order of how things are defined, but how this variant is defined. In the demo that you linked, if you change the fontWeight with font-weight it works as expected, see https://codesandbox.io/s/xenodochial-banach-z7sojt. There is something wrong with how the variant is defined or how the value is fetched (haven't looked into too much details, just wanted to confirm it is not related to the changes done by @garronej as that would have a much bigger impact).

[garronej]

Thanks @mnajdova for figuring this out. I am on vacation so I wasn't able to look at it.

[michaldudak]

@mnajdova Thanks, I'll take a deeper look at this, then.

[ZeeshanTamboli]

I fixed the issue identified by the visual regression test mentioned in #33621 (comment). As @mnajdova pointed out in #33621 (comment), it was about how the variant was fetched in the sx prop implementation. The issue was that, after adding the missing inherit variant in Typography, the default theme mapping for typography fetched the whole inherit variant object from createTypography when there was any CSS font property of inherit value defined in the sx prop.

I added a fix in 224fdaa along with a test in the sx prop implementation. However, I wonder if there is a better way to fix this. Perhaps there is something we can do in the Typography component level instead of the sx prop. But I also believe this is how we should do it, and it makes sense. (We can't change variant="inherit" to something else, that would be a breaking change.)

I am approving it from my side, but DO NOT MERGE it until approved by @mnajdova or @michaldudak .

Addressed. See #33621 (review).

[mnajdova]

@ZeeshanTamboli I changed a bit the fix for the sx prop, by providing the necessary changes to the sx config instead of the styleFunctionSx. The changes are related to the default theme, for e.g. they are not related to Joy UI, so it makes sense for them not to live in the styleFucntionSx intself.

[ZeeshanTamboli]

Just curious, what is this props[prop] === prop for? CSS property's value being equal to it's own property?

[ZeeshanTamboli]

@ZeeshanTamboli I changed a bit the fix for the sx prop, by providing the necessary changes to the sx config instead of the styleFunctionSx. The changes are related to the default theme, for e.g. they are not related to Joy UI, so it makes sense for them not to live in the styleFucntionSx intself.

Nice! Makes sense.

[CarlosAmaral]

You've duplicated if statements here and on line 22

[genepaul]

It appears that this PR may have broken the sx handling of font props? The documentation says that you can do something like this:

sx={{
    fontWeight: 'fontWeightMedium'
}}

But the code in this PR appears to only look for the prop without the fontWeight:

sx={{
   fontWeight: 'medium'
}}

Here's a codesandbox that reproduces the issue: https://codesandbox.io/s/vibrant-brook-572rnc?file=/src/App.js

You can verify that this is a regression by changing the version of @mui/material that is pulled in to 5.11.12 or earlier, and notice that using fontWeightMedium properly changes the font weight.

Here's the documentation that refers to being able to use fontWeightLight (as an example):

https://mui.com/system/getting-started/the-sx-prop/#typography

[genepaul]

I created an issue for this: #36542