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

Breaking behavior in inheriting width, height attributes from 2.3.0 #237

Closed
1 task done
martinpl opened this issue Nov 27, 2024 · 9 comments · Fixed by #238
Closed
1 task done

Breaking behavior in inheriting width, height attributes from 2.3.0 #237

martinpl opened this issue Nov 27, 2024 · 9 comments · Fixed by #238
Assignees
Labels
type:bug Something isn't working.
Milestone

Comments

@martinpl
Copy link

Describe the bug

Hello,
Here are details:
#216 (comment)

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

  • I agree to follow this project's Code of Conduct
@martinpl martinpl added the type:bug Something isn't working. label Nov 27, 2024
@martinpl martinpl changed the title Breaking behavior in inheriting width, size attributes from 2.3.0 Breaking behavior in inheriting width, height attributes from 2.3.0 Nov 27, 2024
@subfighter3
Copy link

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

@smerriman
Copy link

This is breaking my sites too.

@gigatyrant
Copy link

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 jeffpaul moved this from Incoming to To Do in Open Source Practice Dec 2, 2024
@jeffpaul
Copy link
Member

jeffpaul commented Dec 2, 2024

@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
Copy link
Collaborator

dkotter commented Dec 2, 2024

@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
Copy link

smerriman commented Dec 2, 2024

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
Copy link

@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 dkotter self-assigned this Dec 3, 2024
@dkotter dkotter added this to the 2.3.1 milestone Dec 3, 2024
@dkotter dkotter mentioned this issue Dec 3, 2024
4 tasks
@dkotter
Copy link
Collaborator

dkotter commented Dec 3, 2024

@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.

@gigatyrant
Copy link

@dkotter Thank you so much for your outstanding responsiveness!

I tested the #238 changes on one of my affected sites, and everything seems to be working perfectly again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants