material-components-web icon indicating copy to clipboard operation
material-components-web copied to clipboard

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

Open kimurakenshi opened this issue 4 years ago • 5 comments

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

  1. 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
);
  1. Take a look at mdc-icon-button default and disabled states.
  2. 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

image

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 avatar Mar 10 '20 16:03 kimurakenshi

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

kimurakenshi avatar Mar 12 '20 15:03 kimurakenshi

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

asyncLiz avatar Mar 12 '20 16:03 asyncLiz

Thank you!

kimurakenshi avatar Mar 12 '20 16:03 kimurakenshi

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

asyncLiz avatar Mar 13 '20 19:03 asyncLiz

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.

kintz09 avatar Sep 16 '21 23:09 kintz09