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

Buttons: CSS Specificity Clean-up #10031

Merged
merged 3 commits into from Oct 4, 2018

Conversation

@Copons
Contributor

Copons commented Sep 19, 2018

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.

@Copons Copons self-assigned this Sep 19, 2018

@Copons Copons requested a review from youknowriad Sep 19, 2018

@gwwar gwwar requested a review from WordPress/gutenberg-core Sep 20, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@Copons

Copons Sep 21, 2018

Contributor

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!

Contributor

Copons commented Sep 21, 2018

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

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 21, 2018

Member

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,

Member

aduth commented Sep 21, 2018

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,

@mtias mtias added this to the 4.0 milestone Sep 25, 2018

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 25, 2018

Contributor

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

Contributor

gwwar commented Sep 25, 2018

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

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 26, 2018

Contributor

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.

Contributor

jasmussen commented Sep 26, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Copons

Copons Sep 26, 2018

Contributor

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?

Contributor

Copons commented Sep 26, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 26, 2018

Contributor

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:

screen shot 2018-09-26 at 11 50 36

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.

Contributor

jasmussen commented Sep 26, 2018

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:

screen shot 2018-09-26 at 11 50 36

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

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 26, 2018

Contributor

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).

Contributor

afercia commented Sep 26, 2018

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

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 26, 2018

Contributor

Just an additional note:

why it's not both

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

Contributor

afercia commented Sep 26, 2018

Just an additional note:

why it's not both

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

@Copons

This comment has been minimized.

Show comment
Hide comment
@Copons

Copons Sep 26, 2018

Contributor

@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.

Contributor

Copons commented Sep 26, 2018

@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.

color: $white !important;
background-size: 100px 100% !important;
&.is-busy:disabled,
&.is-busy[aria-disabled="true"] {

This comment has been minimized.

@Copons

Copons Sep 26, 2018

Contributor

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

@Copons

Copons Sep 26, 2018

Contributor

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

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 27, 2018

Contributor

re-ran the failing suite here, looks green now

Contributor

gwwar commented Sep 27, 2018

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

Show outdated Hide outdated packages/components/src/button/style.scss Outdated
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 4, 2018

Contributor

Tests are green. Github is stuck somehow :)

Contributor

youknowriad commented Oct 4, 2018

Tests are green. Github is stuck somehow :)

@youknowriad youknowriad merged commit a2285bf into master Oct 4, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@youknowriad youknowriad deleted the update/button-css-cleanup branch Oct 4, 2018

@Copons

This comment has been minimized.

Show comment
Hide comment
@Copons

Copons Oct 4, 2018

Contributor

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

Contributor

Copons commented Oct 4, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment