Skip to content

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

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.

Sign up for GitHub

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

Merged
artemiomorales merged 3 commits into from
Oct 11, 2023
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
094b1d6
Remove the filter and view file of the lightbox when the lightbox set…
afercia Oct 6, 2023
b396642
Make sure to add filter and view file when needed.
afercia Oct 9, 2023
e498bf9
Fix block comment indentation.
afercia Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 32 additions & 30 deletions packages/block-library/src/image/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,45 +32,47 @@ function render_block_core_image( $attributes, $content, $block ) {
$processor->set_attribute( 'data-id', $attributes['data-id'] );
}

$lightbox_enabled = false;
$link_destination = isset( $attributes['linkDestination'] ) ? $attributes['linkDestination'] : 'none';
$lightbox_settings = block_core_image_get_lightbox_settings( $block->parsed_block );

// If the lightbox is enabled and the image is not linked, flag the lightbox to be rendered.
if ( isset( $lightbox_settings ) && 'none' === $link_destination ) {
$view_js_file_handle = 'wp-block-image-view';
$script_handles = $block->block_type->view_script_handles;

if ( isset( $lightbox_settings['enabled'] ) && true === $lightbox_settings['enabled'] ) {
$lightbox_enabled = true;
}
}

// If at least one block in the page has the lightbox, mark the block type as interactive.
if ( $lightbox_enabled ) {
/*
* 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 &&
isset( $lightbox_settings['enabled'] ) &&
true === $lightbox_settings['enabled']
) {
$block->block_type->supports['interactivity'] = true;
Copy link
Contributor Author

@afercia afercia Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@artemiomorales artemiomorales Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@luisherranz luisherranz Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

}

// Determine whether the view script should be enqueued or not.
$view_js_file = 'wp-block-image-view';
if ( ! wp_script_is( $view_js_file ) ) {
$script_handles = $block->block_type->view_script_handles;

// If the script is not needed, and it is still in the `view_script_handles`, remove it.
if ( ! $lightbox_enabled && in_array( $view_js_file, $script_handles, true ) ) {
$block->block_type->view_script_handles = array_diff( $script_handles, array( $view_js_file ) );
}
// If the script is needed, but it was previously removed, add it again.
if ( $lightbox_enabled && ! in_array( $view_js_file, $script_handles, true ) ) {
$block->block_type->view_script_handles = array_merge( $script_handles, array( $view_js_file ) );
if ( ! in_array( $view_js_file_handle, $script_handles, true ) ) {
Copy link
Contributor

@artemiomorales artemiomorales Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screenshot 2023-10-06 at 4 36 22 PM

$block->block_type->view_script_handles = array_merge( $script_handles, array( $view_js_file_handle ) );
}
}

if ( $lightbox_enabled ) {
// 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.
/*
* 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 {
/*
* 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 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();
Expand Down