-
Notifications
You must be signed in to change notification settings - Fork 1k
USWDS - Disabled mixin: Test for color contrast #5455
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
Conversation
@@ -68,7 +68,6 @@ $close: map-merge( | |||
// Placeholder overrides to ensure color contrast compliance accross browsers | |||
&::placeholder { | |||
opacity: 1; | |||
color: color("disabled-dark"); |
There 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.
Browsers set the color for the placeholder via the -webkit-text-fill-color
above. Removing this color definition because it seems unnecessary.
@amyleadem Pulled in the latest and it should be ready for review! |
There 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.
Code looks good. I updated the summary to match the changelog statement.
I was able to test:
- Colors are set using
disable-lighter
(background) anddisable-dark
(text). - Warning appears on compile when contrast is not met.
Assigning PR to @amyleadem for review.
There 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.
Thanks for taking this on! Generally this looks good but I’ve flagged some issues in the list below. I consider some of these items to be out of scope and have added a note when I think they don’t need to be blockers for this PR. Curious to hear if you disagree!
- Confirm color contrast with light settings. Set all disabled settings to “gray-20”. Check contrast ratios for components:
- Button
- Button outline
- Outline buttons can be set to failing contrast ratios (For example, set $theme-color-disabled: “gray-20” and $theme-body-background-color: "white”)
- Character count
- Checkbox/radio
- Checkbox and radio can be set to failing contrast ratios (For example, set $theme-color-disabled: “gray-20” and $theme-body-background-color: "white”)
- Combo box
- Date picker
- The icon color ends up being inconsistent with other disabled icons when the fallback is in place. This is because the icon color is defined with opacity instead of a disabled color. This might not be avoidable without breaking changes, but wanted to flag the inconsistency. I don’t think this needs to be a blocker for this PR.
- The icon color ends up being inconsistent with other disabled icons when the fallback is in place. This is because the icon color is defined with opacity instead of a disabled color. This might not be avoidable without breaking changes, but wanted to flag the inconsistency. I don’t think this needs to be a blocker for this PR.
- File input
- Input mask
.usa-input-mask--content
is hard-coded to “gray-50”. Is it possible to run this through our contrast mixins as well?
- Input prefix/suffix
- Memorable date
- Range slider
- Select
- Text input
- Time picker
- Same issue as date picker. I don’t think it needs to be a blocker
- Confirm color contrast with dark settings. Set all disabled settings to “gray-80”. Check contrast ratios for components.
- All components failed to meet color contrast except checkbox/radio and button outline. I did receive appropriate warnings about the failures during compilation, though. For this reason, I think it meets requirements.
- Changing the text color ($theme-color-disabled-dark) to a light contrasting color against a dark background ($theme-color-disabled-lighter) fixes most of contrast issues, but the icons do not adjust, which can make them hard to see. I did not receive any warnings about this.
- I consider this a failure of the test, but to keep things in scope, I think we can address this later (if at all). Dark disabled colors seem to me to be more of an edge case, potentially used in dark mode designs(?). I am curious to learn how disabled colors are typically handled in dark modes. I don’t think this needs to be a blocker for this PR, but flagging it in case anyone else has thoughts.
- Check high contrast mode
There 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.
Thanks @amyleadem ! Here are my thoughts for each of your concerns:
Button outline
Outline buttons can be set to failing contrast ratios (For example, set $theme-color-disabled: “gray-20” and $theme-body-background-color: "white”)
This is related to my note about this only working for components with defined background colors. There’s no way for us to calculate the background color behind transparent element/
Important
These tests can only be conducted when background color is known. Elements with disabled colors without a defined background color are still at risk for breaking color contrast.
We could only assume the background color is the $theme-body-background-color
but if the button is placed in a different element/section with a different background, then the check would be invalid.
Consider our Sign In page examples where the background color is set to base-lightest
.
What do you think about setting a warning to check the color contrast when it would otherwise fail against $theme-body-background-color
?
Checkbox/radio labels
Checkbox and radio can be set to failing contrast ratios (For example, set $theme-color-disabled: “gray-20” and $theme-body-background-color: "white”)
Resolved in 9bb417c and 96a1c21
Additional notes in file comments.
Input mask
.usa-input-mask--content is hard-coded to “gray-50”. Is it possible to run this through our contrast mixins as well?
I believe this is possible using :has()
. Since the input and the span are siblings, we need some sort of check to see if the input is disabled. We could consider this as a progressive enhancement as Firefox has begun to support it.
.usa-input-mask {
display: block;
position: relative;
&:has(input:disabled) {
.usa-input-mask--content {
@include u-disabled;
// Needed to maintain input border. Bg color set by input.
background-color: transparent;
}
}
}
Alternatively, we could add a disabled class for usa-input-mask
or add styles via JS.
Component icons
Most of the icons are set via add-background-svg
which doesn’t check for color contrast.
We could consider replacing this function with add-color-icon
which can take an optional $contrast-bg
value and can swap the icon for its white variant.
Since these icons are not updated via the mixin or disabled theme colors, maybe we could consider this a potential enhancement beyond the scope of this PR.
What do you think?
&:disabled, | ||
&[aria-disabled="true"] { | ||
@include format-input { | ||
cursor: not-allowed; | ||
} | ||
@include format-label { | ||
cursor: not-allowed; | ||
} | ||
} | ||
|
There 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.
Duplicate styles found in _usa-checkbox.scss
.
Moved styles to checkbox-and-radio-colors.scss
There 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.
Note
It looks like format-input styles were not carried over, but I confirmed that there are no visual regressions after this consolidation.
&:disabled, | ||
&[aria-disabled="true"] { | ||
@include format-input { | ||
cursor: not-allowed; | ||
} | ||
@include format-label { | ||
color: color("disabled"); | ||
cursor: not-allowed; | ||
|
||
@media (forced-colors: active) { | ||
color: color(GrayText); | ||
} | ||
} | ||
} | ||
|
There 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.
Duplicate styles found in _usa-radio.scss
.
Moved styles to checkbox-and-radio-colors.scss
There 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.
Note
It looks like format-input styles were not carried over, but I confirmed that there are no visual regressions after this consolidation.
packages/uswds-core/src/styles/mixins/helpers/checkbox-and-radio-colors.scss
Outdated
Show resolved
Hide resolved
There 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.
This is looking good! Thanks for the thoughtful responses, @mahoneycm. I had one question about some checkbox/radio styles. I think my other concerns have been addressed. Here are my thoughts:
- Button outline - That explanation makes sense. With the transparent background color, I don’t see a way to automate this check. I think we’ll just have to move forward without a contrast check here.
- Checkbox/radio - This fix looks good. I confirmed the following:
- There are no visual regressions after consolidating duplicate styles
- I receive a warning when neither “disabled” nor “gray-50” meet contrast requirements
- Input mask - To keep things moving, I think we can consider this out of scope.
- Component icons - To keep things moving, I think we can consider this out of scope. I vote we open a follow-up issue to consider later.
I'd like to have @mejiaj confirm that the recent changes and discussions.
packages/uswds-core/src/styles/mixins/helpers/checkbox-and-radio-colors.scss
Show resolved
Hide resolved
packages/uswds-core/src/styles/mixins/helpers/checkbox-and-radio-colors.scss
Outdated
Show resolved
Hide resolved
&:disabled, | ||
&[aria-disabled="true"] { | ||
@include format-input { | ||
cursor: not-allowed; | ||
} | ||
@include format-label { | ||
cursor: not-allowed; | ||
} | ||
} | ||
|
There 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.
Note
It looks like format-input styles were not carried over, but I confirmed that there are no visual regressions after this consolidation.
&:disabled, | ||
&[aria-disabled="true"] { | ||
@include format-input { | ||
cursor: not-allowed; | ||
} | ||
@include format-label { | ||
color: color("disabled"); | ||
cursor: not-allowed; | ||
|
||
@media (forced-colors: active) { | ||
color: color(GrayText); | ||
} | ||
} | ||
} | ||
|
There 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.
Note
It looks like format-input styles were not carried over, but I confirmed that there are no visual regressions after this consolidation.
There 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.
No visual regressions found with disabled styles. No issues with code.
Added suggestions for improving documentation.
There 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.
Looks good to me!
There 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 was able to test different values in following settings:
$theme-color-disabled-family
$theme-color-disabled-lighter
$theme-color-disabled-light
$theme-color-disabled
$theme-color-disabled-dark
$theme-color-disabled-darker
Got a warning on colors that weren't meeting contrast.
Warning: Neither the specified preferred color token (`disabled-dark`) nor the fallback color token (`ink`) have AA contrast on a `disabled-lighter` background. Using `ink`. Please check your source code and project settings.
packages/uswds-core/src/styles/functions/color/get-color-token-from-bg.scss 65:7 get-color-token-from-bg()
packages/uswds-core/src/styles/mixins/helpers/set-text-from-bg.scss 14:28 set-text-from-bg()
uswds-core/src/styles/mixins/helpers/set-text-and-bg.scss 16:3 set-text-and-bg()
packages/uswds-core/src/styles/mixins/utilities/_disabled.scss 28:5 u-disabled()
packages/usa-textarea/src/styles/_usa-textarea.scss 9:5 @forward
packages/usa-textarea/src/styles/_index.scss 4:1 @forward
_index.scss 5:1 @forward
_index.scss 22:1 @forward
packages/uswds/_index.scss 51:1 @forward
src/stylesheets/uswds.scss 3:1 root stylesheet
Re: input mask
I noticed that issue too and confirmed the value is hardcoded. We should look into ways to improve this in a follow-up issue.
Hey there, @Miskeyceh94 Did you mean to leave that comment tagging #5372 ? Was there any additional context you wanted to leave? Thanks! |
🥴
Pada Sel, 27 Feb 2024 07.47, Dan O. Williams ***@***.***>
menulis:
… Merged #5455 <#5455> into develop.
—
Reply to this email directly, view it on GitHub
<#5455 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BA3DLOASI3DLPFOTQX4I7RDYVUNJJAVCNFSM6AAAAAA3Y2TR4SVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRHEZDSMBXGUZTCMA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Summary
Added color contrast checks for disabled tokens. On compilation, USWDS will test disabled element color contrast and provide a fallback color if minimum contrast is not met. If the fallback does not meet minimum requirements, a warning will be provided.
Breaking change
This is not a breaking change.
Related issue
Closes #5427
Related pull requests
Changelog PR →
Preview link
Disabled form elements →
Button outline →
Button outline inverse →
Problem statement
#5402 normalized disabled tokens for users. Users could unintentionally set the tokens to colors with poor contrast, causing disabled elements to lose the necessary context.
Solution
Utilize
set-bg-and-color()
mixin which includes a contrast check, fallback colors, and warning message if fallback colors are unable to resolve contrast issue.Important
These tests can only be conducted when background color is known. Elements with disabled colors without a defined background color are still at risk for breaking color contrast.
More information in Further Considerations below
Testing and review
Color contrast failsafe testing:
By default,
u-disabled
is set to the following:To test, we can update these values to have non-compliant color contrast
_settings-color.scss
$theme-color-disabled-dark
to match$theme-color-disabled-lighter
gray-40
http://localhost:6006/?path=/story/patterns-forms--disabled-form-elements
).ink
fallback$theme-disabled-lighter
to a darker color than$theme-disabled-lighter
gray-60
Testing checklist
ink token
Further Considerations
Toggle
Unknown background colors
For elements with an undefined background color, we could use
set-text-from-bg
and assume$theme-body-background-color
as the color to test againstThis would work for (potentially) most cases but is a big assumption.
It could benefit components like checkbox labels or outline buttons, but if those elements are placed in an area with a different background color, we would be testing against the wrong color.
Food for thought. Maybe we can discuss ways to enhance these color contrast tests further down the line.
No mixin parameters used
Now that we've removed the
u-disabled
parameters from_usa-button
, no styles use the optional parameters and the default is used across the board. Its probably fine to maintain the current functionality of the parameters, but flagging in case we want to remove.Secondary fallback value
I’d like to investigate a secondary fallback for contrast checks.
Currently, if the
$preferred-color-token
fails contrast checks, it uses the$preferred-fallback-token
which isink
by default.When that token fails the contrast check, we still use the token but provide a warning in the build pipeline.
I’d like to further investigate adding a secondary fallback to test white against the background color. In my mind, if black still fails color contrast, the next reasonable test would be to use white rather than leave black on a dark background.
We do something similar specifically when the background grade is
45
This will probably be a second issue/enhancement