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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TextField] Accessibility contrast issue with filled variant #24947

Closed
2 tasks done
tcx opened this issue Feb 15, 2021 · 9 comments 路 Fixed by #25046
Closed
2 tasks done

[TextField] Accessibility contrast issue with filled variant #24947

tcx opened this issue Feb 15, 2021 · 9 comments 路 Fixed by #25046
Labels
accessibility a11y component: text field This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@tcx
Copy link

tcx commented Feb 15, 2021

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 馃槸

The current filled variant of TextField does not pass the Lighthouse accessibility check on contrast.

Lighthouse report

Expected Behavior 馃

The text color should follow the material design specs, which will make the Lighthouse accessibility check on contrast pass.

Steps to Reproduce 馃暪

Steps:

  1. Launch Chrome/Chromium
  2. Go to https://next.material-ui.com/components/text-fields/
  3. Launch Developer Tools
  4. Run a Lighthouse accessibility audit

Context 馃敠

I want filled variants of TextField to be accessible out of the box.

Your Environment 馃寧

`npx @material-ui/envinfo`
  System:
    OS: Linux 5.10 Fedora 33 (Workstation Edition) 33 (Workstation Edition)
  Binaries:
    Node: 14.15.4 - /usr/bin/node
    Yarn: Not Found
    npm: 6.14.10 - /usr/bin/npm
  Browsers:
    Chrome: Not Found
    Firefox: 85.0.1
@tcx tcx added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 15, 2021
@oliviertassinari
Copy link
Member

@tcx Thanks, I would propose the following (In case you want to work on it, all 馃煝):

  1. Update the secondary text color to match the update of the spec:

https://material.io/design/color/text-legibility.html#text-backgrounds

Medium-emphasis text and hint text have opacities of 60%

It will also help with @eps1lon's concern around differentiating disabled from secondary:

diff --git a/packages/material-ui/src/styles/createPalette.js b/packages/material-ui/src/styles/createPalette.js
index 1a8e17b707..d827ab21bc 100644
--- a/packages/material-ui/src/styles/createPalette.js
+++ b/packages/material-ui/src/styles/createPalette.js
@@ -16,7 +16,7 @@ export const light = {
     // The most important text.
     primary: 'rgba(0, 0, 0, 0.87)',
     // Secondary text.
-    secondary: 'rgba(0, 0, 0, 0.54)',
+    secondary: 'rgba(0, 0, 0, 0.6)',
     // Disabled text have even lower visual prominence.
     disabled: 'rgba(0, 0, 0, 0.38)',
   },

It also solves our contrast ratio issue.

  1. We reduce the opacity of the filled background, it's simply ugly cc @mbrookes. It looks like the field is disabled.
diff --git a/packages/material-ui/src/FilledInput/FilledInput.js b/packages/material-ui/src/FilledInput/FilledInput.js
index 8d52f09140..1796dcc353 100644
--- a/packages/material-ui/src/FilledInput/FilledInput.js
+++ b/packages/material-ui/src/FilledInput/FilledInput.js
@@ -37,7 +37,7 @@ const FilledInputRoot = experimentalStyled(
 )(({ theme, styleProps }) => {
   const light = theme.palette.mode === 'light';
   const bottomLineColor = light ? 'rgba(0, 0, 0, 0.42)' : 'rgba(255, 255, 255, 0.7)';
-  const backgroundColor = light ? 'rgba(0, 0, 0, 0.09)' : 'rgba(255, 255, 255, 0.09)';
+  const backgroundColor = light ? 'rgba(0, 0, 0, 0.06)' : 'rgba(255, 255, 255, 0.09)';
   return {
     /* Styles applied to the root element. */
     position: 'relative',
@@ -49,14 +49,14 @@ const FilledInputRoot = experimentalStyled(
       easing: theme.transitions.easing.easeOut,
     }),
     '&:hover': {
-      backgroundColor: light ? 'rgba(0, 0, 0, 0.13)' : 'rgba(255, 255, 255, 0.13)',
+      backgroundColor: light ? 'rgba(0, 0, 0, 0.09)' : 'rgba(255, 255, 255, 0.13)',
       // Reset on touch devices, it doesn't add specificity
       '@media (hover: none)': {
         backgroundColor,
       },
     },
     '&.Mui-focused': {
-      backgroundColor: light ? 'rgba(0, 0, 0, 0.09)' : 'rgba(255, 255, 255, 0.09)',
+      backgroundColor,
     },
     '&.Mui-disabled': {
       backgroundColor: light ? 'rgba(0, 0, 0, 0.12)' : 'rgba(255, 255, 255, 0.12)',

Capture d鈥檈虂cran 2021-02-14 a虁 19 54 52

It still reaches AA:

Capture d鈥檈虂cran 2021-02-14 a虁 19 55 18

@oliviertassinari oliviertassinari added accessibility a11y component: text field This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process good first issue Great for first contributions. Enable to learn the contribution process. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 15, 2021
@eps1lon
Copy link
Member

eps1lon commented Feb 16, 2021

Why do we need to change the palette and TextField to change the color contrast?

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 16, 2021

@eps1lon The change of the palette would be to match the latest version of the Material Design Guidelines.

The change of the TextField is optional, it would be to say: "no Material Design, your filled input is ugly, it's too grey, it looks disabled", e.g. https://twitter.com/aardrian/status/1358062642544410624

@eps1lon
Copy link
Member

eps1lon commented Feb 16, 2021

@eps1lon The change of the palette would be to match the latest version of the Material Design Guidelines.

So it's unrelated to this issue?

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 16, 2021

So it's unrelated to this issue?

What do you mean by unrelated? It fixes the contrast issue, as a positive second order effect.

@Dripcoding
Copy link
Contributor

@oliviertassinari is anyone working on this issue? I'd like to work on this as my first contribution to the repository.

@oliviertassinari
Copy link
Member

@Dripcoding You can go ahead :)

@eps1lon
Copy link
Member

eps1lon commented Feb 22, 2021

It fixes the contrast issue, as a positive second order effect.

What does that mean? I think there's something wrong with the filled TextField if we need to change colors in two places. It looks like you're trying to change something else here.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 22, 2021

It looks like you're trying to change something else here.

@eps1lon I see 4 values:

  1. Update the palette to match the spec
  2. Bring AA contrast on the filled input
  3. Fix the ugly grey background-color
  4. Remove the potential confusion around the filled input being disabled

I think that 1. can be solved independently. I don't think that 2,3,4 can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: text field This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants