-
Notifications
You must be signed in to change notification settings - Fork 1k
USWDS - Accordion: Create more consistent high contrast styles #5286
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
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.
@mahoneycm This is looking good!
I plugged these changes into the accordion background color PR (#5269) and tested both “light” and “dark” colors (as defined by is-color-dark
) for $theme-accordion-background-color
, $theme-accordion-button-background-color
, and $theme-input-background-color
.
Then in Chrome’s forced-colors: active
emulator, I confirmed the following:
- Confirmed appearance of accordion in initial state
- Confirmed visuals of accordion in hover state
- When
$theme-accordion-button-background-color
is set to a dark color (like “green-60”), there is an issue with the alignment of accordion icons on hover:
- When
- Confirmed visuals of accordion in focus state
Curious - is there any sense in making the approach for adding the icons consistent for both HC mode (pseudo element) and standard mode (background-image)? I can totally understand if it just doesn't make sense, just wanted to ask.
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 looked really good to me, I appreciated the thick outline. We may want to revisit it in WCAG 2.2 as focusing/visibility is even more important and has new criteria guidelines.
@amyleadem The reason we do the high contrast version as a pseudo-element is that the mask would cover up content within the element, in this case, the button text. The mask allows us to set the color of the icon using the system token Do you think it'd be a good idea to look into moving the standard background image into a pseudo-element to keep them a little more consistent? Any idea why this wouldn't be a good idea? CC: @mejiaj |
… cm-accordion-hc-w-color-token
@mahoneycm Thanks for explaining. Off the top of my head, I don't have any reasons why this would be a bad idea and would be interested in the discussion. Maybe we can flag it as a follow-up discussion item? |
I merged in #5269 and resolved the hover bug Amy mentioned above! I changed the merge target to |
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.
Hi @mahoneycm,
I pulled in the jm-accordion-background
branch and did another round of visual testing. I see that the icons are not quite aligned on hover (the icon shifts by about half a pixel on my monitor). To simplify things - would it be possible to remove the background image when in forced-color mode?
… cm-accordion-high-contrast-fix
@amyleadem Can you try now and let me know if you're still seeing this issue? There were hover styles being applied that seem to be absent from Jame's accordion PR and develop. I removed them and all seems to be well now! |
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.
Works perfectly with the most recent changes
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.
Looking good, but found some issues with banner and nav. Can you take a look?
border: $border-high-contrast; | ||
position: relative; | ||
|
||
&::after { |
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 pulled in the latest changes from jm-accordion-background
and noticed that this update results in similar issues with banner and nav that #5269 had, as detailed in this comment. It might be necessary to manually remove the icon from banner and header nav, similar to what was done in 95de5f9.
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 resolve this by changing the accordion high contrast icon from an ::after
pseudo element to ::before
and then setting the display to none on banner and nav components.
Do you see an issue with resolving it this way?
uswds/packages/usa-nav/src/styles/_usa-nav.scss
Lines 212 to 225 in 4ed6bb7
// Remove icon set from usa-accordion. | |
// Also removed in _usa-banner.scss. | |
&[aria-expanded="false"], | |
&[aria-expanded="false"]:hover, | |
&[aria-expanded="true"], | |
&[aria-expanded="true"]:hover { | |
background-image: none; | |
@media (forced-colors: active) { | |
&::before { | |
display: none; | |
} | |
} | |
} |
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 this sounds reasonable
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.
Switched to content: none
as mentioned by James!
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.
Using :before
in place of :after
here makes sense to me since it will minimize interaction with the :after
pseudo elements on in banner and mobile nav.
… cm-accordion-high-contrast-fix
This resolves visual regression bugs on nav and banner components
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.
@mahoneycm, no issues testing the icons in Windows High Contrast Mode. I tested several dark/light themes.
Just a question and a suggestion for improving clarity of accordion icon map.
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 looks good to me.
- Confirmed appearance of accordion, banner, and header in emulated forced-colors mode (Chrome, dark theme)
- Tested at multiple browser widths
Summary
Updates accordion high contrast styles for increased consistency and to ensure visibility
Breaking change
This is not a breaking change.
Related issue
Closes #5285
Related pull requests
Change log PR →
Preview link
Accordion →
Problem statement
Existing high contrast accordion styles do not guarantee appropriate color contrast in
forced-colors
modesSolution
forced-color-adjust: none
add-color-icon()
to place icons usingButtonText
system tokenTesting and review
Instructions below are for Chrome on Mac.
Emulate CSS media feature forced-colors
forced-colors: active
prefers-color-scheme
emulation and test high contrast in light and dark themesScreenshots
Before:
High contrast dark themeHigh contrast light theme

High contrast with updated accordion colors (Low contrast between button and background)

After:
High contrast dark theme:

High contrast light theme:

Before opening this PR, make sure you’ve done whichever of these applies to you:
git pull origin [base branch]
to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch isdevelop
).npm run prettier:sass
to format any Sass updates.npm test
and confirm that all tests pass.