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

Fix flash when clicking template name in the editor when a plugin registered template matches a default WP theme template #66359

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Oct 23, 2024

What?

Fixes #65584.

Backport PR WordPress/wordpress-develop#7676.

Many themes (including TT4) have templates for specific post types (ie: page.html) but don't specify their postTypes in theme.json. That's because in some cases it's not needed (ie: page.html is automatically used for pages, single.html is automatically used for posts, etc.).

That caused the templates endpoint to filter out plugin-registered templates incorrectly. If the $query passed to the endpoint contained a post_type, theme templates that didn't explicitly match that post type would be ignored. So if a theme had a page.html and a plugin registered a page template, the one from the plugin would be used, which is incorrect.

In the UI, that caused a visual glitch when clicking on the Template setting in the post/page editor.

How?

Removing $query from _gutenberg_get_block_templates_files() so we get all templates, not only the ones matching the specific $post_type. When getting all templates from the theme, we can correctly filter out any duplicate coming from a plugin.

Testing Instructions

  1. Add the following code to your site:
add_action( 'init', function() {
	register_block_template(
		'templates//page',
		[
			'title' => 'My Single Page',
			'description' => 'This is my single page template',
			'content' => '<!-- wp:paragraph --><p>This is a plugin-registered template.</p><!-- /wp:paragraph -->',
			'post_types' => [ 'page' ],
		]
	);
	register_block_template(
		'templates//single',
		[
			'title' => 'My Single Post',
			'description' => 'This is my single post template',
			'content' => '<!-- wp:paragraph --><p>This is a plugin-registered template.</p><!-- /wp:paragraph -->',
			'post_types' => [ 'post' ],
		]
	);
} );
  1. Create a new page.
  2. The Template setting should be set to "Pages".
  3. Click on "Pages" and verify there is no flash.
  4. Repeat with a post.

Screenshots or screencast

Enregistrament.de.pantalla.del.2024-10-23.10-13-53.webm

…emplate from the theme when 'page' post type templates are queried
@Aljullu Aljullu added the [Type] Bug An existing feature does not function as intended label Oct 23, 2024
@Aljullu Aljullu self-assigned this Oct 23, 2024
Copy link

github-actions bot commented Oct 23, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Aljullu <aljullu@git.wordpress.org>
Co-authored-by: anton-vlasenko <antonvlasenko@git.wordpress.org>
Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org>
Co-authored-by: ndiego <ndiego@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: widoz <wido@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Aljullu Aljullu added the [Feature] Templates API Related to API powering block template functionality in the Site Editor label Oct 23, 2024
@Aljullu Aljullu requested a review from ndiego October 23, 2024 08:17
@Aljullu Aljullu changed the title Fix _gutenberg_get_block_templates_files() so it returns the 'page' template from the theme when 'page' post type templates are queried Fix flash when hovering template name in the editor when a plugin registered template matches a default WP theme template Oct 24, 2024
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

I've tested this PR according to the provided instructions, and it appears to resolve the flickering issue.
Please see my note below.
Thanks!

@@ -40,7 +40,7 @@ function _gutenberg_add_block_templates_from_registry( $query_result, $query, $t
}

if ( ! isset( $query['wp_id'] ) ) {
$template_files = _gutenberg_get_block_templates_files( $template_type, $query );
$template_files = _gutenberg_get_block_templates_files( $template_type );
Copy link
Contributor

@anton-vlasenko anton-vlasenko Oct 28, 2024

Choose a reason for hiding this comment

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

If the issue is related to filtering templates by post_type, what if we try removing only the problematic post_type key instead of the entire $query array?
This might resolve the issue while maintaining relatively good performance, as it would remove the need to fetch all block template files.

What do you think, @Aljullu?

Suggested change
$template_files = _gutenberg_get_block_templates_files( $template_type );
// Inline comment explaining why the $query is not used.
$template_files_query = $query;
unset( $template_files_query['post_type'] );
$template_files = _gutenberg_get_block_templates_files( $template_type, $template_files_query );

Additionally, adding an inline comment to explain why the query array isn’t passed to the _gutenberg_get_block_templates_files() function would make it easier to understand the code and reduce the risk of it being overwritten later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I applied the changes in 1641d59.

Copy link
Contributor

@anton-vlasenko anton-vlasenko Oct 29, 2024

Choose a reason for hiding this comment

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

It would be nice to add a unit test to prevent this bug from reoccurring, but it’s not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I added the tests directly to the backport PR in WordPress/wordpress-develop#7676. I'm happy to add tests in Gutenberg too, but I see there are no unit tests for this part of the codebase in Gutenberg, so I assumed it's better to write them in WordPress directly? What do you think? 🤔

Copy link

github-actions bot commented Oct 29, 2024

Flaky tests detected in f1ec716.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11571372008
📝 Reported issues:

@Aljullu Aljullu changed the title Fix flash when hovering template name in the editor when a plugin registered template matches a default WP theme template Fix flash when clicking template name in the editor when a plugin registered template matches a default WP theme template Oct 29, 2024
@Aljullu
Copy link
Contributor Author

Aljullu commented Oct 29, 2024

Thanks for the review, @anton-vlasenko! This is ready for another look, I also created the PR in WP core: WordPress/wordpress-develop#7676.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

Looks good to me. Great job!

@Aljullu Aljullu merged commit f1a4f1c into trunk Nov 8, 2024
67 checks passed
@Aljullu Aljullu deleted the fix/_gutenberg_get_block_templates_files-return-page-template-for-page-post-type branch November 8, 2024 13:18
@github-actions github-actions bot added this to the Gutenberg 19.7 milestone Nov 8, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…istered template matches a default WP theme template (WordPress#66359)

* Fix _gutenberg_get_block_templates_files() so it returns the 'page' template from the theme when 'page' post type templates are queried

* Try different approach: don't use query to get the templates reference to use for filtering

* Only unset post_type from the query

* Add backport file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Templates API Related to API powering block template functionality in the Site Editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin Block Template editor inconsistency
2 participants