Try: Move color and font size from captions to theme

[jasmussen]

This PR moves the color and font size styles for captions to a separate CSS file that themes opt in to.

As a reminder, we have style.scss, which contains structural and base styles that are loaded in the editor and the theme.
There's editor.scss which is only loaded in the editor.
There's theme.scss, which is loaded in the editor and the theme if the theme opts in to them.

Addresses one item surfaced in #12299 (comment), props @joyously.

Here's a caption for at theme that has not opted into theme.scss styles:

Here's a theme that has opted in:

[chrisvanpatten]

❤️ Love this!

[m-e-h]

One small step for caption color. One giant leap for opinionated styles 👨‍🚀 😄

Couple questions while we're here.

1. Why not move the text-align: center to theme.css as well? As the quoted comment here points out, center aligned text isn't the norm default styles have too many styles, styling doesn't match docs #12299 (comment)

2. Why are we repeating our caption styles so often?

.wp-block-audio figcaption {
	@include caption-style();
}

.wp-block-embed figcaption {
	@include caption-style();
}

.wp-block-image figcaption {
	@include caption-style();
}

.wp-block-video figcaption {
	@include caption-style();
}

I understand we're styling figcaption to help out themes that may not have any styles for it at all but those themes that have styled for it, will likely want their figcaption styles used rather than WP's.

Could we not just do a general style that will only apply to those that actually need it?

figcaption {
	@include caption-style();
}

[jasmussen]

Wonderful feedback, thank you Chris and Marty! I have tried to address it in 0666ad8.

But note that the way theme.scss works, there's one for each block, so these, for now, have kept their extended markup. I.e.

.wp-block-audio figcaption {
	@include caption-style-theme();
}

Whereas I took your advice and moved the generic figcaption style to the vanilla editor stylesheet. We don't really have an equivalent of theme.scss for that. @youknowriad what are your thoughts on the changes from a code cleanliness and quality POV?

Thanks all.

[m-e-h]

Keeping the extended markup in theme.css is fine, I think. Added specificity would be expected in "opt-in" styles.

Shaping up the style.css to be a compatibility layer for those that haven't styled blocks and staying out of the way for those that have, seems a good focus here.

In 0666ad8, I see we've got the figcaption {} styles in the editor-styles.scss but I don't see anything for the front-end. Is that intentional?

[jasmussen]

Once again solid feedback. I pushed a fix to move the figcaption style to a place where it actually does get output on the frontend. This is now the status of a WordPress theme that has not opted into block styles using add_theme_support( 'wp-block-styles' );...

Editor:

Theme:

Theme, before this latest commit:

I misremembered when I wrote in the initial description. theme.scss styles are always loaded in the editor, regardless of the theme opting in or not. As you can see in the above screenshots:

• Editor style shows the opininated separator style, the centered small gray caption.
• Theme shows default separator, and non-opinionated figcaption. But it now correctly applies the top and bottom margins to the figcaption, so it isn't all snug and close to the image.

[joyously]

I misremembered when I wrote in the initial description. theme.scss styles are always loaded in the editor, regardless of the theme opting in or not.

This was part of the issue. In order to have the front end match the back end, the theme has to do something. If it's opt-in for the front, it should be opt-in for the back as well. The opinionated styles should only show when the theme opts in. This is especially a problem with colors. You should use a dark theme for your test cases.

And this discussion relates to the other issue about specificity. Styling anything with more than one class should be how the theme overrides the editor styles. If the editor styles had only one class, it would solve a lot of problems. All selectors should be as vague as possible and still get the job done.

[chrisvanpatten]

I misremembered when I wrote in the initial description. theme.scss styles are always loaded in the editor, regardless of the theme opting in or not.
This was part of the issue. In order to have the front end match the back end, the theme has to do something. If it's opt-in for the front, it should be opt-in for the back as well. The opinionated styles should only show when the theme opts in.

Yep, this I think is the fundamental problem… Ignoring any special styles that are used to build the editor itself, the editor should only ever load presentational/block styles that are loaded in both the front-end and back-end.

[m-e-h]

What was the original reason for ignoring a theme's choice and always loading theme.css in the editor?

I remember something about blocks functioning properly but I don't know how valid that is.
If it is the case, that code should be moved somewhere other than theme.css. Right?
Actually I'm not sure I want to get into that discussion here. I've seen too many little fixes like this PR turn into a can of worms. 😕

This PR for figcaption is a good first step. We need more like it. I'll find some time this week to attempt a couple.

[jasmussen]

What was the original reason for ignoring a theme's choice and always loading theme.css in the editor?

I can't recall the specific circumstances, but I know for a fact no malice has ever been intended 😅 — I know that should go without saying, but sometimes it's good to just outright say it: all decisions can be revisited.

In this case, if we're careful and smart about what goes in style.scss and what goes in theme.scss, I don't personally see any blocking reasons for why we can't change it so theme.scss is loaded into the editor and theme only when the theme says so.

The balance to be mindful of is that of ensuring the editor has a sufficient default style. For any theme that is not Gutenberg aware, we have to provide a good experience. Without structural CSS in the Cover block, for example, it won't work at all in older themes. But as is hopefully demonstrated by changes in this PR, we evaluate each and every specific selector and fix them up. In that vein, I'd like to also direct your attention towards #14407 which tackles a tangential but very much editor style related issue. That PR is very much in progress and some of the open questions are around where to put which styles. Any changes we might need to do to theme.scss loading could either happen in that PR, or grow out of it.

[m-e-h]

I know for a fact no malice has ever been intended ;)

Of course not! 😄
I hope my comments don't ever come off as hostile! I'm always smiling while ranting and making snide remarks about code choices. 😅

#14407 looks pretty heavy. I like it!

[jasmussen]

I hope my comments don't ever come off as hostile! I'm always smiling while ranting and making snide remarks about code choices. 😅

Never! I feel grateful that I get to ping you on PRs, your help has been invaluable. The past two years of working on this has just taught me that it's often good to re-state the obvious, if nothing else then as a confirmation ❤️

[jasmussen]

So this one is technically approved and milestoned. But is it actually approved? Since the first approval I made a fair bit of changes. It would be nice to get a second sanity check. It won't make the deadline for 5.3 (today), which is okay, but it'd be nice to merge in right after.

[m-e-h]

@jasmussen I don't think this one is ready yet.

In the editor the margin on figcaption are getting overridden by:

.editor-rich-text__editable {
    margin: 0;

Not sure of the best way to handle that. Add the style again in editor.css with figcaption.editor-rich-text__editable {?

I'm also wondering if the margin-bottom: 1em on figcaption is necessary.
If the block has a margin-bottom, the figcaption's margin-bottom does nothing. Even without a specific margin on the block, figure gets a margin-bottom: 1em from the browser.

Can you see a case where the figcaption {margin-bottom: 1em} actually gets used?

[jasmussen]

In the editor the margin on figcaption are getting overridden by:

Interesting, good call. This is actually "fixed" in that other PR I'm working on: #14407 (comment) — but I suppose I can remove it here too, if it doesn't serve a purpose, that is.

I know you're busy @ellatrix but I feel like you must be the best to know what that margin might be necessary for. Do you think we can remove it?

I'm also wondering if the margin-bottom: 1em on figcaption is necessary.

About to head out for the day, but I seem to recall maybe the figure having a zero bottom margin, which means we need it elsewhere, or we need to revisit that zero bottom margin.

Which begs the question, do I fix that here, or do we remember it and fix it as part of #14407 which will no doubt have to touch that aspect?

Thanks.

[m-e-h]

Yeah, #14407 does kinda blur this PR a bit.

but I seem to recall maybe the figure having a zero bottom margin

You're correct. I do see a couple with margins removed:

.wp-block-audio {
    margin: 0;
}

.wp-block-embed {
    margin: 0;
}

I'm guessing this is to remove the default 40px left and right margins added to figure by browsers.
If so, these could be changed to:

margin-left: 0;
margin-right: 0;

Probably best to avoid shorthand properties like margin: 0 anyway and just manipulate the things we need.

[ellatrix]

@jasmussen Sorry, I missed the ping. I don't know why that rule is there, it's not used by rich text for anything. Just styling.

[jasmussen]

Addressed margin feedback in 710b354, thanks @m-e-h.

As a small iterative improvement, I would very much like to ship this PR even if it isn't yet perfect. As mentioned, #14407 is a much more profound effort to further the improvements that this PR started. In other words, instead of working on two slightly overlapping PRs, I'd love to ship this, rebase the other and continue work there.

If that's okay with everyone, I'd love to get this given a final review. Thanks.

[m-e-h]

@jasmussen I know it's taken care of in #14407
but I think it'll need removed here, in this PR, for the figcaption margin.

.editor-rich-text__editable {
    margin: 0;

[kjellr]

Agreed with @m-e-h — I'm still seeing that rule override the caption's top margin, and it's resulting in different spacing than before this PR. Once that's sorted out, we'll be good to go.

[jasmussen]

Great call folks. I've removed it. Aside from being good feedback that I missed (thanks) — I am ever so slightly nervous about removing that property simply because I'm not sure why it was there in the first place. Which makes now a good time to merge this because it's just after the 5.3 release, so it has a week to simmer.

Looks good to me:

Rebased. This should be good to go now.

[kjellr]

🎉 Looks great! Thanks Joen.

[afercia]

The removal of the margin (especially the top one) is a breaking change for custom blocks that reuse RichText within a block. Example:

Before:

After:

It's also breaking the reusable blocks edit mode:

Before:

After:

I suspect there are also other cases.

I'm not sure this kind of changes can be made so lightly. Even if this is "just" CSS, it can break things because it's changing the expected rendering. There has been a recent discussion on Slack about what can be considered a breaking change and what the level of backwards compatibility should be. There's a tendency to consider CSS changes as something that doesn't need backwards compatibility because not part of a "formalized API". Honestly, I really don't mind when discussions like that are made in a so abstract way without considering the real consequences for the developers ecosystem out there.

Trying to keep up with these changes is exhausting and not sustainable, especially when they're not communicated well in advance and with great relevance.

That said, this is breaking also the core blocks. Will create a new issue.