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.

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

Add focus style to the switch toggle controls #1988

Merged
merged 2 commits into from
Jul 25, 2017

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jul 24, 2017

This PR is a first try to add a focus style to the switch toggle UI control (FormToggle component).

It changes the CSS generated element (:after) previously used to style the switch "thumb" to a span element. This way it's trivial to target it when the input has focus and it is possible to use any style. For a11y reasons, it's better to use a border for the focus style, see the discussion on the issue about Windows High Contrast mode.

Pending design and a11y feedback.

Fixes #1562

screen shot 2017-07-24 at 16 24 23

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Design labels Jul 24, 2017
@jasmussen jasmussen self-requested a review July 25, 2017 09:12
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work, thank you for doing this. This is a vast improvement.

I wish we could attach an outline or box shadow to the whole element, as opposed to just the thumb inside, but given we can't do that (because it's the input that gets focus and we can't attach a ::before to that), this seems like a good compromise. Designwise, thumbs up!

@@ -32,25 +32,34 @@ $toggle-border-width: 2px;
}
}

&:after {
content: '';
&__thumb {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little bit obtuse, coding style wise. I admit to not being 100% sure about exactly what the coding guidelines would say, but I have a feeling it would be better to replace &__thumb with just plainly .components-form-toggle__thumb.

@afercia
Copy link
Contributor Author

afercia commented Jul 25, 2017

I wish we could attach an outline or box shadow to the whole element,

Actually, I think it can be done 🙁 following the same approach and replacing :before with a span element then we move it after the input and we can target both the spans:
input:focus + span
input:focus + span + span
and maybe adjust z-indexes. Will give it a try.

@jasmussen
Copy link
Contributor

Oooh, exciting!

I dig the way they look in Bear app:

bear focus

That is, an outline around the entire switch. We'd want it WordPress style, though.

@afercia
Copy link
Contributor Author

afercia commented Jul 25, 2017

Playing a bit: on the Bear app it's nicer because the box-shadow has a different color than the background. Instead, the WordPress box-shadow default color is blue so when is-checked and focused, it looks a bit too big:

screen shot 2017-07-25 at 14 08 55

One option could be adding a first 1 pixel white box-shadow:

screen shot 2017-07-25 at 14 11 49

or maybe use a different color? It's just a matter of a design choice, I think the approach works.

One thing that needs to be changed for a11y is that the "thumb" shouldn't be filled with the background when focused, we should increase the border-width instead. Because:

High Contrast generally replaces default text and border colors and strips out background colors and shadows

@afercia
Copy link
Contributor Author

afercia commented Jul 25, 2017

I'm going with the white + blue box-shadow for now, can be changed of course. Should be tested on IE 11, not possible at the moment: see #1397 and #1863.

I've tested it also with the "hint" visible (see screenshot below on the left) and in Windows High Contrast mode where the focus state works because the "thumb" is filled increasing the border width (screensnot on the right):

screen shot 2017-07-25 at 15 49 50

@jasmussen
Copy link
Contributor

Love this. Very impressive work. Ship ship ship.

@afercia
Copy link
Contributor Author

afercia commented Jul 25, 2017

The builds fail because of... aliens! 👽

@afercia afercia merged commit 1c7aaf8 into master Jul 25, 2017
@afercia afercia deleted the update/switch-toggle-focus-style branch July 25, 2017 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants