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

Accessibility: Fixed focus state of pressed AM/PM buttons. #15582

Conversation

@nicolad
Copy link
Contributor

commented May 11, 2019

Description

Fixes: #15325 (comment)

Screencast

Peek 2019-05-11 16-53

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@ajitbohra
Copy link
Member

left a comment

LGTM 👍

@afercia
Copy link
Contributor

left a comment

Thank you 👍

@ajitbohra ajitbohra merged commit bd1c616 into WordPress:master May 12, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 13, 2019

@@ -110,6 +110,12 @@
background: $light-gray-300;
border-color: $dark-gray-100;
box-shadow: inset 0 2px 5px -3px $dark-gray-500;
&:focus {
box-shadow:
inset 0 2px 5px -3px $dark-gray-500,

This comment has been minimized.

Copy link
@aduth

aduth May 13, 2019

Member

Where do these values come from? .components-datetime__time-am-button is of type Button, so why do we not simply allow those focus styles to take effect? Or at least to be consistent.

&:focus:enabled {
background: #fafafa;
color: #23282d;
border-color: #999;
box-shadow:
inset 0 -1px 0 #999,
0 0 0 1px $white,
0 0 0 3px $blue-medium-focus;
text-decoration: none;
}

This comment has been minimized.

Copy link
@afercia

afercia May 13, 2019

Contributor

The inset box-shadow value is the one used for the "pressed" state. It needs to be repeated for the :focus state otherwise the new box-shadow values will override it.

This comment has been minimized.

Copy link
@aduth

aduth May 13, 2019

Member

The inset box-shadow value is the one used for the "pressed" state. It needs to be repeated for the :focus state otherwise the new box-shadow values will override it.

That makes sense, thanks. I suppose it may be a separate question then of whether there ought to be the notion of a "pressed" state as an option (prop?) for the Button component. I assume it would be styled roughly equivalent to its :active styles?

My main concern here is maintenance overhead and risk of inconsistencies via duplicated styles for what all ultimately describe a button's appearance.

This comment has been minimized.

Copy link
@afercia

afercia May 13, 2019

Contributor

Looking at the :active style (on the left) and the "pressed" style:

Screenshot 2019-05-13 at 21 09 46

they're very similar and maybe the CSS part could be simplified.

The "pressed" state is used to represent the state of the underlying option via the aria-pressed attribute (based on the isToggled prop) and should not be used for states triggered by user interaction (hover/focus/active).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.