[mdc-icon-button] Icon button does not set colors based on theme values properly

kimurakenshi commented 4 years ago

Bug report

Icon button does not set colors based on theme values properly. Default color doesn't change based on theme color palette and disabled color is always set to text-disabled-on-light which is not the best option for a dark theme (screenshot attached).

Steps to reproduce

Create a custom dark theme:

For example:

@use "@material/theme" with (
    // Background
  $background: #212121,

  // Primary
  $primary: #3d74ff,
  $on-primary: #000,

  // Secondary
  $secondary: #00b8c7,
  $on-secondary: #000,

  // Surface
  $surface: #212121,
  $on-surface: #fff,

  // Status
  $error: #d91e18,
  $on-error: #fff
);

Take a look at mdc-icon-button default and disabled states.
Default color remains full black and disabled color is too dark which makes the component not properly visible for users.

Actual behavior

Colors are not updated based on theme color palette.

Expected behavior

Colors should be set based on theme color palette.

Screenshots

Possible solution

I noticed this part of the base mixins: https://github.com/material-components/material-components-web/blob/master/packages/mdc-icon-button/_mixins.scss#L194 That should probably be updated to use the text-disabled-on-light or text-disabled-on-dark depending on the component's base color.
Regarding the base color i think https://github.com/material-components/material-components-web/blob/master/packages/mdc-icon-button/_mixins.scss#L178 should be changed to use the theme color so it will be updated based on the theme color palette properly.

The above comments are just suggestions based on my understanding of the component implementation but i'm not fully aware of the standard and guidelines defined for this components so please ignore this if it doesn't apply.

Workaround

Currently i'm using these styles to achieve what i wanted in case it adds some info to the bug:

@use "@material/theme" as mdc-theme;
@use "@material/icon-button/mixins" as mdc-icon-button-mixins;

@mixin base_ () {
  @include mdc-icon-button-mixins.ink-color(mdc-theme.$on-surface);
}

@mixin disabled_ () {
  $text-disabled-color: if(mdc-theme.contrast-tone(mdc-theme.$surface) == "dark", text-disabled-on-light, text-disabled-on-dark);

  @include mdc-icon-button-mixins.disabled-ink-color($text-disabled-color);
}

kimurakenshi commented 4 years ago

Any update on this? is it a valid issue or should i close it?

asyncLiz commented 4 years ago

I'll be bringing up this issue with the team during our triage meeting this week. We should have an answer tomorrow 🙂

kimurakenshi commented 4 years ago

Thank you!

asyncLiz commented 4 years ago

We definitely think this could be addressed. Adding it to our backlog to investigate

kintz09 commented 2 years ago

Is this still issue still expected to be fixed? I discovered the same issue today, but noticed this issue had been reported back in 2020.

material-components / material-components-web