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

Pattern Overrides: Image block doesn't render on frontend when it's saved as a placeholder in a pattern #58425

Closed
talldan opened this issue Jan 30, 2024 · 10 comments
Labels
[Block] Image Affects the Image Block [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced [Type] Bug An existing feature does not function as intended

Comments

@talldan
Copy link
Contributor

talldan commented Jan 30, 2024

Description

See the title and the steps to reproduce.

This seems to happen because the image block saved in the pattern doesn't have complete markup (because no image is set). The image block also dynamically renders nothing in its php render callback when it detects there's no image.

The server html replacement that pattern overrides uses can't reproduce block markup, so this causes a problem.

Step-by-step reproduction instructions

  1. Create a synced pattern that contains an image block (but don't add an image to the image block)
  2. Use the image block as a pattern override using the checkbox in the advanced section
  3. Save the pattern
  4. Create a new post, insert an instance of the pattern you just created
  5. Add an image to the image block in the pattern
  6. Preview the post

Expected: the image set in step 5 should render
Actual: the image doesn't render.

Screenshots, screen recording, code snippet

No response

Environment info

No response

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

@talldan talldan added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced labels Jan 30, 2024
@SantosGuillamot
Copy link
Contributor

SantosGuillamot commented Feb 7, 2024

EDIT: Nevermind, it seems just the processing should solve the issue. I must have been doing something wrong in my tests.


I've been triaging this and, although the change in the bindings processing will help, I believe the issue is a bit different. And I believe it is easier to solve.

If I am not mistaken, just changing this conditional to check if bindings exist should solve it:

if ( ! $processor->next_tag( 'img' ) || ( empty( $attributes['metadata']['bindings']['url'] ) && null === $processor->get_attribute( 'src' ) ) ) {
	return '';
}

The problem we are facing is that, in the case of the image, in the database we are storing an empty image without any URL.

<figure class="wp-block-image"><img alt=""/></figure>

So it doesn't matter how we change the bindings processing because the $content passed to the render_block_core_image is gonna be that one unless we change the save.js file.

I believe it is safe to assume that, if the url of the image is bound, we don't want to return an empty string because it the src property will be populated.

Let me know what you think. I can work on a pull request.

@gziolo
Copy link
Member

gziolo commented Feb 7, 2024

So it doesn't matter how we change the bindings processing because the $content passed to the render_block_core_image is gonna be that one unless we change the save.js file.

Have you tried testing it with WordPress/wordpress-develop#6059? The purpose of this refactoring is to run block bindings processing before render_block_core_image is executed so it has $block_content updated with the value sourced from the block binding for src.

@SantosGuillamot
Copy link
Contributor

Nevermind, I must have been doing something wrong, probably registering the custom fields. It seems that it will work just with the changes in the processing. Sorry for the noise 😄

@gziolo
Copy link
Member

gziolo commented Feb 7, 2024

I haven’t tested the PR yet, and there are still no unit tests in place so I can't guarantee it works with this case. In theory, it definitely should unless I missed some nuances.

@michalczaplinski
Copy link
Contributor

michalczaplinski commented Feb 7, 2024

In my tests, the WordPress/wordpress-develop#6059 PR doesn't seem to address the current issue with the Image placeholder.

Does it work for you, @SantosGuillamot ?

@SantosGuillamot
Copy link
Contributor

SantosGuillamot commented Feb 8, 2024

I've been testing if with a fresh instance this morning and there are two different use cases that I believe are causing the differences in the testing:

  1. Add an empty image connected to the Post Meta source: This is solved with the new processing.
  2. Add an empty image in pattern overrides: This still returns an error with the new processing.

EDIT: I believe I discovered the potential issue with patterns. It seems in trunk we are still using the old format of pattern overrides [ 1, "https://wordpress.org" ]. If I just remove the array, and use directly the URL, it works as expected.

I can confirm that, with the latest version of Gutenberg trunk and the PR to change the processing, everything works as expected.

I made a video showing it:

Testing.new.processing.mp4
Pattern.potential.fix.mp4

@gziolo
Copy link
Member

gziolo commented Feb 8, 2024

I was able to confirm that WordPress/wordpress-develop#6059 resolves the issue on the Block Bindings level. I coded a new test case that asserts whether the URL gets correctly replaced during rendering when the placeholder gets saved for the Image block. I still think that we should set some sort of fallback URL immediately after enabling Pattern Overrides for the Image block to better represent the fact that the fallback image will show up on the frontend unless it gets modified by the user. Otherwise, there is no way to let the user crafting the pattern to style the image.

@talldan
Copy link
Contributor Author

talldan commented Feb 9, 2024

It seems in trunk we are still using the old format of pattern overrides [ 1, "https://wordpress.org" ].

That shouldn't be the case, it was reverted before ever being released.

edit: Just tested, and you're right. That indicates that packages haven't been updated since the last stable release of gutenberg.

@gziolo
Copy link
Member

gziolo commented Feb 20, 2024

Is it still an issue with all the latest changes in the Gutenberg plugin and WordPress core? The setting for the Image block when in the placeholder state can be very limited, but it should be functional.

@talldan
Copy link
Contributor Author

talldan commented Feb 20, 2024

Thanks, seems to be resolved now 🎉

@talldan talldan closed this as completed Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced [Type] Bug An existing feature does not function as intended
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants