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

Fix/button css specificity #12005

Merged
merged 4 commits into from Nov 19, 2018

Conversation

Projects
None yet
6 participants
@m-e-h
Contributor

m-e-h commented Nov 16, 2018

Description

Reduces specificity in core button styles to ease clashes with theme styles.
This has been explained in further detail here #11779 (comment)

This takes care of some cases mentioned here #11677

And attempts to solve #11998 by giving the Twenty-* themes 1 single button class selector to style .wp-block-button__link.
It also strengthens the specificity of the color and background-color classes so they'll override when the user chooses them.

The button color was added to the parent .wp-block-button element so that the link could inherit the color whether it be a default button with $white text or an outline button with $dark-gray-700 text.
This drys up the code some and allows for single class selector overrides.

The CSS selectors for the Text Color and Background Color settings have been given more specificity to override .wp-block-button__link even with the pseudo-classes .wp-block-button__link:hover.

Duplication of the class selector is used here for two reasons:

  1. These are utility classes that should be usable anywhere.
  2. Less ambiguity than a nested selector. It's obvious by looking what the purpose is.

How has this been tested?

In depth visual tests were done in browser.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

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

This comment has been minimized.

Contributor

chrisvanpatten commented Nov 16, 2018

If the duplicate selectors for the color names are acceptable, I'd also like to apply the same to the font sizes (in a separate PR) so we can partially close #11200.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 19, 2018

Thanks so much for this pull request. Really appreciate it 👏

It's a little bit hard to follow the CSS changes — they seem good on the face of it, but I'd appreciate a sanity check and good testing across at least the vanilla style and twentynineteen, plus all variations.

Here's TwentyNineteen:

screenshot 2018-11-19 at 09 16 47

screenshot 2018-11-19 at 09 16 52

screenshot 2018-11-19 at 09 17 10

Here's the vanilla style:

screenshot 2018-11-19 at 09 18 54

screenshot 2018-11-19 at 09 18 57

screenshot 2018-11-19 at 09 19 04

screenshot 2018-11-19 at 09 19 09

As you can see from the above, all looks good except for the twentynineteen variation preview.
This issue appears only if you view the variation preview when the button contents are empty. However this issue appears to be unrelated to this branch, as it's present in both master and this branch, so in that way it's not a regression. I'll see if I can take a look.

However an initial 👍 👍 to this, and thanks for the PR. @chrisvanpatten what are your thoughts on these changes? The double-specificity hack is super clever, but is it too clever? Do we need a CSS comment to explain what is going on? Are we actually making it easier for themers, or are we just pushing the complexity elsewhere? How sure are we that the :not selector is not performant when used judiciously?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 19, 2018

As a followup, I fixed the double outline issue in #12051.

@m-e-h

This comment has been minimized.

Contributor

m-e-h commented Nov 19, 2018

Thanks for checking this out @jasmussen!
Sorry for not showing better visuals for testing, ran out of time on Friday but wanted to get this PR submitted.

Twenty Nineteen isn't great for testing this since it's not using the default color methods and options.

Also, while not specifically to blame for the results in your Button Preview screenshot, it's worth noting the influence that the mapped body to editor-styles-wrapper can have on some theme's Button Previews. #11997 (comment)

Not sure I'd call the "duplicate selector for the purpose of specificity" a hack. It does behave the way it's supposed to according to the W3C https://www.w3.org/TR/selectors-3/#specificity

Note: Repeated occurrences of the same simple selector are allowed and do increase specificity.

Though we probably could use a CSS comment here since this rule may be new to some.

I'm not aware of any performance issues with the :not() selector.
I've found using :not() to be a perfect choice in a lot of situations. However, I don't think it's a good fit here.
It seems like we're using it here, in lieu of, relying on proper specificity and cascade. This "crutch" gets exposed whenever a theme attempts to style the button without also using :not(). This can be seen with the default themes in the WP 5.0 branch. #11998

@m-e-h

This comment has been minimized.

Contributor

m-e-h commented Nov 19, 2018

As for @chrisvanpatten's suggestion to use repeated classes for font sizes, I'd absolutely recommend that.
I had to fight the compulsion to do it myself as I was editing the style.scss for this PR. 😁
I'd advocate doing it for all single function utility classes.

Might also be worth pulling these style from the style.scss file and creating a proper utility.scss(or other) file to import at the end of the style.scss

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Nov 19, 2018

Might also be worth pulling these style from the style.scss file and creating a proper utility.scss(or other) file to import at the end of the style.scss

YESSSS this would be amazing. Having these classes separated out would be so cool.

The double-specificity hack is super clever, but is it too clever? Do we need a CSS comment to explain what is going on? Are we actually making it easier for themers, or are we just pushing the complexity elsewhere? How sure are we that the :not selector is not performant when used judiciously?

I think this is more straightforward, and easier to parse, vs :not. That said the pattern isn't common, and I think a code comment is reasonable here.

I think this is a sensible path forward. I don't think it's necessarily the best scenario (that would be Gutenberg generating CSS for color palettes and font sizes so we can ensure correct specificity) but this is definitely a step in the right direction. I think anything we can do to simplify the CSS and reduce specificity to the bare minimum will make things easier for themers to override.

@jasmussen

This seems to work as intended, and have a lot of thought put into it, plus sanity checks. I trust that it improves the situation, so let's keep moving. But do add a comment about the double-use of the selector as a CSS comment, something a la // Double the class name to increase specificity ... something in that vein. Thanks!

@jasmussen jasmussen added this to the 4.5 milestone Nov 19, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 19, 2018

I put it in the 4.5 milestone, which is the release candidate milestone. This is a big one. If you feel unsure about an aspect, let me know and we can move it.

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

This partially fixes #11677

The phrasing here is going to trigger automated closure of #11677, which I suspect is not intended.

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

Duplication of the class selector is used here for two reasons:

  • These are utility classes that should be usable anywhere.
  • Less ambiguity than a nested selector. It's obvious by looking what the purpose is.

On a glance, this wasn't obvious to me at all. Is it not also making a very fragile assumption about the specificity of the thing it is meant to override? Could it be enough to do .has-background.has-pale-pink-background-color ? It's only marginally better. My impression would be that if it's always meant to take precedent over all other things as a user-defined value, then maybe it is a candidate for !important, but I preemptively sense the instinctual disgust that triggers 😉

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Nov 19, 2018

then maybe it is a candidate for !important, but I preemptively sense the instinctual disgust that triggers 😉

This is actually a common use of !important (for utility classes in OOCSS). I think that would actually be the ideal scenario except that it limits themers who want to tweak Gutenberg's color values while keeping the color names. I'm not sure they should be doing that, but I expect it won't be uncommon.

@m-e-h

This comment has been minimized.

Contributor

m-e-h commented Nov 19, 2018

I don't disagree @aduth. I think the case could be made for !important here. But is it worth the backlash?

It is making an assumption that what it's overriding will precede it and have no more than two classes or a class and a pseudo-class such as .wp-block-button__link:hover.
I don't know how "fragile" it is though, considering we're in control of the styles it's meant to override and the placement of these styles.

.has-background.has-pale-pink-background-color would probably be just as good.
I guess in theory though, it wouldn't be as scalable in the long term as it would tie the two classes together. Since the class already says the one and only thing it does in it's name, it shouldn't need to be tied to another.

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

considering we're in control of the styles it's meant to override and the placement of these styles.

Yes, but it makes an assumption that the hypothetical future maintainer is aware of these circumstances should they decide naively to add some specificity to the class selectors these are meant to override.

@m-e-h

This comment has been minimized.

Contributor

m-e-h commented Nov 19, 2018

naively to add some specificity to the class selectors these are meant to override.

That's a possibility.

Would be awesome though if WP could right it's past transgressions and make single class selectors it's norm or even the rule.

I'm going to continue to help however I can to move us in that direction.

I'm not against !important here but the 2 class up in specificity does fix where the previous implementation was broken.

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

Can we at least add the explaining comment? #12005 (review)

Otherwise, I'm fine to have this land as-is as an improvement over the current state.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 19, 2018

Added the comment per @aduth's suggestion. Let's continue iterating on this kind of improvements.

@catehstn

This comment has been minimized.

catehstn commented Nov 19, 2018

test/e2e/specs/links.test.js failed on line 302 on PHP latest. Restarting it.

@catehstn

This comment has been minimized.

catehstn commented Nov 19, 2018

All tests are passing - @aduth or @youknowriad do you want to merge this?

@aduth aduth merged commit cddee64 into WordPress:master Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fix/button css specificity (WordPress#12005)
* Reduce specificity of button CSS

* increase specificity of color utility CSS

* Add a comment in the colors stylesheet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment