mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.91k stars 32.27k forks source link

[material-ui][Rating] Failing on color contrast checks #39135

Open chris-gds opened 1 year ago

chris-gds commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Link to live example: https://mui.com/material-ui/react-rating/

Steps:

  1. Visit: material-ui/react-rating/
  2. Check colour contrast

Current behavior 😯

The rating Design fails colour contrast checks WCAG 1.4.3. The grey on white and the yellow on white.

Demo Demo
ratings-2 on material ui failing colour contrast checks ratings-1 on material ui failing colour contrast checks

Expected behavior 🤔

I would expect the colours chosen to pass.

Context 🔦

Allows people who are blind or visually impaired to consume more easily should it meet standards.

Your environment 🌎

npx @mui/envinfo ``` Don't forget to mention which browser you used. Output from `npx @mui/envinfo` goes here. ```
zanivan commented 1 year ago

Hey @chris-gds good catch! This component should definitely get a bump to contrast. According to w3, it should have at least 3:1, since it's not a text. We could use the warning.main, since it's a token that works both on light #ED6C02 or dark #FFA726 theme. For the gray, the grey[700] for dark and grey[600] for light should do the job.

oliviertassinari commented 1 year ago

I would expect the colours chosen to pass.

I disagree with this expected behavior, icons aren't text, we can't use this reasoning. For example, none of these pass the test for text:

Google Maps:

Amazon:

Material UI

https://mui.com/material-ui/react-rating/

The important aspect is to consider the surface. I think that https://contrast.tools/?tab=apca is a better tool to estimate if the color is accessible or not. Effective, it tells you from which font-size and font-weight the contrast passes the upcoming WCAG v3.0 color contrast thresholds.

With the current color that Material UI uses #FAAF00, I find 42px, font-weight: 700 needed to pass the threshold. It looks like this, feeling close to the current rating experience.


IMHO, we could close the issue with the argument that the icon color is between Google Maps and Amazon the most used places where ratings are used. Which is what ultimately matters: that developers find something they can use without the need to change the default value because it's not what they expect.

If we want to change the color, it feels like Amazon the upper range of the spectrum, it wouldn't work to go darker.

zanivan commented 1 year ago

I disagree with this expected behavior, icons aren't text, we can't use this reasoning. For example, none of these pass the test for text:

WCAG also has a Non-text Contrast guide, so, therefore icons aren't text and shouldn't be treated as it, we surely can bump up the accessibility a bit. However, I agree with you that this won't work for all surfaces, the user will have to tweak it if they want to use on different backgrounds anyway.

Which is what ultimately matters: that developers find something they can use without the need to change the default value because it's not what they expect.

I don't think that default look will be the decisive factor. The more we see the growth of headless components, the more we see that users are keen to use stuff that can be easily changed and customized, so I highly doubt that increasing the contrast for the default look will affect the developer experience. As I see, instead, this will only benefit Material UI accessibility, one of the most decisive factors that makes it stand out from competitors nowadays.

If we want to change the color, it feels like Amazon the upper range of the spectrum, it wouldn't work to go darker.

I don't know if I got it right, but I think we could have something like Amazon indeed, but it'd be a completely different approach to the current component—using the same color tone to filled and empty, and having a visible outline on the filled one.


That said, on #39809, all that I did was to apply MD tokens to it: the warning.main for the filled star, and action.active for the empty one. I can see value in this change just by the fact of using tokens instead of random values. Also, a slight increase in contrast shouldn't hurt IMO.