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

Use WCAG contrast algo #30168

Merged
merged 8 commits into from Mar 23, 2020
Merged

Use WCAG contrast algo #30168

merged 8 commits into from Mar 23, 2020

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented Feb 13, 2020

Getting on this topic again, I followed #22996 discussion again and came up with a potential solution: by including sass-math-pow polyfill in our vendors (beside RFS), there's no dependency issue — and it should work with every Sass engine (didn't test…).

Relates to #25126 somehow.

Let me know if you need further work.

I guess it concerns @patrickhlauke, @MartijnCuppens, @mdo and maybe @ysds?

@ffoodd
Copy link
Member Author

ffoodd commented Feb 13, 2020

Another solution could be to use a lookup table — just as in Material themes:

Not quite sure about this, though.

@ysds
Copy link
Member

ysds commented Feb 13, 2020

Actually, I use the WCAG algorithm in my project. The dart Sass has the built-in pow function, but the old sass does not have it, so I use a lookup table. I chose a lookup table because it is much shorter than the pow function code.

@ysds
Copy link
Member

ysds commented Feb 13, 2020

My code:

$yiq-contrast: 3 !default;

// Returns the color ensure contrast ratio 4.5:1
@function color-yiq($background, $foreground-dark: $yiq-text-dark, $foreground-light: $yiq-text-light) {
  $l1: luminance($background);
  $l2: luminance(opaque($background, $foreground-light));

  $contrast: if($l1 > $l2, ($l1 + .05) / ($l2 + .05), ($l2 + .05) / ($l1 + .05));

  @if ($contrast < $yiq-contrast) {
    @return $foreground-dark;
  } @else {
    @return $foreground-light;
  }
}

// Return WCAG2.0 relative luminance
// https://www.w3.org/WAI/GL/wiki/Relative_luminance

// A list of pre-calculated numbers of pow(($value / 255 + .055) / 1.055, 2.4). (from 0 to 255)
// stylelint-disable-next-line scss/dollar-variable-default
$luminance-list: .0008 .001 .0011 .0013 .0015 .0017 .002 .0022 .0025 .0027 .003 .0033 .0037 .004 .0044 .0048 .0052 .0056 .006 .0065 .007 .0075 .008 .0086 .0091 .0097 .0103 .011 .0116 .0123 .013 .0137 .0144 .0152 .016 .0168 .0176 .0185 .0194 .0203 .0212 .0222 .0232 .0242 .0252 .0262 .0273 .0284 .0296 .0307 .0319 .0331 .0343 .0356 .0369 .0382 .0395 .0409 .0423 .0437 .0452 .0467 .0482 .0497 .0513 .0529 .0545 .0561 .0578 .0595 .0612 .063 .0648 .0666 .0685 .0704 .0723 .0742 .0762 .0782 .0802 .0823 .0844 .0865 .0887 .0908 .0931 .0953 .0976 .0999 .1022 .1046 .107 .1095 .1119 .1144 .117 .1195 .1221 .1248 .1274 .1301 .1329 .1356 .1384 .1413 .1441 .147 .15 .1529 .1559 .159 .162 .1651 .1683 .1714 .1746 .1779 .1812 .1845 .1878 .1912 .1946 .1981 .2016 .2051 .2086 .2122 .2159 .2195 .2232 .227 .2307 .2346 .2384 .2423 .2462 .2502 .2542 .2582 .2623 .2664 .2705 .2747 .2789 .2831 .2874 .2918 .2961 .3005 .305 .3095 .314 .3185 .3231 .3278 .3325 .3372 .3419 .3467 .3515 .3564 .3613 .3663 .3712 .3763 .3813 .3864 .3916 .3968 .402 .4072 .4125 .4179 .4233 .4287 .4342 .4397 .4452 .4508 .4564 .4621 .4678 .4735 .4793 .4851 .491 .4969 .5029 .5089 .5149 .521 .5271 .5333 .5395 .5457 .552 .5583 .5647 .5711 .5776 .5841 .5906 .5972 .6038 .6105 .6172 .624 .6308 .6376 .6445 .6514 .6584 .6654 .6724 .6795 .6867 .6939 .7011 .7084 .7157 .7231 .7305 .7379 .7454 .7529 .7605 .7682 .7758 .7835 .7913 .7991 .807 .8148 .8228 .8308 .8388 .8469 .855 .8632 .8714 .8796 .8879 .8963 .9047 .9131 .9216 .9301 .9387 .9473 .956 .9647 .9734 .9823 .9911 1;

