Posting this after initial discussions with quicksketch in #1407590: Media 7.x-1.0-rc3 breaks image resizing

As noted in that issue the commit in #1186434: Media field image formatter doesn't create width and height attributes. broke the ability of media images to be resized in a WYSIWYG. Custom width/height attributes added to the image (e.g. click and drag resize) are now always overridden by the media module.

After that commit (i.e. as part of rc3) the output from media_token_to_markup() returns an render $element as:

array(
  '#theme' => 'image_style',
  ...
  '#width' => 1000,
  '#height' => 850,
  '#attributes' => array(
    'height' => 85,
    'width' => 100,
  ),
);

Where 1000x850 is the original image size and 100x85 is the resized attributes. When the image comes to be rendered 1000x850 take precedent and override the attributes.

Prior to rc3 the #width and #height properties we not added to the $element, thus allowing the height and width values in #attributes to take effect.

Question is, how to work around this, or how to overcome it?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Argus’s picture

Title: Resizing images in WYSIWYG broken in media-7.x-1.0-rc3 & 2.0-unstable3 » Resizing images in WYSIWYG broken in media-7.x-1.0-rc3

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.

fenstrat’s picture

@Argus, interesting, I hadn't noticed that. But it does fit with the problem here in #0, i.e. the media module overrides all image width and height values when it renders the image.

smira’s picture

Title: Resizing images in WYSIWYG broken in media-7.x-1.0-rc3 » Resizing images in WYSIWYG broken in media-7.x-1.0-rc3 & 2.0-unstable3

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

here is the culprit function

/**
 * Implements hook_file_formatter_FORMATTER_view().
 *
 * Returns a drupal_render() array to display an image of the chosen style.
 *
 * This formatter is only capable of displaying local images. If the passed in
 * file is either not local or not an image, nothing is returned, so that
 * file_view_file() can try another formatter.
 */
function file_entity_file_formatter_file_image_view($file, $display, $langcode) {
  $scheme = file_uri_scheme($file->uri);
  $local_wrappers = file_get_stream_wrappers(STREAM_WRAPPERS_LOCAL);
  if (isset($local_wrappers[$scheme]) && strpos($file->filemime, 'image/') === 0 && $image = image_load($file->uri)) {
    if (!empty($display['settings']['image_style'])) {
      $element = array(
        '#theme' => 'image_style',
        '#style_name' => $display['settings']['image_style'],
        '#path' => $file->uri,
        '#width' => $image->info['width'],
        '#height' => $image->info['height'],
      );
    }
    else {
      $element = array(
        '#theme' => 'image',
        '#path' => $file->uri,
        '#width' => $image->info['width'],
        '#height' => $image->info['height'],
      );
    }
    return $element;
  }
}

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

fenstrat’s picture

Attached is the rollback of the patch committed in #1186434: Media field image formatter doesn't create width and height attributes.

@smiro2000 That is indeed the culprit function, I haven't had any more time to look at this, just need this patch for a make file. I think you've got the right idea to only add the width and height if they are set in #attributes, however at first glance that function doesn't have access to #attributes? Did you get any further with your testing?

fenstrat’s picture

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

smira’s picture

Title: Resizing images in WYSIWYG broken in media-7.x-1.0-rc3 » Resizing images in WYSIWYG broken in media-7.x-1.0-rc3 & 2.0-unstable3

after looking into the media_toke_markup() function is looks like there are other checks in place to verify the structure of the $element array
from line 181 of media.filter.inc

    if (isset($settings['attributes']['style'])) {
      $css_properties = media_parse_css_declarations($settings['attributes']['style']);
      foreach (array('width', 'height') as $dimension) {
        if (isset($css_properties[$dimension]) && substr($css_properties[$dimension], -2) == 'px') {
          $settings[$dimension] = substr($css_properties[$dimension], 0, -2);
        }
        elseif (isset($settings['attributes'][$dimension])) {
          $settings[$dimension] = $settings['attributes'][$dimension];
        }
      }
    }

i'm going to try adding something here, in the mean time though it seems like this error is TinyMCE specific

EDIT:
ok it looks like it is TinyMCE specific

here is the pre-formatted output for the two
Tinly => [[{"type":"media","view_mode":"media_original","fid":"8","attributes":{"alt":"","class":"media-image","height":"554","width":"426"}}]]
CKeditor => [[{"type":"media","view_mode":"media_original","fid":"181","attributes":{"alt":"","class":"media-image","height":"164","style":"width: 250px; height: 124px; ","typeof":"foaf:Image","width":"331"}}]]

notice in fact how CKeditor add the "style" array inside of "attributes"

i've been playing around with the above function to add some checks but i have a feeling i'm off track...

smira’s picture

FileSize
625 bytes

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

  // fix for issue #1411340 "Resizing images in WYSIWYG"
  if ($element['#width'] != $element['#attributes']['width']) {
    $element['#width'] = $element['#attributes']['width'];
  }
  if ($element['#height'] != $element['#attributes']['height']) {
    $element['#height'] = $element['#attributes']['height'];
  }

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

marco_mazzocchi’s picture

I get a resizing problem in WYSIWYG using the "media browser" button. The resize informations are lost after saving content changes.
I'm not able to use the patches above becouse, I think, files of Media module have been updated.
I'm using the last version available:

; Information added by drupal.org packaging script on 2012-03-23
version = "7.x-1.0"
core = "7.x"
project = "media"
datestamp = "1332537952"
betoscopio’s picture

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

betoscopio’s picture

The resize is working in Media 1.1 with patch #7, but when i insert an image in a text field i got this error message:

    Notice: Undefined index: #width in media_get_file_without_label() (line 414 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 414 of media/includes/media.filter.inc).
    Notice: Undefined index: #height en media_get_file_without_label() (line 417 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 417 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 414 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 415 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 417 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 418 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 414 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 415 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 417 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 418 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 414 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 415 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 417 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 418 of media/includes/media.filter.inc).
    Notice: Undefined index: #width in media_get_file_without_label() (line 414 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 414 of media/includes/media.filter.inc).
    Notice: Undefined index: #height en media_get_file_without_label() (line 417 of media/includes/media.filter.inc).
    Notice: Undefined index: #attributes in media_get_file_without_label() (line 417 of media/includes/media.filter.inc).
dandaman’s picture

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:

  // fix for issue #1411340 "Resizing images in WYSIWYG"
  if (isset($element['#attributes']) && isset($element['#attributes']['width'])) {
    $element['#width'] = $element['#attributes']['width'];
  }
  if (isset($element['#attributes']) && isset($element['#attributes']['height'])) {
    $element['#height'] = $element['#attributes']['height'];
  }
fenstrat’s picture

Title: Resizing images in WYSIWYG broken in media-7.x-1.0-rc3 & 2.0-unstable3 » Resizing images in WYSIWYG + TinyMCE broken in media-7.x-1.0-rc3+ & 2.0-unstable3+
Status: Active » Needs review

Finally had some time to look at this again.

Everything here is pointing to this being a specific issue with TinyMCE as it ads width/height tags to the img tag directly, and not on the style attribute of the img tag. There's a feature request to change this in TinyMCE directly: Stop Prepopulating Image Dimensions.

However untill that is fixed, or for any instance where the width/height on the actual image needs to be be rendered (rather than the values returned from image_load() as introduced in 1.0-rc3), there is a work around possible using hook_media_token_to_markup_alter(). This is done in a custom module and as such overcomes the need to make changes/patch media module itself.

/**
 * Implements media_token_to_markup_alter().
 */
function mymodule_media_token_to_markup_alter(&$element, $tag_info, $settings) {
  if (isset($element['#width']) && isset($element['#attributes']['width']) && $element['#width'] != $element['#attributes']['width']) {
    $element['#width'] = $element['#attributes']['width'];
  }
  if (isset($element['#height']) && isset($element['#attributes']['height']) && $element['#height'] != $element['#attributes']['height']) {
    $element['#height'] = $element['#attributes']['height'];
  }
}

