Remove the lightbox filter and view file when the lightbox setting is disabled.

[afercia]

Fixes #55119

What?

With multiple image block on a page, setting one image to open in the lightbox makes all following images open in the lightbox, even if the related setting is disabled.

Why?

Clearly not the expected behavior.

How?

• Makes sure the filter that adds the lightbox markup and the view JavaScript file are removed when not needed.
• Simplifies the code as there is really no need to set a $lightbox_enabled flag that stays true for all the following blocks.

Testing Instructions

• Create a post.
• Add four image blocks.
• Set only the second image block to open the image in the lightbox.
• Publish and view the front end.
• Observe the first image has no lightbox enabled.
• Observe the second image does have the lightbox enabled, as expected.
• Check the lightbox works as expected.
• Observe the third image has no lightbox enabled, as expected.
• Observe the fourth image has no lightbox enabled, as expevted.

Additionally:

• Set only the first image to open in the lightbox.
• Save and view the front end.
• Observe that only the first image opens in the lightbox.
• Check the lightbox works as expected.

Testing Instructions for Keyboard

Screenshots or screencast

[github-actions]

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

• [Flaky Test] should insert elements from multiple completers in a single block #47916 in /test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js

[artemiomorales]

Thanks for opening this PR! I tested and while the code works when the lightbox is enabled for the second or subsequent images, it does not work when the lightbox is enabled on the first image, resulting in an error (see comment below).

I think this requires a little reworking of the logic. Here's a suggested revision that will only remove the view script if the lightbox settings are absent or the lightbox is disabled, which seems to work, though there are lots of ways of doing this and you may have ideas on how else we can clearly express the intent:

/*
* Remove the filter and the JavaScript view file if previously added by
* other Image blocks.
*/
remove_filter( 'render_block_core/image', 'block_core_image_render_lightbox', 15 );

/*
* If the lightbox is enabled and the image is not linked, add the filter
* and the JavaScript view file.
*/
if ( isset( $lightbox_settings ) && 'none' === $link_destination ) {

	if( isset( $lightbox_settings['enabled'] ) && true === $lightbox_settings['enabled'] ) {
		$block->block_type->supports['interactivity'] = true;

		if ( ! in_array( $view_js_file_handle, $block->block_type->view_script_handles, true ) ) {
			$block->block_type->view_script_handles = array_merge( $script_handles, array( $view_js_file_handle ) );
		}

		/*
		* This render needs to happen in a filter with priority 15 to ensure
		* that it runs after the duotone filter and that duotone styles are
		* applied to the image in the lightbox. We also need to ensure that the
		* lightbox works with any plugins that might use filters as well. We
		* can consider removing this in the future if the way the blocks are
		* rendered changes, or if a new kind of filter is introduced.
		*/
		add_filter( 'render_block_core/image', 'block_core_image_render_lightbox', 15, 2 );
	} else {
		// If the script is not needed, and it is still in the `view_script_handles`, remove it.
		if ( in_array( $view_js_file_handle, $script_handles, true ) ) {
			$block->block_type->view_script_handles = array_diff( $script_handles, array( $view_js_file_handle ) );
		}
	}

}

return $processor->get_updated_html();

[artemiomorales]

It appears that when running this code when the lightbox is enabled for just the first image block, $script_handles contains the 'wp-block-image-view' handle, so this conditional erroneously evaluates to false and the view script is not loaded, resulting in the following error:

[afercia]

@artemiomorales thanks. Right, I think one of my last changes aimed to avoid the if / else but I see it is necessary instead.

[afercia]

An E2E test related to the cover image keeps failing.

editor/blocks/cover.spec.js:74:2 › Cover › dims background image down by 50% with the average image color when an image is uploaded 

It passes locally for me. It seems completely unrelated to the changes in this PR. I'll try to rebase onto latest trunk but if it keeps failing I't need some help to understand why.

[tellthemachines]

An E2E test related to the cover image keeps failing.

This was just fixed in #55168 so another rebase should get it to pass!

[afercia]

Rebased on top of latest trunk. Hopefully the failing test should now pass.

[afercia]

Qustion: should this:

$block->block_type->supports['interactivity'] = true;

be reverted to false in the else?
I have no idea what that code line actually does. Any feedback would be appreciated.

[artemiomorales]

This works as expected now 🙌

[afercia]

@artemiomorales @SantosGuillamot when you have a chance, I'd appreciate your feedback on this line of code:

• I have no idea what it does.
• Is it really necessary?
• If it is, should it be changed back to false in the else statement?

[artemiomorales]

From what I understand, that line moves the interactivity scripts to the footer, which is a helpful optimization.
It shouldn't be changed to false, because it should be true so long as any image has the lightbox enabled.

Pinging @luisherranz to see if he'd like to detail better! In any case, I don't believe a change here is a blocker for this PR.

[luisherranz]

We're actually thinking about removing the conditionality and use supports.interactivity: true for any block that might be interactive at some point.

But we also need to figure out how those conditional interactive blocks can declare that conditionality, or even be able to combine different interactive behaviors depending on their configuration.

Related conversation here:

• Mechanism to detect blocks compatible with the Interactivity API #54194

[mikachan]

I just cherry-picked this PR to the 6.4-rc1 branch to get it included in the next release: 63bba04