Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 tasks done
chris-gds opened this issue Sep 24, 2023 · 3 comments · May be fixed by #39809
Open
2 tasks done

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

chris-gds opened this issue Sep 24, 2023 · 3 comments · May be fixed by #39809
Assignees
Labels
accessibility a11y component: rating This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material

Comments

@chris-gds
Copy link

chris-gds commented Sep 24, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the 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.
@chris-gds chris-gds added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 24, 2023
@danilo-leal danilo-leal changed the title Ratings fail colour contrast checks - WCAG 1.4.3 [material-ui][Rating] Failing on color contrast checks Sep 24, 2023
@danilo-leal danilo-leal added accessibility a11y package: material-ui Specific to @mui/material component: rating This is the name of the generic UI component, not the React module! labels Sep 24, 2023
@mnajdova mnajdova assigned zanivan and unassigned mj12albert Nov 8, 2023
@zanivan zanivan removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 8, 2023
@zanivan
Copy link
Contributor

zanivan commented Nov 8, 2023

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.

@zanivan zanivan linked a pull request Nov 8, 2023 that will close this issue
1 task
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 8, 2023

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
Copy link
Contributor

zanivan commented Nov 9, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: rating This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants