Skip to content

USWDS: Button - Forced colors outline #5147

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

Merged
merged 25 commits into from
May 10, 2023

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Feb 13, 2023

Summary

This PR adds an outline to buttons in forced color mode for added clarity and consistency with default styles. It also replaces repetitive CSS with a new $border-high-contrast variable

Breaking change

This is not a breaking change

Related issue

Closes #5099

Preview link

Buttons Preview →

Problem statement

Currently, buttons in forced-colors modes do not have an outline or any styling to appear like buttons. Instead, it just looks like regular text unless focused.

Solution

Define an outline within forced colors mode. Other components utilize similar styling in forced colors mode, so I've created a mixin and included in on usa-button

Previously we set high-contrast outlines via outline: 2px solid transparent because it would not affect the border outside of high-contrast modes. Since we are using the forced-colors media query for all high contrast styles, we can adjust the border without affecting components in normal circustances. Thus, high contrast borders have been turned into a sass variable and made uniform across our components

Testing and review

  1. View the Button story preview
  2. In developer tools, click the 3-dot menu button and select "Rendering" under more tools
  3. Scroll down to Emulate css media feature forced-colors and select `Forced-colors: active"
  4. Verify that buttons have an outline making them appear as buttons
  5. Compare Checkbox, Radio, Search, Range, and Date Picker components to the develop build in high contrast mode to ensure no style change as a result of switching to outline mixin.

Before opening this PR, make sure you’ve done whichever of these applies to you:

  • Confirm that this code follows the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
  • Run npm run prettier:scss to format any Sass updates.

@amyleadem amyleadem assigned amyleadem and unassigned amyleadem Feb 24, 2023
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Hi @mahoneycm, thanks for taking this on. I like that you created a shared value for this. I had some concerns primarily about how this fix affects the focus state outline, so I added some questions in the comments.

Additionally, this might be out of scope, but I do not see a focus outline on usa-date-picker__button in the forced colors mode. Just wanted to flag that for now.

Let me know if you have any questions.

@mahoneycm mahoneycm requested a review from amyleadem February 28, 2023 21:41
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Looking good! I found a couple visual changes and flagged them in comments. I also think we should move the $border-high-contrast variable into another file. Let me know if you have questions!

- Moves and renames -high-contrast to -high-contrast-border
- Switches HC range slider border to outline to resolve visual discrepency
- Switches radio & checkbox border to outline to resolve visual discrepency
- Removes extra line
@mahoneycm mahoneycm requested a review from amyleadem March 22, 2023 14:29
@mahoneycm
Copy link
Contributor Author

@amyleadem Great catches! Changes have been implemented in 98005ed

I believe range slider, radio, and checkbox were originally switched to borders when I was attempting to use a mixin for the high contrast border. Now that it's just a variable, we can have allow these to remain outline while still using the same variable as the other components.

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Very close! I am happy this issue is getting fixed. I had just a couple notes about date picker and the settings page. Let me know if you have questions.


/*
----------------------------------------
High Contrast
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it would be helpful to specify that this is for High contrast mode, possibly with a short description explaining HC mode/CSS forced-color mode is and how to activate it.

Suggested change
High Contrast
High contrast mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in b2f35e6

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Approved. Thanks for all your work on this!

- We did this to prevent users from breaking contrast compliance
- We may be able to move this back after providing high contrast documentation to users
@mahoneycm mahoneycm requested a review from amyleadem March 30, 2023 15:52
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Smart fix to move this into /variables. I like it. I really appreciate that you included the reasoning in the commit as well.

One small thing I did notice when I opened the preview is that the border on the disabled buttons disappears on hover when forced-colors are active. Would you be able to fix that here or open a fast-follow issue?

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Don't forget to run prettier. Added suggestion in comments, otherwise looks good.

@mahoneycm
Copy link
Contributor Author

@amyleadem It came from the border: 0 in button-disabled various state styles.

&:hover,
&.usa-button--hover,
&:active,
&.usa-button--active,
&:focus,
&.usa-focus {
background-color: color("disabled");
border: 0;
box-shadow: none;
}

I removed it and it doesn't seem to change any styles outside of forced-colors. Hover/focus/active states for disabled and aria-disabled buttons appear the same as before removing.

@mahoneycm mahoneycm requested review from mejiaj and amyleadem March 30, 2023 20:33
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Confirmed that removing border: 0 fixed the forced-colors issue and did not have a negative effect in the default presentation.

Confirmed in Chrome, Safari, Edge, Firefox.

@thisisdano
Copy link
Contributor

thisisdano commented May 8, 2023

I'll draw up a changelog PR for this change.
Changelog PR: uswds/uswds-site#2096

@thisisdano thisisdano merged commit f39d0af into develop May 10, 2023
@thisisdano thisisdano deleted the cm-button-forced-colors-outline branch May 10, 2023 21:19
@thisisdano thisisdano mentioned this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USWDS - Feature: Add outline to buttons in forced-colors mode
4 participants