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

CSS should not use :not #11677

Closed
jdevalk opened this issue Nov 9, 2018 · 17 comments
Closed

CSS should not use :not #11677

jdevalk opened this issue Nov 9, 2018 · 17 comments
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Code Quality Issues or PRs that relate to code quality

Comments

@jdevalk
Copy link
Contributor

jdevalk commented Nov 9, 2018

Currently, we have a lot of CSS that is very non-performant, but also not very readable. Specifically, I think we shouldn't use :not as much, in things like:

.wp-block-button__link:not(.has-background):active

and

.wp-block-button__link:not(.has-text-color),.wp-block-button__link:not(.has-text-color):active,.wp-block-button__link:not(.has-text-color):focus,.wp-block-button__link:not(.has-text-color):hover{
    color:#fff
}

This is very slow for browsers to parse, but they're also very hard to read. Our example CSS is going to be built upon a lot, so it needs to be a bit better.

@jdevalk jdevalk changed the title CSS should not use .not CSS should not use :not Nov 9, 2018
@jonoalderson
Copy link

jonoalderson commented Nov 9, 2018

Yeah, the specific of pattern .adaptive-class:not(.has-this-class) seems a little back-to-front.

.has-this-class should set the default behaviour, and then .adaptive-class should override it.

Trying to understand why this is the way it is at present, I suspect that this approach has been used to minimise the number of classes, and to avoid relying on class chaining (e.g., .main-class.adaptive-class).

@chrisvanpatten
Copy link
Member

I totally agree. I'm not sure how much is actionable before 5.0 but I think this is a great goal. not() is conceptually cool, but feels a little bit like a code smell to me.

@chrisvanpatten
Copy link
Member

Also I suspect that some of this is complicated by the specificity mismatch between editor styles and block library styles, as outlined in #11200.

@chrisvanpatten chrisvanpatten added the [Type] Code Quality Issues or PRs that relate to code quality label Nov 9, 2018
@aduth
Copy link
Member

aduth commented Nov 9, 2018

Could you provide references for the specific performance concern?

@timelsass
Copy link
Contributor

I've never heard of negative performance impacts with a single negation for browser parsing. I could see chaining dozens of negations together causing some performance loss though, but I'd think chaining together dozens of class selectors would be a similar impact either way. I too would be interested if some citation and performance tests of the example selectors provided before/after could be added here to outline the issue being with performance being described.

For me, in terms of readability, I think it really depends on the context. I usually find :not selectors to give clear meaning when working with other modular styles on a higher level. That said - I pretty much only use them if I'm writing scss if I'm in a nested context, and needing to break out more than a single layer, but when writing plain CSS it's rare to use them. Using a preprocessor like scss, I think it's important that the development styles are what are the most human-readable/logical opposed to output. I haven't looked at how/where those styles are created specifically, so I can't say one way or another, but am still very interested to hear about the performance loss from using a single negation in a selector.

If this becomes a standard for core to take in writing styles moving forward post 5.0, perhaps someone could update the coding standards documentation and throw in a lint to check and warn to prevent future cases of this. Overall though I agree, I don't think trying to untangle an issue just for preference to readability before a major release is a good idea.

This does bring up a missing facet in documentation for wp.org though - there aren't really any publicly declared scss coding standards dictated for people to follow (at least none that I came across). I think having things set like level of nesting that's acceptable, whether or not it's okay to break selector words apart like .st { &yle {...} &ore {...} } are just a few examples of basic standards that projects generally have in place. Given the increased usage in scss styles for Gutenberg - this seems like something that should be done somewhere.

@Fujix1
Copy link

Fujix1 commented Nov 10, 2018

I need a class something like .wp-block-list for ul>li list outputs to avoid :not. Currently it is output without any class.
List tag is too generic and already used in the gallery block (with ul.wp-block-gallery).
Now we have to code like this to avoid collision:

ul:not(.wp-block-gellery) { li { ... } }

If a different block uses ul>li in the future, we will have to add its class to exclude it.
Beside the performance issue, it is important to make CSS selector to be selective not exclusive.

@afercia
Copy link
Contributor

afercia commented Nov 10, 2018

I'd be more concerned about specificity. Historically, CSS specificity has been a problem in core. There are tools to analyze the overall structure of a stylesheet. For example, passing the block-library/style.css from the Make core blog (it may not be up to date) to cssstats.com:

https://cssstats.com/stats?link=https%3A%2F%2Fmake.wordpress.org%2Fcore%2Fwp-content%2Fplugins%2Fgutenberg%2Fbuild%2Fblock-library%2Fstyle.css this is the specificity graph:

block-library style

Many selectors are around a specificity of ~30, some of them above 40-50. More importantly, selectors with low specificity should come first in the source, followed by selectors with higher specificity and ending with overrides etc. The resulting graph should show an upward trend. Instead, it's clearly a downward trend, which is an anti-pattern.

When it comes to the admin stylesheets, specificity is even higher. For example, this is the specificity graph for components/style.css: https://cssstats.com/stats?link=https%3A%2F%2Fmake.wordpress.org%2Faccessibility%2Fwp-content%2Fplugins%2Fgutenberg%2Fbuild%2Fcomponents%2Fstyle.css

