Try getting Post Content layout on server before editor loads #45299
Conversation
|
Size Change: +3.51 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
| function gutenberg_get_block_editor_settings( $settings ) { | ||
| global $post_id; | ||
| $template_slug = get_page_template_slug( $post_id ); | ||
| $current_template = gutenberg_get_block_templates( array( 'slug__in' => array( $template_slug ) ) ); |
There was a problem hiding this comment.
A few surface notes:
- Since this is a post-editor-only setting, we might want to use the
block_editor_settings_allfilter and run logic only in the right editor$context. - The
get_page_template_slugonly works for Page post types.
I think it would be nice to have a getEditedPostTemplate counterpart on the PHP side. Currently, we have no way to get an edited post-type template before the editor loads.
There was a problem hiding this comment.
The get_page_template_slug only works for Page post types
It works for both posts and pages, though I haven't tried custom post types yet. The problem I'm encountering is that this function returns an empty string if the template in use is the default one, so we'll need some logic to check what post type it is and work out what its slug should be. I wish there were some sort of flag for this function to always return the slug, but it seems to have been written with a very specific purpose 🤷
There was a problem hiding this comment.
Sorry, you're right.
We can probably adopt logic from get_single_template to cover all cases from the template hierarchy.
Here's my untested example:
function wp_get_edited_post_template( $post_id ) {
$object = get_post( $post_id );
$templates = array();
if ( ! empty( $object->post_type ) ) {
$template = get_page_template_slug( $object );
if ( $template && 0 === validate_file( $template ) ) {
$templates[] = $template;
}
$name_decoded = urldecode( $object->post_name );
if ( $name_decoded !== $object->post_name ) {
$templates[] = "single-{$object->post_type}-{$name_decoded}";
}
$templates[] = "single-{$object->post_type}-{$object->post_name}";
$templates[] = "single-{$object->post_type}";
}
$templates[] = 'single';
return resolve_block_template( 'single', $templates, '' );
}There was a problem hiding this comment.
So I'm not sure any solution will work for 100% of cases 😅, but I was looking into the return value of get_default_block_template_types() earlier and it looks like for most block themes we should expect 'single' to be the default template for posts, and 'page' to be the default for pages. But if those templates don't exist, we fall back to a 'singular' template that covers both posts and pages. I tried adding some logic to account for that and so far it's testing well on the handful of themes I looked at.
I think the biggest problem here is that, until the user changes the template used for a specific post or page, we have no template data for it, so unless I'm mistaken we'll always have to infer it from the post type and the standard default template slugs.
There was a problem hiding this comment.
We'll need to calibrate the new function for our needs. This is a rough example based on the core function.
P.S. Starting from WP 6.1, user will be able to create templates for single entities - "single-{$object->post_type}-{$object->post_name}".
| } | ||
| // mismatched naming x(. | ||
| $post_content_block['attributes'] = $post_content_block['attrs']; | ||
| $settings['postContentBlock'] = $post_content_block; |
There was a problem hiding this comment.
Instead of adding the post content block to the settings, can we add the right layout directly instead and avoid all JS computations?
There was a problem hiding this comment.
It's complicated 😅
In the visual editor component we need to add both the relevant layout classes and any styles resulting from user customisation (the ones we usually attach to wp-container classes in blocks). Currently, classes and styles are computed by useLayoutClasses and useLayoutStyles hooks, both of which take in the full block because at some point they need to know the block name.
Another consideration is that eventually we also want other Post Content settings such as typography to be reflected in the editor, so if we already have access to the whole block here it'll allow us to add that easily.
There was a problem hiding this comment.
It doesn't make sense to tie useLayoutClasses and useLayoutStyles to blocks. It seems we may be creating dependencies between different concepts that are just breaking the abstractions.
Why do we need blocks in these two functions?
There was a problem hiding this comment.
I guess maybe we need the "config" of the layout block support, in that case, it should probably be a separate argument no?
There was a problem hiding this comment.
For useLayoutClasses we use the block name to work out what the default layout for that block is. We need this because the layout attribute will be empty unless the block has non-defaults set.
For useLayoutStyles we also use the block name to pass to getLayoutStyle, which uses it to check for block gap support.
I don't think we're creating dependencies here as much as working with existing dependencies 😅 but I'm not sure that's such a bad thing in this case - would we want to leverage those functions for non-block use cases? Does it make sense to abstract them from the block scenario unless we already have other potential use-cases to work with?
There was a problem hiding this comment.
Does it make sense to abstract them from the block scenario unless we already have other potential use-cases to work with?
Isn't the root layout a use-case for a non block case? I mean the post-content block is not even part of the edited entity.
Also, it seems the dependency is actually not the block object (like being passed in this PR) but the block name, so would it be better to pass only the block name (from the server there's no need for block name, I guess it's always core/post-content). Alternatively, we can also pass the "layout block support" config and not the block name.
There was a problem hiding this comment.
Isn't the root layout a use-case for a non block case? I mean the post-content block is not even part of the edited entity.
It's not really a non-block case, because the root layout in the post editor is essentially the content of the Post Content block 😅 What we need to access here is the settings of the specific Post Content block that will render the post being edited, so we're still working with a block.
Also, it seems the dependency is actually not the block object (like being passed in this PR) but the block name, so would it be better to pass only the block name (from the server there's no need for block name, I guess it's always core/post-content). Alternatively, we can also pass the "layout block support" config and not the block name.
I guess for those functions we could pass the layout object and the block name. We do need the block name though, because in the layout type's getLayoutStyle we need to check if the block skips serialisation for gap values.
Even then, if we just add the layout object to the editor settings, later we'll have to add the typography styles object too, so any custom Post Content typography styles are reflected in the editor. And if we ever add other block supports to Post Content, we'll have to do the same for each individually. It seems easier to just add the whole block so we can grab whatever styles and settings we need from it later.
| @@ -13,6 +13,23 @@ | |||
| * @return array New block editor settings. | |||
| */ | |||
| function gutenberg_get_block_editor_settings( $settings ) { | |||
| global $post_id; | |||
| $template_slug = get_page_template_slug( $post_id ); | |||
| $current_template = gutenberg_get_block_templates( array( 'slug__in' => array( $template_slug ) ) ); | |||
There was a problem hiding this comment.
Is it worth us checking whether or not it's a block theme before attempting to grab block templates? (e.g. call wp_is_block_theme()?)
tellthemachines
added
[Package] Edit Post
tellthemachines
force-pushed
the
try/serverside-post-content-layout
branch
from
4ffa1f1
to
b5a3129
Compare
|
Flaky tests detected in c814f47. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4360242206
|
|
I've rebased this PR and addressed all outstanding feedback. Marking ready for review now! |
tellthemachines
requested review from
spacedmonkey and
ellatrix
as code owners
|
It would be great to get an early performance review for PHP changes. cc @spacedmonkey, @felixarntz. |
|
@Mamaduka This would only apply for WordPress 6.3 eventually right? Just asking as I'm sorting priorities here and focusing on 6.2 efforts at this point. I'm definitely going to have a look at this, but probably only next week. |
|
@felixarntz this should be for 6.3, yes. |
I just noticed there's a similar issue if you're running a site that has multiple templates to choose from, with the intention that each template might make changes to the layout — e.g. if there's a template that's centre aligned, and a template that's left or right aligned. On
Would it be useful to retain the JS logic that parses the block, and use the server-generated layout as a fallback if it isn't available? |
|
Hmm yeah maybe we can fetch the current template and update it only if it changes. There'd still be a jump when switching templates, but I'm not sure there's much we can do about that. |
tellthemachines
force-pushed
the
try/serverside-post-content-layout
branch
from
bb125de
to
3992e55
Compare
There was a problem hiding this comment.
This is looking very close to me @tellthemachines! Just noticed a couple of issues in re-testing:
For some reason the Social Icons/Links block with this PR applied is outputting the is-layout-flow classname for me, and I couldn't quite work out why as the useLayoutClasses call looked correctly updated 🤔:
Other than that, it's testing nicely:
✅ The initial template's post content block appears to be loaded correctly
✅ Custom post types defaulted back to the default layout
✅ Logged in as an author user, you can now switch templates on a post, save and reload, and see the template's post content attributes applied in the editor 👍
One other thing I noticed, which doesn't appear to cause any issues, is that if I log out the block editor's postContentAttributes value from within the site editor, it provides a real value:
When the site editor is loaded, I'd have assumed the value would be undefined, so I wasn't sure if a post id is being resolved in this case, and whether or not that's expected?
Happy to do further testing tomorrow!
| // Post template fetch returns a 404 on classic themes, which | ||
| // messes with e2e tests, so we check it's a block theme first. |
There was a problem hiding this comment.
This comment appears to be there for the editedPostTemplate line below — I think it might be explaining why not to call getEditedPostTemplate() directly on classic themes, and why we are checking for supportsTemplateMode first. Shall we keep the comment around?
There was a bug in the logic, fixed now!
That must be a side-effect of the default put in place for custom post types, because this filter is applied to all editor types. I'm not sure if there's a post editors-specific filter we can use instead, or if not perhaps we can check which screen we're on. Ideas welcome! |
Oh, that was a subtle one. Thanks for fixing it up!
Good question. This doesn't answer that question, but I'm doing a little more debugging, and I noticed that from inspecting |
| * | ||
| * @return { Array } Array of CSS classname strings. | ||
| */ | ||
| export function useLayoutClasses( block = {} ) { | ||
| export function useLayoutClasses( blockAttributes = {}, blockName ) { |
There was a problem hiding this comment.
I just realised that useLayoutClasses and useLayoutStyles are exported as experimental. Is that an issue for changing the API signature (sorry I didn't catch this sooner!)? From a quick search it doesn't look like any other public plugins are using the methods (https://wpdirectory.net/search/01GTWH136BKYXYGB9M4EH3110K), but I couldn't quite remember what the approach is for changing the signature on methods that have already been exported 🤔
There was a problem hiding this comment.
They're experimental and undocumented so we don't have to ensure back-compat. We could bump up the major number on the package to signal a breaking change I guess, but we haven't historically been very consistent in doing that 😅
There was a problem hiding this comment.
They're experimental and undocumented so we don't have to ensure back-compat.
Okay, cool. My assumption was that these functions wouldn't be in use anywhere so the risk seems very minimal 👍
From a bit of logging out in PHP, it seems like when we load the site editor, no |
Thanks, that should work! I'll give it a go. |
|
Done, thanks for the suggestions @andrewserong! My only further question is whether |
As far as I can tell, I think it goes something like:
But we might want to double-check with folks who've been actively iterating on that process, I know @oandregal has been working on this for the Theme JSON code lately. |
| global $post_id; | ||
|
|
||
| if ( ! $is_block_theme || ! $post_id ) { | ||
| return $settings; | ||
| } | ||
|
|
||
| $template_slug = get_page_template_slug( $post_id ); | ||
|
|
||
| if ( ! $template_slug ) { | ||
| $post_slug = 'singular'; | ||
| $page_slug = 'singular'; | ||
| $template_types = get_block_templates(); | ||
|
|
||
| foreach ( $template_types as $template_type ) { | ||
| if ( 'page' === $template_type->slug ) { | ||
| $page_slug = 'page'; | ||
| } | ||
| if ( 'single' === $template_type->slug ) { | ||
| $post_slug = 'single'; | ||
| } | ||
| } | ||
|
|
||
| $what_post_type = get_post_type( $post_id ); |
There was a problem hiding this comment.
I think this is nearly there — it turns out the global variable for the post id set by post-new.php is $post_ID (capitalised ID) on this line: https://github.com/WordPress/wordpress-develop/blob/6742d0d7a65e37f24c67eeaae373f4c086c277a5/src/wp-admin/post-new.php#L67 (for existing posts, either variable works thanks to this line, it's just the new posts where it doesn't work with the lowercase variable)
I think we'll need to use global $post_ID instead, otherwise $post_id isn't set when loading the page for creating a new post. For a new post we wind up with a layout shift during loading, once the editor has had a chance to resolve the template:
But it looks to work pretty well locally if I swap it out for $post_ID instead 🙂
There was a problem hiding this comment.
Oh that's weird, but it does seem to work! Updating.
My understanding is the same as Andrew's. If the new function shouldn't be backported until we're sure this is the right API, it'd live in |
|
Thanks @oandregal ! We can leave it in experimental for now to see how it works, and it all goes well stabilise it for 6.3. In any case, the JS changes should work fine without the PHP. |
There was a problem hiding this comment.
Thanks for all the follow-up @tellthemachines, this is testing really well for me now! 👍
✅ Layout is loaded on the server for new and existing posts, pages, and custom post types (tested via creating a custom post type with the Custom Post Type UI plugin)
✅ Layout is loaded on the server for an author role, so the current template's layout rules are applied correctly
✅ Switching templates updates the UI correctly when logged in as an admin user (as an author role it requires a page reload, which we've already discussed — a nice improvement over what we have on trunk), also noted the issue you raised about changing from custom to default template (#48577), which appears to be unrelated to this PR.
✅ Classic themes are unaffected by this change
✅ The postContentAttributes server rendered value is not available in the site editor, as expected 👍
This looks like a good place to land it, I think, and I agree with keeping the PHP in the experimental directory for now — we can move it over to the 6.3 directory once we've had a chance to try this out in the plugin for a while.
LGTM! ✨
| @@ -141,8 +143,9 @@ export default function VisualEditor( { styles } ) { | |||
| deviceType: __experimentalGetPreviewDeviceType(), | |||
| isWelcomeGuideVisible: isFeatureActive( 'welcomeGuide' ), | |||
| isTemplateMode: _isTemplateMode, | |||
| postContentAttributes: getEditorSettings().postContentAttributes, | |||
There was a problem hiding this comment.
So let me clarify my understanding here. We're getting this value from the server.
But at the same time, we're also getting the same value from the client (for instance when the template gets edited...)
So at this point I'm wondering, is the server computation adding any value?
There was a problem hiding this comment.
I think the reason we used a server computation was twofold: to avoid an initial flash of the wrong layout being used before the client has a chance to load, and for users who do not have access to edit templates, so that they still see the right layout?
There was a problem hiding this comment.
Yes that's it! It takes a while for the value to come through from the client so the initial layout shift was very obvious. If it weren't for the possibility of users editing the template and then returning to the post editor without a full page reload, we could probably get away without getting the value from the client at all. The only reason we do so is to make sure any changes to the template are accurately reflected straightaway.
There was a problem hiding this comment.
Ok, so what I think is that we should get rid of the server version in this case. We should only use the client and to address the performance/layout shift we could try to preload the template instead. (add it to the preloaded API calls)
There was a problem hiding this comment.
Ok, so what I think is that we should get rid of the server version in this case.
If we remove the server version, how do we get it working for users who don't have the capability of accessing the raw template? Or do you mean preloading on the server the raw version of the template, bypassing the user capabilities issue? If we do the latter, we need to be careful as templates can potentially have private data in block attributes elsewhere within the template depending on what kinds of 3rd party blocks are in use within the template. I believe with the postContentAttributes the risk is mitigated because of extracting only those attributes for that particular block.
Apologies if I'm missing some detail here! To reproduce, if we're exploring removing the server version, let's test things out via a Contributor user to make sure the layout is still displaying as expected.
There was a problem hiding this comment.
Would preloading work in the case of users with no edit access to templates? Say author type users.
There was a problem hiding this comment.
we could use the view context and make templates available for these users in "view" mode.
There was a problem hiding this comment.
In the view mode, we wouldn't have access to the raw block attributes, though, so how might styles / layout for the post content block work?
Why?
Following up from this conversation as well as issue raised in #45262.
How?
Adds the Post Content block from the template used in the post to the block editor settings, where it can be accessed by the visual editor on first render; this eliminates the flash of unstyled layout on editor load.
TODO:
When Post Content is changed in the template editor, changes should be reflected immediately on returning to the post editor. We might need to keep some of the logic from the current iteration around in order to do that 🤔
Testing Instructions
Screenshots or screencast