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.

Sign up for GitHub

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

Jump to bottom

Button: Improve the aria-disabled focus style #62480

Merged
merged 9 commits into from
Aug 6, 2024
Merged

Button: Improve the aria-disabled focus style #62480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e4ecbcb
Improve Button aria-disabled focus style.
afercia Jun 11, 2024
b05c682
Add changelog entry.
afercia Jun 11, 2024
52c2b45
Adjust changelog entry after rebase.
afercia Jun 19, 2024
41a4cec
Adjust changelog entry after rebase.
afercia Jun 28, 2024
14ccb32
Introduce new gray-500 for disabled controls.
afercia Jul 23, 2024
de838e0
Use gray-500 for all disabled button variants.
afercia Jul 23, 2024
84417c3
Fix is-pressed disabled styles regression
ciampo Jul 31, 2024
f77f13c
Revert the introduction of the gray-500 variable
ciampo Jul 31, 2024
496e303
Merge branch 'trunk' into fix/disabled-button-focus-style
mirka Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- `ColorPalette`: Remove extra bottom margin when `CircularOptionPicker` is unneeded ([#63961](https://github.com/WordPress/gutenberg/pull/63961)).
- `CustomSelectControl`: Restore `describedBy` functionality ([#63957](https://github.com/WordPress/gutenberg/pull/63957)).
- `Button`: Improve the aria-disabled focus style ([#62480](https://github.com/WordPress/gutenberg/pull/62480)).
- `Modal`: Fix the dismissal logic for React development mode ([#64132](https://github.com/WordPress/gutenberg/pull/64132)).

### Enhancements
Expand Down
53 changes: 32 additions & 21 deletions packages/components/src/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,10 @@
}

&[aria-expanded="true"],
&:hover {
&:hover:not(:disabled, [aria-disabled="true"]) {
color: $components-color-accent;
}

// Unset some hovers, instead of adding :not specificity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading this comment, there could be some unintended consequences across the editor in terms of rule specificity with these changes

Copy link
Contributor

Choose a reason for hiding this comment

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

The CSS that was replaced by this was &:not(:disabled):not([aria-disabled="true"]):not(.is-secondary):not(.is-primary):not(.is-tertiary):not(.is-link):hover; I suspect that this was just intended for CSS simplification. It's always possible there's some unexpected consequences, but I don't think it's a significant concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reference: https://github.com/WordPress/gutenberg/pull/19344/files#diff-0688ad494526b3b921c4444dc8bc6f63efc4ad74c7c1123b061490cfc57f293a

&:disabled:hover,
&[aria-disabled="true"]:hover {
color: initial;
}

// Focus.
// See https://github.com/WordPress/gutenberg/issues/13267 for more context on these selectors.
&:focus:not(:disabled) {
Expand Down Expand Up @@ -87,7 +81,6 @@
color: rgba($white, 0.4);
background: $components-color-accent;
border-color: $components-color-accent;
opacity: 1;
outline: none;

&:focus:enabled {
Expand Down Expand Up @@ -132,7 +125,6 @@
color: $gray-600;
background: transparent;
transform: none;
opacity: 1;
}
}

Expand Down Expand Up @@ -206,17 +198,22 @@
&:not(.is-primary):not(.is-secondary):not(.is-tertiary):not(.is-link) {
color: $alert-red;

&:hover:not(:disabled) {
&:hover:not(:disabled, [aria-disabled="true"]) {
color: darken($alert-red, 20%);
}

&:focus:not(:disabled) {
&:focus {
box-shadow: 0 0 0 var(--wp-admin-border-width-focus) $alert-red;
}

&:active:not(:disabled) {
&:active:not(:disabled, [aria-disabled="true"]) {
background: $gray-400;
}

&:disabled,
&[aria-disabled="true"] {
color: $gray-600;
}
}
}

Expand Down Expand Up @@ -244,6 +241,11 @@
&:focus {
border-radius: $radius-block-ui;
}

&:disabled,
&[aria-disabled="true"] {
color: $gray-600;
Copy link
Contributor

@ciampo ciampo Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we wanted to retain the original design, we could also use color-mix() to add an exact amount of opacity to the default text color

Copy link
Contributor

@ciampo ciampo Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, looking at other parts in the repo, $gray-700 could be a better value here, should we not go for the color-mix() and opacity route.

But I'll let design folks comment with better context

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not entirely clear to me why the default and secondary variant use inconsistent color methods (opacity vs specific value). The opacity method seems to pre-date the specific value, so it's probably fine to change this up.

Copy link
Contributor

Choose a reason for hiding this comment

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

$gray-700 would raise the color contrast to be more visible than I think it should be for a disabled control; we still need to pretty clearly convey that this control is disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, then, you suggest using solid color? Which one would be the correct one, the 600 or 700 gray variant? (or any other ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a preference on the opacity vs solid color question; but for a solid color, I'd say 600.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the reasons why I would prefer solid colors over colors with opacity or alpha transparency is that measuring the color contrast ratio is way easier with solid colors. @jameskoster I understand some blocks e.g. the cover text need opacity/alpha but for user interface controls colors I'd prefer to always use solid colors. Any reason, from a design perspective, to not do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

A solid color seems more intentional, but might not work as well when the button appears on a non-white background. However opacity isn't perfect in that regard either, so changing this is probably okay. gray-600 seems a good fit.

Copy link
Member

Choose a reason for hiding this comment

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

As we start adopting more structured theming systems (not necessarily the Theme component, but any system that would allow arbitrary background colors), it would definitely be easier to programatically ensure sufficient contrast if we used solid colors. So if contrast is ever a concern, I would recommend avoiding opacity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mirka, that's a very good point I'd support it totally.

}
}

&:not(:disabled, [aria-disabled="true"]):active {
Expand All @@ -253,7 +255,7 @@
&:disabled,
&[aria-disabled="true"] {
cursor: default;
opacity: 0.3;
color: $gray-600;
}

&.is-busy,
Expand All @@ -266,7 +268,6 @@
@media (prefers-reduced-motion: reduce) {
animation-duration: 0s;
}
opacity: 1;
background-size: 100px 100%;
/* stylelint-disable -- Disable reason: This function call looks nicer when each argument is on its own line. */
background-image: linear-gradient(
Expand Down Expand Up @@ -332,20 +333,30 @@

// Toggled style.
&.is-pressed {
color: $components-color-foreground-inverted;
background: $components-color-foreground;
&,
&:hover {
color: $components-color-foreground-inverted;
&:not(:disabled, [aria-disabled="true"]) {
background: $components-color-foreground;
}
}

&:disabled,
&[aria-disabled="true"] {
color: $gray-600;

&:not(.is-primary):not(.is-secondary):not(.is-tertiary) {
color: $components-color-foreground-inverted;
background: $gray-600;
}
}

&:focus:not(:disabled) {
box-shadow: inset 0 0 0 1px $components-color-background, 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent;

// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 2px solid transparent;
}

&:hover:not(:disabled) {
color: $components-color-foreground-inverted;
background: $components-color-foreground;
}
}

svg {
Expand Down
Loading