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

[A11Y] Fixed: Increase contrast for "required" asterisk in all core themes. #5425

Closed
klonos opened this issue Jan 1, 2022 · 15 comments · Fixed by backdrop/backdrop#3883
Closed

Comments

@klonos
Copy link
Member

klonos commented Jan 1, 2022

This is a sibling issue to #5426, and part of the #5244 meta.

Current style is:

  • Seven: #FF0000 text color over #FAFAFA background, which is AA for large text, but fails for normal size text.
  • Basis/Bartik: #FF0000 text color over #FFFFFF background, which is AA for large text, but fails for normal size text.

Making the text color #E60000 would pass AA for Seven/Basis/Bartik (would make it AAA for large size text, and AA for normal size text).

Making it #AD0000 would pass AAA for Seven/Basis/Bartik, for both large and normal size text.

@klonos klonos self-assigned this Jan 1, 2022
@klonos klonos changed the title [A11Y] Seven: "required" asterisk does not have enough contrast [A11Y] Seven/Basis: "required" asterisk does not have enough contrast Jan 1, 2022
@klonos klonos changed the title [A11Y] Seven/Basis: "required" asterisk does not have enough contrast [A11Y] Seven/Basis/Bartik: "required" asterisk does not have enough contrast Jan 1, 2022
@klonos
Copy link
Member Author

klonos commented Jan 1, 2022

Side-by-side progressive change from #FF0000 (current) to #E60000 (AA) to #AD0000 (AAA) on Seven:

image

@klonos
Copy link
Member Author

klonos commented Jan 1, 2022

@indigoxela
Copy link
Member

indigoxela commented Jan 1, 2022

@klonos your fix works in Seven and Bartik, but not yet in Basis, which ships with a custom color definition in css/component/backdrop-form.css.

Should be easy to fix.

Re AA vs AAA - I could be wrong, but I think until now we were happy with AA. Both colors look OK, but I have a slight preference for #e60000, because the asterisk appears a little more prominent (more contrast to default text color).

@klonos
Copy link
Member Author

klonos commented Jan 1, 2022

@klonos your fix works in Seven and Bartik, but not yet in Basis...

Thanks @indigoxela ...having a look now.

...I think until now we were happy with AA. ...I have a slight preference for #e60000, because the asterisk appears a little more prominent

Yes, I also prefer #e60000 for the same reason. Let's wait for some others to provide feedback - we have PRs for both AA and AAA, so we can merge the preferred one, and close the other 😉

@klonos
Copy link
Member Author

klonos commented Jan 1, 2022

...re Basis, should I change the value in https://github.com/backdrop/backdrop/blob/1.x/core/themes/basis/css/component/backdrop-form.css#L28, or simply remove the entire thing and allow the one set in core/modules/system/css/system.theme.css to take over?

@indigoxela
Copy link
Member

...re Basis, should I change the value in backdrop-form.css or simply remove...

@klonos you have to change the value, because Basis does not load system.theme.css. Otherwise you end up with normal text color.

@klonos
Copy link
Member Author

klonos commented Jan 2, 2022

Right, I missed that. Fixed in both PRs 👍🏼

Thanks @indigoxela 🙏🏼

@stpaultim
Copy link
Member

Re AA vs AAA - I could be wrong, but I think until now we were happy with AA. Both colors look OK, but I have a slight preference for #e60000, because the asterisk appears a little more prominent (more contrast to default text color).

I agree

@indigoxela
Copy link
Member

@klonos many thanks for the update, this works correctly now in Basis. 👍

We still have two PR, so marking as RTBC might not yet be appropriate. How much feedback do we need, to be able to decide? Currently 3 of 3 people prefer #e60000.

@olafgrabienski
Copy link

Currently 3 of 3 people prefer #e60000.

4 of 4 now :-)

@argiepiano
Copy link

I too prefer #e60000

@klonos
Copy link
Member Author

klonos commented Jan 3, 2022

Thank you guys 👍🏼

I guess we have enough feedback now 😅

@indigoxela
Copy link
Member

I guess we have enough feedback now

Sure! Consent like that is rare. 😉 @klonos mind to close the second PR? The first one's RTBC then.

@klonos
Copy link
Member Author

klonos commented Jan 6, 2022

Done 👍🏼

@quicksketch
Copy link
Member

Merged backdrop/backdrop#3883 into 1.x and 1.20.x. Thanks @klonos and all the folks that provided feedback!

@klonos klonos removed their assignment Jan 13, 2022
@jenlampton jenlampton changed the title [A11Y] Seven/Basis/Bartik: "required" asterisk does not have enough contrast [A11Y] Fixed: Increase contrast for "required" asterisk in all core themes. Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants