Media inline filter: Delegate "float" to outer element

I've tested this for the same type of use case with TinyMCE. Something like this would be great to have, I'd love to see this get in.

Jeremiah

media_filter_float_delegate.patch queued for re-testing.

typo

media_filter_float_delegate.patch queued for re-testing.

i wrote a test for this, which works, but i doesnt seem to work from the javascript side o f things..when i try with Ckeditor or TinyMCE i get no classes

Have just updated the patch from #4 to work with the latest dev of Media (a recent Media commit moved all WYSIWYG stuff into a submodule - this issue was behind that change).

I had to omit the test that ParisLiakos added as I'm no PHP wizard and I couldn't get that part of the patch to play nice with Media latest dev (sorry!). Should be sorted now - see #8.

I have some further notes about this issue but I'll follow up in a separate comment to keep things clear.

Fixing "Component" field.

OK, here's an updated patch that takes the best from #6 (works with latest Media dev) and #4 (re-adds ParisLiakos' test back in).

Comments on how to take this further to follow...

So far, this only works for media elements that have at least one extra custom field (this causes the image to be wrapped in a div with the class of media-element-container - this is where our float class is added if required). Due to the way Media works at the moment, this wrapper div seems be applied even if the image's extra fields are empty. This is possibly pointless extra markup but outside the scope of this issue.

I think that there's a good use case for this code to add a float class to the img tag itself if the wrapper div is not present. This adds more useful info for themers to latch on to - currently the float style is only applied as an inline style. I can't see how to add this to the existing code (my PHP is pretty sketchy) - anyone else up to the task? I'm guessing that this would require an additional test?

This is my first shot at trying to add the functionality I describe in #9 (attach a float class to the img tag if floated and not wrapped in another div).

The correct float class is added to the element's class array (confirmed when dumping the array to the screen), but the new class never seems to make it to the actual markup (making this patch very much work in progress).

As I said before, my PHP and knowledge of Drupal's API is pretty basic - can anyone finish this off or point me in the right direction? Cheers.

#8 works and is an extremely helpful UI feature. Thanks!

I was experiencing this issue in Media 7.x-2.x-alpha3 and WYSIWYG-7.x-2.2 and CKEditor 3. In my attempts to try these patches out I updated Media to to the dev branch.

I am now currently trying to replicate this issue with Media: 7.x-2.x-dev, WYSIWYG: 7.x-2.x-dev, and CKEditor 4.4.0 and I am no longer experiencing this issue. When I insert an image via the media module and apply a left or right justification in CKEditor, the inline float style is applied directly to the image and behaves properly.

Perhaps this issue has been fixed with the inclusion of this commit: http://cgit.drupalcode.org/media/commit/?id=1faca42 , or maybe the dev branch of the WYSIWYG module has solved this?

@pkozik The inline style was always applied directly to the image - that's the whole point of this issue: we need more than just an inline style property (which leaves themers with nothing to go on). We need a class applied to the image if it's displayed without any other fields. If there are accompanying fields, we need a class applied to the wrapper div. The patches above are start at trying to add these classes.

thanks, it works now. just a couple issues though:

1. +++ b/modules/media_wysiwyg/includes/media_wysiwyg.filter.inc
   @@ -197,6 +202,12 @@ function media_wysiwyg_token_to_markup($match, $wysiwyg = FALSE) {
   +      $element['content']['file']['#attributes']['class'][] = 'media-float-' . $settings['float'];
   
   @@ -207,6 +218,10 @@ function media_wysiwyg_token_to_markup($match, $wysiwyg = FALSE) {
   +        $element['content']['#attributes']['class'][] = 'media-float-' . $settings['float'];
   

   we should pass the class in drupal_html_class()

2. +++ b/modules/media_wysiwyg/includes/media_wysiwyg.filter.inc
   @@ -197,6 +202,12 @@ function media_wysiwyg_token_to_markup($match, $wysiwyg = FALSE) {
   +    ¶
   

   unneeded extra space

Workaround via template.php:

/**
 * Implements template_preprocess_file_entity
 */
function custom_bartik_preprocess_file_entity(&$vars) {
  $width = NULL;

  // Get image width from override or file metadata settings.
  if (!empty($vars['override']['attributes']['width'])) {
    $width = $vars['override']['attributes']['width'];
  }
  else if (!empty($vars['metadata']['width'])) {
    $width = $vars['metadata']['width'];
  }

  // Add width to wrapper div, to limit space of caption (title and copyright).
  if ($width) {
    if (empty($vars['attributes_array']['style'])) {
      $vars['attributes_array']['style'] = 'width: ' . $width . 'px;';
    }
    else {
      $vars['attributes_array']['style'] .= ' width: ' . $width . 'px;';
    }
  }

  // Add file style attributes to wrapper div too.
  if (!empty($vars['override']['attributes']['style'])) {
    if (empty($vars['attributes_array']['style'])) {
      $vars['attributes_array']['style'] = $vars['override']['attributes']['style'];
    }
    else {
      $vars['attributes_array']['style'] .= $vars['override']['attributes']['style'];
    }
  }
}

See issue #1515186: Current best practice for WYSIWYG Image Insert #14.

Here is a patch that applies to the latest version of 7.x-2.x. It includes the change to use drupal_html_class().

However, this only adapts the patch from #10 enough to apply. One of the changes is inside of a block that is now only relevant to the old field_attach rendering method. I haven't tested this with the new rendering method yet. This also means that the included test case won't be sufficient either. YMMV.

Patch #18 applies good and does what it claims, many thanks for it!
I think it should definitely be merged sooner or later, as this functionality is really needed when you have a fieldable image with some extra fields

Have been using #18 for quite some time now on multiple projects without any problems.

The patch in #18 works great, thanks!

The patch in #18 works for me, as well.

+1 Reviewed and Tested

The patch in #18 doesn't seem to apply cleanly against 2.x / 2.0-beta3 anymore. Any chance this will get some attention soon? Thanks in advance.

#18 needed a reroll, here it is.

This patch applies against the latest 7.x-2.x dev or 7.x-2.0-beta3

Can I get one confirmation that this still works on the latest release (7.x-2.0-beta5)?

The patch #18 applied cleanly on 7.x-2.x-beta5. The only issue that when rendering the image the paragraph containing the image will break.

@nikathone based on your latest test results we should set the status to 'needs work'

I'm not sure if this is the intended behavior or not, but the <img> tag still receives the inline float attribute, whereas the encapsulated html rendered by the selected formatter only adds the classes. The issue summary indicates that the goal is to move this property to the respected formatter, so I'd say this still needs some work.

ok, I'll probably do some work on this soon, my clients likely need this too.

Testing the latest patch #26 ..........................

@RajabNatshah did you find a need for patch #26, how were your test results looking?

review patch #26

beta10 was released, test patch #26 against beta10 and also media_ckeditor 7.x-1.0-alpha3

The patch in #26 works mostly. However, a few issues crop up (as mentioned in comment #30):

1. the float style remains on the image within the media element
2. the css class .media-float-left and .media-float-right don't have an associated style which makes it seem like it's not working unless the theme is updated to account for this

I assume this issue is mute post #2842391: better support for float media left and float media right. Not setting new status as I think this is up to the maintainers to decide.

I have re-rolled the patch in #26 to work with the latest development version.

I can't seem to apply a patch keeps looking for media_wysiwyg module. Any progress on this?

@jrbrown , sounds like you are not applying the patch correctly.

here is what to do:

if you are on windows, use 'git bash'

If you are on Linux or OSX, you are good to go.

cd /path/to/sites/all/modules/media
OR if you prefer (get the idea?)
cd /path/to/media

download the patch #37 to the media folder
so try this:
curl https://www.drupal.org/files/issues/media_filter_float_delegate-2018075-37.patch > media_filter_float_delegate-2018075-37.patch
now test apply
patch -p1 --dry-run < media_filter_float_delegate-2018075-37.patch

if this gives no errors
then apply the patch for real

patch -p1 < media_filter_float_delegate-2018075-37.patch

If it doesn't work for you, you can reverse the patch. To reverse or undo the patch you can do this:

patch -p1 -R < media_filter_float_delegate-2018075-37.patch

I see the float style being applied, image alignments on the other hand like left | right | center don't seem to be applied at all.

@kaare - I agree - I think the scope of this issue should turn to something more like: If there is a float applied to the image, instead use the new alignment classes on the rendered file entity container.

A more ambitious plan would be to also transform these existing floats on edit to using the alignment functionality and unset the floats in tagmap.

Patch 26, nor patch 37 apply cleanly to the latest release (2.9). Here's the output using patch 37:

patching file modules/media_wysiwyg/includes/media_wysiwyg.filter.inc
Hunk #1 succeeded at 238 (offset 50 lines).
Hunk #2 succeeded at 336 (offset 50 lines).
Hunk #3 FAILED at 352.
1 out of 3 hunks FAILED -- saving rejects to file modules/media_wysiwyg/includes/media_wysiwyg.filter.inc.rej
patching file modules/media_wysiwyg/tests/media_wysiwyg.macro.test

I have re-rolled the patch in #37 to work with the latest development version.

So I've noticed that Video elements are not aligning center, seems we still need the -moz and -webkit attributes on the text-align center. I have updated the patch to include those elements in the css file.

committed to 7.x-2.x and merged into 7.x-3.x and pushed