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.

Sign up for GitHub

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

Jump to bottom

Try getting Post Content layout on server before editor loads #45299

Merged
merged 14 commits into from
Mar 8, 2023
Merged

Try getting Post Content layout on server before editor loads #45299

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8d644d8
Try getting Post Content layout on server before editor loads
tellthemachines Oct 26, 2022
58afc98
Add fallback template slug logic
tellthemachines Oct 27, 2022
63b5cbc
Add block editor settings in new filter
tellthemachines Feb 23, 2023
e37f8e5
Fix whitespace issues
tellthemachines Feb 23, 2023
3992e55
Address review feedback
tellthemachines Feb 23, 2023
7bcada7
Get latest post content in editor if template changes.
tellthemachines Feb 28, 2023
c1a48bb
Custom posts default to post template.
tellthemachines Mar 3, 2023
602c4a9
Remove unnecessary variable
tellthemachines Mar 3, 2023
160f791
Update useLayoutClasses to take attributes object.
tellthemachines Mar 3, 2023
87759cf
Fix used layout logic.
tellthemachines Mar 6, 2023
a7994ef
Restore comment
tellthemachines Mar 6, 2023
c3c6e49
Make sure postContentAttributes only exist where they're needed.
tellthemachines Mar 7, 2023
cca103d
Capitalise post ID
tellthemachines Mar 7, 2023
c814f47
Remove layout object from dependencies
tellthemachines Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions lib/experimental/block-editor-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
<?php
/**
* Adds settings to the block editor.
*
* @package gutenberg
*/

/**
* Adds styles and __experimentalFeatures to the block editor settings.
*
* @param array $settings Existing block editor settings.
*
* @return array New block editor settings.
*/
function gutenberg_get_block_editor_settings_experimental( $settings ) {
$is_block_theme = wp_is_block_theme();

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 );
Copy link
Contributor

@andrewserong andrewserong Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

2023-03-07 14 38 14

But it looks to work pretty well locally if I swap it out for $post_ID instead 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's weird, but it does seem to work! Updating.

switch ( $what_post_type ) {
case 'page':
$template_slug = $page_slug;
break;
default:
$template_slug = $post_slug;
break;
}
}

$current_template = gutenberg_get_block_templates( array( 'slug__in' => array( $template_slug ) ) );
andrewserong marked this conversation as resolved.
Show resolved Hide resolved

/**
* Finds Post Content in an array of blocks
*
* @param array $blocks Array of blocks.
*
* @return array Post Content block.
*/
function get_post_content_block( $blocks ) {
foreach ( $blocks as $block ) {
if ( 'core/post-content' === $block['blockName'] ) {
return $block;
}
if ( ! empty( $block['innerBlocks'] ) ) {
$post_content = get_post_content_block( $block['innerBlocks'] );

if ( ! empty( $post_content ) ) {
return $post_content;
}
}
}
}

if ( ! empty( $current_template ) ) {
$template_blocks = parse_blocks( $current_template[0]->content );
$post_content_block = get_post_content_block( $template_blocks );

if ( ! empty( $post_content_block['attrs'] ) ) {
$settings['postContentAttributes'] = $post_content_block['attrs'];
}
}

return $settings;
}

