fix(slider): focus state matches spec, supports forced-colors #2217

mdt2 · 2023-10-17T14:05:59Z

Description

This PR addresses issue #2200.

Adjust focus state to match the spec (previously it looked like the active state which was incorrect)
Standard variant works in high contrast mode. See Xd file in spec for reference.
Disabled slider has the default cursor on hover. This was already set in a couple places (.spectrum-Slider and .spectrum-Slider-handle) but needed to be explicitly set on .spectrum-Slider-controls to take effect since this class has a specified cursor: pointer when the slider isn't disabled.

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

On focus, the slider handle should match the spec (it shouldn't be filled in and should have a focus ring). Confirm this for high contrast mode as well
On hover in high contrast mode, the slider track and handle are the Highlight color
On active/dragged in high contrast mode, the handle fills in with Highlight to match the spec
Disabled sliders should get the default cursor on hover
Focus and disabled states are documented on the docs site

Regression testing

Validate:

A legacy documentation page (such as accordion), including:

The page renders correctly
The page is accessible
The page is responsive

A migrated documentation page (such as action group), including:

The page renders correctly
The page is accessible
The page is responsive

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
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 · 2023-10-17T14:12:42Z

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

jawinn

It's looking much closer to the intended spec now. A few questions and suggestions below.

There doesn't seem to be any indication in the WHCM design spec about how to handle the "Filled", , "Filled-Offset", and "Range". I think perhaps that at least for focus and hover, the darker portion should use the Highlight color (in the PR you can see the reverse of this on "Filled-Offset").
Ideally there'd be a way to differentiate the darkened part of the range by default in WHCM as well, but I'm not sure about the best way; possibly a thicker track since there isn't another appropriate system color to use. But that would need some followup design/a11y involvement/approval.

I also noticed that there are some existing high contrast styles for the ramp that we may want to move next to the rest of the high contrast styles.

components/slider/index.css

mdt2 · 2023-10-24T21:07:08Z

components/slider/index.css

-.spectrum-Slider {
-  &.is-disabled {
-    cursor: default;
-
-    .spectrum-Slider-handle {
-      cursor: default;
-      pointer-events: none;
-    }
-
-    .spectrum-Slider-tickLabel {
-      color: var(--highcontrast-slider-label-text-color-disabled, var(--mod-slider-label-text-color-disabled, var(--spectrum-slider-label-text-color-disabled)));
-    }
-  }
-}


Relocated this to an existing spectrum-Slider.is-disabled selector on new line 628

mdt2 · 2023-10-24T21:08:24Z

components/slider/index.css

-    @media (forced-colors: active) {
-      /* forced-color-adjust required to ensure the "circle" around the handle is transparent */
-      forced-color-adjust: none;
-      box-shadow: 0 0 0 var(--spectrum-slider-handle-gap) ButtonFace;
-      border-color: ButtonText;
-      background-color: ButtonFace;
-    }


We can rely on the --highcontrast custom properties to do this instead.

jawinn

Looks good to me! Thanks for those updates.
I only have one small recommendation to move a few lines of CSS.

components/slider/index.css

pfulton

This looks great. Thanks for taking care of this.

pfulton · 2023-10-27T17:51:44Z

@najikahalsema @majornista This work should address #2200. Would you mind taking a look to be sure that we've fixed everything? PR deploy is here: https://pr-2217--spectrum-css.netlify.app/

najikahalsema · 2023-10-27T21:29:55Z

Looks good to me! Thanks for doing this work.

…y in storybook

pfulton · 2023-11-09T15:18:16Z

@castastrophe Noting that this one will have some Docs changes!

github-actions bot temporarily deployed to pull request October 17, 2023 14:12 Inactive

mdt2 force-pushed the mdt2/css-620-slider-focus branch from fb30aba to ce7ed65 Compare October 17, 2023 14:19

mdt2 requested review from jawinn and jenndiaz October 17, 2023 14:20

github-actions bot temporarily deployed to pull request October 17, 2023 14:24 Inactive

jawinn requested changes Oct 19, 2023

View reviewed changes

components/slider/index.css Outdated Show resolved Hide resolved

components/slider/index.css Outdated Show resolved Hide resolved

components/slider/index.css Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to pull request October 23, 2023 16:01 Inactive

github-actions bot temporarily deployed to pull request October 23, 2023 18:10 Inactive

github-actions bot temporarily deployed to pull request October 24, 2023 14:10 Inactive

github-actions bot temporarily deployed to pull request October 24, 2023 17:36 Inactive

github-actions bot temporarily deployed to pull request October 24, 2023 17:48 Inactive

github-actions bot temporarily deployed to pull request October 24, 2023 18:03 Inactive

github-actions bot temporarily deployed to pull request October 24, 2023 20:36 Inactive

github-actions bot temporarily deployed to pull request October 24, 2023 21:02 Inactive

mdt2 requested a review from jawinn October 24, 2023 21:06

mdt2 commented Oct 24, 2023

View reviewed changes

github-actions bot temporarily deployed to pull request October 25, 2023 18:46 Inactive

mdt2 force-pushed the mdt2/css-620-slider-focus branch from 03f063d to cb33dfd Compare October 25, 2023 19:54

github-actions bot temporarily deployed to pull request October 25, 2023 20:00 Inactive

jawinn reviewed Oct 27, 2023

View reviewed changes

components/slider/index.css Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to pull request October 27, 2023 14:41 Inactive

mdt2 force-pushed the mdt2/css-620-slider-focus branch from 50e7fe7 to c111fdd Compare October 27, 2023 14:59

github-actions bot temporarily deployed to pull request October 27, 2023 15:04 Inactive

jawinn approved these changes Oct 27, 2023

View reviewed changes

pfulton approved these changes Oct 27, 2023

View reviewed changes

mdt2 marked this pull request as ready for review October 29, 2023 23:34

pfulton added the ready-to-merge label Nov 1, 2023

pfulton mentioned this pull request Nov 3, 2023

[Bug][a11y]: Slider focus style should support forced-colors: active / WHCM adobe/spectrum-web-components#3328

Closed

1 task

pfulton force-pushed the mdt2/css-620-slider-focus branch from c111fdd to f070c71 Compare November 8, 2023 21:49

github-actions bot temporarily deployed to pull request November 8, 2023 21:56 Inactive

pfulton added the run_vrt For use on PRs looking to kick off VRT label Nov 9, 2023

github-actions bot removed the run_vrt For use on PRs looking to kick off VRT label Nov 9, 2023

mdt2 added 14 commits November 9, 2023 10:09

fix(slider): focus state doesn't get active state style

1712e65

fix(slider): high contrast mode works for standard variant

4cc718d

fix(slider): disabled slider gets default cursor

943a6dc

fix(slider): hover, focus in WHCM with prefers-color-scheme

78dd648

docs(slider): focus and disabled implementations documented

8400dbf

refactor(slider): ramp variant high contrast properties

9a1ac26

fix(slider): high contrast highlight when control is interacted with

19e799e

chore(slider): adjust high contrast mode nesting

53ab19d

chore(slider): is-disabled styles in one place

b8f7914

fix(slider): is-focused class bubbles to controls parent, styles appl…

bba824f

…y in storybook

fix(slider): disabled sliders have disabled input in docs site

eda90c0

chore(slider): add with focus story

2214996

fix(slider): keyboard focus works in storybook

b0736dc

chore(slider): forced-color-adjusts go in forced-colors media query

a8cca63

pfulton force-pushed the mdt2/css-620-slider-focus branch from f070c71 to a8cca63 Compare November 9, 2023 15:09

github-actions bot temporarily deployed to pull request November 9, 2023 15:13 Inactive

pfulton merged commit 7b9c31b into main Nov 9, 2023
4 checks passed

pfulton deleted the mdt2/css-620-slider-focus branch November 9, 2023 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(slider): focus state matches spec, supports forced-colors #2217

fix(slider): focus state matches spec, supports forced-colors #2217

mdt2 commented Oct 17, 2023 •

edited by jawinn

github-actions bot commented Oct 17, 2023 •

edited

jawinn left a comment

mdt2 Oct 24, 2023

mdt2 Oct 24, 2023

jawinn left a comment

pfulton left a comment

pfulton commented Oct 27, 2023

najikahalsema commented Oct 27, 2023

pfulton commented Nov 9, 2023

fix(slider): focus state matches spec, supports forced-colors #2217

fix(slider): focus state matches spec, supports forced-colors #2217

Conversation

mdt2 commented Oct 17, 2023 • edited by jawinn

Description

How and where has this been tested?

Validation

Regression testing

To-do list

github-actions bot commented Oct 17, 2023 • edited

jawinn left a comment

Choose a reason for hiding this comment

mdt2 Oct 24, 2023

Choose a reason for hiding this comment

mdt2 Oct 24, 2023

Choose a reason for hiding this comment

jawinn left a comment

Choose a reason for hiding this comment

pfulton left a comment

Choose a reason for hiding this comment

pfulton commented Oct 27, 2023

najikahalsema commented Oct 27, 2023

pfulton commented Nov 9, 2023

mdt2 commented Oct 17, 2023 •

edited by jawinn

github-actions bot commented Oct 17, 2023 •

edited