Set the full size dimensions when generating an SVG image tag #44
Conversation
…eration of srcset on SVGs. Ensure we don't add height and width twice to SVG image tags
jeffpaul
requested review from
darylldoyle,
a team and
iamdharmesh
and removed request for
a team
|
|
||
| if ( empty( $image[2] ) ) { | ||
| if ( $dimensions ) { | ||
| $image[1] = $dimensions['width']; |
There was a problem hiding this comment.
Shouldn't we calculate the image width and height respecting the $size here?
For example, when calling wp_get_attachment_image_src filter for the 'medium' size image we expect to reduce or increase width and height to the max 300px side by default (like it works for regular JPG/PNG).
With the current change, having a SVG with original dimensions of 1200x500 will produce <img src="/image.svg" width="1200" height="500" /> regardless the requested $size
There was a problem hiding this comment.
With the current change, having a SVG with original dimensions of 1200x500 will produce
<img src="/image.svg" width="1200" height="500" />regardless the requested $size
As far as I can tell in my testing, this is how it's been working prior to the fix we released in 1.9.10, so this PR attempts to bring that same functionality back.
I do agree that it seems like we should be taking into account the actual requested image size and use that to determine the size of the SVG but I believe that will break backwards compatibility, though maybe we're fine with that? The other issue to think about is SVGs don't get resized like normal images, so there aren't cropped versions of SVGs to match each registered image size like happens with normal image types.
If someone requests a medium image size, we can't just return 300x300 as that could end up returning a much larger SVG or could end up making the SVG stretched in one direction or the other. There would need to be some more logic added here that takes the requested image size and tries to find the closest dimensions that match the original SVG height/width ratio (and ensure we don't return dimensions that are greater than the actual SVG dimensions).
I think this would be good to fix but I'm leaning towards having that in a separate PR, as the approach in this PR is to bring us back to where we were prior to v1.9.10 but still fix the original issue that we were attempting to fix
| return $attr; | ||
| public function disable_srcset( $image_meta, $size_array, $image_src, $attachment_id ) { | ||
| if ( $attachment_id && 'image/svg+xml' === get_post_mime_type( $attachment_id ) ) { | ||
| $image_meta['sizes'] = array(); |
There was a problem hiding this comment.
The root cause of the wrong srcset image sizes is that WordPress core function image_downsize() always returns square size for SVG images, even if they are rectangular.
We could preempt it with image_downsize filter which will perform custom calculations of resized SVG width and height.
There was a problem hiding this comment.
We could preempt it with
image_downsizefilter which will perform custom calculations of resized SVG width and height
Are you suggesting we use that filter and return just the full SVG src?
I guess my opinion here is we don't ever need srcset or sizes for SVG images, since we will only ever have a single SVG file and not have resized versions of that, like WordPress will do for normal images. Unless I'm missing something, seems pointless to ever have those attributes as it will just always serve the exact same image no matter the breakpoint
|
@cadic I fixed a merge conflict here but it won't allow me to merge this in now until another review is done |
Description of the Change
In PR #23, we fixed a bug around overriding the height and width values of an image to be
false, when those values are expected to be of the typeinteger. This was causing an issue with certain aspects of Full Site Editing (the logo block in particular) and could potentially cause issues elsewhere.This caused some other bugs though. In particular, if directly using
wp_get_attachment_image, the size requested will now be used for the image tag, instead of the actual full size of the SVG. As an example, an SVG with dimensions of 30x30 but a requested image size of medium (default to 300x300), 300x300 will be used on the resulting<img>tag, making that SVG larger than expected.This is now fixed in this PR by using the full size of the SVG instead of setting that to the requested size. This is ultimately what the end result was before but we're now doing it in a way that should cause less problems in other places (like in FSE). Note this did initially seem like a bug to me. For instance, if I request a 300x300 image but the SVG has dimensions of 500x500, the resulting image tag is 500x500, which is not what I requested. But because SVGs aren't resized down like normal images, we can't rely on other image dimensions working properly (aspect ratio may be off, for one). Would also break backwards compatibility, which is how this issue was discovered.
Also, because we now set the proper full size dimensions as image attributes, WordPress itself will add those as
heightandwidthvalues, so this PR also removes functionality we had that was adding those attributes (otherwise we end up with double attributes added). Finally, because WordPress now has proper sizes to work with, it tries adding thesrcsetandsizesattributes, which we don't need (since SVGs are a single image, there are no resized versions). This PR removes that from being added to any SVGs.Examples:
Outputting the full size SVG using
wp_get_attachment_image:v1.9.9
<img src="/wp-content/uploads/2022/02/test.svg" class="attachment-full size-full" alt="" loading="lazy" height="32" width="32" />v1.9.10
<img width="200" height="200" src="/wp-content/uploads/2022/02/test.svg" class="attachment-full size-full" alt="" loading="lazy" srcset="/wp-content/uploads/2022/02/test.svg 150w, /wp-content/uploads/2022/02/test.svg 300w, /wp-content/uploads/2022/02/test.svg 1024w, /wp-content/uploads/2022/02/test.svg 1536w, /wp-content/uploads/2022/02/test.svg 2048w, /wp-content/uploads/2022/02/test.svg 200w" sizes="(max-width: 200px) 100vw, 200px" height="32" width="32" />With this PR
<img width="32" height="32" src="/wp-content/uploads/2022/02/test.svg" class="attachment-full size-full" alt="" loading="lazy" />Outputting the thumbnail size SVG using
wp_get_attachment_image:v1.9.9
<img src="/wp-content/uploads/2022/02/test.svg" class="attachment-thumbnail size-thumbnail" alt="" loading="lazy" height="32" width="32" />v1.9.10
<img width="150" height="150" src="/wp-content/uploads/2022/02/test.svg" class="attachment-thumbnail size-thumbnail" alt="" loading="lazy" srcset="/wp-content/uploads/2022/02/test.svg 150w, /wp-content/uploads/2022/02/test.svg 300w, /wp-content/uploads/2022/02/test.svg 1024w, /wp-content/uploads/2022/02/test.svg 1536w, /wp-content/uploads/2022/02/test.svg 2048w, /wp-content/uploads/2022/02/test.svg 200w" sizes="(max-width: 150px) 100vw, 150px" height="32" width="32" />With this PR
<img width="32" height="32" src="/wp-content/uploads/2022/02/test.svg" class="attachment-thumbnail size-thumbnail" alt="" loading="lazy" />You'll notice in those examples that in v1.9.10 we have double
heightandwidthattributes being added,srcsetandsizesbeing added, and the height and width is sometimes wrong.Alternate Designs
We could go back to setting the height and width too
falseand just realize that will cause issues with the block editor (and potentially other places)Possible Drawbacks
None that I've seen
Verification Process
Upload an SVG and then add custom code to output that image with
wp_get_attachment_image. Set a few different sizes and ensure that the full size dimensions are always used.Checklist:
Changelog Entry