Media & text: Update the image replacement logic #62030

carolinan · 2024-05-27T15:02:40Z

What?

Updates the logic that inserts the featured image in the media and text block, taking in to account if the media is set to be on the left or right side.

For #62023

Why?

When the media is on the right, and there is a second media & text block inside the block, the featured image was inserted inside the wrong HTML element.

How?

First, the updated code checks if the block's mediaPosition attribute is set to right.
If it is, it locates the last figure inside the Media & text block, and inserts an img tag inside it.
If not, the img tag is inserted inside the first figure.

It identifies the correct figure element by the class wp-block-media-text__media to not get mixed up with other figure elements.
It identifies the inserted img tag with the class wp-block-media-text__featured_image.

Then the attributes on the img tag are updated with the data for the featured image, such as the src.

Testing Instructions

Apply the PR.
Create a new post and add a featured image.
Insert a media & text block.
Enable using the featured image option on the block.
Enable the option to position the media on the right.

Now insert another media & text block in the first media & text block. It doesn't matter if this block has any content or has a media selected.
View the front of the website, and compare the size and position of the featured image in the editor and front.

Without this PR; the HTML will look something like this on the front.
Note that there are two img elements inside the first figure.

<div class="wp-block-media-text has-media-on-the-right is-stacked-on-mobile has-primary-background-color has-background">
<div class="wp-block-media-text__content">
<div class="wp-block-media-text has-media-on-the-right is-stacked-on-mobile is-vertically-aligned-bottom">
<div class="wp-block-media-text__content"></div>
<figure class="wp-block-media-text__media">
<img decoding="async" width="282" height="159" alt="" class="wp-image-240 size-full" src="...">
<img fetchpriority="high" decoding="async" width="775" height="1024" src="..." alt="" class="wp-image-30 size-full" srcset="..." sizes="(max-width: 775px) 100vw, 775px">
</figure>
</div>
</div>
<figure class="wp-block-media-text__media"><img></figure>
</div>

And with the PR:

<div class="wp-block-media-text has-media-on-the-right is-stacked-on-mobile has-primary-background-color has-background">
<div class="wp-block-media-text__content">
<div class="wp-block-media-text has-media-on-the-right is-stacked-on-mobile is-vertically-aligned-bottom">
<div class="wp-block-media-text__content"></div>
<figure class="wp-block-media-text__media">
<img fetchpriority="high" decoding="async" width="775" height="1024" src="..." alt="" class="wp-image-30 size-full" srcset="..." sizes="(max-width: 775px) 100vw, 775px"></figure>
</div>
</div>
<figure class="wp-block-media-text__media"><img decoding="async" width="282" height="159" alt="" src="..." class="wp-image-240 size-full"></figure>
</div>

Testing Instructions for Keyboard

Screenshots or screencast

… the right

github-actions · 2024-05-27T15:03:56Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ phpunit/blocks/render-block-media-text-test.php
❔ packages/block-library/src/media-text/index.php

carolinan · 2024-05-28T06:13:57Z

This PHP change is hacky and not a complete solution.

If anyone is available, I need help with using the HTML tag processor to locate the last <figure class="wp-block-media-text__media"> in the media and text block when the mediaPosition attribute is set to right.

Why? Because the content area in the Media & text can contain any block, and any number of blocks, including other figure elements and other media & text blocks. When the media is on the right, then the tag processor will first find the elements that are in the content area.

It needs to work with and without imageFill enabled:
Without imagefill, the featured image is an element inside <figure class="wp-block-media-text__media">
With imagefill, the featured image is a background-image in a style attribute on the <figure class="wp-block-media-text__media">

@dmsnell @Mamaduka @t-hamano @ramonjd

… tag.

carolinan · 2024-05-28T06:22:27Z

Perhaps a unique identifier needs to be added to the figure that contains the media, that can then be used in the PHP file, but I'm really not sure.

(This identifier can't just be the image ID since it is possible that the same image is used in the content area)

github-actions · 2024-05-28T06:42:25Z

Flaky tests detected in 0b03266.
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/9561677081
📝 Reported issues:

[Flaky Test] Create a pattern with overrides #58009 in /test/e2e/specs/editor/various/pattern-overrides.spec.js

dmsnell · 2024-05-28T08:51:20Z

If anyone is available, I need help with using the HTML tag processor to locate the last <figure class="wp-block-media-text__media"> in the media and text block when the mediaPosition attribute is set to right.

It's easy enough to find the first and last figure with this class name, but the HTML API doesn't yet support inserting a node.

We can get around this by subclassing, but so far we've been reluctant to add workaround code in Core/Gutenberg so as to be careful not to share the wrong example code and accidentally encourage dangerous behavior.

For instance, will this always inject an IMG element? What if the FIGURE already contains an IMG?

The general approach for finding the first is easy, and for finding the last there's a kind of bookmark crawl to take.

$media_tag_processor   = new WP_HTML_Tag_Processor( $content );
$wrapping_figure_query = array( 'tag_name' => 'figure', 'class_name' => 'wp-block-media-text__media' );
if ( $has_media_on_right ) {
	while ( $media_tag_processor->next_tag( $wrapping_figure_query ) ) {
		$media_tag_processor->set_bookmark( 'last_figure' );
	}
	
	if ( $media_tag_processor->has_bookmark( 'last_figure' ) ) {
		// Modify last figure here.
	}
} else {
	if ( $media_tag_processor->next_tag( $wrapping_figure_query ) ) {
		// Modify first figure here.
	}
}

$content = $media_tag_processor->get_updated_html();

The HTML API tries to avoid handing out string indices, and while design work is still ongoing for insertion/removal operations, there's no built-in support. For subclasses, there's a way to achieve this through bookmarking.

$media_tag_processor = new class( $content ) extends WP_HTML_Tag_Processor {
	public function insert_after( $dangerous_raw_html_reject_this_patch ) {
		if (
			WP_HTML_Tag_Processor::STATE_READY === $this->parser_state ||
			WP_HTML_Tag_Processor::STATE_INCOMPLETE_INPUT === $this->parser_state ||
			WP_HTML_Tag_Processor::STATE_COMPLETE === $this->parser_state
		) {
			return false;
		}

		$this->set_bookmark( 'here' );
		$here = $this->bookmarks['here'];
		$this->lexical_updates[] = new WP_HTML_Text_Replacement(
			$here->start,
			$here->length,
			$dangerous_raw_html_reject_this_patch
		);
	}
};

Inserting an IMG tag is largely benign.

Use ´while´ to locate the correct ´figure´ tag depending on the block settings. Add support for the image fill setting.

carolinan · 2024-05-28T13:06:38Z

@dmsnell Thank you. @sirreal suggested something similar.

With this update I am not extending the class but still using preg_replace to insert the image tag.
I still need someone to review the logic and the code quality with critical eyes.
But also to consider if there are any other scenarios to test where the feature can break, besides adding a media & text block or image block inside a media & text block.

ramonjd · 2024-05-29T01:11:33Z

I didn't have a lot of time sorry, but I did a smoke test locally and the frontend appears as I'd expect, even with multiple nested media & text blocks. 🎉

Before	After

There was one bug I found with the link but I see there's an issue for it already.

Given the constraints I think your approach is fine - the use of the unique id seems like a clever way to target the figure element, and preg_replace does the job.

But also to consider if there are any other scenarios to test where the feature can break, besides adding a media & text block or image block inside a media & text block.

It might help other folks to understand the intention behind the code, and also to have confidence that it doesn't break in various scenarios by getting some test coverage in there, focussing on the expected output.

See other tests for block render functions:

https://github.com/WordPress/gutenberg/tree/7988696ec25a7373337d91a359929aad204307fd/phpunit/blocks

github-actions · 2024-05-29T01:12:52Z

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: carolinan <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: dmsnell <[email protected]>

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

packages/block-library/src/media-text/index.php

carolinan · 2024-05-30T08:32:50Z

It might help other folks to understand the intention behind the code, and also to have confidence that it doesn't break in various scenarios by getting some test coverage in there, focussing on the expected output.

See other tests for block render functions:

https://github.com/WordPress/gutenberg/tree/7988696ec25a7373337d91a359929aad204307fd/phpunit/blocks

I have tried, but I have never written a test class before. Maybe it should be 3 separate tests, if that is easier to read? One for default settings, one for image fill, and one for when the media is on the right. Now they are all in the same test.

commands:
npm run test:unit:php

Or (after starting wp-env)
wp-env run --env-cwd='wp-content/plugins/gutenberg' tests-wordpress vendor/bin/phpunit -c phpunit.xml.dist --verbose --filter Render_Block_MediaText_Test

ramonjd

Thanks for the continued work on this! And also for the tests.

I think it'd be good to break the tests up for readability and also to help isolate fails.

phpunit/blocks/render-block-media-text-test.php

ramonjd · 2024-05-31T03:06:57Z

phpunit/blocks/render-block-media-text-test.php

+		$this->assertStringContainsString( wp_get_attachment_image_url( self::$attachment_id, 'full' ), $rendered );
+
+		$rendered = gutenberg_render_block_core_media_text( $attributes, $content_nested_media_on_right );
+		$this->assertStringContainsString( wp_get_attachment_image_url( self::$attachment_id, 'full' ), $rendered );


Suggested change

$this->assertStringContainsString( wp_get_attachment_image_url( self::$attachment_id, 'full' ), $rendered );

$this->assertStringContainsString( '<img alt="" src="' . wp_get_attachment_image_url( self::$attachment_id, 'full' ) . '"', $rendered );

This one too.

packages/block-library/src/media-text/index.php

phpunit/blocks/render-block-media-text-test.php

packages/block-library/src/media-text/index.php

Neither the variable or the CSS class are needed, and they can be removed in favor of an empty `<img>` tag.

carolinan · 2024-05-31T06:11:45Z

Actually removing the class name from the img did break the nested media & text block so I need to revert it.

…he img tag" This reverts commit c62c46c.

Add an if statement around the figure tag and the image tag, to make sure that the id is removed and the attributes are updated on the correct tags.

Move the closing parenthesis on the array that includes the figure tag name and id. Remove whitespace.

phpunit/blocks/render-block-media-text-test.php

ramonjd

I've tested manually with single media and text and nested. I think this is an improvement 👍🏻

Thanks for sticking with it.

Before	After

If @dmsnell has time, it'd be good to get his keen eye to run over the HTML Processor changes.

I noticed a bug - a nested media and text block with an alignment of left does not align left in the frontend or the editor.

I think this is an existing bug however.

packages/block-library/src/media-text/index.php

ramonjd · 2024-06-04T02:23:53Z

packages/block-library/src/media-text/index.php

+			) ) {
+				$image_tag_processor->set_attribute( 'src', esc_url( $current_featured_image ) );
+				$image_tag_processor->set_attribute( 'class', 'wp-image-' . get_post_thumbnail_id() . ' size-' . $media_size_slug );
+				$image_tag_processor->set_attribute( 'alt', trim( strip_tags( get_post_meta( get_post_thumbnail_id(), '_wp_attachment_image_alt', true ) ) ) );


Idea for possible follow up: When the image is a background image add accessible alt text via aria-label (or whatever's appropriate) on the figure element

phpunit/blocks/render-block-media-text-test.php

Co-authored-by: Ramon <[email protected]>

carolinan · 2024-06-04T04:49:53Z

I noticed a bug - a nested media and text block with an alignment of left does not align left in the frontend or the editor.

This should be fixed: #62184

dmsnell · 2024-06-05T18:39:24Z

If @dmsnell has time, it'd be good to get his keen eye to run over the HTML Processor changes.

It'd be nice to get rid of the regex, but that's not convenient or easy with the current limits of the HTML API. I'm happy with how the code is because at least the search for the right place to add the classes is robust. IMO it's easier to clean up existing code patterns (the regex_replace) than it is to start hunting down custom subclasses of the HTML API that extend its functionality.

In other words, I still think this is appropriate w.r.t. the HTML processing.

* Media & Text: Update the image replacement logic for the featured image. Add a PHPUnit test class for testing the insertion of the featured image. --------- Co-authored-by: carolinan <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: dmsnell <[email protected]>

ellatrix · 2024-06-25T06:47:44Z

I just cherry-picked this PR to the wp/6.6-rc-1 branch to get it included in the next release: ab687f7

* Media & Text: Update the image replacement logic for the featured image. Add a PHPUnit test class for testing the insertion of the featured image. --------- Co-authored-by: carolinan <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: dmsnell <[email protected]>

Media & text: Update the image replacement logic when the media is on…

f84d248

… the right

carolinan added [Type] Bug An existing feature does not function as intended [Block] Media & Text Affects the Media & Text Block labels May 27, 2024

Try to fix PHP coding standard issues.

0f4a432

Look for the figure with the correct class name instead of any figure…

0865ac3

… tag.

carolinan added 4 commits May 28, 2024 13:09

Merge branch 'trunk' into try/media-text--media-on-right

0e67eaa

Update the logic for inserting the featured image

884f732

Use ´while´ to locate the correct ´figure´ tag depending on the block settings. Add support for the image fill setting.

Try to fix PHP CS issues with spacing and the arrays.

c326e19

Try to fix PHP CS issues with spacing inside the arrays.

45b2fb3

carolinan marked this pull request as ready for review May 29, 2024 01:12

carolinan requested a review from ajitbohra as a code owner May 29, 2024 01:12

carolinan commented May 29, 2024

View reviewed changes

packages/block-library/src/media-text/index.php Outdated Show resolved Hide resolved

carolinan commented May 29, 2024

View reviewed changes

packages/block-library/src/media-text/index.php Outdated Show resolved Hide resolved

Add a test class for the Media & Text block

ee92f20

Merge branch 'trunk' into try/media-text--media-on-right

e60ac3d

ramonjd reviewed May 31, 2024

View reviewed changes

Replace the $image_tag variable and remove the CSS class on the img tag

c62c46c

Neither the variable or the CSS class are needed, and they can be removed in favor of an empty `<img>` tag.

carolinan added 3 commits May 31, 2024 08:12

Revert "Replace the $image_tag variable and remove the CSS class on t…

912ae47

…he img tag" This reverts commit c62c46c.

Add an if statement around the figure tag and the image tag

287b230

Add an if statement around the figure tag and the image tag, to make sure that the id is removed and the attributes are updated on the correct tags.

Try to fix PHP CS issues

ff42bab

Move the closing parenthesis on the array that includes the figure tag name and id. Remove whitespace.

Split test_render_block_core_media_text_featured_image into four tests

117267c

carolinan commented Jun 1, 2024

View reviewed changes

phpunit/blocks/render-block-media-text-test.php Outdated Show resolved Hide resolved

ramonjd approved these changes Jun 4, 2024

View reviewed changes

Update phpunit/blocks/render-block-media-text-test.php

5efa53b

Co-authored-by: Ramon <[email protected]>

carolinan and others added 3 commits June 4, 2024 07:27

Merge branch 'trunk' into try/media-text--media-on-right

e0402d5

Move the PHP variables for media size slug and image tag

2bb1e79

Try to fix PHPCS spacing issues

2d44777

carolinan added 3 commits June 10, 2024 08:41

Merge branch 'trunk' into try/media-text--media-on-right

a0b030d

Add a helper method for wp_query (post data) to reduce code duplication

dbe8aeb

PHP coding standards: adjust spacing around equal signs.

0b03266

carolinan added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 18, 2024

carolinan merged commit 9e53779 into trunk Jun 19, 2024
62 checks passed

carolinan deleted the try/media-text--media-on-right branch June 19, 2024 02:15

github-actions bot added this to the Gutenberg 18.7 milestone Jun 19, 2024

ellatrix 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 Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media & text: Update the image replacement logic #62030

Media & text: Update the image replacement logic #62030

carolinan commented May 27, 2024 •

edited

Loading

github-actions bot commented May 27, 2024 •

edited

Loading

carolinan commented May 28, 2024

carolinan commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

dmsnell commented May 28, 2024

carolinan commented May 28, 2024

ramonjd commented May 29, 2024

github-actions bot commented May 29, 2024 •

edited

Loading

carolinan commented May 30, 2024

ramonjd left a comment

ramonjd May 31, 2024

carolinan commented May 31, 2024

ramonjd left a comment

ramonjd Jun 4, 2024

carolinan commented Jun 4, 2024 •

edited

Loading

dmsnell commented Jun 5, 2024

ellatrix commented Jun 25, 2024

	$this->assertStringContainsString( wp_get_attachment_image_url( self::$attachment_id, 'full' ), $rendered );
	$this->assertStringContainsString( '<img alt="" src="' . wp_get_attachment_image_url( self::$attachment_id, 'full' ) . '"', $rendered );

Media & text: Update the image replacement logic #62030

Media & text: Update the image replacement logic #62030

Conversation

carolinan commented May 27, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented May 27, 2024 • edited Loading

carolinan commented May 28, 2024

carolinan commented May 28, 2024 • edited Loading

github-actions bot commented May 28, 2024 • edited Loading

dmsnell commented May 28, 2024

carolinan commented May 28, 2024

ramonjd commented May 29, 2024

github-actions bot commented May 29, 2024 • edited Loading

carolinan commented May 30, 2024

ramonjd left a comment

Choose a reason for hiding this comment

ramonjd May 31, 2024

Choose a reason for hiding this comment

carolinan commented May 31, 2024

ramonjd left a comment

Choose a reason for hiding this comment

ramonjd Jun 4, 2024

Choose a reason for hiding this comment

carolinan commented Jun 4, 2024 • edited Loading

dmsnell commented Jun 5, 2024

ellatrix commented Jun 25, 2024

carolinan commented May 27, 2024 •

edited

Loading

github-actions bot commented May 27, 2024 •

edited

Loading

carolinan commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

github-actions bot commented May 29, 2024 •

edited

Loading

carolinan commented Jun 4, 2024 •

edited

Loading