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

Image Placeholders can be hardened with HTML Tag Processor #1475

Closed westonruter closed 2 weeks ago

westonruter commented 3 weeks ago

Per https://github.com/WordPress/performance/issues/1446#issuecomment-2274643725:

I just realized a case where this drafted code using HTML Tag Processor might not work as expected, and that is specifically in relation to the PICTURE element output by Modern Image Formats. Enhanced Responsive Images is adding the auto value at priority 10 of the wp_content_img_tag filter:

https://github.com/WordPress/performance/blob/e7313851f39749886a4ae0bacbcd620a6effce3d/plugins/auto-sizes/hooks.php#L79

However, Modern Image Formats is also using this same filter and priority to rewrite the IMG as PICTURE:

https://github.com/WordPress/performance/blob/e7313851f39749886a4ae0bacbcd620a6effce3d/plugins/webp-uploads/hooks.php#L769

Since the auto-sizes slug is alphabetically before webp-uploads, then it seems the auto value is injected first so that when the PICTURE is generated by Modern Image Formats, it has auto in all the sizes attributes as expected. However, this seems brittle. To be even more robust, it seems the HTML Tag Processor code here should be updated to support adding auto to the SOURCE tags. Otherwise, the priorities of the filters could be made distinctly different, either having auto_sizes_update_content_img_tag() run at a priority below 10 or have webp_uploads_wrap_image_in_picture() run at a priority over 10.

Nevertheless, take note that Enhanced Responsive Images also has a wp_content_img_tag filter that runs at priority 9, so it would need to be bumped down to priority 8 if we go in that direction:

https://github.com/WordPress/performance/blob/e7313851f39749886a4ae0bacbcd620a6effce3d/plugins/auto-sizes/hooks.php#L226

Lastly, Image Placeholders is also filtering wp_content_img_tag but at priority 20:

https://github.com/WordPress/performance/blob/e7313851f39749886a4ae0bacbcd620a6effce3d/plugins/dominant-color-images/hooks.php#L166

I was worried that this would mean that PICTURE elements generated by Modern Image Formats would be broken with Image Placeholders. But this doesn't seem to be the case. Example image with picture element turned off:

<img
  data-dominant-color="8c7d58"
  data-has-transparency="false"
  style="--dominant-color: #8c7d58"
  fetchpriority="high"
  decoding="async"
  width="1024"
  height="668"
  src="http://localhost:8888/wp-content/uploads/2024/06/bison1-1024x668-jpg.webp"
  alt=""
  class="not-transparent wp-image-13"
  srcset="
    http://localhost:8888/wp-content/uploads/2024/06/bison1-1024x668-jpg.webp 1024w,
    http://localhost:8888/wp-content/uploads/2024/06/bison1-300x196-jpg.webp   300w,
    http://localhost:8888/wp-content/uploads/2024/06/bison1-768x501-jpg.webp   768w,
    http://localhost:8888/wp-content/uploads/2024/06/bison1-1536x1002.jpg     1536w,
    http://localhost:8888/wp-content/uploads/2024/06/bison1-2048x1336.jpg     2048w,
    http://localhost:8888/wp-content/uploads/2024/06/bison1-1568x1023.jpg     1568w
  "
  sizes="(max-width: 650px) 100vw, 650px"
/>

And with picture element turned on:

<picture
  class="not-transparent wp-picture-13"
  style="--dominant-color: #8c7d58; display: contents"
  ><source
    type="image/webp"
    srcset="
      http://localhost:8888/wp-content/uploads/2024/06/bison1-1024x668-jpg.webp 1024w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-300x196-jpg.webp   300w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-768x501-jpg.webp   768w
    "
    sizes="(max-width: 650px) 100vw, 650px" />
  <source
    type="image/jpeg"
    srcset="
      http://localhost:8888/wp-content/uploads/2024/06/bison1-1024x668.jpg 1024w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-300x196.jpg   300w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-768x501.jpg   768w
    "
    sizes="(max-width: 650px) 100vw, 650px" />
  <img
    data-dominant-color="8c7d58"
    data-has-transparency="false"
    fetchpriority="high"
    decoding="async"
    width="1024"
    height="668"
    src="http://localhost:8888/wp-content/uploads/2024/06/bison1-1024x668-jpg.webp"
    alt=""
    class="not-transparent wp-image-13"
    srcset="
      http://localhost:8888/wp-content/uploads/2024/06/bison1-1024x668-jpg.webp 1024w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-300x196-jpg.webp   300w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-768x501-jpg.webp   768w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-1536x1002.jpg     1536w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-2048x1336.jpg     2048w,
      http://localhost:8888/wp-content/uploads/2024/06/bison1-1568x1023.jpg     1568w
    "
    sizes="(max-width: 650px) 100vw, 650px"
/></picture>

This is still working. Interestingly, the result is --dominant-color: #8c7d58; appearing on the PICTURE tag whereas data-dominant-color and data-has-transparency remain on the IMG tag. This is because we added style="display:contents;" to the PICTURE in #1307, so Image Placeholders is accidentally amending the style attribute on the PICTURE tag as oppose to adding/updating the style attribute on the IMG tag:

https://github.com/WordPress/performance/blob/e7313851f39749886a4ae0bacbcd620a6effce3d/plugins/dominant-color-images/hooks.php#L143-L147

Fortunately this didn't break anything, but we just got lucky. We should update Image Placeholders to use the HTML Tag Processor too to be more reliable/consistent.

sagarnasit commented 3 weeks ago

@westonruter I would love to work on this ticket.

sagarnasit commented 3 weeks ago

it seems the HTML Tag Processor code here should be updated to support adding auto to the SOURCE tags.

@westonruter I wanted to confirm whether adding SOURCE tag needs to be updated with HTML Tag Processor?

https://github.com/WordPress/performance/blob/d6c1b0d9a667fa7db3276a3d55bcb5fd8c88d763/plugins/webp-uploads/picture-element.php#L137-L142

westonruter commented 3 weeks ago

Not at that location since that is the Modern Image Formats plugin not the Enhanced Responsive Images plugin. I think you'd want to create a branch off of the branch for PR https://github.com/WordPress/performance/pull/1476 and then update the auto_sizes_apply_tag_parser_updates() function introduced in that PR to apply changes to PICTURE > SOURCE as well as IMG.