Make WordPress Core

Opened 9 years ago

Last modified 8 years ago

#35390 accepted defect (bug)

image_constrain_size_for_editor() should not affect images generated on the front end when `large` size is used.

Reported by: rabmalin's profile rabmalin Owned by: joemcgill's profile joemcgill
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: needs-unit-tests needs-patch
Focuses: Cc:

Description

    <img width="680" height="510" src="http://staging.dev/wp-content/uploads/2011/07/dsc02085-1200x900.jpg" class="attachment-post-thumbnail size-post-thumbnail wp-post-image" alt="Orange Iris" srcset="http://staging.dev/wp-content/uploads/2011/07/dsc02085-300x225.jpg 300w, http://staging.dev/wp-content/uploads/2011/07/dsc02085-768x576.jpg 768w, http://staging.dev/wp-content/uploads/2011/07/dsc02085-1024x768.jpg 1024w, http://staging.dev/wp-content/uploads/2011/07/dsc02085-1200x900.jpg 1200w, http://staging.dev/wp-content/uploads/2011/07/dsc02085.jpg 1600w" sizes="(max-width: 680px) 100vw, 680px">

I am using Twenty Fifteen theme, WordPress version 4.4. post-thumbnail size is 825x510. Original image size is 1600x1200. But I am not getting expected image. After some testing I found that this problem arises if post thumbnail large is used. I could not understand how width and height is set to 680 and 510 respectively.

I am not sure this is related to https://core.trac.wordpress.org/ticket/35108

Attachments (3)

35390.patch (2.5 KB) - added by jaspermdegroot 9 years ago.
35390.2.patch (689 bytes) - added by joemcgill 9 years ago.
35390.3.patch (1.0 KB) - added by joemcgill 9 years ago.

Download all attachments as: .zip

Change History (33)

This ticket was mentioned in Slack in #core by rabmalin. View the logs.


9 years ago

#2 @joemcgill
9 years ago

  • Owner set to joemcgill
  • Status changed from new to reviewing

Hi @rabmalin thanks for the report.

WordPress limits the display size of large images added through the editor to the value of $content_width in image_constrain_size_for_editor(), but if I'm reading your description correctly, it sounds like the issue you're describing isn't related to content images.

I don't fully understand what you mean by 'this problem arises if post thumbnail large is used.' Could you leave some steps to reproduce the issue?

Thanks,
Joe

#3 @rabmalin
9 years ago

I meant in the_post_thumbnail('large'). I will try to elaborate Issue later when I am at laptop.

#4 @joemcgill
9 years ago

Thanks @rabmalin,

That's what I thought you were getting at. I believe this may be an issue with how image_downsize() is calling image_constrain_size_for_editor(). One more question: are you generating these images in the admin or on the front end? In other words, what is the value of is_admin() when you're calling the_post_thumbnail( 'large' )?

Joe

#5 @rabmalin
9 years ago

Hello,

You mentioned $content_width in the comment. So I tried changing value of that global variable. Now width of img tag is changed. This mystry of width value is solved. Is this expected behaviour? Limiting images inside the_content() makes sense. But limiting when using the_post_thumbnail( 'large' ); does not make sense to me. When 1024 is set to large then I would expect that size for the display.

I could not understand your question. But here are the steps to test.

  • Clone _s theme.
  • In content.php, add the_post_thumbnail('large'); just above the_content() call.
  • Create blog post. Set featured image.
  • Check post in the front-end ( single.php template WordPressically ).

Nilambar

Last edited 9 years ago by rabmalin (previous) (diff)

#6 @joemcgill
9 years ago

  • Component changed from Post Thumbnails to Media
  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.5
  • Status changed from reviewing to accepted

@rabmalin I am able to reproduce the issue as you describe. I would not expect $content_width to affect the size of the_post_thumbnail( 'large' ) when generated on the front end, so I would consider this to be a bug. From what I can tell, image_constrain_size_for_editor() has not respected the context for 'large' image sizes for at least 8 years, so I'm not sure what back-compatibility issues might exist, but it's worth giving this a look for 4.5.

#7 @joemcgill
9 years ago

  • Summary changed from Responsive image size issue when `large` is used to display thumbnail to image_constrain_size_for_editor() should not affect images generated on the front end when `large` size is used.

#8 @rabmalin
9 years ago

I checked image_constrain_size_for_editor function. I think we need to check $context before setting max width and max height comparing with content_width. No?

Last edited 9 years ago by rabmalin (previous) (diff)

This ticket was mentioned in Slack in #core by rabmalin. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#12 @jaspermdegroot
9 years ago

  • Keywords has-patch added; needs-patch removed

35390.patch makes it possible to prevent the call to image_constrain_size_for_editor() in image_downsize(). By default the new param $constrain is true and the function does exactly the same as it did before.
When wp_get_attachment_image_src() calls image_downsize(), $constrain is set to false because only images in the content should be constrained to $content_width.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


9 years ago

@joemcgill
9 years ago

#14 @joemcgill
9 years ago

I'm not sure we need to not change the signature of image_downsize() here. 35390.2.patch checks the context in image_constrain_size_for_editor() before constraining the width to the value of $content_width, which is the same way full size images are handled.

We may want to rethink the assumption that we're in the edit context whenever is_admin() is true, since this can apply when doing AJAX calls as well.

#15 @kirasong
9 years ago

I'd like a second look by @azaozz if possible, but this makes sense to me.

@joemcgill
9 years ago

#16 @joemcgill
9 years ago

35390.3.patch is exactly like 35390.2.patch but adds a check for DOING_AJAX to avoid setting an editor context during an admin_ajax() process.

