Add WHCM support for all the form elements #2830
Conversation
| &.is-invalid { | ||
| .spectrum-Checkbox-box { | ||
| &:before { | ||
| border-color: var(--spectrum-checkbox-box-border-color); |
There was a problem hiding this comment.
This conflicts with var(--spectrum-checkbox-box-border-color-error) above. Is the intended border color Highlight or ButtonText?
There was a problem hiding this comment.
It is intended to be ButtonText when unselected and Highlight when selected according to the XD
Checkbox (Windows high contrast mode) UI kit
| --spectrum-checkbox-text-color-error-down: FieldText; | ||
| --spectrum-checkbox-text-color-error-hover: FieldText; | ||
|
|
||
| &.is-invalid { |
There was a problem hiding this comment.
I'd prefer if we can only override variables and not re-define selectors if possible. I think most of this seems unnecessary because the vars are already overridden above?
There was a problem hiding this comment.
The variables as defined assume the same border colour for :checked.
I don't think I can do this without modifying
.spectrum-Checkbox.is-invalid {
/* Extra-specific selectors added here to handle checked state */
.spectrum-Checkbox-input:checked + .spectrum-Checkbox-box,
.spectrum-Checkbox-box {
&:before {
border-color: var(--spectrum-checkbox-box-border-color-error);
}
}
in lines 158-165 where the same colour is used for checked and unchecked.
I could add a new variable there and split that selector. Would that be better?
There was a problem hiding this comment.
@devongovett is that the direction you would like me to go?
packages/@adobe/spectrum-css-temp/components/inputgroup/skin.css
Outdated
Show resolved
Hide resolved
packages/@adobe/spectrum-css-temp/components/textfield/skin.css
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
In Firefox, the disabled state looks wrong. It has a white background. Seems ok in edge/chrome though.
Also, the invalid/valid icons in Firefox are still red/green rather than white.
There was a problem hiding this comment.
Firefox doesn't support forced-color-adjust:none at the moment. To be honest I don't think we have a hope of supporting FF in high contrast until they do unless I simplify the spectrum defined high contrast behaviour.
I'm ok with either direction but as far as I know the high contrast usage with FF is not very high.
There was a problem hiding this comment.
@devongovett are we looking to support FF in WHCM?
There was a problem hiding this comment.
I'm happy to follow your lead there. If it's easy to support, then great, but if it's not possible or a lot of additional work for not much benefit than it's fine with me if we don't. We should just note that so that when we test it's understood which browsers we should be testing.
|
GET_BUILD |
1 similar comment
|
GET_BUILD |
|
Build successful! |
|
Are there XDs for Calendar/DateRange WHCM? I've looked on Spectrum/contributions but couldn't find any. If not thats fine, I've been trying to watch out for any style ambiguities |
No. I don't believe there are up to date XDs for non high contrast mode for this component either. |
|
GET_BUILD |
|
Build successful! |
| @@ -151,3 +151,53 @@ governing permissions and limitations under the License. | |||
| } | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Noticed the following when testing the Calendar/DateRangePicker WHCM. Lemme know if these are as expected due to color limitations or if they aren't/won't be handled in this PR:
- Can't see that the user is currently focused on the datepicker button:
vs
- The current date (aka the 24th) styling is overridden when the range includes it
vs
- The invalid colors are inconsistent between the date range, the validation icons, and the border/focus ring of the input field. The invalid color in the calendar in particular are hard to see on the black blackground (perhaps a limitation of WHCM theming since there isn't an error color?)
vs
There was a problem hiding this comment.
- Working on this.
- I think this is an acceptable compromise. With limited colours knowing today is optional IMO. The colour in the default scheme doesn't meet WCAG so if we insist knowing today is required then we need to fix outside high contrast mode too.
- These variables were introduced after I created this PR. I'm adding these new invalid variables which will look identical to a selection. The error message is the way you tell that there is an error
Some additional things from testing, I'd be happy to approve and handle these in a follow up if you'd like. Apologies for the much delayed review and thank you for working on this!
packages/@adobe/spectrum-css-temp/components/inputgroup/skin.css
Outdated
Show resolved
Hide resolved
packages/@adobe/spectrum-css-temp/components/inputgroup/skin.css
Outdated
Show resolved
Hide resolved
In order to reproduce the overlapping gradients for a ColorArea, the container element .spectrum-ColorArea will have a background-image or background-color over which the .spectrum-ColorArea-gradient renders.
|
GET_BUILD |
|
Build successful! |
Closes #2529
Closes #3260
This concludes the high contrast PRs
Turn on WHCM
Run Storybook for the following components:
Screenshots
Calendar
Before
#### AfterDate Range
Before
#### AfterCheckbox
After (compare to XD design)
Color Components
Before
#### AfterCombobox
After (compare to XD)
Switch
After (compare to XD)
NumberField
Before
#### AfterSlider
Before
After
Radio Group
Before
#### After