components style

Aside: 55 unique colors are a bit too many. In the past there has been an ongoing effort to reduce the amount of colors used in core, but seems we're reintroducing a lot of grey shades.

This is the specificity graph for editor/style.css: https://cssstats.com/stats?link=https%3A%2F%2Fmake.wordpress.org%2Faccessibility%2Fwp-content%2Fplugins%2Fgutenberg%2Fbuild%2Feditor%2Fstyle.css

editor style

Aside: in the admin stylesheets I see things like:
.components-button.is-tertiary:not(:disabled):not([aria-disabled=true]):not(.is-default):hover
or
.components-dropdown-menu__menu .components-dropdown-menu__menu-item:focus:not(:disabled):not([aria-disabled=true]):not(.is-default)

Admittedly, these two examples are edge cases but they're a symptom of a general trend towards high selectors specificity. I guess overriding some admin CSS rules will be a bit challenging for plugin authors. Same applies to the front-end styles. It's very likely this will lead to even higher specificity, resulting in an overall higher complexity, maintenance and performance issues.

@jsnajdr
Copy link
Member

jsnajdr commented Nov 13, 2018

Regarding performance, there is no reason why selectors like

.wp-block-button__link:not(.has-background)

should be slower than ones without :not.

Selectors that can be slow, on the other hand, are descendant selectors whose right-most part matches almost every element:

.wp-block-button__link div:not(.has-background)
.wp-block-button__link *

But our selector in question is not of this kind: its right-most part is a very selective condition that just happens to have the form A and not B instead of just A.

Here is a Stack Overflow question answered by a Gecko engineer that explains why is it so: https://stackoverflow.com/questions/5797014/why-do-browsers-match-css-selectors-from-right-to-left

@chrisvanpatten
Copy link
Member

Personally I'm less concerned about performance; I expect there are other yaks we can shave w/r/t performance that will yield bigger results.

The problem is that the added specificity can make it really tough to override styles, makes the CSS harder to read and mentally parse, and is often a sign that there are architectural issues in the way the CSS is structured.

I'd also say that while I don't love not selectors in the admin, I think there's a better rationale there. It's in the front-end styles where I think we should be avoiding it, to make CSS implementation/overrides easier for users.

@afercia
Copy link
Contributor

afercia commented Nov 13, 2018

Well also in the admin, unless we want to make plugin developers life harder 😉 Plugin developers are users.

@chrisvanpatten
Copy link
Member

@afercia I'd argue that in the admin generally our goal should be to encourage people to use Gutenberg components the way they were designed, for greater consistency — making it harder to change them is a feature, in that case. If you're creating new UI, you probably won't be affected too much.

@afercia
Copy link
Contributor

afercia commented Nov 13, 2018

@chrisvanpatten maybe I wasn't clear.

encourage people to use Gutenberg components the way they were designed, for greater consistency — making it harder to change them is a feature

Now one wants to change the components provided by Gutenberg. However, if you ever have tried to build a custom block, you may have noticed all the Gutenberg styles inherits in your custom block. Also theme styles greatly impact custom blocks, because of the way they're added to the page.

We've already built custom blocks and our experience is that Gutenberg styles often conflict with elements in a custom block. From a simple paragraph used as a description to input fields, to more advanced UIs.

There's only one way to override the Gutenberg / Theme styles: by increasing specificity or by using ! important 😱Both are highly undesirable.

@chrisvanpatten
Copy link
Member

@afercia I know we definitely have some issues with component styles leaking into blocks but are those specifically related to usage of not()? I would guess those are just more generally related to weakly-scoped selectors; not() usage is a small part of that, but my sense is that it involves other selectors too.

#10178 and #11779 cover some of these concerns too.

@afercia
Copy link
Contributor

afercia commented Nov 13, 2018

It's a general issue, not limited to :not(), and more related to selectors with unnecessary high specificity. :not() contributes to increase specificity. CSS specificity in WordPress is a historical issue, and I'd like to see Gutenberg to not do again the same mistakes WordPress has been doing for years.

@m-e-h
Copy link
Member

m-e-h commented Nov 13, 2018

@afercia is exactly right. This is less about :not() and more about how :not() turns a single class selector into a two class selector in terms of specificity.

A CSS best practice would be a single class selector per style. A theme would then only need one targeted selector to style an element.
This becomes especially important when one of the primary "features" of the software is that it allows themes to override styles.

If selectors all have equal specificity, :not() would be unnecessary because we could rely on source order to decide which style gets used.

Unfortunately single class selectors will be tough in some cases since the HTML is structured like this .is-style-squared .wp-block-button__link rather than this .wp-block-button__link--squared.

I'm going to open some PRs and bring up some specific examples in #11779 where lessening the specificity might be possible.

@brettsmason
Copy link

Completely agree with this, and the requirement for specificity improvements in general.

I think it would also be useful if every element within a block had its own class. For example, the audio block. The figcaption and audio elements should have their own class. That way they wouldn't have to be nested selectors.

@youknowriad
Copy link
Contributor

Seems like this issue served its purpose and we all agree that we want less specificity when possible, that said, it seems like a lot of improvements have been made already and that we should create specific actionable issues for the places where things can be improved still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests