Try: Make gallery randomization work when nested

[t-hamano]

Fixes #58404

What?

This PR resolves an issue where the randomization of the Gallery block added in #57477 does not work when nested.

Why?

Randomization is achieved by shuffling the $parsed_block['innerBlocks'] via the render_block_data hook. However, we found a problem with this hook: it cannot override $parsed_block['innerBlocks'] when blocks are nested.

This issue is also reported in the core ticket: https://core.trac.wordpress.org/ticket/56417

Ideally, render_block_data should be improved, but the impact is probably large and there isn't much time left to fix it in WP6.5.

So in this PR, we will try similar randomization in the render callback function instead of render_block_data. If we can't find a good solution, we may need to consider reverting the randomization function.

How?

Sort theIimage blocks within the content HTML based on the following logic:

• Use WP_HTML_Tag_Processor to add a sequential number (data-image-index) to elements with the wp-block-image class. This is to identify duplicate images when they are included in a gallery.
• Use the preg_match_all() function to extract elements with the data-image-index attribute, i.e. Image blocks.
• Randomize the extracted Image blocks.
• Use the preg_replace_callback() function to extract the Image blocks again and replace each Image block with the randomized Image in turn.
• Finally, use WP_HTML_Tag_Processor to remove unnecessary data-image-index attributes.

To be honest, this approach may not be perfect for the following reasons:

• What if the user has already given the data-image-index attribute?
• What if the user had rewritten the wp-block-image class?
• What if the user uses the new allowed_blocks to insert something other than an image block?

Let me discuss whether we should ship this approach or bring back the randomization feature. 🙇

Testing Instructions

• Insert a Gallery block and add images.
• Enable "Randomize order".
• On the front end, check that the image order changes every time you reload the browser.

[github-actions]

Flaky tests detected in ddc7ff9.
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/7800718336
📝 Reported issues:

• [Flaky Test] aria-disabled is correctly hydrated for different values #54370 in /test/e2e/specs/interactivity/directive-bind.spec.ts

[github-actions]

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.

Core SVN

Core Committers: Use this line as a base for the props when committing in SVN:

Props wildworks, aaronrobertshaw, youknowriad, ramonopoly, richtabor.

GitHub Merge commits

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: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>

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

[aaronrobertshaw]

Thanks for the fix @t-hamano 👍

✅ I could replicate the issue when a randomized gallery was nested
✅ This PR fixes the issue

I think there's some unnecessary manipulation of the gallery markup which we could avoid. I've left some suggested code tweaks inline but in case it's easier I'll include a diff in this review.

Apologies if I'm missing some logic as to why we need to add and then remove index attributes.

Diff with suggestion changes

diff --git a/packages/block-library/src/gallery/index.php b/packages/block-library/src/gallery/index.php
index e3eeeef3f0..464449154a 100644
--- a/packages/block-library/src/gallery/index.php
+++ b/packages/block-library/src/gallery/index.php
@@ -123,27 +123,17 @@ function block_core_gallery_render( $attributes, $content ) {
 	// nested.
 	// TODO: In the future, if this hook supports updating innerBlocks in
 	// nested blocks, it should be refactored.
-	// See: https://github.com/WordPress/gutenberg/pull/58733
+	// See: https://github.com/WordPress/gutenberg/pull/58733.
 	if ( empty( $attributes['randomOrder'] ) ) {
 		return $processed_content;
 	}
 
-	$processor = new WP_HTML_Tag_Processor( (string) $processed_content );
-
-	// Add an index to each figure tag to uniquely identify each Image block.
-	$i = 0;
-	while ( $processor->next_tag( array( 'class_name' => 'wp-block-image' ) ) ) {
-		$processor->set_attribute( 'data-image-index', $i );
-		++$i;
-	}
-	$content = $processor->get_updated_html();
-
-	$pattern = '/<figure[^>]*\bdata-image-index\b[^>]*>.*?<\/figure>/';
+	$pattern = '/<figure[^>]*\bwp-block-image\b[^>]*>.*?<\/figure>/';
 
 	// Find all Image blocks.
-	preg_match_all( $pattern, $content, $matches );
+	preg_match_all( $pattern, $processed_content, $matches );
 	if ( ! $matches ) {
-		return $content;
+		return $processed_content;
 	}
 	$image_blocks = $matches[0];
 
@@ -157,16 +147,9 @@ function block_core_gallery_render( $attributes, $content ) {
 			++$i;
 			return $new_image_block;
 		},
-		$content
+		$processed_content
 	);
 
-	// Remove the index from each Image block.
-	$processor = new WP_HTML_Tag_Processor( $content );
-	while ( $processor->next_tag( array( 'class_name' => 'wp-block-image' ) ) ) {
-		$processor->remove_attribute( 'data-image-index' );
-	}
-	$content = $processor->get_updated_html();
-
 	return $content;
 }
 /**

Here's what I see with the suggested diff applied. The top gallery is nested while the second is a duplicate of the first gallery but as a top level block.

Screen.Recording.2024-02-07.at.1.24.35.pm.mp4

[t-hamano]

Thanks for the review, @aaronrobertshaw!

I think the index-less approach you suggested makes sense and also made the code simpler. I think I've handled everything, but if I've missed anything please let me know.

[aaronrobertshaw]

🚀 Thanks for the fast turnaround on these changes @t-hamano!

The code is looking good. I left one small comment regarding another new linting error but if that gets tweaked, I'm happy to give this an approval now.

This will also need to get backported in time for 6.5, correct?

One last thing, I think this is the sort of thing that would be good to have some test coverage for. As a follow-up, maybe a unit test could be added? The cover block might provide an example.

[t-hamano]

This will also need to get backported in time for 6.5, correct?

Yes. My understanding is that if we merge this PR now, it will become part of Gutenberg 17.7, so I believe it will automatically reflect this PR as of WP6.5Beta1. Or do I need to backport it first?

One last thing, I think this is the sort of thing that would be good to have some test coverage for. As a follow-up, maybe a unit test could be added? The cover block might provide an example.

I think unit tests make sense. However, even with randomization enabled, there is a small chance that it will return the markup in exactly the same order, which may make the test a bit unstable 😅

[aaronrobertshaw]

so I believe it will automatically reflect this PR as of WP6.5Beta1

My mistake, I was forgetting this was part of the block library and should, I believe, be covered by the package updates if it makes this Gutenberg release.

Best to get this merged then! 🚢

[getdave]

Thanks for being diligent folks. I can confirm this will not require a manual backport because it has merged prior to the RC for Gutenberg 17.7.0.