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: Don't render image when src attribute is empty #66004

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

SantosGuillamot
Copy link
Contributor

@SantosGuillamot SantosGuillamot commented Oct 10, 2024

What?

In this old pull request, a check was introduced to not render the image when the src attribute is not defined. Following the same logic, I believe the image shouldn't render when the attribute is an empty string. This could be the case when that attribute is connected to an empty custom field through block bindings, for example.

Why?

When the image src is an empty string, this is the resulting HTML in trunk

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

Following the same reasoning of the original PR: "According to MDN, the img element must have src attribute, so I believe this is incorrect markup."

The src attribute is required, and contains the path to the image you want to embed.

For that reason, I think it is better to not render the image.

How?

I'm just changing the conditional to not render the image when the attribute is falsy, not only null.

Testing Instructions

  1. Register a custom field with an empty value by default.
	register_meta(
		'post',
		'empty_field',
		array(
			'label'          => 'Empty Field',
			'show_in_rest' => true,
			'single'       => true,
			'type'         => 'string',
			'default'      => '',
		)
	);
  1. Go to a page and insert an image.
  2. Connect the image src to the empty field. Go to the block settings -> Attributes -> Click three dots -> Select URL -> Connect it to a custom field.
  3. Go to the frontend and check that the HTML doesn't include the empty img.

@SantosGuillamot SantosGuillamot added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Feature] Block bindings labels Oct 10, 2024
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Tested locally and it works perfectly fine. I set one of the connected images to have an empty string as URL:

Screenshot 2024-10-09 at 18 13 18

The second image is a valid URL:
Screenshot 2024-10-09 at 18 13 35

Before:
Screenshot 2024-10-09 at 18 12 38

After (no broken image, no HTML printed):
Screenshot 2024-10-10 at 10 02 18

Copy link

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: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

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

@cbravobernal cbravobernal enabled auto-merge (squash) October 10, 2024 08:44
@cbravobernal cbravobernal merged commit 0d5bced into trunk Oct 10, 2024
74 of 75 checks passed
@cbravobernal cbravobernal deleted the fix/dont-render-image-when-src-is-empty branch October 10, 2024 08:51
@github-actions github-actions bot added this to the Gutenberg 19.5 milestone Oct 10, 2024
gutenbergplugin pushed a commit that referenced this pull request Oct 10, 2024
Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
@github-actions github-actions bot added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 10, 2024
Copy link

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: a0f74e8

@t-hamano
Copy link
Contributor

Thanks for the fix! I can't remember why I used a null check instead of an empty check, but I think the approach in this PR is the best.

ciampo pushed a commit that referenced this pull request Oct 14, 2024
Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Block bindings [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants