The Image block lightbox introduces an undocumented exception on how the render_block_{$this->name} filter and other filters are supposed to work

afercia commented 1 year ago

Description

Reporting here from Trac https://core.trac.wordpress.org/ticket/59559

When an Image block is set to open the image in the lighbox, the actual lightbox markup is added to the block via the render_block_{$this->name} filter.

The filter runs at priority 15. See in [https://github.com/WordPress/gutenberg/blob/15ece32d14d69f17f442dc7456c1781fd3bd5efa/packages/block-library/src/image/index.php#L73 the related code in the github repo]:

I see two issues with this implementation:

1 The implementation actually introduces an exception on how the render_block_{$this->name} is supposed to work. Or, better, on the actual content the filter will receive.

In addition to what initially reported on the Trac ticket, it is important to note that other filters like for example render_block_data will likely keep receiving the original block content without the lightbox markup

Using the render_block_{$this->name} filter like one would normally do with the default priority:

add_filter( 'render_block_core/image', 'my_custom_html' )

will only filter the original block content, regardless of whether lightbox is enabled or not.

As a developer, I would expect this filter to always receive the actual block rendered HTML:

When lightbox ie disabled: I'd expect the normal Image block HTML.
When lightbox ie enabled: I'd expect the Image block HTML including the lightbox HTML.

Instead, to be able to get the lightbox markup, I'd need to change priority. This will return the actual markup including the lightbox one:

add_filter( 'render_block_core/image', 'my_custom_html', 15 )

As said, this is an undocumented exception and at the very least:

It should be clearly explained in the filter documentation.
It should be mentioned prominently in the 6.4 release notes.

In any case, I'm not sure altering the expected behavior of a filter in this way is ideal.

2 The way the lightbox markup is added to the block markup is, in my opinion, inherently fragile. The code assumes to find a figure element. If for any reason plugins are replacing the figure element by using the filter with normal priority, the lightbox will simply not work.

At the very least the documentation part should be addressed for the 6.4 release.

Step-by-step reproduction instructions

Make sure you have a publish post with an Image block and lightbox enabled.
Use the filter with normal priority: put this in your theme's functions.php:

add_filter( 'render_block_core/image', 'my_custom_html' );
function my_custom_html( $html ) {
    var_dump( $html );
    return $html;
}

View the psot on fhe front end.
Tip: easier to test by observing the page source. With Chrome, right-click on the page and choose 'View page source'.
Observe the dumped block content does not contain the lightbox markup.
Add priority 16 to the filter: add_filter( 'render_block_core/image', 'my_custom_html', 16 );
Refresh the page and observe the dumped block content does contain the lightbox markup.

Example of the received block content wirh default priority:

<figure class="wp-block-image size-large"><img src="http://localhost:8889/wp-content/uploads/2012/12/unicorn-wallpaper-1024x768.jpg" alt="Unicorn Wallpaper" class="wp-image-1045"/></figure>

Example of the received block content wirh priority 16:

<figure data-wp-context="{ &quot;core&quot;:
    { &quot;image&quot;:
        {   &quot;imageLoaded&quot;: false,
            &quot;initialized&quot;: false,
            &quot;lightboxEnabled&quot;: false,
            &quot;hideAnimationEnabled&quot;: false,
            &quot;preloadInitialized&quot;: false,
            &quot;lightboxAnimation&quot;: &quot;zoom&quot;,
            &quot;imageUploadedSrc&quot;: &quot;http://localhost:8889/wp-content/uploads/2012/12/unicorn-wallpaper.jpg&quot;,
            &quot;imageCurrentSrc&quot;: &quot;&quot;,
            &quot;targetWidth&quot;: &quot;1600&quot;,
            &quot;targetHeight&quot;: &quot;1200&quot;,
            &quot;scaleAttr&quot;: &quot;&quot;,
            &quot;dialogLabel&quot;: &quot;Enlarged image&quot;
        }
    }
}" data-wp-interactive class="wp-block-image size-large wp-lightbox-container"><img data-wp-effect--setStylesOnResize="effects.core.image.setStylesOnResize" data-wp-effect="effects.core.image.setButtonStyles" data-wp-init="effects.core.image.setCurrentSrc" data-wp-on--load="actions.core.image.handleLoad" src="http://localhost:8889/wp-content/uploads/2012/12/unicorn-wallpaper-1024x768.jpg" alt="Unicorn Wallpaper" class="wp-image-1045"/><button
type="button"
aria-haspopup="dialog"
aria-label="Enlarge image: Unicorn Wallpaper"
data-wp-on--click="actions.core.image.showLightbox"
data-wp-style--width="context.core.image.imageButtonWidth"
data-wp-style--height="context.core.image.imageButtonHeight"
data-wp-style--left="context.core.image.imageButtonLeft"
data-wp-style--top="context.core.image.imageButtonTop"
></button>        <div data-wp-body="" class="wp-lightbox-overlay zoom"
data-wp-bind--role="selectors.core.image.roleAttribute"
data-wp-bind--aria-label="selectors.core.image.dialogLabel"
data-wp-class--initialized="context.core.image.initialized"
data-wp-class--active="context.core.image.lightboxEnabled"
data-wp-class--hideAnimationEnabled="context.core.image.hideAnimationEnabled"
data-wp-bind--aria-modal="selectors.core.image.ariaModal"
data-wp-effect="effects.core.image.initLightbox"
data-wp-on--keydown="actions.core.image.handleKeydown"
data-wp-on--touchstart="actions.core.image.handleTouchStart"
data-wp-on--touchmove="actions.core.image.handleTouchMove"
data-wp-on--touchend="actions.core.image.handleTouchEnd"
data-wp-on--click="actions.core.image.hideLightbox"
>
    <button type="button" aria-label="Close" style="fill: var(--wp--preset--color--contrast)" class="close-button" data-wp-on--click="actions.core.image.hideLightbox">
        <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="15" height="15" aria-hidden="true" focusable="false"><path d="M13 11.8l6.1-6.3-1-1-6.1 6.2-6.1-6.2-1 1 6.1 6.3-6.5 6.7 1 1 6.5-6.6 6.5 6.6 1-1z"></path></svg>
    </button>
    <div class="lightbox-image-container">
<figure class="wp-block-image size-large responsive-image"><img data-wp-bind--src="context.core.image.imageCurrentSrc" data-wp-style--object-fit="selectors.core.image.lightboxObjectFit" src="" alt="Unicorn Wallpaper" class="wp-image-1045"/></figure>
</div>
    <div class="lightbox-image-container">
<figure class="wp-block-image size-large enlarged-image"><img data-wp-bind--src="selectors.core.image.enlargedImgSrc" data-wp-style--object-fit="selectors.core.image.lightboxObjectFit" src="" alt="Unicorn Wallpaper" class="wp-image-1045"/></figure>
</div>
    <div class="scrim" style="background-color: var(--wp--preset--color--base)" aria-hidden="true"></div>
</div></figure>

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

mikachan commented 1 year ago

@artemiomorales Pinging you here as I'm not sure if this is already on your radar. Looks like this is a potential documentation update. Thank you 🙏

bph commented 1 year ago

@afercia Wow, thanks for surfacing this change in behavior.

At the very least the documentation part should be addressed for the 6.4 release.

It's been added to the Misc Editor Changes in 6.4 and linking to this issue.

Would that work for now?

afercia commented 1 year ago

@bph thank you for taking care of the dev note. I think the developers reference should be updated as well, see for example https://developer.wordpress.org/reference/hooks/render_block_this-name/

Would that work for now?

Given today it's release day, it's okay for now. I'm still a little skeptical on the implementation as it's not expected that WordPress uses a filter this way, altering the developers expectations. I'd strongly recommend a follow-up to explore a more standard way to add the lightbox markup.

t-hamano commented 10 months ago

A similar issue was reported in #57966, but improvements to the Interactivity API appear to have resolved the issue. Can you confirm whether this issue can still be reproduced with the latest Gutenberg trunk?

afercia commented 10 months ago

The built-in filter with priority 15 is still there. Nothing has changed. As such, anyone using this filter with a priority less than 16 will not have access to the markup added by lightbox.

To me, anything output by WordPress core should always happen at default priority 10. The 'architectural' decision to use priority 15 is less than ideal and fundamentally it's a hack that should never have landed in core. I'd still think this should be revised and a new way to add the lightbox markup should be implemented.

Note: the only things that have been done related to this issue is that the built-in filter at priority 15 is now:

Mentioned in the WordPress 6.4 field guide, in the miscellaneous editor changes post post on Make.
Mentioned in a user contributed note comment in the filter documentation.

luisherranz commented 10 months ago

ccing @artemiomorales and @michalczaplinski as they were the ones who worked on this functionality (as far as I know), and could tell you why the filter requires that priority and if it can be changed or not.

artemiomorales commented 10 months ago

As noted in a comment in the code, the priority of 15 is to ensure that it runs after the duotone filter has been applied, as well as any other filters that may have applied by plugins:

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.

I'm not sure how we would fix this. We could potentially move the lightbox logic to a separate file and include it in the load.php after the duotone file — perhaps that would work?

I'm not sure how we would allow plugins to apply their own filters to be included in the lightbox though, or if that's a use case we should be concerned with.

t-hamano commented 10 months ago

As I understand it, the only way to intervene in the markup of a rendered block is to use one of the following filters:

render_block
render_block_{block_name}

One approach would be to use render_block instead of render_block_{block_name}. That is, make the following changes to inject the lightbox HTML:

diff --git a/packages/block-library/src/image/index.php b/packages/block-library/src/image/index.php
index add8e5989a..9654cba1d1 100644
--- a/packages/block-library/src/image/index.php
+++ b/packages/block-library/src/image/index.php
-               add_filter( 'render_block_core/image', 'block_core_image_render_lightbox', 15, 2 );
+               add_filter( 'render_block', '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 );
+               remove_filter( 'render_block', '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 ) ) {
@@ -125,6 +125,9 @@ function block_core_image_get_lightbox_settings( $block ) {
  * @return string Filtered block content.
  */
 function block_core_image_render_lightbox( $block_content, $block ) {
+       if ( 'core/image' !== $block['blockName'] ) {
+               return $block_content;
+       }
        /*
         * If it's not possible that an IMG element exists then return the given
         * block content as-is. It may be that there's no actual image in the block

The render_block hook runs before render_block_{block_name} so even if the user has applied a filter with a default priority of 10, like below, they will always have access to the markup containing the lightbox HTML.

add_filter( 'render_block_core/image', 'my_custom_html' );
function my_custom_html( $block_content ) {
    var_dump( $block_content );
    return $block_content;
}

However, this solution is not ideal. Because if a user had the same render_block filter with a priority lower than 15 as below, they would not be able to access the markup containing the lightbox HTML.

add_filter( 'render_block', 'my_custom_html', 10, 2 );
function my_custom_html( $block_content, $block ) {
    if ( 'core/image' === $block['blockName'] ) {
        // Do something.
        // Lightbox HTML is not included.
        var_dump( $block_content );
    }
    return $block_content;
}

t-hamano commented 10 months ago

On the other hand, I think that if the HTML API evolves, we might be able to deal with this problem.

This means that regardless of how the user changes the markup, it will find the right place to inject the lightbox HTML, via the HTML API instead of the unstable preg_replace function.

In that case, we might probably end up injecting a lightbox with minimal priority like this:

add_filter( 'render_block_core/image', 'block_core_image_render_lightbox', PHP_INT_MAX, 2 );

djcowan commented 7 months ago

Apologies for dumping this here randomly. "Expand on click" functionality may conflict with Jetpack > Carousel Reference Jetpack issue # 32668 We spent a few hours unpluggin and deconfiguing a number of image related plugins and hope this comment can be of use. Will return the favour to the jetpack team.

WordPress / gutenberg