angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.33k stars 6.73k forks source link

bug(material/button): mdc unelevated button not getting custom theme color #26179

Closed BenGrn closed 11 months ago

BenGrn commented 1 year ago

Is this a regression?

The previous version in which this bug was not present was

14

Description

When creating custom themes flat buttons revert to default font color instead of using theme contrast colors.

Reproduction

Steps to reproduce:

  1. Create a new Angular project with Material
  2. Apply custom theme

https://stackblitz.com/edit/angular-jixnhd

Expected Behavior

image

Actual Behavior

image

Environment

MikaStark commented 1 year ago

Same here, Buttons theme do not use contrast colors. It seems they are computed by mdc :/

https://github.com/angular/components/blob/f8dd0180cd77b52d980c1ea50b995bb10b9b2d82/src/material/core/mdc-helpers/_mdc-helpers.scss#L141

inlym commented 1 year ago

I got same problem!!!

inlym commented 1 year ago

Notify me if resolved.

NicholaAlkhouri commented 1 year ago

It looks that in the new mdc components, the button label text color is computed

$on-primary: if(contrast-tone($primary) == 'dark', #000, #fff) !default;

while in version 14 it comes directly form the color palette

color: var(--color-contrast-500);

richardsengers commented 1 year ago

Same problem here .... unable to update if those bugs don't get fixed :-(

swierzbicki commented 1 year ago

Same problem. It would be nice to have a fix for that. We use orange color for my company and Angular Material sets the font color as black. So it breaks our company branding.

the-ult commented 1 year ago

It seems like the contrast color is indeed (mostly) too dark. The same with on-secondary

Screenshot 2023-04-12 at 11 10 15 Screenshot 2023-04-12 at 11 11 08 Screenshot 2023-04-12 at 11 13 45

With a slider it does seem to work Screenshot 2023-04-12 at 11 11 25

$pink-accent-palette: (
  50: #fde4ea,
  100: #fbbcca,
  200: #f791a7,
  300: #f36685,
  400: #ee486b,
  500: #e93053,
  600: #d92a51,
  700: #c4254e,
  800: #af1f4b,
  900: #8c1445,
  A100: #fff,
  A200: #ffc3d4,
  A400: #ffaac1,
  A700: #ff9bb6,
  contrast: (
    50: #000,
    100: #000,
    200: #fff,
    300: #fff,
    400: #fff,
    500: #fff,
    600: #fff,
    700: #fff,
    800: #fff,
    900: #fff,
    A100: #000,
    A200: #000,
    A400: #000,
    A700: #000,
  ),
);
NicholaAlkhouri commented 1 year ago

It seems like the contrast color is indeed (mostly) too dark. The same with on-secondary

Screenshot 2023-04-12 at 11 10 15 Screenshot 2023-04-12 at 11 11 08 Screenshot 2023-04-12 at 11 13 45

With a slider it does seem to work Screenshot 2023-04-12 at 11 11 25

I believe that is because the toggle-slider bg color is darker than the button. so the white color will create a sufficient contrast.

wagnermaciel commented 1 year ago

Based on the team's current understanding, this should be resolved when we complete the new token based theming api, which will grant fully supported finer-grained control over component styles

I say "should" because I'm afraid of promising this based on something that hasn't been finalized. If at any point it becomes clear that this will not be resolved by the new api, I will raise this issue again to make sure it gets handled

ProjectBay commented 1 year ago

Same with checkboxes

jpduckwo commented 1 year ago

This issue has broken our apps theming when trying to migrate to mdc components. We generate a generic theme using css vars and set those vars at run time. We have user-defined themes... However after trying the migration, we'd need to manually set an overriding style that pulls in the colour. Fingers crossed the token-based theming api will fix it!

AlvaroP95 commented 1 year ago

Angular 16 is already live, and this is not fixed. Any news on this?

jpduckwo commented 1 year ago

@AlvaroP95 see @wagnermaciel's comment above from 3 weeks ago - the solution hasn't been developed yet so it won't be available in v16. If anyone knows the 'token based theming' issue / feature links can they please tag them here so we can track progress?

jpduckwo commented 1 year ago

I also want to note that basic themed buttons have a hardcoded black text rather than inheriting the text color (previous behaviour), so if you put a basic button on darker background then the text colour might not be the correct contrast. Since the button background is clear and not white, I would think the previous behaviour makes more sense. (this is when generating a light theme - but I guess it's the same but opposite for dark however I haven't tested that)

p3pp8 commented 1 year ago

I had to override text color for raised button as custom contrast palette wasn't applied! Version 16.1.3, that's a shame!

BenGrn commented 1 year ago

This is still impacting our applications when using custom palettes. Is there a road map for theming issue resolution?

Tntpot commented 11 months ago

Just joining in to this thread, but this is an issue I've recently ran into also.

@wagnermaciel any update for this issue following the token-based theming updates?

wagnermaciel commented 11 months ago

I tried this out on the latest which has the buttons partially swapped to using tokens, but the issue persists :( I'm going to dedicate some time to this tomorrow to see why this bug is so sticky

Here's the latest stackblitz for reference https://stackblitz.com/edit/z7ywyp?file=src%2Ftheme.scss

Tntpot commented 11 months ago

Much appreciated 😀

I actually came across this bug whilst trying to figure out why some of my accented buttons are coming out with darker text than I thought they should have, but now concerned I might also just not be fully understanding how to develop a colour palette...

Is the intention that these are calculated against the button colour so that the text contrasts appropriately?

wagnermaciel commented 11 months ago

Ok! Took a look into this and confirmed that the text/icon color is being set to white or black based on a11y contrast ratios.

This is working as intended and has not changed in the latest implementation because we still want to encourage best practices for a11y when using our library

But with the new tokens API, there is a supported & maintainable way to manually override this. Just override the css variables corresponding to the button text color (you can find these by looking through devtools - we're hoping to make these tokens easier to find in the future on our docs site)

https://stackblitz.com/edit/z7ywyp?file=src%2Ftheme.scss

.mat-primary {
  &.mat-mdc-button-base {
    --mat-fab-foreground-color: #fff;
    --mdc-filled-button-label-text-color: #fff;
    --mdc-protected-button-label-text-color: #fff;
  }

  &.mat-mdc-fab,
  &.mat-mdc-mini-fab {
    --mat-icon-color: #fff;
  }
}

P.S. The button tokens have just landed and overriding this is kinda janky atm. Once these are polished up, you should be able to just override all these css variables at the .mat-primary level (no need for &.mat-mdc-button-base or &.mat-mdc-fab, etc

P.S. P.S. We're still working on this "token" system and considering different ways to surface a cleaner interface for these types of overrides

angular-automatic-lock-bot[bot] commented 10 months ago

This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.