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

get-token-from-bg should warn not error #3926

Closed
plummer-flex opened this issue Jan 8, 2021 · 11 comments
Closed

get-token-from-bg should warn not error #3926

plummer-flex opened this issue Jan 8, 2021 · 11 comments
Assignees
Labels
Type: Bug A problem in the code
Milestone

Comments

@plummer-flex
Copy link

Describe the bug

When upgrading a project using an existing USWDS theme, the theme fails to compile due to a color contrast issue. The contrast failure output does not match manual testing of the colors referenced in the error. This leads to the supposition that the function used to verify color contrast does not return a correct result.

Steps to reproduce the bug

Set $accent-warm: #ff6839;
Set $base-ink: #292a2b;
Compile returns error SassError: Neither 'white' nor 'ink' have AA contrast on a 'accent-warm' background.

Expected behavior

At the least, the function should throw a warning instead of an error. While the utility of the system preventing a user from creating a non-compliant color combination has merit, this decision should ultimately be in the hands of the user.

System setup

  • **USWDS version: 2.10.0
@plummer-flex plummer-flex added the Type: Bug A problem in the code label Jan 8, 2021
@mejiaj
Copy link
Contributor

mejiaj commented Jan 8, 2021

Could be related to #3906.

@jAigret-Bix jAigret-Bix added this to Sprint backlog in USWDS Sprint Board Jan 11, 2021
@mejiaj
Copy link
Contributor

mejiaj commented Jan 11, 2021

In looking into #3906, I've updated the code to return a warning. But it will still cause an error because it will return false and error because it's not a valid color token.

a {
    @include set-link-from-bg("base-darkest", "white");
  }
Error in plugin "sass"
Message:
    src/stylesheets/core/mixins/_set-link-from-bg.scss
Error: "`false` is not a valid USWDS color token. See designsystem.digital.gov/design-tokens/color for more information."
   ╷
31 │   color: color($link-token);
   │          ^^^^^^^^^^^^^^^^^^
   ╵
  src/stylesheets/core/mixins/_set-link-from-bg.scss 31:10  set-link-from-bg()

@astiefvater
Copy link
Contributor

We are getting the following error when trying to compile:
{
"status": 1,
"file": "prod/scss/core/_functions.scss",
"line": 1734,
"column": 5,
"message": "Neither 'white' nor 'ink' have AA contrast on a 'accent-cool' background.",
"formatted": "Error: Neither 'white' nor 'ink' have AA contrast on a 'accent-cool' background.\n on line 1734 of scss/core/_functions.scss, in function get-token-from-bg\n from line 1749 of scss/core/_functions.scss, in function get-color-from-bg\n from line 7 of scss/core/mixins/_set-text-from-bg.scss, in mixin set-text-from-bg\n from line 21 of scss/core/mixins/_set-text-from-bg.scss, in mixin set-text-and-bg\n from line 54 of scss/elements/_buttons.scss\n from line 10 of scss/packages/_uswds-components.scss\n from line 13 of scss/uswds.scss\n from line 71 of scss/theme/styles.scss\n>> @error "Neither '#{$preferred-text-color}' nor '#{$fallback-text-color}'\n ----^\n"
}
I have attached a .txt version of our _uswds-theme-color.scss" file since github won't let me directly upload .scss files
_uswds-theme-color.txt

@mejiaj
Copy link
Contributor

mejiaj commented Jan 13, 2021

@astiefvater would you mind sharing your theme colors? You can find them in _uswds-theme-color.scss

@astiefvater
Copy link
Contributor

@astiefvater would you mind sharing your theme colors? You can find them in _uswds-theme-color.scss

Hi James it is the text file attached to my original comment. It would not let me upload with the .scss extension. Let me know if that won't convert back for some reason though.

@mejiaj
Copy link
Contributor

mejiaj commented Jan 13, 2021

My apologies. Thanks for that.

@plummer-flex
Copy link
Author

But it will still cause an error because it will return false and error because it's not a valid color token.

I think the core problem is that the function iterates over a list of tokens for find a suitable match, but fails is no suitable match is found.

One solution might be to store the comparison outputs to an array, run comparisons to the magic number, then return the best one by comparison and log a warning if it's not in the threshold.

@function get-link-tokens-from-bg-part-deux(
  $bg-color,
  $preferred-link-color: $theme-link-color,
  $fallback-link-color: $theme-link-reverse-color,
  $wcag-target: "AA"
) {

  $magic-numbers: (
    "AA": 50,
    "AAA": 70,
    "AA-large": 40,
  );
  $grade-step: 10;
  $decomposed: false;

  @if $preferred-link-color == default {
    $preferred-link-color: $theme-link-color;
  }

  $target-magic-number: map-get($magic-numbers, $wcag-target);
  $bg-grade: get-color-grade($bg-color);
  $defined-color-tokens: ($preferred-link-color, $fallback-link-color, "ink");

  // set an array to store the color token scores
  $color-token-values: ();

  // roll through each $defined-color-token and store to $color-token-values
  @each $color-token in $defined-color-tokens {
    $color-token-values: append($color-token-values, get-color-grade($color-token));
  }

  $winner-winner-chicken-dinner: false;

  @each $color-token-value in $color-token-values {
    @if not $winner-winner-chicken-dinner {
      $winner-winner-chicken-dinner: index($color-token-values, $color-token-value); 
    } @else {
      @if ($color-token-value > $winner-winner-chicken-dinner) {
        $winner-winner-chicken-dinner: $color-token-value;
      }
    }
  }

  @return nth($defined-color-tokens, $winner-winner-chicken-dinner);
}

@mejiaj
Copy link
Contributor

mejiaj commented Jan 14, 2021

@plummer-flex you can find more details in the thread on #3906 (comment).

I like the proposal and I think it could be part of the solution. There are a few more things I think we can fine-tune to produce more reliable results. Right now I'm looking at how the luminance numbers are generated and why some colors that are accessible still fail.

@plummer-flex
Copy link
Author

@plummer-flex you can find more details in the thread on #3906 (comment).

I like the proposal and I think it could be part of the solution. There are a few more things I think we can fine-tune to produce more reliable results. Right now I'm looking at how the luminance numbers are generated and why some colors that are accessible still fail.

Yeah, I guess it's two issues.

  1. get-link-tokens-from-bg returning a value and posting a warning.
  2. get-color-grade returning an accurate result.

Godspeed.

@mejiaj
Copy link
Contributor

mejiaj commented Jan 15, 2021

Case 1

Colors:

$theme-color-base-lightest: #f2f2f2;
$theme-color-base-lighter: #dcdcdc;
$theme-color-base: #707070;
$theme-color-base-dark: #535353;
$theme-color-base-darkest: #141414;
$theme-color-base-ink: #141414;

Using:

@include set-link-from-bg("red-warm-60v", "ink", "base-lighter");

Tests

Ink

#141414 on #9c3d10

Variables Value
Background grade 60
Link grade 90
Link magic number 30
Target magic number 50

This one fails automatically so we can move on to $fallback-link-color #dcdcdc

Base lighter

#dcdcdc on #9c3d10

Variables Value
Background grade 60
Link grade 14.9
Link magic number 45.1
Target magic number 50

Both custom $theme-color-base-lighter #dcdcdc and it's next grade $theme-color-base-lightest #f2f2f2 pass on

But fail in our tests.

In our code it looks like:

// $color-token: base-lighter
// $lum: 0.709
// $min: 0.75
// $max: 0.82
// $next-max: 0.65

@if $next-max and ($lum < $min) and ($lum > $next-max) {
  @return $grade + 4.9;
}

Case 2

Colors

$theme-color-accent-warm: #ff6839;
$theme-color-base-ink: #292a2b;

Using

@include set-text-and-bg("accent-warm");

Tests

White

❌White #ffffff on #ff6839.

Variables Value
Background grade 34.9
Text grade 0
Magic number 34.9
Target magic number 50

Ink

❌ Ink #292a2b on accent warm #ff6839

Variables Value
Background grade 34.9
Text grade 80
Magic number 45.1
Target magic number 50

Note

For buttons, like this case, there are also more considerations. The same set-text-and-bg() mixin is used for the :visited, :hover, :active states with different grades of tokens.

So your initial default component could pass, but the SCSS would still fail because the active states are failing. I feel like this is important to note since we've seen many defaults passing on https://contrast-ratio.com


I wasn't able to reproduce the error on #3926 (comment)

Conclusion

These test cases reveal something interesting in the way we calculate grades in get-color-grade() (_functions.scss:1229). In some cases we expect 10, but with the additional 4.9 it ends up failing.

We need to audit get-color-grade() to get consistent results. And possibly functions like get-link-tokens-from-bg() so we can have better fallbacks.

@mejiaj mejiaj moved this from Sprint backlog to In progress in USWDS Sprint Board Jan 15, 2021
@jAigret-Bix jAigret-Bix added this to the USWDS 2.10.1 milestone Jan 25, 2021
@mejiaj mejiaj moved this from In progress to Development review in USWDS Sprint Board Jan 27, 2021
@mejiaj mejiaj moved this from Development review to Fed final review in USWDS Sprint Board Feb 5, 2021
@jAigret-Bix
Copy link

PR merged and closing issue 2/9

USWDS Sprint Board automation moved this from Fed final review to Done Feb 9, 2021
This was referenced Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug A problem in the code
Projects
Development

No branches or pull requests

4 participants