-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Update focus outline color to meet WCAG 2.2 contrast standards #6854
Conversation
Generate changelog in
|
This comment was marked as outdated.
This comment was marked as outdated.
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 feels like a borderline breaking change, since it has the potential to make UI snapshots fail. I don't think it could cause some cases to have worse contrast because the current focus color already does not have enough contrast with the extremes of the color spectrum
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as outdated.
This comment was marked as outdated.
// this should become default in bp6, providing new behavior as a class for now to avoid breaking changes | ||
// to focus colors | ||
/* stylelint-disable-next-line @blueprintjs/no-prefix-literal */ | ||
.bp5-focus-enhanced-contrast & { | ||
outline: $pt-focus-indicator-color solid 2px; | ||
} | ||
/* stylelint-disable-next-line @blueprintjs/no-prefix-literal */ | ||
.#{$ns}-dark.bp5-focus-enhanced-contrast & { | ||
outline-color: $pt-dark-focus-indicator-color; | ||
} |
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 think we should just do it. I wouldn't call this a breaking change. If we do introduce this class, you should use #{$ns}
instead of bp5
, which is what this lint rule is trying to tell you.
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.
Okay cool, I wasn't sure the history of how careful we've been with minor things like this that could be viewed as a break.
Ack on the prefix - I hard coded bp5 since this was only intended to work for the bp5 version and removed in bp6, but will just remove this.
This comment was marked as outdated.
This comment was marked as outdated.
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.
TODO:
-
Audit intent color contrast,
pt-input-intent
andpt-dark-input-intent
- $pt-intent-colors
- $pt-dark-input-intent-box-shadow-colors
-
Audit focus outline of all components (planning to leave out of this PR) - a few more here but highlighting some main ones:
- [-]
Card
no focus state - [-]
FileInput
no visual focus state - [-]
Tree
items cannot be focused - [-] Tag focus outline does not have offset like other components
- [-]
-
[-] Check on history of why components that appear like text inputs have a shadow focus style while other components have offset outline style
- For now, went with an option that retains
box-shadow
for text inputs, but styled in a way that makes it look closer to a solid outline. I figured this reduces chances of regressions since this will just change the color/opacity used but not the method of adding a focus indicator for these components.
- For now, went with an option that retains
-
Consider Consider using
:focus-visible
instead of:focus
for styling focus states #6076 along with this PR, or as a follow up in the same release- This was already tagged a breaking change - we could consider at the next major version bump but not needed for now.
// this should become default in bp6, providing new behavior as a class for now to avoid breaking changes | ||
// to focus colors | ||
/* stylelint-disable-next-line @blueprintjs/no-prefix-literal */ | ||
.bp5-focus-enhanced-contrast & { | ||
outline: $pt-focus-indicator-color solid 2px; | ||
} | ||
/* stylelint-disable-next-line @blueprintjs/no-prefix-literal */ | ||
.#{$ns}-dark.bp5-focus-enhanced-contrast & { | ||
outline-color: $pt-dark-focus-indicator-color; | ||
} |
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.
Okay cool, I wasn't sure the history of how careful we've been with minor things like this that could be viewed as a break.
Ack on the prefix - I hard coded bp5 since this was only intended to work for the bp5 version and removed in bp6, but will just remove this.
@@ -25,6 +25,10 @@ $pt-app-elevated-background-color: $light-gray4 !default; | |||
$pt-dark-app-elevated-background-color: $dark-gray3 !default; | |||
|
|||
$pt-outline-color: rgba($blue3, 0.6) !default; |
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.
Not changing this color because of the comment at the top of this file stating that is generally considered a breaking change. Perhaps we want to make an exception here...
commentBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
@@ -25,6 +25,11 @@ $pt-app-elevated-background-color: $light-gray4 !default; | |||
$pt-dark-app-elevated-background-color: $dark-gray3 !default; | |||
|
|||
$pt-outline-color: rgba($blue3, 0.6) !default; | |||
// 0.752 opacity to meet WCCAG 2.2 minimum contrast guidelines with all light-gray backgrounds in light theme | |||
// and all dark-gray backgrounds in dark theme | |||
$pt-focus-indicator-color: rgba($blue2, 0.752) !default; |
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.
$blue2 on $white - 3.77:1 contrast
$blue2 on $light-gray1 - 3:1 contrast
// and all dark-gray backgrounds in dark theme | ||
$pt-focus-indicator-color: rgba($blue2, 0.752) !default; | ||
// dark theme opacity could be less and still meet minimum contrast, but keep 0.752 to stay consistent | ||
$pt-dark-focus-indicator-color: rgba($blue5, 0.752) !default; |
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.
$blue5 on $black - 5.77:1 contrast
$blue5 on $dark-gray5 - 3.39:1 contrast
for comparison - $blue4 on $dark-gray5 - 2.24:1 contrast
@@ -15,14 +15,14 @@ $input-color-disabled: $button-color-disabled !default; | |||
$input-placeholder-color: $pt-text-color-muted !default; | |||
$input-background-color: $white !default; | |||
$input-background-color-disabled: rgba($light-gray1, 0.5) !default; | |||
$input-shadow-color-focus: $pt-intent-primary !default; | |||
$input-shadow-color-focus: $pt-focus-indicator-color; |
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.
if this can't be relied on externally - remove and use $pt-focus-indicator-color
directly in the handful of places it's used. Same for dark theme version below.
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.
it looks like we explicitly distribute _colors.scss
, _color-aliases
and _variables
, but not this file. could consider updating to use $pt-focus-indicator-color
directly
"dist:variables": "generate-css-variables --retainDefault true ../../colors/src/_colors.scss common/_color-aliases.scss common/_variables.scss",
input focus styleBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Add generated changelog entriesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
inset border-shadow(1, $color, 1px), | ||
border-shadow(0.3, $color, 2px); | ||
inset border-shadow(0.752, $color, 1px), | ||
border-shadow(0.752, $color, $outer-shadow-focus-width); |
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.
border-shadow width stayed the same in the case of intent shadows (since they already start with 1px in un-focused state, the focus state is a 3px shadow outline)
for non-intent inputs, the shadow width reduced by 1px. I expect this to mean that shadows that worked in previous situations will continue to work (ex there won't be clipping introduced from something like a new outline offset or wider shadow)
@@ -197,7 +197,6 @@ $control-indicator-spacing: $pt-grid-size !default; | |||
|
|||
input:focus ~ .#{$ns}-control-indicator { | |||
@include focus-outline(); | |||
outline: $blue3 solid 2px; |
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.
from what I can tell this was just to provide increased contrast, no reason to not use the consistent focus-outline
style now
@@ -632,10 +631,6 @@ $control-indicator-spacing: $pt-grid-size !default; | |||
background-color: $dark-control-background-color-hover; | |||
} | |||
|
|||
input:focus ~ .#{$ns}-control-indicator { | |||
outline: $blue5 solid 2px; |
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.
same comment as line 200 in this file
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.
colors have changed - but not the method of how the focus indicator is applied (2px offset 2px outline) so I expect minimal chance of breaking changes
input group not form groupBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Compare public docs vs latest "documentation" link from latest @svc-palantir-github comment Design review checklist:
|
Looks good! Some comments and questions below: Comments
Questions
|
Ah great find on the Since this is already a delicate change to get released, I'd strongly prefer to track all of the border overlap and other issues noted separately, but am happy to continue to work on them, thanks for flagging explicit examples! For this PR I want to keep a close eye on making sure we don't have any regressions, but would expect existing bugs to remain in place for now. If any of the bugs are particularly bad in your opinion, and perhaps become even more obvious with the increased contrast, I could fix as needed as pre-requisites to this PR. My thinking on moving to a solid border color is that we need to get much darker and 2px width to meet minimum contrast standards, and having 0.752 opacity right next to 1 opacity looks a little off in a way the previous light shadow didn't. |
Sounds good!
I don't think these bugs are particularly blocking, so I think it is ok to tackle this in a FLUP PR so you can get these changes in.
Makes sense, I think the solid border also looks cleaner! |
From the demo app it looks like there's some custom toast focus styles that look like they don't have high contrast current contrast bumping $white focus opacity to 0.75 from 0.5: updating toast focus color to match text color with 0.75 opacity |
I'm wondering if we're conflating focus and selection styles and may be anchoring on the focus state because that's what this PR is changing. FWIW that style is not reused and is unique to There is an argument to be made that the current selection state already follows the current focus state, so if we're updating one we should update the other. But I could also see this staying the same, since from what I've read so far this is a guideline for focus state which is more transient than a selection state and needs to grab the users attention. |
increase toast focus contrastBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Toast focuses look good! Yeah agreed, I don't want to over-index on the control card selection state since it is different. After mocking up the 2px border for the selection state on the control card, I do like visually how the solid border looks – it is almost more attention-grabbing compared to the current. I'd be curious to get some more thoughts from the design team here, but this shouldn't be blocking for this PR! |
Let me know if you end up wanting to change selected styles and we can make sure there's at least a ticket open to track |
Merge branch 'develop' into evanj/focus-indicatorBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
@@ -47,7 +47,7 @@ $toast-intent-colors: ( | |||
|
|||
&:focus { | |||
// blue outline color shows poorly on colored bg | |||
outline-color: rgba($white, 0.5); | |||
outline-color: rgba($text-color, 0.75); |
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.
Was this intentional? Seems odd to use the text color for the outline.
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 left an exploration of this here: #6854 (comment)
The text color is what allows us to easily flip to a dark outline when rendering against a light color background. Alternate option to define a separate focus color variable, but I'm not sure what we'd really choose differently - these are already against intent backgrounds so it's a bit of a weird case.
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'm not saying the color itself is weird, and choosing the same color seems fine. It's just odd to use the variable for text color for something that isn't text.
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.
ah yes - updated to $text-and-focus-color
, didn't think of anything good to combine them generically, ex $content-color
doesn't sound right
Sorry, I missed this last week! Will let you know if we want to make a change here and FLUP as needed over Slack. |
more descriptive var nameBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes no issue
Checklist
Changes proposed in this pull request:
Update color of focus outline to meet 3:1 contrast standard for UI component focus states
https://webaim.org/resources/contrastchecker/
Reviewers should focus on:
$white
/$black
?WCAG Guidelines
From what I can tell, WCAG did not require minimum contrast in 2.1, but in 2.2 has a section on focus appearance which specifies the 3:1 minimum ratio
Screenshot
Current:
This PR: