Skip to content

fix(material/core): special-case icon button color token #31625

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 31, 2025
Merged

fix(material/core): special-case icon button color token #31625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7737d1e
fix(material/core): special-case icon button color token
andrewseguin Jul 30, 2025
977a07a
fix(material/core): remove [disabled]
Jul 31, 2025
4178f31
fix(material/core): add validation against m3 theme configs
Jul 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/dev-app/theme.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ $candy-app-theme: mat.m2-define-light-theme((

@include mat.app-background();
@include mat.elevation-classes();
@include mat.m2-theme($candy-app-theme);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have this if there's also another theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only defines the system vars - I'd like to make a dev-app page that shows all the variables and this makes that work for both M2 and M3. This mixin, unlike M3, isn't meant to be a replacement for the individual component mixins

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plus this helps validate that nothing changes on the site if it's included. This is how I found the issue with the icon button's styles.

Part of this is to encourage people to use this mixin and use systems vars instead of our m2 APIs to pull from the theme config. If we find an issue in the dev-app from this, we should fix it


// Include the default theme styles.
@include mat.all-component-themes($candy-app-theme);
Expand Down Expand Up @@ -58,6 +59,7 @@ $candy-app-theme: mat.m2-define-light-theme((
typography: mat.m2-define-typography-config(),
)
);
@include mat.m2-theme($dark-colors);

// Include the dark theme color styles.
@include mat.all-component-colors($dark-colors);
Expand Down
18 changes: 18 additions & 0 deletions src/material/core/tokens/_system.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
@use '../style/elevation';
@use '../theming/config-validation';
@use '../theming/definition';
@use '../theming/inspection';
@use '../theming/m2-inspection';
@use '../theming/palettes';
@use '../style/sass-utils';
Expand Down Expand Up @@ -322,6 +323,11 @@
// Unlike M3's `mat.theme()`, this mixin does not replace the need to call
// individual component theme mixins for Angular Material components.
@mixin m2-theme($theme-config, $overrides: ()) {
@if inspection.get-theme-version($theme-config) == 1 {
@error '`m2-theme` mixin should only be called for M2 theme ' +
'configs created with define-light-theme or define-dark-theme';
}

$config: m2-inspection.get-m2-config($theme-config);

$color: map.get($config, color);
Expand Down Expand Up @@ -350,6 +356,18 @@

@include _define-m2-system-vars(m2.md-sys-shape-values(), $overrides);
@include _define-m2-system-vars(m2.md-sys-state-values(), $overrides);

Copy link
Member

Choose a reason for hiding this comment

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

Since this is in system.scss, does it mean that we'll generate the concrete styles next to token styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the icon button? Yeah unfortunately. I thought about adding a modification in icon-button's theme mixin but that introduces the same sort of issue

Copy link
Member

Choose a reason for hiding this comment

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

Should we at least have a @if (isM2) around it so it doesn't affect M3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this is specifically an M2-only mixin, different entirely from the implementation for @mixin theme.

This mixin explicitly takes an M2 theme config and is meant to convert that to system CSS vars. I don't expect M3 apps to ever call this

// The icon button's color token is set to `inherit` for M2 and intended to display
// the color inherited from its parent element. This is crucial because it's unknown
// whether the icon button sits on a container with background like "surface" or "primary",
// where both may have different contrast colors like white or black.
// However, variables set to inherit AND define a fallback will always use the fallback,
// which is "on-surface-variant". This mixin now defines this value.
// To avoid this, and continue using `inherit` for the icon button color, set the color explicitly
// to the token without a fallback.
.mat-mdc-button-base.mat-mdc-icon-button:not(.mat-mdc-button-disabled) {
color: var(--mat-icon-button-icon-color);
}
}

@mixin _define-m2-system-vars($vars, $overrides) {
Expand Down
Loading