-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Media & text: Update the image replacement logic #62030
Conversation
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 |
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) |
Flaky tests detected in 0b03266. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9561677081
|
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 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 |
Use ´while´ to locate the correct ´figure´ tag depending on the block settings. Add support for the image fill setting.
@dmsnell Thank you. @sirreal suggested something similar. With this update I am not extending the class but still using |
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. 🎉
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
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: |
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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: Or (after starting wp-env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
$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 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$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.
Neither the variable or the CSS class are needed, and they can be removed in favor of an empty `<img>` tag.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
![Screenshot 2024-06-04 at 11 52 53 AM](https://private-user-images.githubusercontent.com/6458278/336284348-6e5602e2-ec5d-4154-80e2-2c8687ecc78a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxMTgyMDMsIm5iZiI6MTcyNDExNzkwMywicGF0aCI6Ii82NDU4Mjc4LzMzNjI4NDM0OC02ZTU2MDJlMi1lYzVkLTQxNTQtODBlMi0yYzg2ODdlY2M3OGEucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDgyMCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA4MjBUMDEzODIzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZTY1YzZkYWZjNWMzNDQ3MmJiNmUzMWY4OWE3NDc3MWRiMTdhZjM3MTJmMmU0ZGUwYWMwN2ZjODIwYzVhZmRiMSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.IORWX6_A6GqyaNJrHVnoJYR7LxfRG72JqaKbuQ4r-C0)
I think this is an existing bug however.
) ) { | ||
$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 ) ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
This should be fixed: #62184 |
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 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 <poena@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: dmsnell <dmsnell@git.wordpress.org>
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 <poena@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: dmsnell <dmsnell@git.wordpress.org>
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 toright
.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.
And with the PR:
Testing Instructions for Keyboard
Screenshots or screencast