Breaking behavior in inheriting width, height attributes from 2.3.0

martinpl commented 6 days ago

Describe the bug

Hello, Here are details: https://github.com/10up/safe-svg/pull/216#issuecomment-2503662519

Thanks :)

Steps to Reproduce

wp_get_attachment_image() without full thumbnail will not get original width and height

Screenshots, screen recording, code snippet

No response

Environment information

No response

WordPress information

No response

Code of Conduct

[x] I agree to follow this project's Code of Conduct

subfighter3 commented 6 days ago

same issue here! this is a breaking behaviour, please fix it!

smerriman commented 6 days ago

This is breaking my sites too.

gigatyrant commented 4 days ago

Same issue for all my clients. Now, the maximum size specified in wp_get_attachment_image is being used instead of the native SVG size.

jeffpaul commented 1 day ago

@iamdharmesh @dkotter @gabriel-glo as this potentially relates to #216, I wanted to ping you all for review on this issue report to see if there's a fix to consider here?

dkotter commented 1 day ago

@martinpl @subfighter3 @smerriman @gigatyrant Thanks all for the report. Curious if you're all using wp_get_attachment_image to output the SVG? Or some other function? And if using wp_get_attachment_image, are you passing a custom size attribute to that or just leaving that blank, like echo wp_get_attachment_image( 1 );?

We did change behavior here where it now respects that optional size argument (which by default, WordPress sets that as thumbnail). So if you aren't setting anything there, the height and width will now output using your site's thumbnail settings (typically 150x150). If you want the full SVG size, you can pass full for that and it should have the same behavior as previously.

In the meantime, we're going to discuss whether we want to revert / change this new behavior, keeping in mind backwards compatibility concerns while also wanting to support whatever size argument someone sets here, rather than always using the full SVG size.

smerriman commented 1 day ago

If I call wp_get_attachment_image with size set to full, it works fine.

However, basically everything else breaks:

if I call it with size to thumbnail, it includes width="150" height="150", even when the SVG isn't a square (which means browsers will reserve the wrong size for it).
if I call it with size in array format (e.g. array(100,120) ), width and height parameters are entirely missing
if I call it with an unregistered size, width and height parameters are entirely missing, rather than reverting to full as expected. (This isn't an unusual use-case; e.g. when you upload a favicon to WordPress, the core code only crops that image to the sizes required, rather than registering a size which would cause favicon sizes to be generated for every image uploaded to the media library).

Since SVGs can't be cropped, having the full dimensions output has always been expected.

Edit - correction; array format outputs width + height, but specifically as listed, again often wrong if they don't match the SVG proportion. I'm usually using the third case, as I allow clients to upload a grid of logos, and only generate the optimised cropped size for non-SVG sizes.

gigatyrant commented 17 hours ago

@dkotter, thank you for this explanation!

Let’s take a simple example:

To display the image, I use: wp_get_attachment_image( 1, 'large' ), which has a size limit of 1024 x 1024 pixels.

If my client uploads a PNG larger than 1024 x 1024 pixels, WordPress crops it while maintaining the image’s format to avoid it being too heavy. Perfect!
If my client uploads a PNG (e.g., a pictogram) smaller than 1024 x 1024 pixels, WordPress displays the pictogram at its native size. Perfect!
If my client uploads an SVG larger than 1024 x 1024 pixels, Safe-SVG stretches the SVG to 1024 x 1024.
If my client uploads an SVG (e.g., a pictogram) smaller than 1024 x 1024 pixels, Safe-SVG also stretches it to 1024 x 1024.

The correct behavior is WordPress’s handling of PNGs: respecting the maximum size only when necessary, without stretching. In the case of Safe-SVG, the maximum size seems to be forced regardless, which is problematic.

Setting the size to “full” is not a viable solution because clients sometimes have the option to upload either a pictogram or a photo for the same element. This would mean that if a photo is used, the site would load it at its full size, which is not optimal.

In my opinion, the correct solution is the same as WordPress’s approach for PNGs:

If the SVG’s size is smaller than the allowed size, it should be displayed at its native size.
If the SVG exceeds the allowed size, it should be reduced to that size while maintaining its aspect ratio.

As it stands, the plugin causes issues by forcing all SVGs, including icons and pictograms, to display at 1024 x 1024, which disrupts the layout of many websites. My clients were surprised when all their icons suddenly appeared stretched to 1024 x 1024 pixels xD

dkotter commented 13 hours ago

@smerriman @gigatyrant Thanks so much for all the helpful information. While we were attempting to make things more flexible here by respecting the requested image size, we definitely missed the mark on that so I apologize for any issues that cause. I've opened #238 which reverts the changes made in #216. If anyone has time to test and ensure that works for you, that would be great.

Once that change in #238 is merged, the actual SVG dimensions should always be used again, even if you pass in thumbnail or large or a custom image size. This matches existing behavior though as @gigatyrant so helpfully details, I do think there's a more intelligent way to handle this that we can look into. I'll look to open a new Issue with those details so we can track that.

10up / safe-svg