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/Select contrast color #21861

Open
1 task done
fbarbare opened this issue Jul 20, 2020 · 22 comments
Open
1 task done

TextField/Select contrast color #21861

fbarbare opened this issue Jul 20, 2020 · 22 comments
Labels
component: text field This is the name of the generic UI component, not the React module! ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@fbarbare
Copy link
Contributor

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

Summary 馃挕

For Button and IconButton we have the possibility to set the color to things like "inherit" which allows us to add readable buttons (like a drawer-menu button) on top of the AppBar:
image

However, if we want to add a TextField or a Select, we cannot get its "default state" color to be changed accordingly to the background:
image

Examples 馃寛

I suspect it wouldn't be easy to just use inherit here as different opacity applies to different elements (label/text/borders/disabled/...). So I was thinking that we could have another prop on the component like contrastFrom= "primary" | "secondary" or maybe theme= "light" | "dark" and the input could then adapt its color to the used background color:
image

Motivation 馃敠

Some of the pages (not all of them) use the primary color as the main background color and I would like to add a Select in the footer so the user can change the language of the website.
I am able to achieve this by using the classes prop and overwriting the color of elements, but it's not ideal.


P.S. If we can agree on a way to do this, I could try to submit a PR

@fbarbare fbarbare added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 20, 2020
@oliviertassinari oliviertassinari added component: text field This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 20, 2020
@oliviertassinari
Copy link
Member

@fbarbare Interesting concern, I imagine there are a bunch of overrides required to make this work without any built-in solutions. The label, the input, the icon, the underline + the different states. I think that it could make sense to push #17891 further with a color="inherit" variant.

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Jul 20, 2020
@fbarbare
Copy link
Contributor Author

I don't think the color="inherit" would work. You would want to change the initial color of the TextField, but not the color it gets when focused and I guess should still be able to choose which one you would want for that.

@oliviertassinari
Copy link
Member

@fbarbare Ok thanks, in this case, overrides sounds more appropriate

@fbarbare
Copy link
Contributor Author

@oliviertassinari
I don't think overrides are a good solution for this as elements/opacities/... are susceptible to change. It's I guess more about finding what default color should be applied to the TextField as the base color (black or white). Maybe using "inherit" is a good solution but not for the color prop. We could use the parent element css color to determine automatically if the TextField should be rendered in Black or White.

@oliviertassinari
Copy link
Member

Are you suggesting to have two different props?

@fbarbare
Copy link
Contributor Author

@oliviertassinari
Because detecting the background color of the parent element to determine if the TextField should be displayed in white or black could be hard and not always right (absolute positioned elements), it might indeed be easier to create a new prop (e.g. baseColor="white"|"black" or theme="light"|"dark") where the default would be based on your actual theme type

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2020

I'm against this proposal, if you need the dark theme, use the dark theme.

What about something like this?

TextFieldProps ={
  color?: 'text.primary' | 'inherit'; // default text.primary
  accentColor?: 'primary' | 'secondary'; // default primary
}

@fbarbare
Copy link
Contributor Author

That could work, though I would not call them color and accentColor so we don't induce a breaking change:

TextFieldProps = {
  textColor?: 'default' | 'inherit'; // default: default
  color?: 'primary' | 'secondary'; // default: primary
}

How would you get the inherited color? would it be possible to just apply color: 'inherit' on the element and move the opacity to opacity: 0.42 rather than use color: 'rgba(0,0,0,0.42)'? Otherwise we would need to somehow fetch the computed style color (most probably using Refs) and process it to add the opacity to it

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2020

@fbarbare Yeah, I was thinking of using color: inherit + opacity + currentColor for the implementation. For the breaking change concern, let's answer this question. What's would you intuitively associate the color prop with? (we are working on v5, so a BC can be OK if more intuitive).

@fbarbare
Copy link
Contributor Author

I am not sure I understand what you mean about color: inherit + opacity + currentColor. Do you have a link to some doc I could read up on or something?

If a breaking change is not a big problem, I guess color and accentColor or highlightColor is fine

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2020

I am not sure I understand what you mean about color: inherit + opacity + currentColor. Do you have a link to some doc I could read up on or something?

@fbarbare It's not very important, you were already on the right track :)

If a breaking change is not a big problem, I guess color and accentColor or highlightColor is fine

I don't know if it's that's better, I think that as long it's intuitive, we will be good. color+ highlightColor is an interesting proposal too.

@fbarbare
Copy link
Contributor Author

I quite like color + highlightColor or maybe even just highlight.

The only problem I have with color changing is that it's a pretty big breaking change:

  • when upgrading people will have to change pretty much half of their TextFields/Selects
  • the change might not be super intuitive as we're keeping the color prop but it will then need different values and being used for something different

@fbarbare
Copy link
Contributor Author

@oliviertassinari
What about not changing the props at all but just always use "inherit" for the base color?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2020

What about not changing the props at all but just always use "inherit" for the base color?

How often do we need the color inherit behavior vs how would we need to restore the color to text.primary because the color was changed by the container? Not sure 馃

@fbarbare
Copy link
Contributor Author

Yeah, I guess this would come back as an issue at some point...

I could start drafting a PR if you want to tell me more about this color: inherit + opacity + currentColor 馃槀
We can always review the names of the props later as long as we have the logic ready

@oliviertassinari
Copy link
Member

@fbarbare
Copy link
Contributor Author

@oliviertassinari
A couple of things from digging a bit in the code:
We apply a different opacity on the font color depending on the theme (body1 color for light theme rgba(0, 0, 0, 0.54) and for dark theme rgba(255, 255, 255, 0.7)) so we would need to base ourselves on the theme to define the opacity we would need to apply.
Looking at Material Design guidelines and web library, they do not set any opacity on their typography. So maybe there is something wrong there?
Looking at Material Design guidelines for TextField, they seem to always set the label opacity to 0.6 but I could not see any demo of the dark mode.

Do you have any advice on how to tackle that?

@fbarbare
Copy link
Contributor Author

@oliviertassinari Any ideas? 馃檪

@oliviertassinari
Copy link
Member

@fbarbare I assume you are referring to:

Capture d鈥檈虂cran 2020-07-23 a虁 16 12 48

https://material.io/design/color/dark-theme.html#ui-application

Capture d鈥檈虂cran 2020-07-23 a虁 16 13 20

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

We cover it in #21093.

We could extract the alpha value from the color to apply it as an opacity.

@tomByrer
Copy link

tomByrer commented Jul 24, 2020

detecting the background color of the parent element to determine if the TextField should be displayed in white or black

TinyColor (used by the VSCode plugin Peacock) has a function to help find the best contrast out of a list of possibles, if you guys are looking for something like that?

@oliviertassinari
Copy link
Member

@tomByrer We already have color helpers in colorManipulator.js.

@RemyMachado
Copy link

Any update on this?
What is the current way to work around this issue?

Using a dark theme doesn't make sense, for instance when you are putting light text on a dark hero image, but the page is still using a light theme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

No branches or pull requests

4 participants