Image Block: Add data-id attribute on server side render for core galleries

[ramonjd]

Resolves #35907

Description

This PR renders a data-id attribute on the image element of Image Blocks on if a data-id property exists in the block's attributes.

The Gallery Block now has a filter render_block_data which adds a data-id to Image Blocks before the Gallery Block is processed.

Why?

The answer is phrased very succinctly in the issue

Block gallery image markup has included a data-id attribute on each gallery image tag since WordPress 5.0. This attribute includes the image attachment post object's id for images hosted locally in the media library. It allows plugins and themes to access other structured data related to the image through the core attachment post type. This can be used in both PHP and JS to pre- or post-process the post content markup.

Testing

Enable the Gallery experiment on Gutenberg

Insert a gallery and a single image into a post.

Make sure that the published gallery images and single image markup includes the data-id attribute on the <img /> tags.

Check that other galleries are still working, e.g., Jetpack tiled gallery.

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[ramonjd]

An example of the resulting breakage can be seen with the Arbutus theme. Using the gallery post format, the front page excerpt view should display a partial gallery with the first several images from in the post content. This doesn't work with the refactored gallery block - a blank bar displays instead.

@celloexpressions I couldn't get things working with Arbutus. The data-id attribute is added to rendered images with this PR, but I couldn't see the gallery excerpts for gallery post types that contain a gallery. Just a blank area:

Do you have any ideas of how I could test this? Thank you!

[andrewserong]

Nice simple fix @ramonjd! This is testing nicely for me with normal images. Just left a note about adding an isset check to prevent a warning when dealing with images that are linked to external images (and so don't have an id), but otherwise LGTM!

[ramonjd]

I appreciate the test @andrewserong It was a bit of a push and run today, so thanks for catching those oversights. 🙇

[glendaviesnz]

This tests well for me in terms of adding the data-id to the frontend. The issue with the Arbutus theme is that the method that is creating the gallery excerpt is running against the raw post content before this callback is applied, eg.:

$result = preg_match_all( '/(<img[^>]+data-id="([^\\"]+)"[^>]+\\/>)/', $post->post_content, $matches ); 

is running against:

<!-- wp:image {"id":321,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://localhost:4759/wp-content/uploads/2021/10/3840x2160-bright-green-solid-color-background-scaled.jpeg" alt="" class="wp-image-321"></figure>
<!-- /wp:image -->

I will have a look and see if there is someway of modifying the post content before it hits the theme functions.

[glendaviesnz]

I have also tried add_filter( 'render_block' with a priority of 1, but that also seems to add it too late in the chain.

[ramonjd]

Thanks for running those tests @glendaviesnz

I don't know if I'll get further than you but I'll do some further digging around as well.

[ramonjd]

I think by using the_post action hook we can modify $post->content.

I didn't get very far yet, and I'm not sure this is the right direction. We need to:

• check for the existence of blocks
• get the image id
• attach the right data-id to the right img tag

 function get_block_core_image_post_content( $post, $query ) {
	if ( has_blocks( $post->post_content ) ) {
        $content = $post->post_content;
        $blocks = parse_blocks( $content );
		foreach ( $blocks as $block ) {
			if ( 'core/gallery' === $block['blockName'] ) {
				foreach ( $block['innerBlocks'] as $inner_block ) {
					if ( 'core/image' === $inner_block['blockName'] ) {
						if ( isset( $inner_block['attrs']['id'] ) ) {
							$data_id_attribute = 'data-id="' . esc_attr( $inner_block['attrs']['id'] ) . '"';

							// do something with the content
						}
					}
				}
			}
		}

		$post->post_content = $content;
    }
    return $post;
}

add_action( 'the_post', 'get_block_core_image_post_content', 10, 2 );

Seems very convoluted to satisfy a theme. I wonder if there are other themes doing something similar to Arbutus. Or maybe we could somehow suggest a patch for the theme.

[celloexpressions]

Thanks for looking into this. Confirming that any server side filtering on the output needs to run on a hook before the global $post object might be used to build the frontend markup. the_post is likely a good place to do that.

The Arbutus theme is an example, but this fix would apply to any themes or plugins that need to find the image's attachment ID in pre- or post-processing. This data attribute has been in the markup for a few years now, so there are likely to be a lot of different things expecting this markup (including non-public themes and plugins).

Filtering as early as possible on the output is probably the second-best option in terms of strictly maintaining backwards compatibility, after saving the data attribute with the markup in the post content. Any solution here of course needs to be balanced with performance; however, the burden of preserving backwards compatibility should generally be with core first, then themes/plugins, then end users last.

[ramonjd]

Thanks for the feedback @celloexpressions 🙇

after saving the data attribute with the markup in the post content

Maybe there is a case to be had to continue on with #30710 and add the deprecation.

Though you make a good point about the order of responsibility of backwards compatibility. I'm not completely over @glendaviesnz's work in WordPress/wordpress-develop#1788, but hopefully it might serve to mitigate the need for post processing.

[glendaviesnz]

@ramonjd the change I just pushed seems to add the data-id to the post content at a point that prior to when the Arbutus theme templates parses the content, and with this change in place the gallery excerpt appears as expected. For some reason the id is gone again when it is actual output to the front end though - which may be an issue if other themes were using it as a css or js target - we could combine the_post and the block render I guess, but that seeks like a lot of extra parsing?

@celloexpressions we are also hoping to get https://core.trac.wordpress.org/ticket/43826 merged into 5.9, in which case get_post_gallery_images would return successfully so this additional parsing of data ids would not be needed, so I am wondering if we need both? The advantage of just using get_post_gallery_images is that the block content is only parsed if there is a theme activated that is using it, whereas with using the_post action we are parsing and string replacing on every post regardless of whether the data-id is needed or not.

@mtias FYI - in order to get the data-id added server side in a place that themes could make use of it we had to do it with the the_post action, so not sure if that affects your view about doing this server side or not - it means a lot of extra overhead parsing every post and doing string replaces on content when mostly not needed?

[ramonjd]

Suggested change

	if ( is_admin() ) {
	if ( is_admin() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) {

Is there are case where we'd want to run this code for rest requests?

I'm thinking it's safe to return early if we're targeting post data on the frontend, unless I'm missing a use case.

	if ( is_admin() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) {
		return;
	}

For example, add a var_dump('I LOVE THE BAND KISS!') or some other logging after this if statement, and try to save a post. It doesn't have to contain a gallery.

The POST response will contain invalid, but possibly true, content: string(21) "I LOVE THE BAND KISS!" {id: 115, date: "2021-11-02T03:18:06", date_gmt: "2021-11-02T03:18:06",…}

Pinging a WP API endpoint that contains gallery post content still works for me with this check in place:

E.g., http://localhost:4759/wp-json/wp/v2/posts/104

[TimothyBJacobs]

I'd be somewhat wary about using is_admin() in a condition check here. All it means is that the context when this post is setup is happening without wp-admin, it doesn't actually mean that the post is being edited or the like. Somewhat could just as easily be expecting to get this data over an admin-ajax.php call or simply trying to render their gallery in an admin page.

The same goes for REST_REQUEST. It can both be used in the Block Editor. But it can also be used by someone making an API call to grab data for somewhere on the front-end of their website where they would expect the ids to be present.

[ramonjd]

Thanks for the advice @TimothyBJacobs 🙇

I think @glendaviesnz was trying to find ways to isolate the logic to frontend rendering to avoid block validation errors, given that data-id isn't output by the save method of the image block.

🤔 Assuming block validation wasn't an issue, do you see any issues with removing the if check here entirely?

Cheers!

[TimothyBJacobs]

Assuming block validation wasn't an issue, do you see any issues with removing the if check here entirely?

No, I think removing the check entirely would be ideal.

[ramonjd]

Thanks for the change @glendaviesnz

The data-id attribute appears for Gallery v2 images on the frontend, and in the Site Editor post content query, where a gallery is my blog roll.

I checked TwentyTwentyOne/Twenty/Nineteen.

I also checked in Arbutus and the gallery posts appear as expected.

A question just to make sure: do we need to cater for v1 Galleries? Or will the migration to v2 ensure that this script picks up the image blocks?

For some reason the id is gone again when it is actual output to the front end though

Is that just for Arbutus? I checked a few core themes and things were working as expected.

I assume because Arbutus is taking the img tag with the data-id attribute and rebuilding the HTML, including the img tag without the attribute.

[glendaviesnz]

I assume because Arbutus is taking the img tag with the data-id attribute and rebuilding the HTML, including the img tag without the attribute.

It was missing even on the standard post page as well as the gallery summary that Arbutus is constructing, strange.

Given the fact that we can't reliable stop the modified content ending up back in the editor, and that we are hopefully getting a fix for get_post_galleries into 5.9, I am not sure that adding this value back in server side is worth the extra overhead. get_post_galleries can hopefully work as a fallback for this, and if that isn't enough then plugin/theme code could be modified to get the id from the wp-image-{id} classname - what do you think?

[ramonjd]

f1d27b6ae971ff73fad2156a1afa81e048fae737 adds a render_block_data filter to core/gallery.

This filter adds a custom data-id attribute to inner Image Blocks.

The Image Block render callback looks out for this custom attribute and will edit the image tag accordingly.

This way, we can target gallery images only. @glendaviesnz rightly pointed out that we'd be safer to avoid adding the attribute to all images to avoid unknown unknowns.

The imaginary scenario was:

if a lightbox plugin was grabbing gallery images via the data-id, and now it will get individual images as well

[ramonjd]

There's an issue in blocks.php where the gallery styles (plugins/gutenberg/build/block-library/blocks/gallery/style.css) for block themes aren't being loaded... 👀

It doesn't seem to be related to the filters, only the fact that we've added 'gallery.php' => 'core/gallery', to the block_names array.

And it's specific to the gallery block. Other blocks in the block_names array, such as Social Links, have stylesheets that load fine.

[ramonjd]

And it's specific to the gallery block. Other blocks in the block_names array, such as Social Links, have stylesheets that load fine.

So it turns that that, for styles to be enqueued for block based themes, I didn't know that index.php requires:

• a priority of at least 20 in the init action hook
• a render_callback function, even though the callback does no processing.

I'm not sure why however.

[glendaviesnz]

This is working as advertised for me now

[andrewserong]

This is testing well for me, @ramonjd! Just left a couple of comments where I think we need to add an isset check to avoid throwing PHP Notices, but otherwise it's looking good to me.

[andrewserong]

Thanks for fixing up those array access issues @glendaviesnz! This PR is testing well for me now, the data-id is added to images within a Gallery block that have an id attribute, images without that attribute, or that are rendered outside of a gallery block, do not have the data-id added to the markup.

LGTM!

[ramonjd]

I forgot those issets again. Thanks!