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

Post Content: Layout styles not applied in editor (e.g. Social Icons, Buttons, etc.) #35376

Closed
ramonjd opened this issue Oct 6, 2021 · 25 comments
Assignees
Labels
[Block] Post Content Affects the Post Content Block [Block] Social Affects the Social Block - used to display Social Media accounts CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Bug An existing feature does not function as intended

Comments

@ramonjd
Copy link
Member

ramonjd commented Oct 6, 2021

Description

The Social Icons Block doesn't receive the appropriate layout styles in the Post Content Block/Site Editor.

In the Block Editor, things look great.

By default the Social Icons block container has the following layout block support styles in the Block Editor:

{
    display: flex;
    gap: var( --wp--style--block-gap, 0.5em );
    flex-wrap: wrap;
    align-items: center;
}

Since #32083 we're rendering these styles via wp_footer in lib/block-supports/layout.php.

However since the styles are no longer rendered inline, and therefore no longer part of the post content, the Post Content Block does not retrieve them. Reverting this change sees things working as expected.

This is because the Post Content Block renders server-side, and uses the REST API to fetch the post content. The REST API’s response doesn’t include wp_footer, so the layout styles are not passed along.

Step-by-step reproduction instructions

  1. In a site where your homepage displays your latest posts, insert a new post and add a Social Icons Block in the Block Editor.
  2. Add a bunch of social media icons. The icons should display in a flex layout, with the appropriate layout styles applied to the container.
  3. Save the file, then open up the Site Editor.
  4. Observe that the Social Icons block does not have the required layout styles applied to the container.

Screenshots, screen recording, code snippet

Block Editor
Screen Shot 2021-10-06 at 12 17 14 pm

Site Editor

Displayed within a Post Content Block

Screen Shot 2021-10-06 at 12 17 21 pm

Environment info

  • WP 5.8, Gutenberg 11.7, TT1 theme

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended [Block] Social Affects the Social Block - used to display Social Media accounts [Feature] Full Site Editing CSS Styling Related to editor and front end styles, CSS-specific issues. [Block] Post Content Affects the Post Content Block labels Oct 6, 2021
@andrewserong andrewserong self-assigned this Oct 7, 2021
@andrewserong
Copy link
Contributor

Just assigning myself to this one. Because the REST API responses don't render out the wp_footer, I'm wondering if we'll be able to append the styles to the end of the_content, but just for REST API requests. I'll have a play 🙂

@andrewserong
Copy link
Contributor

Potential fix in #35413

@andrewserong
Copy link
Contributor

Another potential workaround (if #35450 doesn't appear to be viable) would be for us to swap out the rawHtml usage in the edit view of the Post Content block (when it's a child of the Query block) and render as blocks instead. It might not be as performant, but might (in theory?) be a workaround. I'll continue hacking away at it tomorrow.

@andrewserong
Copy link
Contributor

I've started a WIP workaround PR for this issue in #35863 and noted some of the known issues with the approach. I'll continue hacking around on Monday to see if it looks viable!

@andrewserong
Copy link
Contributor

I think I've fixed most of the issues in #35863 (there's an odd one when clicking between blocks within a Query Loop block, but I believe the issue appears on trunk, so could be something for us to look into separately). I'll keep chipping away at this PR, but happy for feedback if anyone has any on the approach!

@andrewserong andrewserong changed the title Social Icons: layout styles not applied in the Post Content Block Post Content: Layout styles not applied (e.g. Social Icons, Buttons, etc.) Nov 22, 2021
@andrewserong andrewserong changed the title Post Content: Layout styles not applied (e.g. Social Icons, Buttons, etc.) Post Content: Layout styles not applied in editor (e.g. Social Icons, Buttons, etc.) Nov 22, 2021
@andrewserong
Copy link
Contributor

andrewserong commented Nov 22, 2021

Update: the approaches in both #35450 and #35863 turned out not to be viable for a variety of reasons. So, we're back to square one! 😅 I've been doing a bit more thinking about this issue, so to recap:

The issue

Layout block supports styles are server-rendered here and get output to wp_footer here. Because wp_footer isn't included in API responses, there is no way currently to retrieve these generated styles for a given post via the REST API.

Within the block editor, the Post Content block, when rendered within the editor, needs to use the rendered view of the markup, which doesn't currently include the generated styles, nor any way to generate them based on the rendered markup. The only suggestion of the layout styles in the rendered markup is the classname with the random string appended wp-container-619b28e04c1c1.

🤔 Another possible solution to explore

Before I go in refactoring, I have an idea that I'd like to explore, so I thought write up the idea here. Currently for every single one of the blocks that opts-in to the layout block support, we render an additional style tags that's generated based on the layout attributes for that block instance. We tie the styles within that tag to the rendered block with the random wp-container-xxx class.

What if instead of rendering out one style tag for each block instance, we instead generate a number of additional classnames (particular to the layout support attributes) and attach them to the block. Then, we enqueue an additional style tag, if, and only if, it hasn't already been generated. This would have the following benefits:

  • The generated classnames would have names that more logically connect to their behaviour
  • Limit the number of generated style tags to just the number of unique layout settings we're using
  • It would be possible to recreate the layout styles in JS based on the presence of classnames without having to retrieve the style tags themselves.

As an example, here is the markup and corresponding CSS that currently gets generated for a Buttons block within a Post Content block:

Markup and CSS we have now for Layout support

Markup:

<div class="wp-container-619b28e04c2be entry-content wp-block-post-content">
    <div style="--wp--style--block-gap: 7px" class="wp-container-619b28e04c1c1 wp-block-buttons">
       ...
    </div>
</div>

CSS:

.wp-container-619b28e04c2be > * {
    margin-top: 0;
    margin-bottom: 0;
}
.wp-container-619b28e04c2be > * {
    max-width: 650px;
    margin-left: auto !important;
    margin-right: auto !important;
}
.wp-container-619b28e04c1c1 {
    display: flex;
    gap: var( --wp--style--block-gap, 0.5em );
    flex-wrap: wrap;
    align-items: center;
}

💡 Markup and CSS we could have if storing values in CSS class names

Instead, the above could be:

<div class="entry-content wp-block-post-content wp-container-layout__type--default wp-container-layout__contentSize--650">
    <div style="--wp--style--block-gap: 7px" class="wp-block-buttons wp-container-layout__type--flex wp-container-layout__flexWrap--wrap wp-container-layout__orientation--horizontal">
         ...
    </div>
</div>

With generated stylesheets:

.wp-container-layout__type--default {
    margin-top: 0;
    margin-bottom: 0;
}
.wp-container-layout__contentSize--650 {
    max-width: 650px    
    margin-left: auto !important;
    margin-right: auto !important;
}
.wp-container-layout__type--flex {
    display: flex;
}
.wp-container-layout__flexWrap--wrap {
    flex-wrap: wrap;
}
.wp-container-layout__orientation--horizontal {
    align-items: center;
}

💭 Some potential trade-offs

The above example is just one way to do it, and it'd be neater if the prefix was shorter than .wp-container-layout, but the general idea is that we construct class names with a prefix + attribute + value syntax. And once we've generated a style tag for that class, we enqueue it only once. I think the main benefits are that we can then generate styles for the Layout support separately from the rendered markup, the class names are bit more logical than the random uniqid, and for longer posts or pages, we should on balance wind up generating less markup (since we'll wind up using fewer style tags).

The main downside I can think of is a proliferation of class names being added to the markup for the blocks in the rendered view. However, since it would unblock being able to render styles within the site editor (once we replicate the logic for generating the styles in JS), I think on balance this kind of refactor might be worth it?

At any rate, I just thought I'd write up this idea in case it spurs other thoughts or feedback, but I'm happy to do a bit more investigating to see whether or not it might be a viable approach, particularly given that since the other two ideas didn't work out, I'm keen to see what lateral approaches we might be able to take to solving the issue at hand 😀

@ramonjd
Copy link
Member Author

ramonjd commented Nov 22, 2021

One of your terrific "hmmm" moments! 😄

What if instead of rendering out one style tag for each block instance, we instead generate a number of additional classnames (particular to the layout support attributes) and attach them to the block. Then, we enqueue an additional style tag, if, and only if, it hasn't already been generated

If I understand things correctly, we'd trade n style tags, which are related directly to n blocks via a unique id, for one style tag containing a finite set of class definitions.

The main downside I can think of is a proliferation of class names being added to the markup for the blocks in the rendered view.

Bootstrap anyone? :trollface:

If the above assumption I've made is true then I'd hope the reduction in rendered style tags would balance things out as you've already noted.

I like the idea so far, thanks for writing it up.

Just some random thoughts:

  • How do we get the classnames on the blocks themselves? I suppose server side in layout.php for backwards compatibility?
  • The existing preg_replace might be useful, since I guess we'd keep the wp-container-${ id } class.
  • Keeping track of whether a layout class has been already invoked might be done with a temp array?

I reckon it might be worth rattling the cage on this idea and see what falls out.

@andrewserong
Copy link
Contributor

If I understand things correctly, we'd trade n style tags, which are related directly to n blocks via a unique id, for one style tag containing a finite set of class definitions.

Yes, that's the ultimate goal. Even if each class definition has its own style tag, it'd still be a finite set of style tags, rather than an n number of style tags 🤞😀

Bootstrap anyone?

It'll probably look a little Bootstrap-ey and/or a bit like Tailwind CSS, so while it might not be the neatest, at least it's familiar 😄

How do we get the classnames on the blocks themselves? I suppose server side in layout.php for backwards compatibility?

Yes, I was thinking we'd re-use that and the existing preg_replace for injecting the class names. We might need an additional function for generating the class names, though, and to refactor the style generation. But I think if this works we might be able to remove the random wp-container-${ id } class name, if all the styles can be safely generated deterministically.

Keeping track of whether a layout class has been already invoked might be done with a temp array?

That's probably a good way to do it, perhaps in an associative array keyed by the class name.

I reckon it might be worth rattling the cage on this idea and see what falls out.

Thanks @ramonjd! That's just the kind of confidence check I needed before diving down the rabbithole. Cheers! 🙇

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Jan 12, 2022

The switch away from using --wp--style--block-gap in the Navigation block also causes a difference in layout between frontend and editor if the Social block is a child of Navigation block. (Noted here). @andrewserong let me know if you think a separate issue should be added for this.

@andrewserong
Copy link
Contributor

Thanks @glendaviesnz! That might be a slightly different problem than rendering layout styles within the editor for the Post Content block (which is more about how we get the generated styles back into the editor from rendered post content). It could be worth adding a separate issue if we'd like to fix it in the shorter-term, as this current issue doesn't appear to have an obvious short-term solution (the potential future approaches are being discussed in #37495)

@glendaviesnz
Copy link
Contributor

That might be a slightly different problem than rendering layout styles within the editor for the Post Content block

Ah yeh, now that I read the issue more closely it is unrelated I think.

@tellthemachines I went to add a new issue for the problem you mentioned here, but I wasn't able to replicate the issue on trunk with that PR merged, ie. a Social Icons block as a child of Navigation didn't inherit the gap setting of the Navigation block for me so default gap displayed in editor and the frontend.

Are you able to add an issue for this if you are still able to replicate it? I am happy to add the issue if you can provide some replication details.

@tellthemachines
Copy link
Contributor

@glendaviesnz I can still reproduce it on latest trunk, will create an issue.

@tellthemachines
Copy link
Contributor

tellthemachines commented Jan 13, 2022

Actually, I might need a bit of help with this 😅

I can only reproduce the issue on TT2, seemingly because other themes that allow spacing to be set in Navigation also have custom styles overriding the default block styles. I tried testing on empty theme, but it's not set up to allow spacing in Nav. How can I go about enabling that setting in a theme?

UPDATE: my empty theme was an old version, testing on the latest the settings are there! And I can still repro the bug. Creating issue.

UPDATE 2: Here it is: #37935 🙂

@andrewserong
Copy link
Contributor

Thanks @tellthemachines! 🙇

@fwazeter
Copy link
Member

fwazeter commented Feb 3, 2022

What if instead of rendering out one style tag for each block instance, we instead generate a number of additional classnames (particular to the layout support attributes) and attach them to the block. Then, we enqueue an additional style tag, if, and only if, it hasn't already been generated. This would have the following benefits:

I was just working out a solution to this very problem. As a developer who runs an agency, and an early adopter of block themes, it was chronically an issue generating the random .wp-container-id values for a number of reasons. 1) diagnosing CSS files became a nightmare because of all the rendered <style> tags, 2) we'd end up with 80+ duplicate code snippets in pages for layout values and 3) overriding the CSS (e.g. replacing margin-left & margin-right for margin-inline to make a site internationalized by default) became a nightmare.

It was especially worrisome because the intent was to switch blocks using 'flex' properties to this API - which would lead to even more bloated style tags.

I created this plugin to address the issue - and so far it's worked perfectly at preserving the intended values for blockGap, contentSize and wideSize + any custom user settings for any of those properties.

In our tests on TwentyTwentyTwo, we reduced the number of auto generated styles from 29 down to 4.

The current plugin code is a bit rough around the edges (it's the just works version), and what ultimately we want to do with it is enable a user to change the values that are autogenerated, right now I'm working on slimming down the code and compressing it to be more efficient.

After generating the code, my thoughts lie around using a class to handle the logic of CSS generation (since this is used quite a bit throughout the theme support API, and we were wanting the flexibility to tweak and tune it on demand).

Essentially, the global settings would be grabbed, saved to an array, classes generated based on those inputs, and then, if duplicate occurrence exists, apply the appropriate class (e.g. it's very common in our theme development to do a 'quick and dirty' layout adjustment by doing something like, say, adding 2rem margin-bottom to a bunch of blocks - in this scenario, that user action would result in a new class generated that's then enqueued on the appropriate styles.

I'd be happy to submit a PR with my code once I've finished streamlining and cleaning it up - as I was doing anyway to submit to the official plugin directory. In the mean time, if you'd like to see this behavior in live time working, feel free to grab a copy of the plugin code (it dequeues both WP & Gutenberg layout support) and play around - it's at least a working copy of the concept (although we're still using uniqid() to generate user custom settings, I like the idea of adding a value based ending to the class better and will refactor for that.

@mrwweb
Copy link

mrwweb commented Feb 14, 2022

I love love love love love the approach @andrewserong is attempting in #36847 and I would love to see it go farther. Core already uses this approach with colors, gradients and font sizes, but it very clearly now could do the same with things like alignment, justification, gaps, margin, and padding.

I would love to see a standardized set of CSS utility classes that both incorporate values from theme.json and allow blocks to reuse shared CSS styles. If standardized, it would become very plausible for 3rd-party plugins to ship things with complex custom front-end layouts and almost no additional CSS! One good example is vertical alignments that are needed for at least Cover, Media & Text, and columns.

The downsides to this approach I think are mostly around changing direction in the project and requiring people to agree to some standards. However, the upsides are huge: smaller and more interoperable styles for blocks, plugins, and themes.

I expand on this thought on this comment on #38674.

@andrewserong
Copy link
Contributor

andrewserong commented Feb 15, 2022

Thanks for the input here @fwazeter and @mrwweb! There's quite a lot at play in how styles are generated and rendered. I'll be closing out my old attempt in #36847, as we're exploring moving toward having a separate package for consolidating how styles are generated (the style engine, tracked in #38167) — but the insights here into classnames and style duplication are super helpful. (I plan to make another attempt at server-rendering block styles / generating class names soon)

There is definitely the goal to consolidate all the style generation into the smallest number of style tags / stylesheets to be enqueued (my hope is that we can come up with a way to have a single generated style tag that contains all the generated styles for a particular post), but it looks like there's still some discussion to work out the desired behaviour for generating classnames, and how descriptive (or numerous) they should or shouldn't be. I'm keen to see how the discussion over in #38694 evolves!

@fwazeter
Copy link
Member

@andrewserong I've been tinkering with this quite a lot and it certainly gets pretty complex fast. (Note, none of this is meant as critical, just observation and trials-and-tribulations from the trenches...Gutenberg definitely is like it was once described as "trying to replace a jet engine while the plane is in flight."

Part of the issue is that while most plugin or theme developers won't care as deeply to what's happening under the hood with backend processes (except to adapt to how things are done or hook into something, etc), CSS ends up being a whole different ballpark because of the impact on the visual front end - and as someone who runs an agency, CSS is almost always the area where the most time is spent/lost. Worse, it's also by far the most opinionated (or at least vocally opinionated) because it's the area someone's most likely to notice.

For example - FSE radically sped up our dev process (been building them in production since 5.8) for the average small business site in terms of setup, migrating features from one to the next and so on.

That said it's a chronic fight with CSS on a scale not seen before in WP - a strength of a classic theme is that, well, you can just dequeue everything and load up whatever you want.

That simply isn't possible with blocks - or more specifically - with theme.json (which is something that's great ), since that has to be a dynamically generated stylesheet. This means that opinionated styling is the default, because, well, it has to be in this iteration.

Let's evaluate a seemingly simple problem. CSS works best when it's leveraged to it's full inheritance properties - setting smart, global defaults that are only overridden for specific, actual, exceptions to the global standard.

Simple Problem 1: On mobile, a paragraph or heading block on it's own within a container without a background by default ends up flush against the side of the device screen.

Easy Solution: Make the the global default box-sizing: border-box & direct child of the main content box section default to content-box model.

/* because everything in CSS is a box, just make border-box the global default */
* {
box-sizing: border-box;
}

/* main, or entry-content or whatever hook you want to use for the_content */
main > * {
box-sizing: content-box;
padding-right: var(--wp--style--block-gap);
padding-left: var(--wp--style--block-gap);
}

Now, the content will align perfectly on a screen size larger than contentSize or wideSize and on fullwidth, as originally intended, but on smaller screen sizes won't be flush against the size of the device.

Here's the problem: wp-block-group, column, etc all explicitly state to be border-box...so now I've got to override those styles by either dequeuing them and loading my own styles or overriding their CSS, and anything else that explicitly says border-box, when it wasn't necessary with a global default like box-sizing: border-box.

But hey, that's easy enough to override - we can override the block styles, so we'll just dequeue them and enqueue what we want after reading through every style and eliminating what we don't need and re-enqueuing a new style. It's a PITA the first time, but once we got it, we got it mostly.

But, let's go to the next problem - okay, my page load speeds are pretty awesome with FSE, so I don't really need to worry about dynamic .wp-container-uuid, even though it's a pain to read through to diagnose CSS, it's functional. But, now I want to make it so my theme doesn't have to account for different reading modes by writing new CSS - which is solved with single line logical properties instead of margin-right / left etc, we can use margin-inline-start / margin-inline-end etc and the layout will automatically change based on the user's reading mode preference. Awesome! I can keep the integrity of my design, be internationalized by default, and not write anymore code.

Except now I can't because Layout Support ties me to being margin-right / margin-left...and to overwrite it, I have to rewrite the whole Layout Support section of the Block Support API. Now I've got to maintain a fix that core could override or change at any time, or I'll have to adjust to account for new features.

Then, there's the whole tied into global core colors and such, which is livable, but annoying to read through and well, would be great if it could just be opt-out gone - the reason it's global and not overridable is for block patterns, but it feels like a relatively simple version of an if loop could just be attached to the render_block hook to pull the global style into the common global CSS style sheet if the property is used.

Plus, then that brings the thought - so great, we made global colors and gradients non-opt out for custom props, so, why not make a custom prop for contentSize and wideSize and simply change that value on a one off tag (or call via an id property) when a user input is put in place?

Then, on top of that, half of the Block Support API registers values from theme.json one way (e.g. apply) and the other half uses render .

So we've now got four different ways CSS is rendered without getting into anything complicated, not even including something like - colors - all of them default to HEX overrides, when it wouldn't be too much extra work to have a JS or PHP script that powers the color wheel and converts the values to whatever the custom property set it as or let the end user set whatever color code they want to use - and that would further enable for opacity to work (e.g. avoiding the cover block opacity thing that has to use a filter to override the color) - and be easy enough to say, hey, RGB to RGBA, Hex to 8 digit hex, HSL to HSLA, etc.

Here's the solution I've been building within a plugin for our internal use so far.

  1. a CSS parsing class takes data from different sources (e.g. block attr's, theme.json, from custom post type)
  2. After parsing the input is universalized into a single standard dividing it into declaration blocks and declaration prop-values
  3. a CSS generator class compiles the values into the appropriate stylesheet(s) wanted.
  4. The output is registered as an inline-style as the default.
  5. The Block Support mechanism (mostly just Layout Support) assigns the class if no block attr for layout/content size is set.
  6. If a custom value is saved by the user, it saves the value in a custom post type and adds it to render when that property is used - if a second block uses the same value, it doesn't create a new class, and uses the other class.

We save different copies of the parsed data in a custom post type - so that if a user wanted to change what the default output is, say change a property from margin-right to margin-inline-end, they could simply change one value and it goes out - though this probably isn't something core would really need to care about - but I think giving a plugin or theme author the ability to hook into this mechanism would be widely applauded.

CSS is complex enough that I feel that a class / API that powers parsing and creating declaration blocks and assigning them to stylesheets would radically simplify the development of the block editor - and even provide a framework for blocks themselves to radically simplify the amount of CSS they have to render for options, while giving plugin & theme authors a way to change those values if they so desired, without taking away the power of choice from the user.

@westonruter
Copy link
Member

Related: #38889. At at the very least the generated class names should be consistent across page loads. So instead of using uniqid() it should use wp_unique_id() so that you get something like wp-container-5 which is consistent across page loads instead of wp-container-620e88a091db4 with the ID part being random.

@ramonjd
Copy link
Member Author

ramonjd commented Feb 17, 2022

it should use wp_unique_id() so that you get something like wp-container-5 which is consistent across page loads instead of wp-container-620e88a091db4 with the ID part being random.

Maybe a hash would work better in the interim (?) until we work out a neater solution (hopefully as part of the style engine work)

@andrewserong
Copy link
Contributor

@andrewserong I've been tinkering with this quite a lot and it certainly gets pretty complex fast.

Thanks again for sharing your thoughts @fwazeter (and over on the style engine tracking issue)! I'm still gathering my thoughts about a lot of all this, and how it might ideally work, but just wanted to say sharing the thoughts and ideas is so helpful. Thank you!

I think a lot of the goals behind the layout support in particular was for WordPress to do more of the heavy lifting surrounding how blocks are laid out so that (hopefully) themes don't need to work so hard to provide the base layout. 🤞 with PRs like #38816 where @ramonjd is looking at switching the layout support to use logical properties, fewer of these overrides will be needed.

For some things that might not be captured anywhere (like the use case for box-sizing: content-box), feel free to add an issue for it and link it to the tracking issue so we don't miss it as folks start to explore solutions. I'm sure we'll discover some of the limitations of any potential fix along the way, but hopefully we can find a solid path forward, and these discussions are super helpful for the project! 🙇

@ajlende
Copy link
Contributor

ajlende commented Mar 9, 2022

It seems this issue also causes problems with ajax/pjax refreshes for pages as reported in #39054

@andrewserong Hopefully that issue gives a little more context for how it affects duotone too since duotone also has to include the SVGs before the filtered content in the body of the document.

@ndiego
Copy link
Member

ndiego commented Jul 26, 2022

@ramonjd this appears resolved when using the latest version of Gutenberg (13.7.3). See the screenshot below. Is this issue safe to close now?

image

@ramonjd
Copy link
Member Author

ramonjd commented Jul 26, 2022

Thanks for the ping @ndiego I've double checked and it appear to work for me too!

@ramonjd ramonjd closed this as completed Jul 26, 2022
@andrewserong
Copy link
Contributor

Oh, sorry folks, I should have added an update here — #40875 added base layout styles so that flex and a fallback gap are now rendered correctly in the editor, which resolved the main issue here. We still need to add support for content justification / alignments which will be done in a follow-up and I'll post updates in #39336. Since the main issue is resolved, we probably don't need to re-open this issue, but just thought I'd give a quick update on the current status!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Content Affects the Post Content Block [Block] Social Affects the Social Block - used to display Social Media accounts CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Bug An existing feature does not function as intended
Projects
None yet
9 participants