WordPress / performance

Performance plugin from the WordPress Performance Group, which is a collection of standalone performance modules.
https://wordpress.org/plugins/performance-lab/
GNU General Public License v2.0
350 stars 94 forks source link

Update `wp_calculate_image_sizes` to Reflect Changes in sizes attribute #1381

Open mukeshpanchal27 opened 1 month ago

mukeshpanchal27 commented 1 month ago

Bug Description

As discussed in https://github.com/WordPress/performance/pull/1354#pullrequestreview-2185993737, the wp_calculate_image_sizes() function does not currently account for the changes made to the sizes attribute in PR #1329.

We need to update the sizes value in wp_calculate_image_sizes() so that any usage of this core function benefits from the improved sizes attribute.

Alternatively, we should ensure that the enhanced sizes attribute is applied via the wp_calculate_image_sizes filter, so any other code that directly calls wp_calculate_image_sizes() will also benefit from the optimizations.

joemcgill commented 1 month ago

Thanks for creating the issue, @mukeshpanchal27. I agree that it would be best if third party code that calls wp_calculate_image_sizes() directly would benefit from the improvements we're making. In order to do so, we would need to be able to pass a layout constraint to the function, which wouldn't be backward compatible, so applying our optimizations via the filter is probably the best approach.

mukeshpanchal27 commented 1 month ago

@joemcgill @westonruter I have searched in WPdirectory to see if any themes or plugins use the wp_calculate_image_sizes() function to get the image sizes, but no one uses the function directly.

As explored in POC #1382, we can't add wp_calculate_image_sizes filter in an accurate sizes filter without creating some issues that Weston pointed out. Can we go ahead and close this issue for now? We can re-open it if a possible solution arises.

Let me know if I missed anything.

joemcgill commented 1 month ago

The WP Directory search uses Regex, so you need to modify the search a little bit. Looks like there are some uses of the function after all:

mukeshpanchal27 commented 1 month ago

Thanks @joemcgill. Since we don't have a next step for this, we'll move it to the next release.

felixarntz commented 1 week ago

@mukeshpanchal27 @joemcgill Just discovered this issue via the doc for further enhancing the sizes accuracy, I'm wondering: Why can't we enhance wp_calculate_image_sizes() to pass an array with layout constraints to it? If it's an optional parameter, I don't think it would be backward incompatible.

If there's layout constraints provided, they could be used by the function and the filter. If none are provided, it would just be an empty array, the function would behave the same it does now, and the filter would also only receive that empty array.

If we make this change in Core, I think we would need to define a bit more what the shape of those layout constraints would look like (e.g. which array keys to support etc), but I think it's worthwhile pursuing as indeed this would allow us to make the optimizations in the central function intended for this purpose, rather than in the arbitrary wp_content_img_tag filter. This could also bring the enhancements to more areas of WordPress out of the box, e.g. functions like wp_get_attachment_image() that call wp_calculate_image_sizes() directly.

mukeshpanchal27 commented 6 days ago

This is what i and @joemcgill discussed when we exploring the possible approaches for ancestor blocks definition work.

As the beta for 6.7 is coming in few weeks, mean time can we add the new array parameter for wp_calculate_image_sizes() and also passed it to filter so we can get more time to think the keys that we can add for support?

/**
 * Creates a 'sizes' attribute value for an image.
 *
 * @since 4.4.0
 * @since 6.7.0 Added the `$layout_constraints` parameter.
 *
 * @param string|int[] $size               Image size. Accepts any registered image size name, or an array of
 *                                         width and height values in pixels (in that order).
 * @param string|null  $image_src          Optional. The URL to the image file. Default null.
 * @param array|null   $image_meta         Optional. The image meta data as returned by 'wp_get_attachment_metadata()'.
 *                                         Default null.
 * @param int          $attachment_id      Optional. Image attachment ID. Either `$image_meta` or `$attachment_id`
 *                                         is needed when using the image size name as argument for `$size`. Default 0.
 * @param array        $layout_constraints {
 *     Optional. The layout constraints for accurate calculating the sizes.
 *
 *     @type string $block_name  The block name.
 *     @type string $block_align The block alignment.
 *     @type array  $column      The column data.
 * }
 * @return string|false A valid source size value for use in a 'sizes' attribute or false.
 */
function wp_calculate_image_sizes( $size, $image_src = null, $image_meta = null, $attachment_id = 0, $layout_constraints = array() ) {

Some other key can be added in the allow list for $layout_constraints array.

WDYT?

felixarntz commented 6 days ago

@mukeshpanchal27 I think overall what you're proposing makes sense. Though I think we shouldn't introduce this to Core until we also have an idea about what the supported (and documented) keys would be for the $layout_constraints parameter.

If we can figure that out in time for 6.7 Beta, that's great. However, it's not super urgent because we can still work towards that solution even without the Core function adjusted: For example, if we implement our enhanced logic in the plugin, we could use a workaround, e.g. temporarily set a global or something like that which the filter then uses. This would of course be a hack, but okay for this kind of thing where we have a clean path forward for once this would be in Core.

joemcgill commented 2 hours ago

I'm definitely open to the idea that we may need to modify the signature of wp_calculate_image_sizes() to allow for additional context to be passed.

However, it's important to consider where this function is called:

wp_get_attachment_image() – Creates standalone image markup from an attachment ID, which won't have any of the context we need.

wp_image_add_srcset_and_sizes() – Dynamically adds responsive image attributes to images included in content, which can be the block template or when filtering the_content, the_excerpt, or widget_text_content. In these cases, the call stack looks something like this:

wp_filter_content_tags()
  > wp_img_tag_add_srcset_and_sizes_attr()
  > wp_image_add_srcset_and_sizes()

Right now, wp_filter_content_tags() (link) takes a blob of HTML and uses preg_match to store all of the img elements to an array. Next, it iterates over the array to apply dynamic changes (e.g. adding srcset and sizes) to the img tags before using str_replace to replace the original img in the content with the modified img tag. Because of this approach, the modifications are all happening outside of the context of the full HTML structure and the context we need is not available.

To work around this, we either need to redesign the way wp_filter_content_tags() works, so that tags are being modified within the context of the HTML being filtered, or we need to move the place where wp_calculate_image_sizes() is being added to image markup to another part of the system.

From a plugin perspective, we can likely work around these limitations by adding a filter to the wp_filter_content_tags hook which passes the additional context we need before calling wp_filter_content_tags() directly. Here's an experimental gist that adds a temporary filter during block rendering to add sizes directly to an image block using attributes from the block object itself.

This works, but a more direct approach that modifies core directly would be best.