Skip to content
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.

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

Fix/block img tag sizes #5776

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Fix/block img tag sizes #5776

wants to merge 6 commits into from

Conversation

itsTallulah
Copy link
Contributor

Description

Addresses incorrect img tag width|height values in core/image blocks by filtering render_block, where it can get the attachment ID and intended size from the classes of the img and figure tags.
From there it looks for the intended size's w|h values in the attachment's postmeta, and add them to the img tag for the return.
Also fixes incorrect srcsets for block images without having to touch them, since it's upstream of them and supplies the values they need.
Returns original safely and silently on failure. Matches narrowly, and can operate on already-filtered blocks as long as they have the required size and ID classes.
Optional switch for resize args can be omitted, but it could be nice to give folks the choice. I defer to plat as to how to make this switchable (or if we should just lose it for simplicity and brevity).

Changelog Description

  • First iteration - Added filter to address unexpected size values in core/image blocks

Added

  • fix_img_block_sizes filter for render_block

Fixed

  • Fixed incorrect image sizes and srcsets for core/image blocks

Pre-review checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally or in Codespaces (or has an appropriate fallback).
  • This change works and has been tested on a sandbox.
  • This change has relevant unit tests (if applicable).
  • This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

  1. Check out PR / use relevant file as generic mu-plugin
  2. Examine any page/post with core/image block/s
  3. Examine src of any core/image block (other than size:full)
  4. Its width and height values and srcset should all be congruent with its intended size
  5. It should also visually look the same size in the frontend and the editor
  6. Repeat as required for any/all registered sizes
  7. Optional: register some custom sizes and repeat

addresses incorrect w/h values in core/image block img tags
remove preamble comment, remove informal test
@itsTallulah itsTallulah requested a review from a team as a code owner August 5, 2024 22:06
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 30.26%. Comparing base (e69f10b) to head (ec2b0f6).

Files with missing lines Patch % Lines
fix-block-img-tag-sizes.php 0.00% 20 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #5776      +/-   ##
=============================================
- Coverage      30.27%   30.26%   -0.02%     
  Complexity      4812     4812              
=============================================
  Files            287      288       +1     
  Lines          20711    20719       +8     
=============================================
  Hits            6271     6271              
- Misses         14440    14448       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

// Add the width and height attributes to the img tag
$block_content = preg_replace(
'/<img ([^>]+)>/',
'<img foo="bar" $1 width="' . esc_attr($width) . '" height="' . esc_attr($height) . '">',
Copy link
Member

Choose a reason for hiding this comment

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

Is this foo="bar" thing really necessary? :-)

Copy link
Member

Choose a reason for hiding this comment

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

Also, can the <img> tag contain other attributes (say, alt)? If so, is it possible to avoid overwriting them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that foo is a testing leftover! Thanks for the spot. And yeah, the img and figure tags can contain other values and classes too.

Copy link
Contributor Author

@itsTallulah itsTallulah Aug 6, 2024

Choose a reason for hiding this comment

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

See the first commit: it has the now-removed add_block_bits, which I was using for as an informal test for extra values in the tags by filtering a bunch of extra stuff into the block markup just upstream.

$height = $metadata['sizes'][$size_name]['height'];
$new_file = $metadata['sizes'][$size_name]['file'];

if ($resizearg) {
Copy link
Member

Choose a reason for hiding this comment

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

This condition is always false because we don't seem to set $resizearg to true anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, we don't set it true anywhere - yet. I'm deferring to y'all as to how we should let the user set this (if we include it at all).

}
return $block_content;
}
add_filter('render_block', 'fix_img_block_sizes', 10, 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're only handling core/image blocks, perhaps we can hook into render_block_core/image here? https://github.com/WordPress/wordpress-develop/blob/50af37a9083f003f8e98d089091d2cc428797cc5/src/wp-includes/class-wp-block.php#L581

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - Should behave exactly the same on that filter, since it's right after the first one and uses the same values. Will give that a try too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works well and makes the function smaller. Nice, thx!

I'd left a foo=bar in the returned img tag from testing on non-VIP envs, so I took it out.
@itsTallulah
Copy link
Contributor Author

Looking at the resizearg option I had in here, it would arguably make more sense to have that as a separate filter combo that can hit all contexts where image scaler args would otherwise be w= style, so I'm going to remove it from this. I'll pitch that as a separate PR, since it's far lower prio than this fix.

removed resizearg switch, used render_block_core/image filter, added skip for fullsize, prepended vip to function name to avoid collisions with existing fix_img_block_sizes filters in use
@itsTallulah
Copy link
Contributor Author

itsTallulah commented Aug 15, 2024

Removed resizearg section, added a skip for fullsize image size, changed filter to render_block_core/image, renamed function to vip_fix_img_block_sizes for specificity and to avoid collisions (since pervious versions of this fix have been shipped to some customers as a stopgap).

Copy link

sonarcloud bot commented Aug 27, 2024

Copy link

sonarcloud bot commented Oct 8, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants