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

Contrast calculation for identifier inaccurate #3906

Closed
stphnwlkr opened this issue Dec 20, 2020 · 12 comments · Fixed by #3949
Closed

Contrast calculation for identifier inaccurate #3906

stphnwlkr opened this issue Dec 20, 2020 · 12 comments · Fixed by #3949
Assignees
Labels
Type: Bug A problem in the code
Milestone

Comments

@stphnwlkr
Copy link

stphnwlkr commented Dec 20, 2020

When compiling the sass I get an error stating "Neither "primary" nor "base-lightest" can be AA contrast links and hovers on a "base-darkest" background." This is driven by the link settings in _uswds-theme-colors. Using the default works because "white" is ignored, but any changes cause this error. Below are the setting and clearly the difference between 90 and 5 or 30 is more than 50.

$theme-color-base-lightest: "gray-5";
$theme-color-base-darkest: "gray-90";
$theme-identifier-background-color: "base-darkest";

// Links
$theme-link-color: "primary";
$theme-link-visited-color: "violet-70v";
$theme-link-hover-color: "primary-dark";
$theme-link-active-color: "primary-darker";
$theme-link-reverse-color: "white";
$theme-link-reverse-hover-color: "gold-30";
$theme-link-reverse-active-color: "white";

Using Visual Studio Code and Live Sass Compiler.

I believe the issue may be that set-link-from-bg() doesn't actually test the fallback colors.

@stphnwlkr stphnwlkr added the Type: Bug A problem in the code label Dec 20, 2020
@thisisdano
Copy link
Member

thisisdano commented Dec 21, 2020

Let me check — but I think this is because the checker thinks there isn't sufficient difference between the base-lightest link and its hover, white. I will check. Does it compile if base-lightest is set to gray-10? I could, perhaps, update the rule to accept differences of 5 for hover states.

@thisisdano
Copy link
Member

Perhaps what the system should do is always, when possible, use the background color at grade 10 for reverse links.

@stphnwlkr
Copy link
Author

I am not sure why you would test against a grade 10 color for reverse. The error is giving results that are not based on the actual setting - I don't have base-lightest anywhere. I would think it should be testing the link colors - normal and reverse - relative to the theme-identifier-background-color and if they both fail, throw the error. If not, there would need to be a way to set whether to use a light or dark identifier.

@sawyerh
Copy link
Contributor

sawyerh commented Jan 5, 2021

I'm running into this too after upgrading to 2.10.0. Not entirely following the above, so not sure how to debug it. Happy to provide more details if there are specific things that are helpful. Full error I see:

error - ./styles/app.scss (./node_modules/next/node_modules/css-loader/dist/cjs.js??ref--5-oneOf-7-1!./node_modules/next/dist/compiled/postcss-loader/cjs.js??ref--5-oneOf-7-2!./node_modules/resolve-url-loader??ref--5-oneOf-7-3!./node_modules/next/node_modules/sass-loader/dist/cjs.js??ref--5-oneOf-7-4!./styles/app.scss)
SassError: 'Neither "ink" nor "base-lighter" can be AA contrast links and hovers on a "emergency" background.'
   ╷
7  │     $link-tokens: get-link-tokens-from-bg(
   │ ┌─────────────────^
8  │ │     $bg-color,
9  │ │     $preferred-link-color,
10 │ │     $fallback-link-color,
11 │ │     $wcag-target
12 │ │   );
   │ └───^
   ╵
  node_modules/uswds/dist/scss/core/mixins/_set-link-from-bg.scss 7:17     set-link-from-bg()
  node_modules/uswds/dist/scss/core/mixins/_alert-status-styles.scss 25:7  alert-status-styles()
  node_modules/uswds/dist/scss/components/_alerts.scss 83:5                @import
  node_modules/uswds/dist/scss/packages/_uswds-components.scss 25:9        @import
  node_modules/uswds/dist/scss/uswds.scss 13:9                             @import
  styles/app.scss 23:9                                                     root stylesheet

@thisisdano
Copy link
Member

OK — I think the issue here is that the link color calculator doesn't respect the value of $theme-link-reverse-hover-color and wants to set a hover that's 10 grade units lower than $theme-link-reverse-color (or the specified link color) for reverse links on some components (like identifier and site alert). This means that you'll see errors when you have a value of $theme-link-reverse-color that's less than grade 10.

For now a fix could be to set $theme-link-reverse-color to something like gray-10. This means that the link color calculator would be able to set the hover to white for reverse links.

@sawyerh — I think you could change the color of $theme-color-emergency to a color that's 50 grade units from the value of ink... possibly something like gold-30v. This would cause the system to use ink for the link color in the site alert (instead of $theme-link-reverse-color).

These are just patches, though... I'll need to work through what the proper system fix should be.

Let me know if either of these work, and I'll try to make progress on a real fix.

@sawyerh
Copy link
Contributor

sawyerh commented Jan 6, 2021

Thanks @thisisdano. Those didn't do the trick. In case it's helpful for working through the system fix, here's what I get when trying those two patches:

When setting $theme-color-emergency to gold-30v (and a few other variations like gold-10):

SassError: "`ink-dark` is not a valid USWDS color token. See designsystem.digital.gov/design-tokens/color for more information."
   ╷
28 │     color: color($hover-token);
   │            ^^^^^^^^^^^^^^^^^^^
   ╵
  node_modules/uswds/dist/scss/core/mixins/_set-link-from-bg.scss 28:12    set-link-from-bg()
  node_modules/uswds/dist/scss/core/mixins/_alert-status-styles.scss 25:7  alert-status-styles()
  node_modules/uswds/dist/scss/components/_alerts.scss 83:5                @import
  node_modules/uswds/dist/scss/packages/_uswds-components.scss 25:9        @import
  node_modules/uswds/dist/scss/uswds.scss 13:9                             @import
  styles/app.scss 23:9                                                     root stylesheet

When setting $theme-link-reverse-color: 'gray-10'; I get the error in my original comment above.

@thisisdano
Copy link
Member

OK, thanks. I'll dig into this. Now I need to understand why this didn't come up as we tested this, since we'd tested nearly these exact cases. More to come!

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

mejiaj commented Jan 12, 2021

@sawyerh can you share your base, primary, and link colors(_uswds-theme-colors.scss:133) ?

I didn't get any issues with:

$theme-color-emergency: "gold-30v";
$theme-link-reverse-color: "gray-10";

Tried with gold-10 too.

@sawyerh
Copy link
Contributor

sawyerh commented Jan 12, 2021

@mejiaj Our project isn't themeing the link colors, but the base/primary are below:

// Base 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;

// Primary colors
$theme-color-primary-lighter: #e8eef4;
$theme-color-primary-light: #8aaac7;
$theme-color-primary: #14558f;
$theme-color-primary-dark: #0e3c64;
$theme-color-primary-darker: #0a2b48;

@mejiaj
Copy link
Contributor

mejiaj commented Jan 12, 2021

@sawyerh thanks for that. Sharing my findings, still working a proper fix.

It seems like custom ink grade isn't being set properly.

Narrowed it down to:

// _uswds-theme-color.scss
$theme-color-base-darkest: #141414;

// _alert-status-styles.scss:26
@include set-link-from-bg($bgcolor, "ink", "base-lighter");

Possible fixes:

  • Use a token for $theme-color-base-darkest
  • Patch "ink" in alert styles from @include set-link-from-bg($bgcolor, "ink", "base-lighter"); to "base-darkest"
  • Patch $custom-family in _functions.scss so it defaults to base family if ink

I'm sorry about these issues and we'll be more conscious about these settings moving forward.

@sawyerh
Copy link
Contributor

sawyerh commented Jan 12, 2021

@mejiaj Thanks! The following workaround worked for me:

$theme-color-base-darkest: "gray-90";
$theme-color-emergency: "gold-30v";
$theme-link-reverse-color: "gray-10";

@mejiaj
Copy link
Contributor

mejiaj commented Jan 13, 2021

Given the these custom base values:

$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;

And trying to set a #dcdcdc link on a #9c3d10 background. I get the following error:

Error in plugin "sass"
Message:
    src/stylesheets/core/mixins/_set-link-from-bg.scss
Error: 'Neither "ink" nor "base-lighter" can be AA contrast links and hovers on a "emergency" background.'

If I check the color contrast in this contrast-ratio app ↗.
It passes with a score of 4.95. It's next lightest value #f2f2f2 passes with 6.07

I had thought the fallback link color variable wasn't working in get-link-tokens-from-bg(). However in calculating the luminance values it seems like it's working as intended(?).

Trying #dcdcdc in our luminance() we get a value of 0.709. Table of ranges below:

grade minimum luminance maximum luminance
0 1.000 1.000
5 0.850 0.930
10 0.750 0.820
20 0.500 0.650
30 0.350 0.450
40 0.250 0.300
50 0.175 0.183
60 0.100 0.125
70 0.050 0.070
80 0.020 0.040
90 0.005 0.015
100 0.000 0.000

From https://designsystem.digital.gov/design-tokens/color/overview/#magic-number


Looking into how $link-grade is calculated with custom values and why it's returning false for both $link-token and $hover-token. I think is is also related to #3926.

@jAigret-Bix jAigret-Bix modified the milestones: USWDS 2.8.1, USWDS 2.10.1 Jan 25, 2021
@mejiaj mejiaj moved this from In progress to Development review in USWDS Sprint Board Jan 27, 2021
@mejiaj mejiaj linked a pull request Jan 28, 2021 that will close this issue
@mejiaj mejiaj moved this from Development review to Fed final review in USWDS Sprint Board Feb 5, 2021
USWDS Sprint Board automation moved this from Fed final review to Done Feb 5, 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

Successfully merging a pull request may close this issue.

5 participants