add_filter( 'block_editor_settings_all', 'gutenberg_get_block_editor_settings_experimental', PHP_INT_MAX );
1 change: 1 addition & 0 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ function gutenberg_is_experiment_enabled( $name ) {
// Experimental features.
remove_action( 'plugins_loaded', '_wp_theme_json_webfonts_handler' ); // Turns off WP 6.0's stopgap handler for Webfonts API.
require __DIR__ . '/experimental/block-editor-settings-mobile.php';
require __DIR__ . '/experimental/block-editor-settings.php';
require __DIR__ . '/experimental/blocks.php';
require __DIR__ . '/experimental/navigation-theme-opt-in.php';
require __DIR__ . '/experimental/kses.php';
Expand Down
26 changes: 13 additions & 13 deletions packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,23 +35,23 @@ const layoutBlockSupportKey = '__experimentalLayout';
/**
* Generates the utility classnames for the given block's layout attributes.
*
* @param { Object } block Block object.
* @param { Object } blockAttributes Block attributes.
* @param { string } blockName Block name.
*
* @return { Array } Array of CSS classname strings.
*/
export function useLayoutClasses( block = {} ) {
export function useLayoutClasses( blockAttributes = {}, blockName ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

const rootPaddingAlignment = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return getSettings().__experimentalFeatures
?.useRootPaddingAwareAlignments;
}, [] );
const globalLayoutSettings = useSetting( 'layout' ) || {};

const { attributes = {}, name } = block;
const { layout } = attributes;
const { layout } = blockAttributes;

const { default: defaultBlockLayout } =
getBlockSupport( name, layoutBlockSupportKey ) || {};
getBlockSupport( blockName, layoutBlockSupportKey ) || {};
const usedLayout =
layout?.inherit || layout?.contentSize || layout?.wideSize
? { ...layout, type: 'constrained' }
Expand Down Expand Up @@ -100,14 +100,14 @@ export function useLayoutClasses( block = {} ) {
/**
* Generates a CSS rule with the given block's layout styles.
*
* @param { Object } block Block object.
* @param { string } selector A selector to use in generating the CSS rule.
* @param { Object } blockAttributes Block attributes.
* @param { string } blockName Block name.
* @param { string } selector A selector to use in generating the CSS rule.
*
* @return { string } CSS rule.
*/
export function useLayoutStyles( block = {}, selector ) {
const { attributes = {}, name } = block;
const { layout = {}, style = {} } = attributes;
export function useLayoutStyles( blockAttributes = {}, blockName, selector ) {
const { layout = {}, style = {} } = blockAttributes;
// Update type for blocks using legacy layouts.
const usedLayout =
layout?.inherit || layout?.contentSize || layout?.wideSize
Expand All @@ -118,7 +118,7 @@ export function useLayoutStyles( block = {}, selector ) {
const blockGapSupport = useSetting( 'spacing.blockGap' );
const hasBlockGapSupport = blockGapSupport !== null;
const css = fullLayoutType?.getLayoutStyle?.( {
blockName: name,
blockName,
selector,
layout,
layoutDefinitions: globalLayoutSettings?.definitions,
Expand Down Expand Up @@ -350,7 +350,7 @@ export const withInspectorControls = createHigherOrderComponent(
*/
export const withLayoutStyles = createHigherOrderComponent(
( BlockListBlock ) => ( props ) => {
const { name, attributes, block } = props;
const { name, attributes } = props;
const hasLayoutBlockSupport = hasBlockSupport(
name,
layoutBlockSupportKey
Expand All @@ -372,7 +372,7 @@ export const withLayoutStyles = createHigherOrderComponent(
? { ...layout, type: 'constrained' }
: layout || defaultBlockLayout || {};
const layoutClasses = hasLayoutBlockSupport
? useLayoutClasses( block )
? useLayoutClasses( attributes, name )
: null;
// Higher specificity to override defaults from theme.json.
const selector = `.wp-container-${ id }.wp-container-${ id }`;
Expand Down
45 changes: 30 additions & 15 deletions packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,19 +80,20 @@ function MaybeIframe( { children, contentRef, shouldIframe, styles, style } ) {

/**
* Given an array of nested blocks, find the first Post Content
* block inside it, recursing through any nesting levels.
* block inside it, recursing through any nesting levels,
* and return its attributes.
*
* @param {Array} blocks A list of blocks.
*
* @return {Object | undefined} The Post Content block.
*/
function findPostContent( blocks ) {
function getPostContentAttributes( blocks ) {
for ( let i = 0; i < blocks.length; i++ ) {
if ( blocks[ i ].name === 'core/post-content' ) {
return blocks[ i ];
return blocks[ i ].attributes;
}
if ( blocks[ i ].innerBlocks.length ) {
const nestedPostContent = findPostContent(
const nestedPostContent = getPostContentAttributes(
blocks[ i ].innerBlocks
);

Expand All @@ -108,6 +109,7 @@ export default function VisualEditor( { styles } ) {
deviceType,
isWelcomeGuideVisible,
isTemplateMode,
postContentAttributes,
editedPostTemplate = {},
wrapperBlockName,
wrapperUniqueId,
Expand All @@ -116,8 +118,8 @@ export default function VisualEditor( { styles } ) {
const {
isFeatureActive,
isEditingTemplate,
__experimentalGetPreviewDeviceType,
getEditedPostTemplate,
__experimentalGetPreviewDeviceType,
} = select( editPostStore );
const { getCurrentPostId, getCurrentPostType, getEditorSettings } =
select( editorStore );
Expand All @@ -141,8 +143,9 @@ export default function VisualEditor( { styles } ) {
deviceType: __experimentalGetPreviewDeviceType(),
isWelcomeGuideVisible: isFeatureActive( 'welcomeGuide' ),
isTemplateMode: _isTemplateMode,
postContentAttributes: getEditorSettings().postContentAttributes,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would preloading work in the case of users with no edit access to templates? Say author type users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could use the view context and make templates available for these users in "view" mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// Post template fetch returns a 404 on classic themes, which
// messes with e2e tests, so we check it's a block theme first.
// messes with e2e tests, so check it's a block theme first.
editedPostTemplate:
supportsTemplateMode && canEditTemplate
? getEditedPostTemplate()
Expand Down Expand Up @@ -230,10 +233,13 @@ export default function VisualEditor( { styles } ) {
return { type: 'default' };
}, [ isTemplateMode, themeSupportsLayout, globalLayoutSettings ] );

const postContentBlock = useMemo( () => {
const newestPostContentAttributes = useMemo( () => {
if ( ! editedPostTemplate?.content && ! editedPostTemplate?.blocks ) {
return postContentAttributes;
}
// When in template editing mode, we can access the blocks directly.
if ( editedPostTemplate?.blocks ) {
return findPostContent( editedPostTemplate?.blocks );
return getPostContentAttributes( editedPostTemplate?.blocks );
}
// If there are no blocks, we have to parse the content string.
// Best double-check it's a string otherwise the parse function gets unhappy.
Expand All @@ -242,10 +248,19 @@ export default function VisualEditor( { styles } ) {
? editedPostTemplate?.content
: '';

return findPostContent( parse( parseableContent ) ) || {};
}, [ editedPostTemplate?.content, editedPostTemplate?.blocks ] );
return getPostContentAttributes( parse( parseableContent ) ) || {};
}, [
editedPostTemplate?.content,
editedPostTemplate?.blocks,
postContentAttributes,
] );

const layout = newestPostContentAttributes?.layout || {};

const postContentLayoutClasses = useLayoutClasses( postContentBlock );
const postContentLayoutClasses = useLayoutClasses(
newestPostContentAttributes,
'core/post-content'
);

const blockListLayoutClass = classnames(
{
Expand All @@ -255,12 +270,11 @@ export default function VisualEditor( { styles } ) {
);

const postContentLayoutStyles = useLayoutStyles(
postContentBlock,
newestPostContentAttributes,
'core/post-content',
'.block-editor-block-list__layout.is-root-container'
);

const layout = postContentBlock?.attributes?.layout || {};

// Update type for blocks using legacy layouts.
const postContentLayout = useMemo( () => {
return layout &&
Expand All @@ -271,6 +285,7 @@ export default function VisualEditor( { styles } ) {
? { ...globalLayoutSettings, ...layout, type: 'constrained' }
: { ...globalLayoutSettings, ...layout, type: 'default' };
}, [
layout,
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
layout?.type,
layout?.inherit,
layout?.contentSize,
Expand All @@ -280,7 +295,7 @@ export default function VisualEditor( { styles } ) {

// If there is a Post Content block we use its layout for the block list;
// if not, this must be a classic theme, in which case we use the fallback layout.
const blockListLayout = postContentBlock?.isValid
const blockListLayout = postContentAttributes
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
? postContentLayout
: fallbackLayout;

Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/provider/use-block-editor-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ const BLOCK_EDITOR_SETTINGS = [
'locale',
'maxWidth',
'onUpdateDefaultBlockStyles',
'postContentAttributes',
'postsPerPage',
'readOnly',
'styles',
Expand Down