-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Use black and white as color-contrast fallback colors #30468
Conversation
LGTM. @ffoodd, could you check the PR content?
Throw a warning and return the color with the most contrast |
site/content/docs/4.3/examples/floating-labels/floating-labels.css
Outdated
Show resolved
Hide resolved
5998f8d
to
8b8f75e
Compare
To sum up changes:
To check the Be sure to double-check things and play around: I increased the minimum contrast ratio to 4.5, which could affect every components… To see obvious changes, please refer to the Theming page:
Using Firefox's Accessibility DevTools, this PRs has 2 failures (excpeting anchor links "#") versus master which has 18; WAVE extension gives me 2 vs 18 too. The two failures are Success and Info swatches using |
The green button text is now black and white on hover, which feels a bit odd. Maybe we should darken the green color a bit? Maybe #048A24? |
Nice catch, I'll try this.
However outline buttons for Warning, Info and Light aren't. |
This PR works as intended. While testing results, I came across three types of contrast failure through our docs — I fixed a few of them, the most straightforwards. Color utilitiesUtilities like Bad component variantsSome components or items are conceptually unable to comply (for now):
Unfortunate examplesSome more in-depth examples should be reviewed: color schemes in navbar don't work well, but are only examples. Not quite sure about this kind of failures… But I think they can be quite common in our docs and example templates. |
Reviewing related #25126, the Theming page is now fixed (at least with this PR). Color utilities still have failures: Apart from those, #25126 would be fixed as of this PR. |
So @twbs/css-review here's a summary, I don't really what to do with this.
I think this should not be addressed in this PR, I already fixed the simplest examples and occurrences through our docs. Should I open issues, make some PRs? Or wait and see, since we're already improving contrasts everywhere? |
0815c57
to
b77372e
Compare
My only issue thus far with this is that the new green isn't the best visually—instead of matching with the overall color vibe, it feels like it sticks out as a forest green. However, I know green is notoriously difficult for getting color contrast right. I'd like to see what can be done with the colors before this gets merged. To help with that, thoughts on putting together a colors page outside the docs (Examples or CodePen?) that includes our colors and their current contrast values against white and black? |
55010ff
to
1fc5ec5
Compare
@mdo just a POC for now but I temporarily display contrast ratios in main color swatches:
This is more to show you the values than to publish this — it's not accessible enough, I think — but was fairly the simplest way to show contrast ratio values since the only function to calculate them in Bootstrap resides in scss… BTW I got the previous green back for now. We may add a "Color Palette" example template, but I'm not convinced about its interest. |
95b6e0b
to
e0ab580
Compare
@mdo Another approval on this? It'll help me a lot with other color-related topics :) |
4271ad6
to
2f40940
Compare
color-contrast()
failuresThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this, but want another approval from @mdo just to be sure.
2f40940
to
2988e48
Compare
Made a few more tweaks to stick to the brand new "Customize" section :) |
@mdo friendly ping on this, since #30550 is on-hold from now on. FYI, this PR now:
This fourth point may be dropped, since it's partially covered by #30044 — or at least, would be solved differently in a more elegant way. I'd like to see this merged before going on with colors :) |
a24068d
to
df87acd
Compare
…ht-first), the most contrasted one otherwise
ebbc9ab
to
b138199
Compare
While trying to fix a few things related to #29315 I tried to increase the ratio against white threshold used in
color-contrast()
to 4.5.This led to unexpected results, as I didn't totally understood what was going on this function: basically, we only test contrast against white, and if it fails to cross the threshold we use a dark foreground color instead.
This doesn't ensure that our dark foreground color will cross the threshold, neither than its contrast ratio with background will be better than white…
$indigo-300
in "Theming" is a good example where#fff
doesn't meet 4.5:1 ratio required by WCAG 2 (giving only 4.06:1) but our$color-contrast-dark
neither (defined as#212529
in_variables.scss
, giving less than white: 3.8:1). FYI in that specific case, using#000
instead of#212529
makes it to 4.5:1.Depending on the minimum contrast ratio defined, we'll always fail to meet 4.5 and increasing this
$min-contrast-ratio
would in fact lead to more insufficient contrasts everywhere — since white wouldn't match, we'd fallback to a worst value…).I'd recommend the following:
$color-contrast-light
crosses the threshold; if so, return it;$color-contrast-dark
does; if so, return it;#fff
;If none of those lead to sufficient contrast ratio… Well, I don't know!
@twbs/css-review @patrickhlauke any opinion on this?
Preview: https://deploy-preview-30468--twbs-bootstrap.netlify.com/