feat(switch): token mapping updates for s2-foundations#6065
feat(switch): token mapping updates for s2-foundations#6065marissahuysentruyt merged 2 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 16775f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 83 packages
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 |
marissahuysentruyt
added
1.0.0
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
marissahuysentruyt
force-pushed
the
marissahuysentruyt/swc-1070-switch-token-mapping
branch
2 times, most recently
from
0e92c94 to
4eab149
Compare
| --system-switch-border-width-themed: 0px; | ||
| --system-switch-border-color-selected-default: transparent; | ||
| --system-switch-border-color-selected-hover: transparent; | ||
| --system-switch-border-color-selected-down: transparent; | ||
| --system-switch-border-color-selected-focus: transparent; |
There was a problem hiding this comment.
There is no border around the switch input in S1, but there is in S2. These tokens allow for the theme switching.
| --system-switch-border-width-themed: 0px; | ||
| --system-switch-border-color-selected-default: transparent; | ||
| --system-switch-border-color-selected-hover: transparent; | ||
| --system-switch-border-color-selected-down: transparent; | ||
| --system-switch-border-color-selected-focus: transparent; |
There was a problem hiding this comment.
There is no border around the switch input in Express, but there is in S2. These tokens allow for the theme switching.
| --system-switch-border-width-themed: var(--spectrum-border-width-200); | ||
| --system-switch-border-color-selected-default: var(--spectrum-neutral-background-color-default); | ||
| --system-switch-border-color-selected-hover: var(--spectrum-neutral-background-color-hover); | ||
| --system-switch-border-color-selected-down: var(--spectrum-neutral-background-color-down); | ||
| --system-switch-border-color-selected-focus: var(--spectrum-neutral-background-color-key-focus); |
There was a problem hiding this comment.
There is no border around the switch input in Express or S1, but there is in S2. These tokens allow for the theme switching.
|
|
||
| @media (hover: hover) { | ||
| :host(:hover) #input:not(:checked) + #switch { | ||
| border-color: Highlight; |
There was a problem hiding this comment.
This should override the new border colors that are defined in non-WHCM.
| :host([checked]:hover) #input:focus-visible + #switch { | ||
| background-color: var(--highcontrast-switch-background-color-selected-focus, var(--mod-switch-background-color-selected-focus, var(--spectrum-switch-background-color-selected-focus))); | ||
| } |
There was a problem hiding this comment.
If I'm remembering correctly, this was a duplicate of another selector block. Let me know if you're seeing regressions without this though!
|
|
||
| :host([checked]) #input + #switch::before { | ||
| transform: translateX(calc(var(--mod-switch-control-width, var(--spectrum-switch-control-width)) - 100%)); | ||
| transform: translateX(calc(var(--mod-switch-control-width, var(--spectrum-switch-control-width)) - 100% - 2 * var(--mod-switch-border-width-themed, var(--spectrum-switch-border-width-themed)))); |
There was a problem hiding this comment.
The transforms now have to accommodate the new input border widths in each theme. For S1 and Express, the border width is 0, so the ending position of the handle should be in the same spot as main. With S2 having a new 2px border, the handle had to be positioned the additional 2px inward.
|
|
||
| #input:focus-visible + #switch::after { | ||
| margin: calc(var(--mod-focus-indicator-gap, var(--spectrum-focus-indicator-gap)) * -1); | ||
| margin: calc((var(--mod-focus-indicator-gap, var(--spectrum-focus-indicator-gap)) + var(--spectrum-switch-border-width-themed)) * -1); |
There was a problem hiding this comment.
Same here- this margin has to account for the changes in the input border width between the themes.
marissahuysentruyt
force-pushed
the
marissahuysentruyt/swc-1070-switch-token-mapping
branch
from
83680f2 to
7eb2fd2
Compare
marissahuysentruyt
added
run_vrt
There was a problem hiding this comment.
Some questions for you!
I didn't look too closely at the code, but wanted to ask about two things I noticed as I was looking over this visually:
- Hopefully the video below displays ok, but: I'm seeing, when the switch is keyboard-focused, that if I'm hovering over the switch, the handle color is darker (looks like it matches the switch outline), but if I move my mouse outside of the switch, the handle color is lighter.
focused-switch-handle-hover.mov
- It doesn't look like we test HCM for Spectrum 2 in VRTs, which is unfortunate. So I looked at this in WHCM as well as in Chrome with forced-colors, here are some things I saw (I'm tucking the screenshots into disclosures), it seemed a little off to me as I can see that switch border in these modes and there's a little tiny bit of gapping (I'm not sure if I should call it that? But the little specks of black peeking through between the border and the switch outline) in those cases too. If it seems like there's something off with my setup and that's not what you're seeing, by all means let me know!
Off, hover
WHCM:
Chrome forced-colors:
Off, not hover
WHCM:
Chrome forced-colors:
On
WHCM:
Chrome forced-colors:
marissahuysentruyt
added
Status:Addressing feedback
marissahuysentruyt
force-pushed
the
marissahuysentruyt/swc-1070-switch-token-mapping
branch
from
3a6f738 to
d21fde9
Compare
|
@rise-erpelding I think I fixed the WHCM styles you mentioned. There was an extra box-shadow because the old styles didn't have a border. With the border added in this PR, we don't need it! I also should have fixed the handle background color for keyboardFocus+hover. |
|
🚨 Should I implement the approach for the borders without actually implementing the colors in S1/Express? That way, we don't introduce VRT diffs but we'd have a way via mods to update the docs? I need to the look at the ticket once more. |
rise-erpelding
left a comment
rise-erpelding
left a comment
There was a problem hiding this comment.
Leaving a few WHCM/forced-colors callouts I noticed in S1/Express now that those have changed:
Seeing this in forced-colors preferred-color scheme: dark, but on the Light Spectrum 1 theme, when the switch is not hovered, which seems like a bit of a low contrast to me:
This is the same story in AssistivLabs, so the issue is there as well:
Weirdly it shifts and it's fine when you switch the theme to Dark, which I didn't expect?
(forced-colors)
(AssistivLabs)
It's the same situation if it's a lighter forced-colors/WHCM theme... the contrast is hard to see if you're in the spectrum dark theme, but it's fine in the light theme. So maybe that's fine? I'm not sure, I don't recall ever needing to change the Storybook color theme to match the forced-colors mode theme so it seemed a little odd to me.
Otherwise, I have a few questions about the consumer impact here. Obviously this is a fix for accessibility so overall it's good, and there are no layout shifts, which is also good, but the colors are a big change which might receive some pushback from consumers. I can't find a way to get back to what it looked like in Spectrum 1/Express before using custom properties to emulate what a consumer might try to do if they wanted to revert the colors back to what they were before, but again, since this is an accessibility fix, maybe they shouldn't be able to?
| margin-block: calc(var(--mod-switch-height, var(--spectrum-switch-min-height)) - var(--mod-switch-control-height, var(--spectrum-switch-control-height)) - var(--mod-switch-spacing-top-to-control, var(--spectrum-switch-spacing-top-to-control))); | ||
| margin-inline: 0; | ||
| vertical-align: middle; | ||
| border: var(--mod-switch-border-width, var(--spectrum-switch-border-width)) solid var(--spectrum-switch-border-color-default); |
There was a problem hiding this comment.
What are your thoughts on providing a --mod for --spectrum-switch-border-color-default? Or was this more of a decision not to in order to enforce an a11y-friendly contrast?
There was a problem hiding this comment.
Yes! That should be pushed up now. Good catch.
| block-size: calc(var(--mod-switch-control-height, var(--spectrum-switch-control-height)) - var(--mod-switch-border-width, var(--spectrum-switch-border-width)) * 2); | ||
| border-style: solid; | ||
| border-width: var(--mod-border-width-200, var(--spectrum-border-width-200)); | ||
| border-width: var(--mod-switch-border-width, var(--spectrum-switch-border-width)); |
There was a problem hiding this comment.
Nice added fix here. I see it's mentioned in the changeset, optionally it might be helpful to also mention that this was changed from --mod-border-width-200 just in case any consumers were using it.
There was a problem hiding this comment.
I ended up reverting this change, and brought back the comment about keeping it the global token.
| inline-size: calc(var(--mod-switch-control-height, var(--spectrum-switch-control-height)) - var(--mod-switch-border-width, var(--spectrum-switch-border-width)) * 2); | ||
| block-size: calc(var(--mod-switch-control-height, var(--spectrum-switch-control-height)) - var(--mod-switch-border-width, var(--spectrum-switch-border-width)) * 2); |
There was a problem hiding this comment.
As I'm playing with this, it looks like --mod-switch-border-width (and --spectrum-switch-border-width) control border widths on both the switch overall (I guess the switch background, if you will?) and the switch handle, like if I set --mod-switch-border-width to 0, I get a switch that looks like this in Spectrum 1:
I'm wondering if this could potentially be a concern if a consumer wanted to modify one but not the other? More specifically, I'm wondering if there's a way to use custom properties to bring the switch back to what it looked like before, if a consumer wanted that, since visually this is a big change. But I also know that going back to the previous switch isn't recommended, because it's not accessible, I'm just wondering if the option should still be available?
There was a problem hiding this comment.
I think this bug is related to the --mod-switch-border-width comment you had above. Incorrectly, I was using the switch input border custom prop on the switch handle. Let me know if you are still getting this, but I think it should be taken care of since I tried to separate those items once more.
marissahuysentruyt
added
Status:Addressing feedback
marissahuysentruyt
force-pushed
the
marissahuysentruyt/swc-1070-switch-token-mapping
branch
from
f009b4f to
fdbe424
Compare
| :host([disabled][checked]:hover) #input + #switch, | ||
| :host([disabled][checked]:hover) #input + #switch { | ||
| background-color: GrayText; | ||
| box-shadow: inset 0 0 0 1px GrayText; |
There was a problem hiding this comment.
@rise-erpelding There appears to be a few tradeoffs for the #switch border and the #switch::before elements that comes down to the themes.
For S1/Express, there is no visual switch input border. However, for S2, we added a legitimate border to the component. The box-shadows create the border in WHCM. So in order to keep the WHCM S1 VRTs from tripping, I kept a few of the box-shadows. That should resolve the issue you noticed where it just kind of looked like the switch handle was floating there. And then I also added --highcontrast-switch-border-color: ButtonText; so that the S2 switch input border had the correct styling (instead of the low contrast you screenshotted).
I could try to redefine the border width itself in WHCM, but that feels a little hacky, and doesn't feel like the path we've been taking for WHCM styles.
For the handle border color, in S1/Express it changes from being a totally separate color to the switch background color when selected. That mainly works, but in S2, the new 2px border follows the selected switch background color, so the S2 handle border color had to change to the handle background color. For WHCM, the handle in S2 visually looks smaller because of the 2px border and the handle border color ending up the same color now.
I think these are fine tradeoffs for the S2 Foundations theme for now. S1 and Express should hopefully 🤞 be untouched now. But the S2 foundations WHCM switch will have that duplicate box-shadow, a slightly smaller looking handle when selected.
What do you think? Maybe I'll throw these options out in the channel to get some more feedback on them as well.
marissahuysentruyt
added
Status:Ready for re-review
marissahuysentruyt
force-pushed
the
marissahuysentruyt/swc-1070-switch-token-mapping
branch
from
f959fe3 to
19dcf19
Compare
marissahuysentruyt
added
Status:Ready for re-review
rise-erpelding
left a comment
rise-erpelding
left a comment
There was a problem hiding this comment.
This looks good! The only small recommendation I'd make here is to add box-shadow to the #switch transition to keep all the colors in the hover state transitioning with the same timing in WHCM.
marissahuysentruyt
force-pushed
the
marissahuysentruyt/swc-1070-switch-token-mapping
branch
from
6565f7a to
db56e0b
Compare
- preserve handle border colors for s1/express - use new system tokens for switch theme overrides - update handle background color for s2 foundations theme - emphasized down state color in s2 foundations theme - add themed border width variable so s2 can have a border; s1 and express remain intact, borderless - fix highcontrast variable typo and adds border color to switch input - fix typo for focus ring * refactor(switch): selected border color tokens - adds `--spectrum-switch-border-color-*` tokens to control the switch track border across all selected interaction states (needs to preserve s1/express and introduce new style behaviors in s2-foundations) - adds full set of emphasized selected handle border color tokens - set transparent border colors for s1/express where the track border is not visible * refactor(switch): disabled border color tokens * chore(switch): add changeset * fix(switch): remove circular references in custom props * fix(switch): enhance WHCM box-shadows and border colors * fix(switch): keyboard focused+hover bg color of handle * fix(switch): add box-shadow in transition to smooth the color changes
marissahuysentruyt
force-pushed
the
marissahuysentruyt/swc-1070-switch-token-mapping
branch
from
db56e0b to
16775f7
Compare
marissahuysentruyt
added
Status:Ready for merge
marissahuysentruyt
deleted the
marissahuysentruyt/swc-1070-switch-token-mapping
branch
* feat(switch): switch token mapping to s2 border colors - preserve handle border colors for s1/express - use new system tokens for switch theme overrides - update handle background color for s2 foundations theme - emphasized down state color in s2 foundations theme - add themed border width variable so s2 can have a border; s1 and express remain intact, borderless - fix highcontrast variable typo and adds border color to switch input - fix typo for focus ring * refactor(switch): selected border color tokens - adds `--spectrum-switch-border-color-*` tokens to control the switch track border across all selected interaction states (needs to preserve s1/express and introduce new style behaviors in s2-foundations) - adds full set of emphasized selected handle border color tokens - set transparent border colors for s1/express where the track border is not visible * refactor(switch): disabled border color tokens * chore(switch): add changeset * fix(switch): remove circular references in custom props * fix(switch): enhance WHCM box-shadows and border colors * fix(switch): keyboard focused+hover bg color of handle * fix(switch): add box-shadow in transition to smooth the color changes * chore: update golden image hash
3 participants
Description
This PR further distinguishes the S2 Foundations switch CSS styles so that the component reflects more of the S2 design.
Switch theme overrides: The switch component now has some new
--system-switch-*variables so the switch track has border colors, some adjusted handle border colors, and background colors are theme-specific.--spectrum-switch-border-width-themed. All themes will have a track border property, but only the S2 foundations theme defines it as a 2px border (S1/Express define that border as 0px).switch.cssfrom--highcontrast-switch-handle-border-color-selelcted-downto--highcontrast-switch-handle-border-color-selected-downso forced-colors styling applies correctly for the selected-down state. (just an extra "l" in selected originally)Video description if you want it:
pr6065.mp4
Motivation and context
Aligns switch styling with Spectrum 2 design more (track border, handle colors), while keeping Spectrum and Express untouched.
Related issue(s)
Screenshots (if appropriate)
No changes are expected in Express or S1. There are VRT diffs in the
Themestories because there's a switch in the example story.🚫 Before
Express / S1 / S2 Foundations
✅ After
Express / S1 / S2 Foundations
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Please see the review channel Slack thread for the Figma file with these interim designs! (option 2)
Design discussion and approval
Switch in Spectrum theme [@cdransf]
main.main.Switch in Express theme [@cdransf]
main.main.Switch in Spectrum Two theme [@cdransf]
main. But some new styles to look for include:i. New background color of switch input and switch handle
ii. Visible border color of switch input
iii. Switch handle border color changes (it matches the switch input background color when unchecked, and then matches the handle background color when checked)
main. Some new styles to look for include:i. New background color of switch input and switch handle during the interactions
ii. Visible border color of switch input during the interactions (should match the background color of checked switches)
iii. Active switch background color remains the same as hover & keyboard focus (S2 doesn't have a specific active background color)
High-contrast (forced-colors) [@cdransf]
#switch::beforeelement (the handle) to make sure the--highcontrast-switch-handle-border-color-selected-downcustom property is rendering atHighlight(this was the typo fix in the custom prop name).Highlightcolors in WHCM as they do onmain.VRTs are expected only in the S2 Foundations theme.
Device review
Accessibility testing checklist
Required: Complete each applicable item and document your testing steps (replace the placeholders with your component-specific instructions).
Keyboard (required — document steps below) [@cdransf] — What to test for: Focus order is logical; Tab reaches the component and all interactive descendants; Enter/Space activate where appropriate; arrow keys work for tabs, menus, sliders, etc.; no focus traps; Escape dismisses when applicable; focus indicator is visible.
Screen reader (required — document steps below) [@cdransf] — What to test for: Role and name are announced correctly; state changes (e.g. expanded, selected) are announced; labels and relationships are clear; no unnecessary or duplicate announcements.