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

.has-vivid-cyan-blue-background-color.has-vivid-cyan-blue-background-color #12986

Closed
manake opened this issue Dec 18, 2018 · 18 comments · Fixed by #15167
Closed

.has-vivid-cyan-blue-background-color.has-vivid-cyan-blue-background-color #12986

manake opened this issue Dec 18, 2018 · 18 comments · Fixed by #15167
Labels
Needs Technical Feedback Needs testing from a developer perspective. [Type] Code Quality Issues or PRs that relate to code quality

Comments

@manake
Copy link

manake commented Dec 18, 2018

.has-vivid-cyan-blue-background-color.has-vivid-cyan-blue-background-color {
    background-color: #0693e3;
}

The selector is duplicated in /wp-includes/css/dist/block-library/style.css. It should not be.

@swissspidy swissspidy added [Type] Code Quality Issues or PRs that relate to code quality Needs Technical Feedback Needs testing from a developer perspective. labels Dec 18, 2018
@swissspidy
Copy link
Member

Class names are doubled to increase specificity to assure colors take effect over another base class color.

See https://github.com/WordPress/gutenberg/blob/cddee64697706f6b797a9fba39d075eff95ee0b0/packages/block-library/src/style.scss and discussion at #12005.

Certainly looks odd though.

@manake
Copy link
Author

manake commented Dec 18, 2018

Ok, I understand but in such case the problem is that https://wordpress.org/gutenberg/handbook/designers-developers/developers/themes/theme-support/ says that

Themes are responsible for creating the classes that apply the colors in different contexts. Core blocks use “color” and “background-color” contexts.

.has-strong-magenta-background-color {
    background-color: #313131;
}

.has-strong-magenta-color {
    color: #f78da7;
}

so this instruction is wrong?

@youknowriad
Copy link
Contributor

That's a good question, do we still need the duplicate now that we did some work on the CSS specificity? cc @jasmussen @kjellr

Should we update the docs?

@jasmussen
Copy link
Contributor

do we still need the duplicate now that we did some work on the CSS specificity?

From a quick attempt, we don't: #15104.

I would very much like a sanity check on that one — it appears to be working as intended in my tests, and from looking at the CSS, the reduced specificity of the button block selector should mean it works as intended. But I would very much like testing regardless, in case I'm missing something obvious.

If it works as I hope it does, I'm very happy that the big PR I struggled with is yielding benefits now!

@m-e-h
Copy link
Member

m-e-h commented Apr 23, 2019

Themes are responsible for creating the classes that apply the colors in different contexts. Core blocks use “color” and “background-color” contexts.

The docs seem correct to me. I'm not sure what theme-created classes have to do with the specificity of the core color classes.

do we still need the duplicate

Pretty sure we do. see #15104 (comment)
It's likely the extra specificity will always be needed here. I don't think it's a bad thing though.

I'm thinking this has more to do with the confusion that duplicate classes might cause?

If so, there are other ways to add specificity. The duplicate classes seemed like the best option to me.
Here are some other options:

Single class

/* specificity = 10 */
.has-luminous-vivid-amber-color {
	color: #fcb900;
}
  • Not strong enough to handle link states etc.

Duplicate class

/* specificity = 20 */
.has-luminous-vivid-amber-color.has-luminous-vivid-amber-color {
	color: #fcb900;
}
  • It can be used anywhere in the document
  • Specificity can be increased at any time by adding the class again.
  • Can be confusing for those not familiar with this browser spec.

:root qualifier

/* specificity = 20 */
:root .has-luminous-vivid-amber-color {
	color: #fcb900;
}

This is my method of choice for personal projects.

  • It can be used anywhere in the document
  • Specificity can be increased by stacking :root selectors together. ie :root:root.
  • I'm guessing this won't work with the editor-styles morphing.

:not() qualifier

/* specificity = 20 */
:not(.anythingWorks) .has-luminous-vivid-amber-color {
	color: #fcb900;
}
  • It can be used anywhere in the document
  • Not scalable. If, in the future, specificity needs strengthened we'll need to resort to one of the methods above because stacking :not() only counts the specificity of the strongest :not.

Parent class qualifier

/* specificity = 20 */
.wp-block-button .has-luminous-vivid-amber-color,
.wp-block-pullquote .has-luminous-vivid-amber-color {
	color: #fcb900;
}
  • Not scalable. Will need to create a new selector for additional uses.
  • If, in the future, specificity needs strengthened we'll need to resort to one of the methods above.
  • Imposible to tell by looking at the code if there's a reason for the nesting other than specificity. "Will something break if I use this class outside of wp-block-button?"

Element qualifier

/* specificity = 11 */
a.has-luminous-vivid-amber-color,
blockquote.has-luminous-vivid-amber-color {
	color: #fcb900;
}
  • Not strong enough
  • Not scalable. Will need to create a new selector for additional uses.
  • If, in the future, specificity needs strengthened we'll need to resort to one of the methods above.
  • Won't always be the correct target in the editor view.
  • Imposible to tell by looking at the code if there's a reason for the nesting other than specificity.

!important

.has-luminous-vivid-amber-color {
	color: #fcb900 !important;
}
  • It can be used anywhere in the document

@m-e-h
Copy link
Member

m-e-h commented Apr 23, 2019

I see now what you mean by "updating the docs". I guess they should be updated since copying and pasting what's there now wouldn't work for links.

Pretty sure link pseudo-classes are the only area giving us an issue. So we could always just add those specific pseudo-classes.

So something like this maybe:

.has-luminous-vivid-amber-color,
.has-luminous-vivid-amber-color:link,
.has-luminous-vivid-amber-color:visited {
    color: #fcb900;
}

:link and :visited is just a less verbose version of :hover,:focus, :active, and :visited. But the long version could be used too if it's determined :link is too obscure here as well.
Some day we could use :any-link to cover all of them but not with IE.

I personally like a higher specificity on every utility class but I also understand keeping things familiar for the average developer. This may be a good compromise.

What do you think @jasmussen? Wanna try this in #15104 or are we fine with the duplicate classes?

@chrisvanpatten
Copy link
Contributor

The docs seem correct to me. I'm not sure what theme-created classes have to do with the specificity of the core color classes.

It seems like the problem might be themes that wish to use the core color names, but override the rendered colors.

Not saying that's a good thing — but it seems to be the edge case being solved for.

@m-e-h
Copy link
Member

m-e-h commented Apr 23, 2019

Yeah, just now noticed that the example code in the docs didn't match up with the actual code. I could see that causing an issue for some every now and then.

Btw, where you been @chrisvanpatten ?! 😄

@jasmussen
Copy link
Contributor

What do you think @jasmussen? Wanna try this in #15104 or are we fine with the duplicate classes?

I'm generally fine with whatever solves the problem at hand.

Though I do think that the following might be much easier to parse:

.has-luminous-vivid-amber-color,
.has-luminous-vivid-amber-color:link,
.has-luminous-vivid-amber-color:visited {
    color: #fcb900;
}

and I can totally take a stab at that, including updating the docs to match (in a new PR), but before I go about that, is there consensus that this will work for us? You sure :link will cover the hover style?

And yes, I think :link is easier to parse than the duplicated selector, that really looks like a typo. And if anything, we can just explain what :link does in a code comment.

Some day we could use :any-link to cover all of them but not with IE.

For better or worse, that some day might be sooner than you think!

@kjellr
Copy link
Contributor

kjellr commented Apr 24, 2019

Though I do think that the following might be much easier to parse:

Just wanted to second this. Since the doubling up of classnames is somewhat rarely seen, I'd support either a re-write along these lines, or just some more explanatory code comments above.

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Apr 24, 2019

Btw, where you been @chrisvanpatten ?! 😄

I got a new job! Still lurking (and reading most tickets) but haven't had as much time to actively contribute. Hoping we start onboarding our platform to Gutenberg later this year so I can get back into it :)

Though I do think that the following might be much easier to parse:

I really like this solution. It's much more explicit, which is nice!

@m-e-h
Copy link
Member

m-e-h commented Apr 24, 2019

You sure :link will cover the hover style?

Pretty sure.. I mean, it'll definitely count towards the hover style. And it should be the same specificity as what we have now, 20.

But I'm wondering, what happens when a theme does something like

.entry a { /* specificity: 11 */ }

.entry a:hover { /* specificity: 21 */ }

Maybe something like this would be safer:

.has-luminous-vivid-amber-color,
a.has-luminous-vivid-amber-color:link,
a.has-luminous-vivid-amber-color:visited {
    color: #fcb900;
}

That would match the 21 specificity.

