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

[material-ui][AppBar] Fix inherit color is inconsistent between ThemeProvider and CssVarsProvider #42714

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Jun 22, 2024

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work component: app bar This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material customization: theme Centered around the theming features labels Jun 22, 2024
@mui-bot
Copy link

mui-bot commented Jun 22, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 6bf7699

@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review June 22, 2024 16:43
@DiegoAndai
Copy link
Member

Hey @ZeeshanTamboli, thanks for working on this. I cannot access the Before and After Sandboxes:

Screenshot 2024-07-01 at 14 57 32

@siriwatknp may I ask you to review this as well?

@ZeeshanTamboli
Copy link
Member Author

@DiegoAndai Sorry, I didn't realize it was private. Seems like these days CodeSandbox sets them to private by default. I've made it public now, so you should be able to access it.

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the fix. Doesn't inherit rely on the --AppBar-color var being used?

'--AppBar-color': 'inherit',
. If color: var(--AppBar-color) is not added, the inherit color style doesn't work, does it?

Doesn't the transparent style apply very similar styles, and overrides these?

props: { color: 'transparent' },
style: {
'--AppBar-background': 'transparent',
'--AppBar-color': 'inherit',
backgroundColor: 'var(--AppBar-background)',
color: 'var(--AppBar-color)',
...theme.applyStyles('dark', {
backgroundImage: 'none',
}),
},

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented Jul 4, 2024

@DiegoAndai It can be a bit confusing, so I'll explain in detail.

When you provide a color prop to Material UI components, it doesn't just set the CSS color. It also handles the background color since there's no separate background color prop.

The issue is with the AppBar component's background color when the inherit color prop is used. When inherit is passed, it should apply the background color of the Paper component (which AppBar inherits from), not the containing and inherited element, in this case<div>. This logic is different from the CSS inherit value. The color value should be inherit, but the background-color should match the Paper component. I asked this in #42379 (comment). This is discussed in #19393 which was mentioned in #42379 (comment).

For transparent, the background-color should be transparent, revealing the element beneath it.

Inspecting the before code sandbox, you'll see the AppBar's background color when inherit is used is:

background-color: var(--AppBar-background);

But --AppBar-background isn't defined, causing the bug. Instead, we want the background color to apply that of Paper when inherit:

  • With theme provider:
background-color: #fff;
  • With CSS variables:
background-color: var(--mui-palette-background-paper);

So the fix is to avoid using background-color: var(--AppBar-background) for inherit and transparent, handling them separately. background-color: var(--AppBar-background) will be applied for other color prop values.


Proof that this is a regression from #32835 which was released in v5.8.5:

  • Works in 5.8.4: code
  • Breaks in 5.8.5: code

As mentioned in the PR description, there's a separate PR for v5: #42713 due to different code from v6.

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks @ZeeshanTamboli 💙

@ZeeshanTamboli ZeeshanTamboli merged commit 701996c into mui:next Jul 11, 2024
19 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the issue-42379-AppBar-color-inherit-inconsistent-next branch July 11, 2024 10:31
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
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: app bar This is the name of the generic UI component, not the React module! customization: theme Centered around the theming features package: material-ui Specific to @mui/material regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][AppBar] color="inherit" is inconsistent between ThemeProvider and experimental_CssVarsProvider
3 participants