Material-ui: Button text color does not respect the themes contrastDefaultColor

[dangerousdan]

When overriding the contrastDefaultColor of a theme, the text color of a button does not change.

import { createMuiTheme } from 'material-ui/styles'
import cyan from 'material-ui/colors/cyan'

const theme = createMuiTheme({
  palette: {
    primary: {
      ...cyan,
      contrastDefaultColor: 'light'
    },
  },
})

• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

The following button should now have light text, on a cyan background

<Button raised color="primary">Example</Button>

Current Behavior

The button still has dark text on a cyan background.

Environment

Material-UI: 1.0.0-beta.9
React: 15.6.1

[oliviertassinari]

I can't find any application of the contrastDefaultColor property. @nathanmarks Any idea why this was added in the first place?

[AhmedShaalan]

I have the same issue, Primary button doesn't respect contrastDefaultColor. it calculates text color internally and decide if it should be light or dark.

[dangerousdan]

@AhmedShaalan as an interim solution I am overriding the theme's getContrastText function

var originalGetContrastText = theme.palette.getContrastText

theme.palette.getContrastText = color => {
  if (color === theme.palette.primary[500]) {
    return theme.palette.common.white
  }
  return originalGetContrastText(color)
}

[ianschmitz]

Have we had any more thoughts on the status of contrastDefaultColor? I just ran into the same thing as @dangerousdan.

Ideally we could support the contrastDefaultColor, as sometimes getContrastText returns the color with the most contrast, but which is not visually appealing (we ran into a darker green background button which defaulted to black text for example, yet white text was easier to read and more visually appealing)

[mbrookes]

I can't find any application of the contrastDefaultColor property. @nathanmarks Any idea why this was added in the first place?

@oliviertassinari I believe that's orphan code from early testing before colorManipulator.js was added to v1. I had been meaning to remove it at some point, but forgot about it.

I think the problem may in part lie here. We are checking whether the contrast ratio falls below a defined threshold, and switching to the alternate text color if not, without checking whether the alternate actually has a better contrast ratio. It's entirely possible it doesn't with background colors that are around mid-point luminance.

I believe this biases the selection towards black.

For the most accurate selection we should perhaps be testing which color has the highest contrast ratio, and selecting accordingly. Failing that, biasing towards white might be preferable.

That said, there should probably also be a way to override this behavior.

[grzegorzjudas]

I don't know what logic is used right now, but this YIQ solution seems to work (with cyan at least):

function getContrastYIQ(hexcolor) {
    const r = parseInt(hexcolor.substr(0, 2), 16);
    const g = parseInt(hexcolor.substr(2, 2), 16);
    const b = parseInt(hexcolor.substr(4, 2), 16);
    const yiq = ((r * 299) + (g * 587) + (b * 114)) / 1000;
    return (yiq >= 128) ? 'black' : 'white';
}

_Source: https://24ways.org/2010/calculating-color-contrast/_

[mbrookes]

getContrastRatio() is implementing this WCAG spec, which is fine for testing whether a color combination meets a minimum contrast ratio*, but the way it's used in getContrastText (switching to black if white has a contrast ratio < 7:1 ) doesn't guarantee the highest contrast text.

Instead we should be using getLuminance:

function getContrastText(color) {
  return (getLuminance(color) <= 0.5) ? dark.text.primary : light.text.primary;
}

That should perhaps go into colorManipulator.js as getContrastColor()?

I don't have any strong views on how overriding that should work, but I know @oliviertassinari is planning to revisit theming as a high priority (#8075), so perhaps could consider it then.

*https://www.w3.org/TR/2008/REC-WCAG20-20081211/#visual-audio-contrast 1.4.6 Contrast Enhanced.

[grzegorzjudas]

Since it looks like the whole thing is about finding good contrast ratio for specific usecase, perhaps it'd be good to instead, give developer an easy way to provide the contrast ratio he wants to use (for that "switch" point), with a "works-for-most-cases" default. Simple parameter for createMuiTheme config object?

[mbrookes]

Here's a sample using the revised approach.

Before:

After:

A significant improvement IMHO.

[oliviertassinari]

A significant improvement IMHO.

@mbrookes But it's no longer what we have in the specification: https://material.io/guidelines/style/color.html#color-color-palette.
Maybe we should only add the missing logic to support the contrastDefaultColor option 🤔 .
But I'm not sure what this logic should look like.

[dangerousdan]

Wouldn't implementing contrastDefaultColor as expected cause further issues?
Ie: When set to 'light' I'd then be getting light text regardless of if I used a Cyan[50] or Cyan[900] background.

@oliviertassinari I think matching the spec is important but the defaults are a bit off, so I think lots of devs will want to override this behaviour.

Since the colours are sequential, I'd be happy with something like this (with better naming):

const theme = createMuiTheme({
  palette: {
    primary: {
      ...cyan,
      contrastThreshold: 300
    },
  },
})

[mbrookes]

@oliviertassinari

But it's no longer what we have in the specification

1) That isn't the spec for contrast color it's the spec for the color pallette. The text is simply labels, so it doesn't matter that they're different.
2) our current color tables' labels don't match the spec in places if you compare them.
3) This approach maximizes the contrast regardless of background color which _does_ address the spec: https://material.io/guidelines/style/color.html#color-usability (Legibility.)

@dangerousdan You're correct, contrastDefaultColor isn't very useful which is why it wasn't implemented in the end. I can remove it as part of the related PR.

As you say, adding contrastThreshold (range 0 - 1 ) and using it here:

function getContrastText(color) {
  if (getLuminance(color) <= theme.palette.contrastThreshold) {
    return dark.text.primary;
  } else {
    return light.text.primary;
  }
}

would additionally give devs the ability to adjust the crossover point (although that may be less useful with the default now at midpoint). If we give devs that rope, I'd be keen to add a check and warning in development if the results fall below WCAG contrast guidelines (the correct use of getContrastRatio 😄 ).

[mbrookes]

So I added a warning to getContrastText():

"Warning: Material-UI: the contrast ratio of 2.59:1 for #fff and #90a4ae falls below the WACG recommended minimum contrast ratio of 3:1.
https://www.w3.org/TR/2008/REC-WCAG20-20081211/#visual-audio-contrast-contrast"

...and tested it with the Color page using the new getLuminance() based approach, and got a _ton_ of warnings.

After further testing, it transpired that neither getLuminence nor getContrast are linear. After some analysis I found the value that maximizes the contrast, but subjectively, the results are less attractive than the current output. I'm retracting my position that maximizing contrast is the preferred default output. Instead it now matches the current output.

I'm struggling however to know how to allow devs to override the default threshold. Since getContrastText is currently part of the theme, it can't access the theme.

[jtag05]

Has there been any movement on this?

[omeryagmurlu]

@mbrookes How can we supress this warning? I am getting dozens of it and couldn't find a way suppressing it in docs.

[mbrookes]

How can we supress this warning?

Use colors that meet the minimum contrast ratio? Tweak the thresholds?

It's a dev warning anyway, so if you're determined to use a low contrast color scheme, it will go away in production.

[oliviertassinari]

Yeah, I'm not sure we should encourage people to dodge the contrast issue by allowing them to disable this warning.

[ChristyCakes]

I'm currently using Material UI in my app (I'm a React student). I found this thread and am wondering what came of it becaue I really would like to implement white contrast text on some of my colorful buttons. I've tried 'overriding' in themes with no luck. Can you help me?

[joshwooding]

@ChristyCakes Have you tried the solution here? https://material-ui.com/demos/buttons/#customized-buttons

[ChristyCakes]

found a solution here: https://stackoverflow.com/questions/53876245/text-color-not-working-in-material-ui-theme/53889500#53889500

const theme = createMuiTheme({
  palette: {
    primary: { main: "#e91e63", contrastText: "#fff" },
    secondary: { main: "#03a9f4", contrastText: "#fff" }
  }
});