jasmussen pushed a commit that referenced this issue Apr 25, 2019
This fixes #12986 and is based on the conversation there.

Instead of doubling the selectors to increase specificity, we are supplying the element as well.
@jasmussen
Copy link
Contributor

So, I took a stab at this just now, and created a draft pull request: #15167. It seems to work as intended so far:

Screenshot 2019-04-25 at 10 47 46

Screenshot 2019-04-25 at 10 47 42

The reason it's a draft is that I realized that the syntax discussed is very biased towards foreground colors, but doesn't take into account background colors. So the question becomes: do we still need high specificity for the background colors, or is it okay for those to have lower specificity?

One thing we could do to increase the specificity of the background color, while still avoiding the duplicate selector, would be to do the following:

.has-background.has-pale-pink-background-color {
	background-color: #f78da7;
}

Curious what your thoughts are.

@m-e-h
Copy link
Member

m-e-h commented Apr 25, 2019

These are single function, utility type classes so I don't think more specificity is going to hurt anything.

I think we focused on links color because a lot of times a:hover will be styled with color: and a:hover would override the single class selector. While it's probably not as common for a background-color on an a:hover, it wouldn't hurt to cover that base.

Is .has-background given to all background color instances. I kinda remember an issue about it not being on every block.

@m-e-h
Copy link
Member

m-e-h commented Apr 25, 2019

And just a reminder to check the front-end too @jasmussen. The button links in the editor are inline-styles atts nm. I see now that you have 2 screenshots there. 😄

@jasmussen
Copy link
Contributor

Yep indeed those two screenshots look like one continuous one! But yes, I've remembered it 💪

Is .has-background given to all background color instances. I kinda remember an issue about it not being on every block.

Excellent question. Testing now:

  • Paragraph has it
  • Button has it
  • Cover does not have it, but has .has-background-dim. Though this is present even if you don't explicitly choose a color. But targetting that one seems to have suffucient specificity.
  • Pullquote does not have it, but in its default style it uses an inline style to set border color, and it it solid style it has is-style-solid-color.

So you see where this is going.

Which brings us to an existential question around #15167: is it actually in improvement overall? It looks like if we mean to have additional specificity on at least the background colors, we either have to write a long explicit selector such as this:

.has-background.has-pale-pink-background-color,
.has-background-dim.has-pale-pink-background-color,
.is-style-solid-color.has-pale-pink-background-color {
	background-color: #f78da7;
}

... and we'll likely need to maintain that as well, as additional blocks add colors in a haphazard way ...

OR, we can accept the duplicate classnames:

.has-pale-pink-background-color.has-pale-pink-background-color {
 	background-color: #f78da7;
}

At this point, I'm back to where it feels like if we DO actually need to increase the specificity of those background colors, the duplicate classes even if they look wrong, is still the right way to go.

And if that's the case, then we might as well use that same pattern even for the text colors too, then at least it's consistent.

Any final thoughts before I close that PR and create a new one that just augments the code comment instead?

@m-e-h
Copy link
Member

m-e-h commented Apr 26, 2019

At this point, I'm back to where it feels like if we DO actually need to increase the specificity of those background colors, the duplicate classes even if they look wrong, is still the right way to go.

I agree. But it is very "typo" looking.
Would something like this cause less confusion?

:root .has-luminous-vivid-amber-color {
	color: #fcb900;
}

It definitely looks more deliberate. And now that I see we've begun accounting for :root in the editor-styles rewrite, this would work if copied and pasted to theme styles.
The above acts the same as html .has-luminous-vivid-amber-color but :root carries the same specificity as a class selector.

Like I mentioned previously :root is my go-to for adding portable specificity.
https://developer.mozilla.org/en-US/docs/Web/CSS/:root

@jasmussen
Copy link
Contributor

I most definitly think :root reads less like a typo. It may be more advanced CSS, but advanced CSS is google-able, whereas the duplicated class definitely looks like a mistake.

Tried it here: 9132137 — I personally like this a lot, and it's working for me:

Screenshot 2019-04-29 at 09 45 07

jasmussen pushed a commit that referenced this issue May 3, 2019
* Try tweaking the specificity of the custom color syntax

This fixes #12986 and is based on the conversation there.

Instead of doubling the selectors to increase specificity, we are supplying the element as well.

* Try :root

* Address feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Technical Feedback Needs testing from a developer perspective. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
7 participants