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.

Sign up for GitHub

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

Jump to bottom

fix(picker): high contrast and other color fixes #2326

Merged
merged 7 commits into from
Dec 14, 2023
Merged

fix(picker): high contrast and other color fixes #2326

merged 7 commits into from
Dec 14, 2023

Conversation

jawinn
Copy link
Collaborator

@jawinn jawinn commented Nov 30, 2023 •
edited by mdt2
Loading

Description

Picker: A few fixes for forced-colors/Windows High Contrast Mode to address the bugs in CSS-636, along with a few more discovered when working on it. Along with some cleanup work.

  • The focus indicator ring now only shows on keyboard focus. It was showing all the time by default.
  • The color of the focus indicator and the border are no longer reversed. This now matches the visual behavior that was decided upon for Text field, where the indicator is blue and the border color inside it is the default grey (keyboard focused).
  • The hover Highlight border color now appears on hover (except when keyboard focused).
  • Proper matching foreground/background pairs are now used for the system colors defined. The ButtonText foreground needed a ButtonFace background to ensure appropriate contrast. The ButtonBorder system color is also now used.
  • Some cleanup of a few things including removing some unnecessary declarations, organizing some styles, and slimming down the list of highcontrast custom properties as a lot were unnecessary and repeating the same values.

In this work, I discovered and fixed these additional issues (not WHCM):

  • Fixes invalid red border color appearing on docs example "Closed and Disabled with Thumbnails", on hover.
  • Fixes issue with dark grey background color appearing on hover for quiet picker (open + side label).

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

  • WHCM: Focus indicator ring only shows when you tab focus with the keyboard, and is the Highlight color
  • WHCM: When the focus indicator is visible, the border color is the same as the default and not Highlight
  • WHCM: The border color shows as Highlight when hovering over the picker, except when the focus indicator is visible
  • On docs example "Closed and Disabled with Thumbnails", hover does not show a red border (or any change in border color)
  • Hover on quiet picker, including w/ open & side label on the docs, does not show a grey background

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

  • I have read the contribution guidelines.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 30, 2023 •
edited
Loading

🚀 Deployed on https://pr-2326--spectrum-css.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 30, 2023 •
edited
Loading

File metrics

Summary

Total size: 3.97 MB*
Total change (Δ): ⬆ 1.68 KB (0.04%)

Package Size Δ
picker 31.19 KB 🚨 deleted, moved, or renamed
Details

picker

File Size Base Δ
Total 30.64 KB 31.19 KB ⬆ 0.57 KB (1.82%)
index-base.css 28.75 KB 29.31 KB ⬆ 0.57 KB (1.94%)
index-theme.css 2.46 KB 2.46 KB -
index-vars.css 30.64 KB 31.19 KB ⬆ 0.57 KB (1.82%)
index.css 30.64 KB 31.19 KB ⬆ 0.57 KB (1.82%)
mods.json 2.71 KB 2.71 KB -
themes/express.css 1.47 KB 1.47 KB -
themes/spectrum.css 1.01 KB 1.01 KB -
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@jawinn jawinn changed the title fix(picker): high contrast mode fixes fix(picker): high contrast and other color fixes Dec 4, 2023
@jawinn jawinn force-pushed the jawinn/css-636-fix-picker-whcm branch 3 times, most recently from a590ca4 to b166d19 Compare December 5, 2023 19:54
mdt2
mdt2 approved these changes Dec 6, 2023
View reviewed changes
Copy link
Collaborator

@mdt2 mdt2 left a 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!

@jawinn
fix(picker): whcm - fix focus indicator and border colors
7ce46c3 
High contrast mode fixes for Picker. By default, the focus indicator
outline was showing on the picker when it did not have focus. The colors
were also reversed; these now mirror the focus indicator color and
border color combination shown in textfield when keyboard focused. Also
fixes some other high contrast colors that were a little off on hover
and when the picker was open.
@jawinn
fix(picker): whcm - use matching background-foreground pair
19e63ac 
The foreground ButtonText needs to be paired with the matching
background pair ButtonFace to ensure appropriate contrast (per W3C
system colors spec).
@jawinn
fix(picker): whcm cleanup and fixes
717b93d 
- Use more appropriate ButtonBorder system color.
- Make sure quiet variation has a matching background-foreground pair
  instead of having a transparent background with ButtonText foreground
  to ensure appropriate contrast.
- Fix extra text backplate showing in WHCM edge default theme.
- Simplify disabled border color by moving to a custom property.
- Remove unnecessary repeated border-width declarations that were
  already inherited.
@jawinn
refactor(picker): whcm - prune redundant custom properties
e41ef09 
There were a lot of unnecessary highcontrast specific custom properties
that were all repeating the same values. Refactors these to slim them
down significantly, reducing the number of custom properties and making
the WHCM styles much more readable.

There was no need to have a highcontrast property for every state + open
when the colors only change in very few places.
@jawinn
refactor(picker): organize some quiet styles
981eaa2 
Move some quiet styles to existing quiet selector in the file, to keep
things organized. The quiet disabled focus-visible rule from the moved
block of CSS could be removed because it didn't do anything.
@jawinn
fix(picker): fix some color issues with invalid and disabled
34432a7 
- Fix issue with "Closed and Disabled with Thumbnails" docs example
  showing the invalid border color on hover by adding a :not selector.
  The invalid hover rules were more specific than the disabled rules.
- Fix wrong hover color appearing in WHCM on invalid examples.
@jawinn
fix(picker): background color fix for quiet variant
12f233e 
Fix issue with grey background color showing on hover/click with the
quiet variant. Due to the styles for .is-open being more specific.
@jawinn jawinn force-pushed the jawinn/css-636-fix-picker-whcm branch from b166d19 to 12f233e Compare December 14, 2023 15:54
@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label Dec 14, 2023
@pfulton pfulton merged commit c80bbd6 into main Dec 14, 2023
@pfulton pfulton deleted the jawinn/css-636-fix-picker-whcm branch December 14, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdt2 mdt2 mdt2 approved these changes

Assignees
No one assigned
Labels
run_vrt For use on PRs looking to kick off VRT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jawinn @mdt2 @pfulton