Setting to Needs review, dispite this not being a patch. Would be interested if this works for others with this issue.

betoscopio’s picture

Title: Resizing images in WYSIWYG + TinyMCE broken in media-7.x-1.0-rc3+ & 2.0-unstable3+ » Resizing images in WYSIWYG in media-7.x-1.0-rc3+ & 2.0-unstable3+
Status: Needs review » Active
FileSize
662 bytes

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’s picture

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

Oops, title changed in wrong way.

fenstrat’s picture

Status: Active » Needs review

@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’s picture

@frenstrat, thanks for the explanation, now i understand the diference better. With your approach, wich would be a suitable module to implement it? I mean, this is a general use case and not specific for a particular site then i think this fix should go in some contrib module and no site specific code. Maybe not Media as you are suggesting, but some related module.

fenstrat’s picture

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

betoscopio’s picture

@fenstrat, sure, beers while discussing issues would be great. I'm sending you a message.

zylootino’s picture

#7 Solved the issue for me

drzraf’s picture

didn't understood all, but in case it helps : #1448366: width and height attributes ignored

drzraf’s picture

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, ...)

fenstrat’s picture

@drzraf Using Full content or Teaser view mode. But this issue isn't dependent of display mode. It occurs whenever a media item is included in a text field with a text format which uses the "Convert Media tags to markup" text filter.

drzraf’s picture

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", ...

fenstrat’s picture

Sorry @drzraf, I'm using "Original" file display mode. Though I just double checked, "Small", "Large" or any other mode, all have the same issue. This is because this issue is caused by media not respecting width and height attibutes on an img tag, please see #12 for more detail.

drzraf’s picture

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.

fenstrat’s picture

@drzraf The "original" file display is bound to the "Image" display. All those settings are default, I've not changed any of them. Looking at the filepath you're using the 2.x branch of media, I'm on the 1.x where the path is /admin/config/media/file-types/manage/image/file-display/media_original. But this issue occurs on both the 1.x and 2.x branch of media.

It looks like you've outlined almost exactly the same issue in #1448366: width and height attributes ignored. Width and height styles set on an image are not respected.

I've just changed the "original" file display at /admin/config/media/file-types/manage/image/file-display/media_original from "Image" to "File style: original" and bingo, resizing starts working. This is exactly the same as your finding in #1448366: width and height attributes ignored. So this seems to be an issue only with the Image display formatter. Next question, where/how to fix this?

fenstrat’s picture

Status: Needs review » Needs work

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’s picture

Status: Needs work » Active

I'm wondering if all of this patching can be avoided by just using Image Resize Filter, as in http://drupal.org/node/1599070 ? It's my understanding that Media wouldn't support this functionality natively, and using the input filter is the way to go if you need resizable images in the WYSIWYG. Please correct me if I'm wrong.

This was also addressed in #944184: Generate scaled cached image when scaling in WYSIWYG editor, but I don't get why it is reported to not work in TinyMCE... the doc page goes over that.

fenstrat’s picture

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

mpgeek’s picture

I'm not certain either, as i was assuming that overriding the native width and height attributes wasn't supposed to work. Thats the stance we've taken with our users, and that seems to be ok... they are now using display formatters instead of just resizing in the WYSIWYG. I think we'd need a decision as to whether or not this is *supposed* to work natively with Media or not.

Two cents deposited.

nrambeck’s picture

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

cameron prince’s picture

Patch #13 works for me as well (media 7.x-2.0-unstable6+83-dev).

drzraf’s picture

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

ahillio’s picture

Very grateful for the fix in #12

Using media-7.x-1.2

arthurf’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev

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.

arthurf’s picture

FileSize
4.61 KB

Ok here's a rough concept of what this could look like. This is not sanity checked code. The data that the WYSIWYG passes back should be checked. I'm not sure if the way I've implemented the image style is a good idea either.

arthurf’s picture

Status: Active » Needs review
arthurf’s picture

FileSize
4.65 KB

Slight tweak to allow for multiple crops of the same image. Probably needs more thought.

fenstrat’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Needs review » Active

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

fenstrat’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Active » Needs review

Cross post with @arthurf

Interesting approach, will try it out and report back.

fenstrat’s picture

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?

arthurf’s picture

@fenstrat - sorry, I set the issue to 2.x from some of the comments above. I think the issue is still with the image theme functions is still the same and so likely that this patch in some form could apply to 1.x. I think it needs some work before backporting (if at all) it to 1.x.

If you're trying to apply it, the main thing is that you need the $element['content'][file']['#theme']. I'm not sure if 1.x is passing the $tag_info along as well, but it shouldn't be that hard to make work.

arthurf’s picture

FileSize
5.64 KB

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

drzraf’s picture

thanks a lot! I'll test that soon.
But as you dug far into that code, it could be useful to bring your insights to core dev' so that the bug (it is) in Drupal Image module could be fixed. If not somewhere else: #1448366: width and height attributes ignored

arthurf’s picture

@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’s picture

I'm not sure if the inclusion of image_style_create_derivative() is a good direction for this. A quick glance at theme_media_wysiwyg_image() shows it's trying to replicate the functionality of http://drupal.org/project/image_resize_filter which seems a better, already polished solution for this.

Remember that this commit caused this issue. I think the key thing here is to get the width and height values which TinyMCE and CKEditor place in the *style* attribute out and into the base level *width* and *height* attributes.

arthurf’s picture

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

fenstrat’s picture

@arthurf That does indeed seem to be an issue here, that editors do not handle nor apply attributes in a consistent way.

At the moment I've not got time to dig into this more, however I still feel the image_style_create_derivative() section in #43 is not the way a solution should go.

arthurf’s picture

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.

ewijdemans’s picture

From my point of view (I'm not deeply involved here), the user inserts and image and specifying an image_view. The image get inserted into the editor and he's done.

Next, the user decides to resize the image, thus making the image_view obsolete by overriding it via the image it's width/height attributes.

So, in my opinion, these width/height values should be respected as #12 does and thus making it a valid solution, or am I completely missing something here?

drzraf’s picture

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.

dalin’s picture

Status: Needs review » Needs work

I tend to agree that we should do 2) wysiwyg is an "additional hint". If you want derivatives to be created of the exact proper size, use Image Resize Filter module.

This means that the patch in #13 is the direction to go (though it needs some work).

guy_schneerson’s picture

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

dalin’s picture

I should elaborate what I meant by "needs some work": In other places we do something like:

foreach (array('width', 'height') as $dimension) {
  // code common to both goes here.
}
guy_schneerson’s picture

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:

<?php
  $style = sprintf('width: %spx; height: %spx;', $element['#width'], $element['#height']);
  $element['#attributes']['style'] = $style;
?>

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)

guy_schneerson’s picture

The attached patch is an update for #13 that fixes the issue for me with CKEditor 3.6.6.7689

Not sure it fixes it on any editor and I am not sure I am happy with this approach.
Also managed to get it working with media_token_to_markup_alter().

drzraf’s picture

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)

mikeker’s picture

Status: Needs work » Needs review
FileSize
1.05 KB

Slight update to #56:

  • Changed the leading comment (we don't usually do "fixes #1234" comments).
  • Doesn't overwrite existing style attributes, if they exist

Patch is against the latest -dev. I've also verified it works with 7.x-2.0-unstable7.

checker’s picture

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

glnielsen’s picture

I installed this patch from #58 on latest Drupal 7, tinyMCE and media module. Worked fine for me.

mariacha1’s picture

Status: Needs review » Reviewed & tested by the community

[#58] is also working great for me using tinyMCE, media-2.0-unstable7, and Drupal 7, and as far as I can tell the code looks good.

mariacha1’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.3 KB
1.72 KB
80.58 KB
31.91 KB
49.67 KB
87.66 KB
86.4 KB

Looks like I was too hasty with rbtc. I found several issues while working with patch #58.

The first one was that all images loaded through media are given a style tag of their original dimensions.

So I have an image of 729px wide by 485px tall. I also have a teaser display mode set up, that should change the image into a max of 320px wide. When using patch #58, my file code was:

{
  "fid":"8",
  "view_mode":"teaser",
  "type":"media",
  "attributes":{
    "height":213,
    "width":320,
    "style":"width: 729px; height: 485px;",
    "class":"media-element file-teaser"
  }
}

Or, in visual form (in the tinymce window)

Because height and width should always come from the "style" css attribute first (because that's what the browser uses), my image was always looking un-resized both my wysiwyg editor.

So we really don't want to add the initial height and width to an image when we select its display styles in this window.

Luckily, media already gives us a clue to when the display style window loads. It's the $settings['wysiwyg'] = TRUE.

So I fixed the issue by changing this line:

if (isset($element['#height']) && isset($element['#width'])) {

to:

if (!isset($settings['wysiwyg']) && isset($element['#height']) && isset($element['#width'])) {

The second issue occured when I decided maybe I'd have a user savvy enough to put in their own dimensions in the style input area on the wysiwyg. Let's say they wanted to resize their 300px wide image to be 100px wide and 100px tall using only the style tag.

The image is now resized via php to be 100px by 100px, but it's showing up at 300x200 -- leading to an image blurry and squished.

Looking at the html it became clear why:

<img width="100" height="100" title="" alt="" src="http://211info.dev/sites/default/files/resize/styles/service_images/public/resource_photos/211info_images0112-100x100.png?itok=Twcbht2A" typeof="foaf:Image" class="media-element file-teaser image-style-service-images" style="width: 100px; height: 100px; width: 300px; height: 200px;">

The trick there is the double width and height styles:

style="width: 100px; height: 100px; width: 300px; height: 200px;"

The browser was reading from the second set of numbers, while image_resize_filter is reading from the first set.

The culprit there was this line:

$element['#attributes']['style'] .= " $style";

That doubles height and width styles. The better way is to see what styles already exist, replace height and width, then re-save any existing styles, like so:

    $existing_styles = media_parse_css_declarations($element['#attributes']['style']);
    $existing_styles['height'] = $element['#height'] . 'px';
    $existing_styles['width'] = $element['#width'] .'px';
    $styles = array();

    foreach($existing_styles as $style_name=>$style_value) {
      $styles[] = $style_name . ":" . $style_value;
    }

    $element['#attributes']['style'] = implode(';', $styles);

At least, that's what I thought. And indeed, in the wysiwyg editor, it showed the image at 100x100px. But on real page, everything was reverting back to what was in the height and width attributes instead of what was in my style attribute.

So then I had to dig through the code and figure out how to make that all work. I discovered the the $settings variable also gives you clues as to what the size should actually be. $settings['height'] and $settings['width'], if they exist, are the "style='height:100px;width:200px;'" values in html, aka the desired values.

If they don't exist, pull in the $settings['attribute']['height'] and width, as these represent your height="100" width="200" properties.

Finally, if none of that is set (which is only the case on the screen to pick a display type, from what I can tell) you have to just rely on what's in the $element['#attributes']['height'] and $element['#attributes']['width'] variables.

Attached is the code that should fix all issues, and an interdiff between this patch and #58.

Status: Needs review » Needs work

The last submitted patch, interdiff.patch, failed testing.

moonray’s picture

Status: Needs work » Needs review

The patch actually passed, only the interdiff failed, so setting to needs review.

aaron’s picture

Status: Needs review » Needs work

Applying the patch works however I get the following error message:

Notice: Undefined index: style in media_get_file_without_label() (line 711 of C:\wamp\www\drupal\sites\all\modules\media\includes\media.filter.inc).

moonray’s picture

I applied the patch to latest dev, updated to latest dev of file_entity, then updated db (update.php).
No errors for me; worked as advertised.

mrfelton’s picture

Seems to work well. Two warnings though.

Notice: Undefined index: width in media_get_file_without_label() (line 667 of /Users/tom/workspace/sac/sites/all/modules/contrib/media/includes/media.filter.inc). Backtrace:

Notice: Undefined index: height in media_get_file_without_label() (line 668 of /Users/tom/workspace/sac/sites/all/modules/contrib/media/includes/media.filter.inc). Backtrace:

mrfelton’s picture

Status: Needs work » Needs review
FileSize
1.83 KB

Updated patch with minor fix for undefined index warnings.

alar’s picture

This patch media-resize_images_in_wysiwyg-1411340-68.patch appears to have solved our resize image issue.
Many thanks!

eigentor’s picture

For those running into this issue and using Media 2.x with Image resize filter: use the "original" preset to avoid the problems.
I guess once the patch gets committed and into the 1.x and 2.x branch, it will also be possible to use other Image Style presets.

mariacha1’s picture

The logic for #68 is slightly wrong. This code:

  if(isset($settings['attributes']['width']) || isset($settings['attributes']['height'])) {
    $width = isset($settings['width']) ? $settings['width'] : $settings['attributes']['width'];
    $height = isset($settings['height']) ? $settings['height'] : $settings['attributes']['height'];
  }

means that the ideal settings for height and width ($settings['width'] & $settings['height']) aren't being looked at unless the $settings['attributes'] height and width are set. But we can't count on the two having anything to do with eachother. Instead, I think we're going to need to break this into another if statement like so:

  if (isset($settings['width']) || isset($settings['height'])) {
    $width = isset($settings['width']) ? $settings['width'] : FALSE;
    $height = isset($settings['height']) ? $settings['height'] : FALSE;
  }
  elseif (isset($settings['attributes']['width']) || isset($settings['attributes']['height'])) {
    $width = isset($settings['attributes']['width']) ? $settings['attributes']['width'] : FALSE;
    $height = isset($settings['attributes']['height']) ? $settings['attributes']['height'] : FALSE;
  }

I'm adding a patch to do this. I'm also cleaning up some coding standards errors in #68 patch.

jags880’s picture

Anyone else find that the none of these patches work with the latest dev release? 2013-Sep-04
I've tried both #68 and #71 but once I insert the image into the wysiwyg area with the Media Browser button (using display as "Default") and then resize the image via the ckeditor image tool (i've tried TinyMCE too) everything looks fine...then I save and none of the size changes are saved. In fact no style settings are saved...float, border, padding, margin, none of them are output. Am I doing something wrong?

jrao’s picture

Those patches do not seem to work with image_formatter theme function, which needs width/height/attributes to be inside item array. Here's a hack to add them based on #71, but I don't know if this is the best way to fix this:

  if ($width && $height) {
    $element['#height'] = $height;
    $element['#width'] = $width;

    if (!isset($settings['wysiwyg'])) {
      $existing_styles = isset($element['#attributes']['style']) ? media_parse_css_declarations($element['#attributes']['style']) : array();
      $existing_styles['height'] = $height . 'px';
      $existing_styles['width'] = $width . 'px';
      $styles = array();

      foreach ($existing_styles as $style_name => $style_value) {
        $styles[] = $style_name . ":" . $style_value;
      }

      $element['#attributes']['style'] = implode(';', $styles);
    }
    // HACK: special handling for image_formatter theme function
    if ($element['#theme'] == 'image_formatter' && isset($element['#item'])) {
    	$element['#item']['width'] = $width;
    	$element['#item']['height'] = $height;
    	$element['#item']['attributes'] = $element['#attributes'];
    }
  }
arthurf’s picture

Here's a snippit that allows content creators to use the CKEditor image options while not breaking responsive layouts. This is being done in JS because of how CKEditor updates image attributes. Additionally it captures the image floats and moves them to the parent element so that things like captions will follow the image. It's not the best approach as it would be better to have the markup correct when it is output from Drupal- hook_media_token_to_markup() would be the way to go there but CKEditor seems to get in the way.

  $('img.media-element.file-default').each(function () {
        // Remove the 'px' from the CSS values so we can use height/width on the img tag.
        var remove = /px$/;

        var height = 0;
        var width = 0;

        if ($(this).css('height')) {
          height = $(this).css('height').replace(remove, '');
          $(this).attr('height', height);
          $(this).css('height', '');
        }

        if ($(this).css('width')) {
          width = $(this).css('width').replace(remove, '');
          $(this).attr('width', width);
          $(this).css('width', '');
        }

        // Media uses a wrapping container when an image is being displayed with
        // additional fields. In this case the float, margin, and width need to
        // be applied to the container.
        if ($(this).parent('.media-element-container').length) {
          if ($(this).css('float')) {
            $(this).parent('.media-element-container').css('float', $(this).css('float'));
            $(this).css('float', 'none');
          }

          if ($(this).css('margin')) {
            $(this).parent('.media-element-container').css('margin', $(this).css('margin'));
            $(this).css('float', 'none');
          }

          // If a width was defined, set the width of the container.
          if (width) {
            $(this).parent('.media-element-container').css('width', width).css('max-width', '100%');
          }
        }
  });
sylus’s picture

Attaching updated patch based on #73. It seems to make image styles work again.

Yaron Tal’s picture

The above patch wasn't working for me. I had to change the $element['#height'] = $height; line to $element['#item']['height'] = $height; to get this to work.

I attached the changed version of the patch by @sylus I used to get my resizing to work again. It now works in the node and in the editor (wysiwyg dev verson with ckeditor 4.x). I don't know why the above patch works for others, but since theme_image_formatter() uses the width and height from inside the item I can't see how it could.

Besides this I also need a patch for the JS to get resizing to work again. A colleague is currently working on that. It looks like all changes to the markup version are thrashed because of the way the tagmap is used.

Status: Needs review » Needs work

The last submitted patch, media.wysiwyg-integration.1411340-76.patch, failed testing.

Rob C’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, media.wysiwyg-integration.1411340-76.patch, failed testing.

kaidjohnson’s picture

Ugh -- I believe we've been working on the same issue in this thread here: https://drupal.org/node/2067063 and have taken slightly different approaches to resolve the issue. We should probably consolidate the work. The latest patch (https://drupal.org/files/media-missing-attributes-2067063-42.patch via https://drupal.org/node/2067063#comment-7936175) resolves a lot of issues surrounding general attribute handling (including height/width).

In addition to resizing images, we have been tackling floats and toggling the WYSIWYG filter, because they are all related to how media get handled in the WYSIWYG and how it ultimately gets rendered on the front end.

We should probably mark this thread or the other as a duplicate. Thoughts?

Also, It appears that sometime between September 30 and now, media is failing on simpletests, thus the patch in #76 won't pass and the patch in https://drupal.org/node/2067063#comment-7936175 won't pass either. I've poked at it a bit, but can't find the issue. It happens in the setUp of the simpletest and fails on enabling the media and file_entity modules, but I can't get it to tell me why...

Yaron Tal’s picture

I agree with closing this issue.
Maybe a good idea to cross post the last patch in #76 and possibly #75 (with the mention that it was created by @sylus, @jrao, @mariacha1 and @mrfelton).
Somehow the patches in that other issues don't work on my installation, but the above one at least fixes the PHP part of the problem.

mariacha1’s picture

FWIW, I agree that https://drupal.org/node/2067063 is fixing a lot more of the problems I'm seeing with media in general, and that the code there is cleaner, once it's working fully.

kaidjohnson’s picture

Status: Needs work » Closed (duplicate)

Closing this issue as a duplicate. Let's consolidate to https://drupal.org/node/2067063.

Also, via #2107063: SimpleTests are failing. and #2109293: Media's 7.x-2.x dependencies are not being downloaded, it appears simpletests are working again.