3.5 Media: Correct display of dimensions for large-size images in size dropdown

[jond3r]

To better comply with the behavior in WP 3.4 here is a proposed small fix for computing the dimensions shown in the image size drop-down box in the "Media modal" when inserting images from the media library.

This applies mostly to large-size images, which generally are constrained in size by the global variable $content_width set by the theme. For example TwentyTwelve sets $content_width = 625. When an image that is wider than $content_width is inserted, it is "constrained" in size to the value of $content_width, that is its width and height attributes ("hwstring") are adjusted.

In 3.4 the constrained dimensions are shown in the dropdown when an image is inserted, in the current 3.5 trunk the unconstrained dimensions are shown, e.g. Large is generally shown to have a width of 1024, whereas in 3.4 it is shown as 625 (for TwentyTwelve). The code inserted in the editor is however the same for both versions.

(As a side note, the image class gets a value of "size-large" even though the size attributes are constrained. But this is a "legacy" behavior.)

The proposed fix makes a call to image_constrain_size_for_editor() in wp_prepare_attachment_for_js() (file wp-includes/media.php).
Referring to #22598 there was a worry that repeatedly calling image_downsize() would be too slow. However, it is sufficient in this case to call image_constrain_size_for_editor(), and just for large-size (or custom) images. (The only drawback i found was that the filter 'editor_max_image_size' will not be applied for thumbnails and medium images if these are bypassed image_constrain_size_for_editor()).

I also added a small performance improvement to wp_constrain_dimensions(), which now returns immediately if the current and max dimensions are equal.

The attached image shows the image size dropdown before and after the fix, which also complies with 3.4.

(This was a long description to a small fix, phew.)

[nacin]

image_constrain_size_for_editor() only does something ! is_admin() for all custom sizes. Not sure what that means because 3.4 always went through wp-admin/media-upload.php.

[jond3r]

Replying to nacin:

I do not fully understand your comment, but image_constrain_size_for_editor() is and was called by image_downsize(), and before 3.5 image_downsize() was called in image_size_input_fields() (in wp-admin/includes/media.php) to "Retrieve HTML for the size radio buttons". (That is radio buttons was used before instead of a drop-down list.)

In other words image_constrain_size_for_editor() was called for admin pre-3.5 (provided image_size_input_fields() was used, which I believe).

I do understand if you are suspicious and a bit stressed at the moment, but I'm quite sure that I have a solid case here (a statement which probably is not helpful).

[nacin]

Replying to jond3r:

I do understand if you are suspicious and a bit stressed at the moment, but I'm quite sure that I have a solid case here (a statement which probably is not helpful).

Nope, not really. :) I wasn't challenging the basis of your patch. Rather, I saw that your patch is thumbnail/medium and else. Else would include not only large, but any custom sizes as well. (Full is handled a few lines after, as I'm sure you noted.)

I then looked at image_constrain_size_for_editor(). It does thumbnail, elseif medium, elseif large, elseif a custom size, else full. Large is scaled down to content_width, as your patch does. A custom size is scaled down but only in the admin, with a comment:

if ( intval($content_width) > 0 && is_admin() ) // Only in admin. Assume that theme authors know what they're doing.

I didn't investigate why it was there. Hence my (short, admittedly confusing) question.

[jond3r]

Well, it's starting to get late for me, but one reason is_admin() is put there might be that custom sizes should not be changed on the front-end, as this might confuse theme editors, who use for example wp_get_attachment_image() (which eventually calls image_downsize()), and get a resized image in return. On the other hand large-size images probably get resized, which would be equally confusing.

On the other hand, in our case is_admin() is true, and it was in 3.4 so nothing has changed there.

[nacin]

On the other hand, in our case is_admin() is true, and it was in 3.4 so nothing has changed there.

But on the frontend, it will have been true in 3.4 but false in 3.5.

I will dig deeper. Thanks for the report and help.

[nacin]

History is of limiting it to the admin is here: #12146.

[nacin]

Your initial patch was solid, jond3r. Thanks!

I refreshed it to match our coding standards (if you use braces for a control statement, use them throughout). See 22738.2.diff​.

The only issue is that due to #12146, inserting Large size on the frontend won't be constrained. It seems like we may need to run our own custom logic for a custom size that sidesteps the is_admin() check. We might actually be able to use editor_max_image_size for it.

[jond3r]

Replying to nacin:

On the other hand, in our case is_admin() is true, and it was in 3.4 so nothing has changed there.

But on the frontend, it will have been true in 3.4 but false in 3.5.

Ok, I'm back. I would challenge the statement that is_admin() is true on the front-end in 3.4, but I might be missing something.

I replied to fast, I will look more into #12146.

[jond3r]

The only outcome of #12146 was the addition of is_admin() in the if-statement in image_constrain_size_for_editor(), see [13140]. There was another patch 12146.2 that never got committed.

[jond3r]

Replying to nacin:

I refreshed it to match our coding standards (if you use braces for a control statement, use them throughout). See 22738.2.diff​.

Ok, I actually did go to some lengths to try to comply, by doing "backwards comparison" in the if-statement, which I normally don't do.

The only issue is that due to #12146, inserting Large size on the frontend won't be constrained.

I think it will be, Large size is not affected by is_admin().

[jond3r]

The _wp_post_thumbnail_html() function, which was referred to in #12146, should not be affected by the current patch.

[jond3r]

I actually tested to insert a large image on the front-end by adding a line like:

 echo wp_get_attachment_image($attachment_id, 'large');

to a template file, and it was constrained to $content_width as expected.

I also added a featured image (post thumbnail) to a post, and it worked as far as I could see. A custom image size named "post-thumbnail" was created when uploading, and it was used in the output code with the actual dimensions of the image.

At my first test with a featured image i selected an image which did not have the custom size "post-thumbnail", and in this case a full-size image was used, even though a large-size was available, which would be more appropriate. The image was however "constrained" to the post-thumbnail dimension (624 px for TwentyTwelve) which seems correct.

So, so far I have not been able to break the patch. :)

[jond3r]

Replying to nacin:

I refreshed it to match our coding standards... See 22738.2.diff​.

I saw that you left out the optimization to wp_constrain_dimensions(), but that was maybe intentional?

[nacin]

Replying to jond3r:

I saw that you left out the optimization to wp_constrain_dimensions(), but that was maybe intentional?

Yes. It's not a change we should be making at this point. Needs unit tests, etc.

[nacin]

Replying to jond3r:

I would challenge the statement that is_admin() is true on the front-end in 3.4, but I might be missing something.

Say you had a theme that implemented an editor (with media insertion) on the frontend. In 3.4, wp-admin/media-upload.php loaded up in an iframe, so the is_admin() check would be true when constraining.

Try P2 with the patch. Large size images won't be constrained.

[jond3r]

Replying to nacin:

Say you had a theme that implemented an editor (with media insertion) on the frontend. In 3.4, wp-admin/media-upload.php loaded up in an iframe, so the is_admin() check would be true when constraining.

Good, it shows that you know something more than me. I believe you.

[nacin]

I think the cleanest approach here is for a $context parameter for image_constrain_size_for_editor(). 'display' or 'edit', with the default being 'display' on the frontend and 'edit' in the admin. 22738.3.diff​

[nacin]

To test:

Try to insert an image size that is either Large or custom (so not full, thumbnail, or medium). The stated width should match your content width, in both the admin and on the frontend. (Test with P2, which has a content width of like 632.) When the image is inserted, it should be a scaled down image using the width/height attributes on the image tag to match the constraints of your theme.

[nacin]

We should really just inject max-width: 100% and let it scale up in the future if you switch to a theme with a greater content_width, but that is a image_constrain_size_for_editor() enhancement for another day. This is simply to keep compatibility with 3.4.

[sabreuse]

22738.3.diff works correctly for me.

[markjaquith]

Works in my testing. I like the context parameter.

[nacin]

More test code:

add_filter( 'image_size_names_choose', function( $sizes ) {
    $sizes['post-thumbnail'] = 'Post Thumbnail';
    return $sizes;
} );

Constrain will kick in when a post thumbnail for a theme is larger than its content width.

[nacin]

... like Twenty Ten or Twenty Eleven.

[Paddleman]

Nacin: Is this related the issue I have with Internet Explorer, thumbnail size attributes in this post​http://wordpress.org/support/topic/35-rc3-thumbnail-pictures-looking-skewed-on-internet-explorer-8?

[nacin]

I was chasing ghosts on this one for a while. Thought I found something wrong, but couldn't reproduce after a while. Part of that led to #22781. The rest, apparently nothing. This one is good.

[markjaquith]

In 23096:

Present the correct downsized image dimensions in the Media modal when inserting. Introduces a context parameter for image_constrain_size_for_editor() instead of relying on is_admin(). props jond3r, nacin. fixes #22738

[jond3r]

Replying to nacin:

More test code:

add_filter( 'image_size_names_choose', function( $sizes ) {
    $sizes['post-thumbnail'] = 'Post Thumbnail';
    return $sizes;
} );

Constrain will kick in when a post thumbnail for a theme is larger than its content width.

For the record, I tested the image_size_names_choose filter, and it works as expected. (It's a really useful filter by the way.)

One thing to note is that for the "constrain to kick in" the theme must also have registered the custom image size in question, using the function add_image_size(), so that the size is in the $_wp_additional_image_sizes array. I experienced this when using a theme that had not registered the size 'post-thumbnail'.

[jond3r]

I found some flaw in the present solution. Filed a new ticket #23102.

[nacin]

In 23264:

Media: Pass thumbnail and medium sizes to image_constrain_size_for_editor() to force constraints based on the current DB options for those sizes. History: see #22598, #22738.

props jond3r.
see #23102.
for trunk.

[ryan]

In 23282:

Media: Pass thumbnail and medium sizes to image_constrain_size_for_editor() to force constraints based on the current DB options for those sizes. History: see #22598, #22738.

props jond3r
fixes #23102
for 3.5