Resizing images in WYSIWYG broken in media-7.x-1.0-rc3+ & 2.0-unstable3+

To add some info about the behavior: also when not resizing images, I mean when setting width and height to 'empty', the original width and height of the image are still being passed on and rendered.

changed title to reflect issue present in media-7.x-2.0-unstable3 as well

here is the culprit function

can we just ad an if statement to check for the values in the #attributes array to this for now?

EDIT:
on another similar installation this error doesn't seem to be present, here are the details on the difference between the two:
broken installation =>
media-7.x-2.0-unstable3
wysiwyg-7.x-2.1 with TinyMCE 3.4.3.2
pathologic-7.x-1.4
image_resize_filter-7.x-1.13
with filter processing order => "Converts Media tags to Markup" => "Correct URLs with Pathologic" => "Image resize filter"

working installation =>
media-7.x-1.0-rc3
wysiwyg-7.x-2.1 with CKEditor 3.6.2.7275
and that's it... when i go to set an image size the changes apply as they should. I'll try installing the image resize filter to see what happens and let you know....

Hmm, lets try that again, this time actually reversing the commit in #1186434: Media field image formatter doesn't create width and height attributes.

i opted to mess with the media_get_file_without_label function on line #373 and add

however it seems to me that the patch that caused this issue is pretty useless to begin with...

here is my patch wich i guess solves the problem without reversing the file_entity patch

I was having the same problem with media-7.x-1.1, but patch #7 fixed the problem.

You're getting those errors because those values are not set when we're checking their value. I think code like this may make more sense, to just see if they're set before we overwrite the width and height:

dandaman's code seems to work for me, i can resize pictures in WYSIWYG and i'm not having those error messages anymore. I'm submiting a patch for this fix.
@fenstrat , seems that the issue is not TinyMCE specific, i'm having this problem with CKEditor 3.6.3. I'm not sure where to apply your code for test it, i'm just starting to watch the code of this module.
Thank you both for your help.

@betoscopio Thanks for pointing out this is also happening with CKEditor.

It is unlikely that @smiro2000 and @dandaman approach you've rolled into a patch in #13 would make it into media module core. That's why I worked on #12. To implement that you need a cusotm module (in the example I've called it mymodule). Implemented this way you do not need to make any changes to the media module itself.

@betoscopio Yep, good point, this doesn't seem like site specific code. However this issue only effects a small number of media users (those inserting media inline into a WYSIWYG field with an editor which places width/height attibutes on the img tag and not on its style attribute, such as TinyMCE) and as such a patch to media module itself doesn't seem to be getting any traction.

Additionally a module to implement the work around would litterally consist of the code in #12, nothing more. I'm honestly not sure of the best way forward with this. Would love some additional feedback.

p.s. Just saw you're in Chile, I'm currently living in Santiago, maybe we should meet up for a beer and discuss!

#7 Solved the issue for me

In order to clarify about this : which display mode are you using ? does your issue depends upon the one chosen ? (image, rendered file, file style, ...)

I was wondering about which file display mode (chosen in the window following the views-based media manager) : the select-box which appears after a given media has been selected and "Submit" has been clicked. Could be "small", "large", "original", ...

thanks fenstrat, thus my last question is : which "display" your "original" file display is bound to ?
If you customized this file display, the information can be found in :
/admin/structure/file-types/manage/image/file-display/media_original
by looking at checked checkboxes.

The patch in #13 nor the work around in #12 address the root cause of this issue as outlined in #21 - #26, so back to needs work.

@mpgeek I originally raised this issue in the Image Resize Filter issue queue #1407590: Media 7.x-1.0-rc3 breaks image resizing. Using Image Resize Filter does not fix this issue, rather it causes it.

The fact is that anything which adds width and height attributes to the img tag will cause this issue. Whereas adding width and height attributes to the style attribute (i.e. inline css) of the img tag works fine.

Additionally this only effects the Image display, and not the File display as noted in #21 - #26. This is due to the change introduced in #1186434: Media field image formatter doesn't create width and height attributes., specifically this commit.

At this stage I'm not sure how to move this forward. I'm using the work around in #12, and the patch in #13 also achieves the same result. But niether address the root cause of this issue.

Patch #13 worked for me so that the "Convert Media tags to markup" filter actually honors the modified height/width attributes.

finally works for me (7.x-2.x); congratz !

After some pain I now went from file_styles to entity_view_mode (so without the ability to link thumbnails to original & co anymore, ...) anyway, the patch #13 works as expected for them.

To my knowledge:
* file_styles : wysiwyg resizing supported
* entity_view_mode : wysiwyg resizing supported
* direct use of "image" module or old (1.x) media view_mode : wysiwyg resizing not supported
is that right ?

oooops... spoke too soon.
This does not work more with entity_view_mode than any other formatter.
It only works with my "original" view_mode which is bound to the "no image style" file view mode. Others seems not supporting resizing.
This's a major (and hard to tackle) issue...

I think the issue that is being observed here is that file styles output through theme_image_styles. The "original" formatter only outputs through theme_image. theme_image_styles() forces the dimensions of the file from the image style, not the file- which to be fair is probably the right thing to do. It seems like to address this issue we may have to change the actual theme function that media_token_to_markup() uses to display. There are actually some advantages to this. Because we have $tag_info which has data from the WYSIWYG we could actually create our own image style which takes arguments from the WYSIWYG to properly size the image. This would also allow for cropping of the image.

@firestickme Glad it was of help. We're still using that to solve this issue. I'm quite amazed that more people are not having this problem. I'm still unsure of the best way to solve it in terms of patching the module, but the custom module solution in #12 is a clean work around.

Hmm hard to test this against 7.x-1.x due to the fact that it does not have the whole if ($wysiwyg) { hunk 7.x-2.x does. So applying the patch fails on includes/media.filter.inc

What would this look like in 7.x-1.x?

Here's a modified version of the above patch which seems to work for ckeditor and tinymce. It is probably the case that this should be a module rather than media core... not sure

@drzaf - from my perspective this is not a bug- it is code that is manipulating images that does not do it correctly. theme_image() correctly renders images with their attributes set. The problem is the way that code in wysiwyg alters the markup of the image. In order to correctly resize the image the wysiwyg must implement a filter hook that can apply the correct attributes which would override what the theme_image_style() function applies. The patch that I created here- though completely barbaric- handles this by looking at what attributes wysiwyg has set and applying those rather than the ones in the image style.

The protocode here works with images inserted by media- it won't work with images inserted by wysiwyg without it. The reason for this is that media provides a hook to alter the inserted code before it is rendered.

@fenstrat when I look through the code it seems like the issue is the view mode that is selected. Because view modes that implement image styles will run the resulting images through theme_image_style() I think the height and width attributes of those images will always be reset to the image style.

I think the issue is ultimately that there needs to be a better mechanism of handling attributes on objects in the wysiwyg and how to format their output. Unfortunately this means making sure that different editors handle these attributes in the same way which is a whole other thing.

Well I don't think the patch here is necessarily the answer, though I do think that anything being passed through the wysiwyg should be correctly resized because it sets the stage for properly sizing images as well as other options (crops, rotations, etc). Granted there isn't a derivative API that will let us track this yet and that's beyond the scope of this issue, however it at least gets us in the direction of uniformly handling content modified by the wysiwyg. For the mean time we may just want to document media_token_to_markup() a bit more clearly and add some information to the wiki pages about this.

not tested yet, but from a quick look at the patch it seems that they are 2 distinct ways to consider the issue and possible solutions:
1) wysiwyg is the "reference", so derivative images are created (and stored) from the original file->uri without consideration to the requested file view mode (through the media button)
That's what the patch appears to do.

2) wysiwyg is an "additional hint": when we insert a media, we most often already selected a file view_mode which matched our needs (otherwise we wouldn't use it in the first place).
So wysiwyg is only a small modification of the dimension on top of this; not worth a new exact thumbnail generation.
In such a case the minimal solution is to only pass-on width and height without the need for a new derived image so we end up (if thumbnail was selected in the media popup) like:
<img src="files/styles/thumbnail/a.jpg" width="XXX" height="YYY" />
= still using the file view mode requested, but with a client-side resizing (that's why height and width attributes exists).

please correct me if I've been misleaded in any way.

Yes #52 option (2) gets my vote, I am much more comfortable having the image style defined with a width & height set by the WYSIWYG.

Will try #13 with CKEditor 3.6.6.7689

Re #13
Looks to me like the rendering of the image is controlled by the "style" element so I need to add the following to the patch to get it to work:

Not sure I like this approach as the media patch will probably conflict with the WYSIWYG :(
I can't restrict my users to only one way of setting the size.
(but still investigating)

This apparently works with both "image" and "rendered file" file view modes !
(although worth being tested with file_view_mode and, why not, file_styles too)

I tested successfully patch #58 with a dev from 2013-02-05 and CKEditor 3.x