Buttons: CSS Specificity Clean-up

[Copons]

Description

Simplify the Button component's CSS to make it easier to integrate it in external projects.

In particular this PR:

• Removes all !important.
• Reduces the over-specificity by simplifying the disabled and enabled selectors. Some examples:
  • Replace :not(:disabled):not([aria-disabled="true"]) with :enabled.
  • Replace [disabled] with :disabled.
  • Remove the pseudo-selector when obviously not needed.

How has this been tested?

Tested locally on Chrome 69, Firefox 62, Safari 12, Edge 17, and IE 11.

To test this, please smoke-test the editor to make sure the Button behave as expected.

Types of changes

Code quality enhancement

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[jasmussen]

Impressive! AMAZING work getting rid of all those !importants. This is much cleaner.

I would love to approve this, but I'd like Riad and @aduth eyes on this because there's a fair bit of history to why we got to where we are today, for example the hover style should not work on an aria-disabled button (like the up mover on the first block), but this seems to work as intended even with this refactor.

But this will be really nice to get in. Thank you!

[Copons]

there's a fair bit of history to why we got to where we are today

There's probably even more history than you might expect!
Those !importants are based upon some many-years-old code: https://github.com/WordPress/WordPress/blame/master/wp-includes/css/buttons.css#L169-L183

the hover style should not work on an aria-disabled button (like the up mover on the first block)

I guess that works because it's an IconButton, which has its own aria-disabled style.

But this is a good point indeed!
I've searched for aria-disabled and found it in ClipboardButton, that can be rendered either as an IconButton or as a Button, and my changes here would probably "break" it.
Though, the only ClipboardButton usage with aria-disabled is also an icon (PostPermalink).

I'll test again by forcing some Buttons to be aria-disabled and see what happens!

[aduth]

I guess that works because it's an IconButton, which has its own aria-disabled style.

Related : #9588 (comment) , #9702

In that I don't know that these styles ought to be implemented specific to IconButton vs. Button generally,

[gwwar]

Anything we can do to help? Were there specific items we needed to test for visual regressions for?

[jasmussen]

From a quick reading, the next step to get this PR merged is in this comment:

I'll test again by forcing some Buttons to be aria-disabled and see what happens!

If there are no visual regressions to find, it's probably totally okay to move forward even though these changes might theoretically break some thing. As Andrew suggests, the Button and IconButton components are due for a refactor anyway, so let's not let perfect be the enemy of good.

[Copons]

Sorry for the delay folks!

I tried applying aria-disabled="true" to some buttons and I can confirm that this would cause a regression: aria-disabled="true" buttons would not look disabled (hover or not).

My change incorrectly assumed that disabled and aria-disabled="true" were always applied together, but in fact, even if there are no examples in the codebase, it can happen (or it's at least expected) that aria-disabled="true" is applied alone.
In this case, the :disabled and :enabled pseudo-classes wouldn't correctly select the element.

The solution is simple enough: replace &:disabled with &:disabled, &[aria-disabled="true"].
Compared to the original code (a convoluted :not():not()), this would still look a bit cleaner.

Though, what is the a11y reason behind the possibility of using aria-disabled="true" without disabled?
If the aria-disabled="true" style is the same as disabled, wouldn't disabled be enough?

AFAIK (but I'm not an a11y expert), disabled would be skipped by the screen reader, while aria-disabled="true" would be focusable, read, and reported as disabled.
Is this why we want to have both choices?

[jasmussen]

Though, what is the a11y reason behind the possibility of using aria-disabled="true" without disabled?

@afercia can you add some color commentary here?

I don't know that we use this aspect anywhere but here:

See the Up arrow. Because it's the first block in the list, you can't move it upwards. This button is aria-disabled, but not disabled. I can't recall why it's not both, but I seem to recall there was a good reason.

[afercia]

disabled would be skipped by the screen reader, while aria-disabled="true" would be focusable, read, and reported as disabled.

Yes and no :) That's certainly true when using the Tab key to navigate content. However, screen readers offer the ability to navigate any element in the page using the arrow keys. When doing so, even a disabled button would be read out and announced, like any other element in a page (say, a paragraph).

That said, I seem the reason why the mover buttons are not really disabled is 6b7b163

See also the comment on the related PR #730 (comment)

So it wasn't implemented this way for an a11y requirement. It was done to repair a problem with focus. From an a11y perspective I have no strong objections as long as the button does nothing and is communicated both visually and semantically as "disabled".

Note there's also an aria-describedby attribute pointing to clarifying messages, e.g. Block Paragraph is at the beginning of the content and can’t be moved up.

If you want to try to make it really disabled with a disabled attribute, I have no objections as long as there are no regressions with focus. That means it should be tested thoroughly using only the keyboard especially with old browsers (IE11).

[afercia]

Just an additional note:

why it's not both

Ideally, disabled is preferred and shouldn't be used together with aria-disabled.

[Copons]

@afercia thank you so much for the explanation and the context!
I was definitely looking at it from the wrong point of view. Since the distinction was created to solve a cross-browser focus issue, the risk of creating regressions is simply too high for such a widespread change.

I'll replace &:disabled with &:disabled, &[aria-disabled="true"] ASAP.

[Copons]

Note: these two are losing the &.is-primary selector because we are already nested inside &.is-primary {} (line 63).

[gwwar]

re-ran the failing suite here, looks green now

[youknowriad]

LGTM 👍 I didn't notice any regression on my testing but it's not easy to check everything.

Awesome job

[youknowriad]

this is probably useless because we use auto-prefixer

[youknowriad]

Tests are green. Github is stuck somehow :)

[Copons]

Thanks for reviewing and merging @youknowriad!
I was about to do that myself but got caught up (you know how it is these days 🙂).

[afercia]

Looks like this change broke the focus style on buttons that are links. Seems only Chrome understands :focus:enabled on a link. Firefox, Safari, Edge, and IE 11, don't.

I tend to think the Chrome behavior is wrong, as the :enabled pseudo class is supposed to work for elements that have a corresponding disabled state. Links don't have a disabled state.

Will create an issue.