@function luminance($color) {
  $rgb: (
    "r": red($color),
    "g": green($color),
    "b": blue($color)
  );

  @each $name, $value in $rgb {
    $value: if($value / 255 < .03928, $value / 255 / 12.92, nth($luminance-list, $value + 1));
    $rgb: map-merge($rgb, ($name: $value));
  }

  @return (map-get($rgb, "r") * .2126) + (map-get($rgb, "g") * .7152) + (map-get($rgb, "b") * .0722);
}

// Return opaque color
// opaque(#fff, rgba(0, 0, 0, .5)) => #808080
@function opaque($background, $foreground) {
  @return mix(rgba($foreground, 1), $background, opacity($foreground) * 100);
}

@ysds
Copy link
Member

ysds commented Feb 13, 2020

It looks good to be able to pass the case given a color that includes alpha transparency.

@MartijnCuppens
Copy link
Member

I would also prefer @ysds' method. Anyway, nice idea!

@mdo
Copy link
Member

mdo commented Feb 13, 2020

Agreed with the team on this—my biggest gripes with these expanded functions has always been approachability and the volume of (source) code we bring in. For myself, and for the millions of folks using the project :). All for improving what's there now though to give folks the right level of accessibility here.

@ffoodd
Copy link
Member Author

ffoodd commented Feb 14, 2020

Agreed; I'll update this to use @ysds code and see if it's better :)

@ffoodd
Copy link
Member Author

ffoodd commented Feb 14, 2020

Up!

I reverted first commits: you'd probably want to squash this — and I messed up with a variable name, sorry about that -_-

@patrickhlauke
Copy link
Member

i'll just leave, as an oblique/side comment here (not necessarily about this PR in particular, but about contrast calculations): while it's good to calculate ratio to ensure it's on or above 4.5:1, it then still results in too low a contrast once an opacity of < 1 is used on the element itself (i seem to remember finding instances where even in the core components here a subtle opacity variance is used...this immediately lowers the contrast again, making values pass "on paper" while in actuality things still fall below 4.5:1)

@ffoodd
Copy link
Member Author

ffoodd commented Feb 14, 2020

@patrickhlauke You're right about this. This is why #29315 is so important I guess (could allow to add a sample page with every color combinations based on Bootstrap's variables, for example).

In this particular case of color-yiq() function, since it's meant to choose text color depending on background-color, taking opacity into account would require to know what's behind the background-color… Hard.

FYI I'm testing outputs in Boosted #323 — in which v4-dev uses sass-math-pow as an external dependency, v5-dev uses included _pow.scss, and PR stands for this current one using @ysds lookup table. I'm comparing results to Contrast Finder ones (don't have CCA yet, will try).

To make it short, @ysds lookup table is the only version matching Contrast Finder results :)

@ffoodd
Copy link
Member Author

ffoodd commented Feb 14, 2020

I confirm that CCA gives the same result as Contrast Finder (thus as @ysds's code).

@ffoodd

This comment has been minimized.

@XhmikosR

This comment has been minimized.

@XhmikosR

This comment has been minimized.

@ffoodd
Copy link
Member Author

ffoodd commented Feb 17, 2020

@patrickhlauke Could you please review WCAG related content?

I updated the firsth paragraph of "Color contrast" section, and added a note after examples. I'm not a native english speaker so I'd prefer a review on sensitive topics…

@ffoodd
Copy link
Member Author

ffoodd commented Feb 17, 2020

@XhmikosR Done in #30207

@patrickhlauke
Copy link
Member

@patrickhlauke Could you please review WCAG related content?

I updated the firsth paragraph of "Color contrast" section, and added a note after examples. I'm not a native english speaker so I'd prefer a review on sensitive topics…

looks fine to me, with one exception: i'd change "should" to "must" in that note (as the must doesn't preclude the existence of exceptions)

@ffoodd
Copy link
Member Author

ffoodd commented Feb 18, 2020

@MartijnCuppens @ysds A question remains: shouldn't we rename $yiq-* and color-yiq() to something unrelated to the YIQ colorspace?

  • The function returns a sufficiently contrasted color;
  • $yiq-contrasted-threshold is the contrast ratio to reach;
  • $yiq-text-dark and $yiq-text-light never had anything to do with YIQ.

And if you agree, how do you handle breaking changes for this kind of things?

@MartijnCuppens
Copy link
Member

A question remains: shouldn't we rename $yiq-* and color-yiq() to something unrelated to the YIQ colorspace?

I'm not that familiar with the YIQ colorspace, but if the naming is incorrect we should definitely rename it.

And if you agree, how do you handle breaking changes for this kind of things?

As long as v5 is not released, we can make BCs if they are documented in the migration guide

@xi
Copy link
Contributor

xi commented Feb 26, 2020

In this particular case of color-yiq() function, since it's meant to choose text color depending on background-color, taking opacity into account would require to know what's behind the background-color… Hard.

I wrote a blog post about that a while back: http://tobib.spline.de/xi/posts/2016-03-05-color-contrast/. TLDR: You can get decent results by assuming the worst case (which is actually quite easy to calculate).

@andresgalante
Copy link
Collaborator

👋 If this fixed our color contrasts issues, should probably change this part of the docs:

http://localhost:9001/docs/4.3/getting-started/accessibility/#color-contrast

@patrickhlauke
Copy link
Member

@andresgalante i would leave that as is, since as soon as there's things like opacity changes that also happen, i believe there are still places where vanilla bootstrap has contrast issues

@ffoodd
Copy link
Member Author

ffoodd commented Mar 13, 2020

@xi I first tried to use sass-planifolia (in Boosted, the fork I'm working on) — so I'm really interested by your opinion. My remark about opacity was more about the fact that this function returns a color.

For now, this PR only improves the current feature by using the WCAG algo (and computing an opaque color when foreground has alpha transparency, which is already better than nothing); assuming the worst case scenario to better handle opacity would probably result in very different outputs, am I right?

If so, I guess it should be tested in another PR and double-checked by the team.

@andresgalante If #29315 makes it into master and results in satisfying contrast everywhere (which I really doubt), we may change it by only mentionning things that would lower contrasts (obviously changing colors, but decreasing font-size too).

@MartijnCuppens @ysds @XhmikosR IMHO this PR is ready; any thoughts?

@MartijnCuppens
Copy link
Member

I would personally rename $foreground-dark and $foreground-light to $color-contrast-dark and $color-contrast-light. Other then that, this LGTM

@ffoodd
Copy link
Member Author

ffoodd commented Mar 17, 2020

Makes sense, done — and amended in migration.md :)

@ffoodd ffoodd self-assigned this Mar 17, 2020
scss/_variables.scss Outdated Show resolved Hide resolved
Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from the SCSS side

@ffoodd
Copy link
Member Author

ffoodd commented Mar 18, 2020

@ysds Anything else to check?

@ysds
Copy link
Member

ysds commented Mar 18, 2020

@ffoodd Could you clean up the commit log?

@ffoodd ffoodd requested a review from ysds March 23, 2020 09:49
@ffoodd
Copy link
Member Author

ffoodd commented Mar 23, 2020

@ysds Log should look better now :)

scss/_functions.scss Outdated Show resolved Hide resolved
@MartijnCuppens MartijnCuppens merged commit 03908ea into twbs:master Mar 23, 2020
@ffoodd ffoodd deleted the feature/wcag-algo branch March 24, 2020 08:03
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants