fix(picker,infieldbutton): updates custom properties for background/border colors #3536

marissahuysentruyt · 2025-02-04T21:46:51Z

Description

🐛
For the S2 foundations theme, a bug was caught where the picker, in the context of a form, where the picker just looked out of place. It was decided that the default border color of the picker was the culprit. Sure enough, the default border color was incorrectly defined as gray-800, while all other form elements were using gray-500 as the default border color. This PR fixes that bug. 🥾

After design input, it was also decided to update the stepper button background color (spectrum-InfieldButton) to match the new S2 picker styles.

Jira/Specs

CSS-1112
S2 picker Figma file
Slack conversation with Lynn & Matt

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

We now expect the border colors of all form components (including the picker) use gray-500 as their default border color.

Pull down the branch to run locally or visit the deploy preview (@cdransf)
Navigate to the form docs page (@cdransf)
Inspect the form's text field components to verify the --spectrum-textfield-border-color is defined as gray-500 (@cdransf)
Inspect the picker component in the form and verify the --spectrum-picker-border-color-default is now defined as gray-500 as well. (@cdransf)
When the picker is keyboard focused, verify the font color resolves to gray-900, or neutral-content-color-focus. (@cdransf)
Inspect the stepper buttons (both .spectrum-Stepper-buttons and .spectrum-InfieldButton-fill elements) to verify the --spectrum-stepper-buttons-background-color and --spectrum-infield-button-background-color both are defined at gray-100. (@cdransf)
When in the hover state for a .spectrum-Stepper-button, --spectrum-infield-button-background-color-hover should be defined as gray-200. (@cdransf)

Additional Validation

Visit the stepper default story (@cdransf)
Toggle the Chromatic testing grid and turn on dark mode (the change is most visible in dark mode) (@cdransf)
Inspect the focused stepper (spectrum-Stepper is-focused ) in the browser. Verify that --mod-stepper-border-color is resolving to --spectrum-stepper-border-color-focus. (@cdransf)
When inspecting the same element, if you toggle the hover state in the inspector, --mod-stepper-border-color should then resolve to --spectrum-stepper-border-color-focus-hover. (@cdransf)

NOTE: The "Additional Validation" for the stepper focus-hover state is only partially fixed. A more thorough fix can be found on this branch: https://github.com/adobe/spectrum-css/tree/marissahuysentruyt/fix-stepper-border-focus-hover-color

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

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.

Screenshots

Before 🚫

After ✅

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.
✨ This pull request is ready to merge. ✨

changeset-bot · 2025-02-04T21:46:55Z

🦋 Changeset detected

Latest commit: a09845c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@spectrum-css/infieldbutton	Patch
@spectrum-css/stepper	Patch
@spectrum-css/picker	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-02-04T21:49:49Z

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

github-actions · 2025-02-04T21:51:29Z

File metrics

Summary

Total size: 2.25 MB^*
✅ No change in file sizes

Package	Size	Minified	Gzipped
infieldbutton	15.55 KB	14.97 KB	1.87 KB
picker	28.12 KB	26.85 KB	3.38 KB
stepper	18.36 KB	17.49 KB	2.42 KB

File change details

infieldbutton

Filename	Head	Minified	Gzipped	Compared to base
index-base.css	14.77 KB	14.22 KB	1.78 KB	-
index-theme.css	1.65 KB	1.61 KB	0.57 KB	-
index.css	15.55 KB	14.97 KB	1.87 KB	🔴 ⬆ < 0.01 KB
metadata.json	7.74 KB	-	-	🟢 ⬇ 0.03 KB
themes/express.css	1.37 KB	1.33 KB	0.57 KB	-
themes/spectrum-two.css	1.38 KB	1.35 KB	0.57 KB	🔴 ⬆ < 0.01 KB
themes/spectrum.css	1.39 KB	1.36 KB	0.58 KB	-

picker

Filename	Head	Minified	Gzipped	Compared to base
index-base.css	27.17 KB	25.94 KB	3.30 KB	🔴 ⬆ < 0.01 KB
index-theme.css	1.84 KB	1.80 KB	0.57 KB	-
index.css	28.12 KB	26.85 KB	3.38 KB	🔴 ⬆ < 0.01 KB
metadata.json	14.89 KB	-	-	🟢 ⬇ 0.03 KB
themes/express.css	1.45 KB	1.41 KB	0.55 KB	-
themes/spectrum-two.css	1.55 KB	1.51 KB	0.56 KB	-
themes/spectrum.css	1.56 KB	1.51 KB	0.57 KB	-

stepper

Filename	Head	Minified	Gzipped	Compared to base
index-base.css	16.67 KB	15.89 KB	2.27 KB	🔴 ⬆ 0.01 KB
index-theme.css	2.77 KB	2.69 KB	0.66 KB	-
index.css	18.36 KB	17.49 KB	2.42 KB	🔴 ⬆ 0.01 KB
metadata.json	8.96 KB	-	-	🔴 ⬆ 0.01 KB
themes/express.css	2.06 KB	1.99 KB	0.65 KB	-
themes/spectrum-two.css	2.23 KB	2.15 KB	0.65 KB	🔴 ⬆ < 0.01 KB
themes/spectrum.css	2.23 KB	2.16 KB	0.65 KB	-

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

marissahuysentruyt · 2025-02-06T20:58:15Z

components/picker/themes/spectrum-two.css

@@ -20,7 +20,7 @@
 		--spectrum-picker-background-color-active: var(--spectrum-gray-200);
 		--spectrum-picker-background-color-key-focus: var(--spectrum-gray-200);

-		--spectrum-picker-border-color-default: var(--spectrum-gray-800);
+		--spectrum-picker-border-color-default: var(--spectrum-gray-500);


we were using the wrong border color for the default picker.

marissahuysentruyt · 2025-02-06T20:59:21Z

components/infieldbutton/themes/spectrum-two.css

+		--spectrum-infield-button-background-color: var(--spectrum-gray-100);
+		--spectrum-infield-button-background-color-hover: var(--spectrum-gray-200);
 		--spectrum-infield-button-background-color-down: var(--spectrum-gray-200);
-		--spectrum-infield-button-background-color-key-focus: var(--spectrum-gray-100);
+		--spectrum-infield-button-background-color-key-focus: var(--spectrum-gray-200);


These changes, along with the stepper changes, are not reflected in Figma yet. They're working on updating them, but decided to have the stepper buttons' background color match the s2 picker background color.

marissahuysentruyt · 2025-02-06T21:59:38Z

components/picker/index.css

@@ -265,7 +265,7 @@
 			border-color: var(--highcontrast-picker-focus-indicator-color, var(--mod-picker-focus-indicator-color, var(--spectrum-picker-focus-indicator-color)));
 		}

-		&.is-placeholder {
+		.is-placeholder {


Let me know if I'm missing anything by removing & here. I felt like I was fighting the specificity when further down in this file, there's another color style declaration.

The reason I removed it (besides that it was a bug caught by design!) was because these are the tokens/styles we want and design pointed out we were missing (for the key-focus tokens), but in the other &.is-placeholder selector under .spectrum-Picker-label (line 364ish), the color was computing to the default instead.

This looks like the right correction to the selector, since .is-placeholder doesn't live on the root class according to the template (it's on the nested span with the spectrum-Picker-label class). 👍

My only concern is in having an is- prefixed class unattached to a specific element. Should we add .spectrum-Picker-label in front of it to ensure we don't end up with an unexpected nested class getting targeted?

marissahuysentruyt · 2025-02-07T15:17:40Z

~~todo: add changeset~~

edit: ✅ done

rise-erpelding

Borders, background, and key-focused placeholder color looking as expected given the context.

cdransf

Ran through all of the validation steps and everything looks good! ✨

rise-erpelding

I think this is good to go! Left an additional comment to confirm what I'm supposed to be seeing with these latest changes.

rise-erpelding · 2025-02-12T17:05:10Z

components/stepper/index.css

@@ -170,7 +170,7 @@
 		--mod-textfield-border-color-disabled: var(--spectrum-stepper-border-color-disabled);
 	}

-	&:hover:not(.is-invalid, .is-disabled) {
+	&:hover:not(.is-invalid, .is-disabled, .is-focused) {


So to clarify, this means that we do not want the border color on the stepper to lighten (or I guess darken, in dark mode) when we hover when it's focused, right? So visually focus + hover should look the same as focus only?

Pretty much, yes. In the focus state, we do not want the borders (any of the borders) to be --spectrum-stepper-border-color-hover- we would want --spectrum-stepper-border-color-focus-hover. This was another bug Matt Davey found from before foundations was merged. You can really see it in dark mode, where the infield button border color is black (or almost black?) when hovering over the focus state. Currently, on production, the focus+hover and keyboardFocus+hover are less apparent, but still incorrect.

But yes- we do not want the hover border color to take over. We should see the focus-hover border color instead.

* fix(picker): update default border-color to gray-500 - also updates metadata.json * fix(infieldbutton): update tokens for background color - updates gray-50 to gray-100 for default background color - updates gray-100 to gray-200 for down and hover state background color - updates metadata.json to reflect changes These fixes should align the stepper infield buttons with the style behavior of other form elements (particularly to match picker). * fix(picker): update font color for focus state - in the keyboard focus state, the picker value/placeholder font color was resolving to gray-800, when gray-900 was expected. - create changeset * fix(stepper): fix border color for focus&hover

castastrophe

One suggestion regarding the .is-placeholder modifier but otherwise this looks great.

castastrophe · 2025-02-20T16:07:34Z

components/picker/index.css

@@ -265,7 +265,7 @@
 			border-color: var(--highcontrast-picker-focus-indicator-color, var(--mod-picker-focus-indicator-color, var(--spectrum-picker-focus-indicator-color)));
 		}

-		&.is-placeholder {
+		.is-placeholder {


My only concern is in having an is- prefixed class unattached to a specific element. Should we add .spectrum-Picker-label in front of it to ensure we don't end up with an unexpected nested class getting targeted?

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 98d4003 to 7ed42b6 Compare February 5, 2025 14:39

marissahuysentruyt self-assigned this Feb 5, 2025

marissahuysentruyt added the bug Results from a bug in the CSS implementation label Feb 5, 2025

marissahuysentruyt changed the title ~~fix(picker): change background color token~~ fix(picker): updates token for default border color Feb 5, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 7ed42b6 to 267be6c Compare February 6, 2025 14:36

marissahuysentruyt changed the title ~~fix(picker): updates token for default border color~~ fix(picker,infieldbutton): updates custom properties for background/border colors Feb 6, 2025

marissahuysentruyt added the run_vrt For use on PRs looking to kick off VRT label Feb 6, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from ff7b723 to b2e29f1 Compare February 6, 2025 20:18

marissahuysentruyt marked this pull request as ready for review February 6, 2025 20:37

marissahuysentruyt commented Feb 6, 2025

View reviewed changes

marissahuysentruyt marked this pull request as draft February 6, 2025 21:46

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from b2e29f1 to 9551a1a Compare February 6, 2025 21:58

marissahuysentruyt commented Feb 6, 2025

View reviewed changes

marissahuysentruyt marked this pull request as ready for review February 6, 2025 22:02

marissahuysentruyt requested review from rise-erpelding and castastrophe February 6, 2025 22:18

marissahuysentruyt added the size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. label Feb 7, 2025

rise-erpelding approved these changes Feb 7, 2025

View reviewed changes

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 9551a1a to 0d10404 Compare February 10, 2025 14:59

marissahuysentruyt added the ready-for-review label Feb 10, 2025

cdransf approved these changes Feb 10, 2025

View reviewed changes

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 0d10404 to 983d985 Compare February 11, 2025 15:00

marissahuysentruyt added ready-to-merge and removed ready-for-review labels Feb 11, 2025

marissahuysentruyt requested a review from cdransf February 12, 2025 15:24

marissahuysentruyt requested a review from rise-erpelding February 12, 2025 15:24

rise-erpelding approved these changes Feb 12, 2025

View reviewed changes

marissahuysentruyt removed the ready-to-merge label Feb 12, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 63c40ff to 89ab810 Compare February 14, 2025 20:35

marissahuysentruyt added ready-for-review ready-to-merge labels Feb 17, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 89ab810 to 408ed9a Compare February 18, 2025 20:19

marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 408ed9a to a09845c Compare February 18, 2025 21:02

marissahuysentruyt mentioned this pull request Feb 18, 2025

fix(stepper): border focus hover color #3558

Open

9 tasks

castastrophe approved these changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(picker,infieldbutton): updates custom properties for background/border colors #3536

fix(picker,infieldbutton): updates custom properties for background/border colors #3536

marissahuysentruyt commented Feb 4, 2025 •

edited by cdransf

Loading

changeset-bot bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

infieldbutton

picker

stepper

marissahuysentruyt Feb 6, 2025

marissahuysentruyt Feb 6, 2025

marissahuysentruyt Feb 6, 2025 •

edited

Loading

jawinn Feb 12, 2025

castastrophe Feb 20, 2025

marissahuysentruyt commented Feb 7, 2025 •

edited

Loading

rise-erpelding left a comment

cdransf left a comment

rise-erpelding left a comment

rise-erpelding Feb 12, 2025

marissahuysentruyt Feb 12, 2025

castastrophe left a comment

castastrophe Feb 20, 2025

fix(picker,infieldbutton): updates custom properties for background/border colors #3536

Are you sure you want to change the base?

fix(picker,infieldbutton): updates custom properties for background/border colors #3536

Conversation

marissahuysentruyt commented Feb 4, 2025 • edited by cdransf Loading

Description

Jira/Specs

How and where has this been tested?

Validation steps

Additional Validation

Regression testing

Screenshots

To-do list

changeset-bot bot commented Feb 4, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 4, 2025 • edited Loading

github-actions bot commented Feb 4, 2025 • edited Loading

File metrics

Summary

infieldbutton

picker

stepper

marissahuysentruyt Feb 6, 2025

Choose a reason for hiding this comment

marissahuysentruyt Feb 6, 2025

Choose a reason for hiding this comment

marissahuysentruyt Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

jawinn Feb 12, 2025

Choose a reason for hiding this comment

castastrophe Feb 20, 2025

Choose a reason for hiding this comment

marissahuysentruyt commented Feb 7, 2025 • edited Loading

rise-erpelding left a comment

Choose a reason for hiding this comment

cdransf left a comment

Choose a reason for hiding this comment

rise-erpelding left a comment

Choose a reason for hiding this comment

rise-erpelding Feb 12, 2025

Choose a reason for hiding this comment

marissahuysentruyt Feb 12, 2025

Choose a reason for hiding this comment

castastrophe left a comment

Choose a reason for hiding this comment

castastrophe Feb 20, 2025

Choose a reason for hiding this comment

marissahuysentruyt commented Feb 4, 2025 •

edited by cdransf

Loading

changeset-bot bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

marissahuysentruyt Feb 6, 2025 •

edited

Loading

marissahuysentruyt commented Feb 7, 2025 •

edited

Loading