#17 @azaozz
9 years ago

In my opinion $context in image_constrain_size_for_editor() was a bad idea. It is very confusing: the function is called *_for_editor(), and the editor has one context: edit. Giving it another context makes no sense.

Further #22738 made the media modal show wrong information: https://core.trac.wordpress.org/attachment/ticket/22738/AttachmentDisplaySettings.png. The actual size is 1024 X 768, the image inserted in the editor is 1024 X 768, but it is shown as 625 X 468 in the media modal?!?. The fact that we display wrong, seemingly arbitrary values in there has caused a lot of confusion to many users.

This is one of the most misused functions in WordPress. Over the years it had several "hacks" to make it do stuff it wasn't intended to do. Setting context based on is_admin() is very unreliable. An editor can easily be on the front-end. Similarly, setting context just because it is an ajax request is wrong and it makes it even more fragile and unreliable.

The right thing to do here is to deprecate image_constrain_size_for_editor() and replace it with two or three new functions that can handle all user cases properly.

Also agree with @joemcgill that we probably shouldn't change image_downsize(). It shouldn't be using image_constrain_size_for_editor() just to calculate the new image size anyway. New function that does only that would have been perfect here (too late now for 4.5).

If I have to pick, I'd rather add another param to image_downsize() instead of tweaking image_constrain_size_for_editor() more.

This ticket was mentioned in Slack in #core by mike. View the logs.


9 years ago

#19 @kirasong
9 years ago

  • Milestone changed from 4.5 to Future Release

Punting based on feedback here and in Slack, linked above.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

#21 @joemcgill
9 years ago

  • Milestone changed from Future Release to 4.6

Moving this into the 4.6 milestone. To move this forward, Let's look into an approach that replaces image_constrain_size_for_editor() with new helper functions that is only called when preparing markup for the editor (rather than requiring context switching) and avoids possible is_admin() AJAX issues.

#22 @joemcgill
8 years ago

  • Milestone changed from 4.6 to Future Release

Punting this from 4.6, since we don't have a patch for the new approach.

#23 @rabmalin
8 years ago

  • Keywords needs-patch added; has-patch removed

This ticket was mentioned in Slack in #core by pressupinc. View the logs.


8 years ago

#25 @joemcgill
8 years ago

Was thinking about this again today and the simplest solution for the purposes of this ticket would be to add context awareness for large image sizes to image_constrain_size_for_editor() like we do for custom sizes.

However, I'm wondering if we can look at the places where image_downsize() is used and move image_constrain_size_for_editor() out to the parent function when appropriate instead. So for example, get_image_tag() — which is only used in core to create image markup for the editor — would run the dimensions returned from image_downsize() through image_constrain_size_for_editor(). That way, we can remove image_constrain_size_for_editor() from image_downsize() entirely.

It would also be nice if image_constrain_size_for_editor() knew nothing about the context and just returned whatever the image size should be when constrained by $content_width.

#26 @pressupinc
8 years ago

My business partner and I are trying to make progress on this ticket. We like @joemcgill's suggestion to have functions call image_constrain_size_for_editor() directly, rather than doing it through image_downsize().

So we looked at the five functions that call image_downsize(), and we've broken them into what we think (but aren't sure) should and should not call image_constrain_size_for_editor():


Yes
get_image_tag() - we're pretty sure this is what actually generates images in the TinyMCE Visual Editor, so it should be scaled in an Editor context.

No
wp_xmlrpc_server::_prepare_media_item() - can't be in admin if xmlrpc
wp_get_attachment_thumb_url() - its only caller is get_attachment_icon_src(), which isn't admin-y and is deprecated
image_size_input_fields() - This only seems to be used by a Flash-based legacy media uploader. We're pretty sure that shouldn't be aware of the editor's image size constraints, but we're not sure; perhaps it should?
wp_get_attachment_image_src() - just based on the name, it should just be giving you dimensions that aren't adulterated by editor considerations.


wp_get_attachment_image_src() is really tricky, though, because it's called in a further fifteen functions, and we don't know if TinyMCE is using any of those in its process of rendering images in the Visual editor.

This relates to a more general question: Is image_constrain_size_for_editor() just for making sure that the TinyMCE Visual Editor doesn't attempt to render 5000px-wide images? Or is it used for another purpose, somewhere else in the admin?

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-media by pressupinc. View the logs.


8 years ago

#29 @pressupinc
8 years ago

Hi all, want to sharpen the question mentioned in comment 26, as I think knowing the answer will make the route forward a lot clearer:

To everyone's knowledge, is the only proper use of image_constrain_size_for_editor() to reduce the size of images in the TinyMCE Visual editor, so that the editing experience isn't slowed down by the display of excessively large images? Is that what "constraining an image's size for editor" is for?

Or is there something else it's also for? In other words, is there another place - in the admin or on the frontend - where it's correct for editor-scaled dimensions to be used? I know these dimensions are being used in other places (like wp_prepare_attachment_for_js(), and in other uses of wp_get_attachment_image_src(), which caused my original bug, #38906); but if I know that the only proper use of it is for scaling images within the Visual editor, then I think we can narrow down the functions that should be calling image_constrain_size_for_editor() to just one - get_image_tag(), the function that outputs properly scaled img tags into the Visual editor - without needing to chase down every single caller of wp_get_attachment_image_src() and see what it does.

So if we can get confirmation that that's exclusive intent behind editor-constraining image sizes, we should be quite a bit closer to a fix.

Thanks!

This ticket was mentioned in Slack in #core-media by pressupinc. View the logs.


8 years ago

Note: See TracTickets for help on using tickets.