I did a fresh install of Media 7.x-1.0-beta3 and Styles 7.x-2.0-alpha5 and when I tried to use the Image Alignment tool in CK Editor, it would add "float" to the JSON code in the editor, however it wouldn't save the alignment when I saved the node.

I made a couple of changes to try to make this work, however I'm not sure if this was done correctly. Styles 2.0 seems to have completely redone how it handles image styling, and now all the attribute handling seems to be processed by the FileStyles class rather than the old theme functions in file_styles.theme.inc. I say I'm not sure if this is being done correctly since @aaron seems to put all of the needed support for floats in that class, however they did not seem to work when setting alignment in CKEditor. So basically, this is just the quickest way I could come up with to make this work with CKEditor and hoping this could help someone else in a similar predicament that needs a solution ASAP and contribute to a more permanent solution.

There are two patches attached, one for /media.filter.inc in Media 7.x-1.0-beta3 and one for /contrib/file_styles/includes/styles/FileStyles.inc in Styles 7.x-2.0-alpha5. I'm not recommending these to be committed per se, however it would be nice if this could help the maintainers add support for this feature. Regarding the media module, however, it does allow better handling of finding styles supplied by the WYSIWYG editor, without the caveat highlighted in the notes: "Also won't work if the style has anything between width and height (or if they are in reverse order).". Again, these patches are not made from the dev release since I have next to no time at the moment to checkout from CVS or re-download and install the dev versions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, FileStyles.inc_.float_.patch, failed testing.

Jackinloadup’s picture

This is a duplicate of #1018606: Attribute for embedded images not in output but my patch there is way janky. Glad to see some other progress!

rnyberg’s picture

I can verify this behaviour. Here are some example cases:

Tag:
[[{"type":"media","view_mode":"media_preview","fid":"78","attributes":{"class":"media-image","style":"float: right; ","typeof":"foaf:Image"}}]]

Shows up like this during body edit:
<img class="media-image attr__typeof__foaf:Image img__fid__78 img__view_mode__media_preview attr__format__media_preview" src="http://site.com/sites/default/files/styles/medium/public/image.jpg" data-cke-saved-src="http://site.com/sites/default/files/styles/medium/public/image.jpg" style="float: right; " typeof="foaf:Image">

When viewing the node after submit:
<img typeof="foaf:Image" src="http://redi.dyndns.info/sites/default/files/styles/square_thumbnail/public/views_in_action_0.jpg">

I have no ideas how to patch\fix myself, but hopefully you guys can resolve this issue! :)

matrobin’s picture

In our local Drupal7 test installation I modified cocoloco's patches in #1 to enable the margin and border styles too:

In function media_token_to_markup() I tuned the regex to match short margin styles like "margin: 10px 20px" and added the margin/border tags to the list of allowed tags:

    if (isset($settings['style'])) {
//      if (preg_match('@width: (.+?)px; height: (.+?)px;@i', $settings['style'], $matches)) {
//        $settings['width'] = $matches[1];
//        $settings['height'] = $matches[2];
//      }
// PATCH from http://drupal.org/node/1043570
// + modified regex, allowed more styles
      if (preg_match_all('/([a-z\-]+):\s*([a-z0-9\-]+(\s+[a-z0-9\-]+)*);/i', $settings['style'], $matches)) {
        $allowed_tags = array('width', 'height', 'float', 
        	'margin-top', 'margin-bottom', 'margin-left', 'margin-right', 'margin',
        	'border-width', ' border-style');
        for($i=0; $i<count($matches[0]); $i++) {
          $tag = $matches[1][$i];
          $val = $matches[2][$i];
          error_log("$tag:$val;");
          if (in_array($tag, $allowed_tags)) {
            switch ($tag) {
              case 'width':
              case 'height':
                $pixels = preg_replace('/[^\d]+/', '', $val);
                $settings[$tag] = $pixels;
                break;
              default:
                $settings[$tag] = $val;
                break;
            }
          }
        }
      }
    }

In function thumbnail($effect) of class FileStyles I used $value = $this->override($attribute) instead of $value = $this->get($attribute) in addition to cocoloco's patch:

    // Set any WYSIWYG prescribed styles.
    $styles = array();
    foreach (array('border-width', 'border-style', 'margin', 'margin-top', 'margin-right', 'margin-bottom', 'margin-left') as $attribute) {

      $value = $this->override($attribute);
      // using override() fixed disappearing wysiwyg styles
      // before: $value = $this->get($attribute);
      
      if (isset($value)) {
        $styles[$attribute] = $attribute .':'. check_plain($value);
      }
    }

    // cocoloco's patch from http://drupal.org/node/1043570
    // Set float style if set
    $float = $this->get_float();
    if (isset($float)) {
        $styles['float'] = 'float:' . check_plain($float);
    }
    
    if (!empty($styles)) {
      $attributes['style'] = implode(';', $styles) . ';';
    }
travismiller’s picture

I had these same problems, the original patches + matrobin's changes fixed this problem for me.

It didn't effect me, but it seems odd to have the full field, field-items, field-item, etc div classes wrapped around the media. I assume this is from letting the built-in field code handle rendering the content, but IMO it seems like there should be a cleaner way. With more time, I'll try to look into this myself.

This is a nice feature to have WYSIWYG and media module work well together. Thanks everyone for your work on this!

cocoloco’s picture

Thanks for improving the patch, matrobin.

puckbag: I agree, I feel that there is a cleaner way - especially since I saw code in the FileStyles class to add float-left and float-right classes if a float was specified instead of adding the CSS inline. However, as I mentioned in my previous post, this was the quickest way I found that worked, and I decided to share it in case it could help anyone else - it would be great if this could be properly updated to work the way the original author intended.

ogi’s picture

subscribe

traceelements’s picture

Subscribing...

Kreativs’s picture

subscribe

JacobSingh’s picture

I support this, but it would be nice if it was coupled with an interface to do it in the UI. As it is, there is no way to set the float AFAIK. Also, this could be handled not through media, but simply through wrapping divs and CSS...

rnyberg’s picture

You need to use WYSIWYG and CKEDITOR to get the Media button to show up, which you can use to add an image inline. You can then doubleclick the image and use CKEDITORS align left\right dropdown menu as well as type in descriptions etc.

Doesn't work yet though, hoping the bug to be fixed soon so I could launch my site, this is the last thing that I'm waiting for. ^^

aaron’s picture

crud, i thought i had all this working in v2 of styles. i'll need to take another look. thanks for the work towards this!

discipolo’s picture

What the patches did for me was make ckeditor at least remember the changes in the Backend. still doesnt output the styles in the frontend though.

Mac Ryan’s picture

subscribe

grandcat’s picture

subscribe

kirkofalltrades’s picture

Subscribed. Sure, we can correct this via CSS but my sites' content needs to be managed by non-technical people.

EDIT: Sorry, was new to patching. I applied the patches described above by matrobin and my images are now floating correctly.

mcaden’s picture

subscribe

AlanAtLarge’s picture

Subscribing

Bios Element’s picture

I can confirm the above patches and patch changes are correct, they simply need a patch made and then committed. I'll give it a shot later today, but I haven't worked with patches often so no guarantees.

bryancasler’s picture

subscribe

baronmunchowsen’s picture

I applied the changes outlined in #4 (http://drupal.org/node/1043570#comment-4032316) to the latest dev versions of media and styles (Media: 7.x-1.x-dev March 5th and Styles: 7.x-2.x-dev March 10th) and it seems to have fixed the problem.

Cheers.

bryancasler’s picture

Status: Needs work » Reviewed & tested by the community
aaron’s picture

yes, #4 looks like the right approach. i'm not able to roll a patch from this computer, so that will need to wait till the sprint tomorrow.

camdarley’s picture

(from http://drupal.org/node/1084082)
Hi,
I don't know if it's a Wysiwyg module's issue but i can't align image inserted in Ckeditor from the media browser at the center.
When i select the image, then click the "align center" wysiwyg's button, the image is well aligned.
Disabling the Rich Text editor show that the image is embeded in a div with align="center".
But when i click save, the image remain left aligned. When i look at the source code, the image is embeded outside the <div>.
It's annoying because there is no other way to align image at center.

lelizondo’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.24 KB
1.78 KB

Here they are. I made the two patches with the code from #4 so anyone can test this.

The problem is fixed but there's an incompatibility with the Image Resize Filter, I think it has to do with what @cocoloco said:

Also won't work if the style has anything between width and height (or if they are in reverse order).

lelizondo’s picture

@camdarley it seems that your problem is not fixed with the couple of patches in this issue. It might be that when you align to the center, the style is applied to the < p > and the image will be surrounded by a < div > which has no style.

camdarley’s picture

That's what i thought, so I bring my issue back there: http://drupal.org/node/1084082

Status: Needs review » Needs work

The last submitted patch, styles-1043570.patch, failed testing.

lelizondo’s picture

Status: Needs work » Needs review

I think the patch will fail because is from another module, but I don't really know how the automated tests work.

oxyc’s picture

I added the filter to allow the display attribute as well, this is because TinyMCE uses display:block; margin-left:auto; margin-right:auto; to align images.

I also committed this to the branch but as aaron pointed out in the following thread: http://drupal.org/node/1091394
Something's definitely wrong with the git branches, I committed this to the 1086958-file-entity branch (it's listed on the git instructions for media module) before noticing it. I know squat about git as well so don't know what to do about this.

I'll add the display attribute to the styles patch as well and move it to the Styles forum. I'll crosslink once done.

Edit: The Styles patch is located in the Styles forum: http://drupal.org/node/1094016#comment-4214802

Edit 2: Damn, I'm sorry cocoloco, I misspelled your name.

lelizondo’s picture

@oxyc just merge the two branches. I know about the tarball not being generated but some of us can clone the project.

If you're having problems with git, I'm not an expert, but I think I can help. I stop using CVS about a year ago and started to use git. The best advice I got is forgetting everything I learned from CVS.

Edit: I don't see any commit made to the 1086958-file-entity branch.

justafish’s picture

This is working for me, except when I turn the images into links and then they display as blocks on separate lines

JacobSingh’s picture

I fixed the branch in the git instructions. I think that happened by default since we moved to the 7.x branch.

Best,
Jacob

mcaden’s picture

I applied #30 and it seems to work great except that my image gets put inside divs.

<div class="field field-name-file field-type-file field-label-hidden">
  <div class="field-items">
    <div class="field-item even">
      <span id="styles-1-0" class="styles file-styles original">  
        <img class="media-image" style="float:left;" typeof="foaf:Image" src="path/to/image">
      </span>
    </div>
  </div>
</div>

Is there a patch I'm missing or have I set something up wrong?

EDIT: This is on media dev and styles dev.

oxyc’s picture

Currently you have to override the field.tpl.php template file to strip the extra markup. You can read more about the discussion here: http://drupal.org/node/1084082#comment-4218840

mcaden’s picture

Why is Media using fields if it's inside the body and not a separate field? Is this a styles issue?

Weird that it works fine on the wysiwyg.

EDIT: I've looked around a bit. By skimming the code it's looking like the answers to my question above is: A) It can't tell the difference between field and otherwise...filter would have to use a different method to render and B) WYSIWYG plugin strips the outer div elements via javascript. Sound about right?

JulioGuedes’s picture

justafish’s picture

patch in #30 is missing a comma after 'display'

$allowed_tags = array('width', 'height', 'float', 'display'
        'margin-top', 'margin-bottom', 'margin-left', 'margin-right', 'margin',
        'border-width', 'border-style');
mcaden’s picture

Oh yeah, I forgot I intended to mention that in my comment #36.

lelizondo’s picture

Here is the patch with the error in #38 fixed.

JacobSingh’s picture

Where is the corresponding issue in the filestyles queue which makes this work?

lelizondo’s picture

David_Rothstein’s picture

The regexp change looks like a good improvement (seems like it will likely fix #841116: Media filter sometimes sets incorrect image widths).

I'm not sure I understand the hardcoded whitelist of style attributes (and why it's at this point in the code)... The previous code just dealt with 'width' and 'height' because those are actually attributes, but why would we want to pull other things out of 'style' (that aren't attributes) too?

Also, with Styles 7.x-1.x this used to work fine (no whitelist needed at all), so it seems to me that #1094016: Allow Wysiwyg styles is actually the main bug here.

If we want to add a whitelist to the media filter, ideally it should be possible to turn it off. For example, if you're using the media filter along with something like HTML Purifier, or with the Full HTML format (for trusted admins), then I don't think you really want it to be limiting the style attributes you can use at all.

DerekAhmedzai’s picture

The final style seems to fail if the attribute is missing a final semi-colon (which is how CKEditor generates the style anyway).
E.g., the following float will be ignored

width:200px;height:100px;float:left

Adding the semi-colon seems to fix it.

      if (isset($settings['style'])) {
+        // add trailing semi-colon
+        if (substr($settings['style'], count($settings['style'])-2) != ';') {
+          $settings['style'] .= ';';
+        }
JacobSingh’s picture

Sorry I didn't post a patch today. I started a branch for this. But it's not working yet because of styles weirdness I think.

Also, we'll need to mess w/ the wysiwyg JS file since it has hardcoded stuff looking for height and width... ugh.

Anyway, 1043570-float_images

contains most of the original patch, but a little more concise and moved the hardcoded list to a variable. This is fundamentally a styles issue and needs to be fixed there.

mcaden’s picture

I'm still sorta stuck trying to figure out a decent solution for #35 - There should be a way to output this into the WYSIWYG without including the field containers.

lelizondo’s picture

Actually, there is one. You could use a filter to remove all the HTML tags, but I'm sure that will have some non wanted effects.

mcaden’s picture

I assume you mean just the field-added tags. Yeah, that's one method but it feels dirty. I think it should be able to be done through the field api to grab the content of the field without theming it. I'm not sure how this would be done but I haven't had much chance to actually research it.

There's obviously a way to do it because in the edit screen the field template stuff doesn't show up. It only shows up when drupal tries to theme it.

lelizondo’s picture

Perhaps someone who knows more about the Field API could tell us how to do it. Currently the Media module does remove the label, I don't know if it could remove all the

's as well.
mcaden’s picture

Nvm, I totally forgot in #48 that I had gotten that far in my research and I found that when it's added to the wysiwyg the container divs added via the field template are removed via javascript. So nothing different is used in the wysiwyg as far as the field goes.

mcaden’s picture

Got it.

In function 'media_get_file_without_label' in file 'medie.filter.inc' add the following line right before the return:

unset($field_view['#theme']);

This plus patch #40 and VOILA! I get left/right alignment without the field container divs inside my body. This will only affect images added through the media filter and will not affect the Media fields that are separate. This way you can still theme your fields and at the same time have images in the body of your node without extra container divs.

lelizondo’s picture

Can you please align the image to the center using the Text align button in the editor? That might actually solve #1084082: Center aligning images in WYSIWYG CKEditor doesn't work too

mcaden’s picture

When I use the text align center button it centers it just fine after my fix in #51.

lelizondo’s picture

Status: Needs review » Needs work

Great. I'll create a patch then.

lelizondo’s picture

Status: Needs work » Needs review
FileSize
2.16 KB

Here it is. I incorporated comment #51 and #44 and its working. Even #1084082: Center aligning images in WYSIWYG CKEditor doesn't work is fixed now. I'm marking it as a duplicate.

Patch was made against branch 1043570-float_images

One more thing, here is the order of Filters I'm using, so anyone can test under the same circumstances.

Convert URLs into links
Converts Media tags to Markup
Convert line breaks into HTML (i.e. <br> and <p>)
Correct faulty and chopped off HTML
Thomas Bosviel’s picture

subscribe

tomgf’s picture

subscribing

camdarley’s picture

I applied #55 and my image is still outside the <p style="text-align: center; "> (because of the <div> in field-file.tpl). And my image is not in center.
Maybe i did something wrong but without overwriting field-file.tpl with <span>, http://drupal.org/node/1084082 is not fixed

lelizondo’s picture

do you have the filters exactly as I said?

mcaden’s picture

@camdarley - got a site you can show? I've got multiple sites and this fixes all.

anavarre’s picture

Subscribing

thumb’s picture

Subscribe

lnovell30’s picture

I cant' find that the patch above fixed anything. In tinyMCE or CKEditor.... the result is the same.... the alignment of images/application of styles does not persist!

lelizondo’s picture

Can you post how are your filters ordered?

lnovell30’s picture

FileSize
23.48 KB

Thanks alot for your help lelizondo!

I've attached an image of the order of the text formats/filters

Also, just to confirm:
I using media 7.x-1.0-beta3 patched with #55 from above
Wysywyg "7.x-2.0"
styles "7.x-1.0-alpha4"
and either CKEditor 3.52 or Tiny MCE 1.4.1
After having added any image via media browser, and adjusting the size, horizontal position, nothing is actually saved...

I really appreciate your help!

lnovell30’s picture

FileSize
8.54 KB

Forgot to add the actual input filter order

lnovell30’s picture

Ok.

I think that I finally found out what the issue might be...

If I use the media browser control in CKEditor or TinyMCE it does not persist the style markup.
However: If I use the image control, the style properties are persisted....

The dilema however, is that contributors who are not html centric, are not going to know where to find the image via a url, which is what is presented in the image dialog box. The media browser, of cource, allows you to select either from an upload or from the library or even web, which is much easier for a contributor to work with.
My question, and perhaps this should be a part of a different 'issue track' is why the media browser behaves, in it's saving behavior, differently from that of the image system.

I understand that the tags are different underneath, but if the media browser is going to be the preferred way to browse for media for inclusion into rich text via wysiwyg editors in drupal, why would there be a significant difference in its final rendering.

mcaden’s picture

Are you using the patch from #1094016: Allow Wysiwyg styles?

idflood’s picture

subscribe

tim.plunkett’s picture

sub

lelizondo’s picture

Status: Needs review » Reviewed & tested by the community

can we mark this as RTBC?

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Reviewed & tested by the community » Needs review

If you have to ask, the answer is probably no :)
Especially since the last patch was yours...

I'll test this tomorrow.

aaron’s picture

the approach looks good and the code looks solid. i'll see if i can test later today as well, if someone else hasn't. thanks for the hard work, everyone!

anthonyR’s picture

Subscribe

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

This only works in conjunction with #1094016-10: Allow Wysiwyg styles, and only for some definitions of "works".

If I set the width, height, and float, this is what is output: img width="300" height="100" style="float: right; width: 300px; height: 100px; ;;float:right;;float:right;". Obviously not ideal.

roborn’s picture

subscribe

lelizondo’s picture

Status: Needs review » Needs work

@tim.plunkett then I guess this needs work.

mcaden’s picture

hmm...not what I get at all. I get:

<img width="100" height="100" class="media-image" style="float:left;" typeof="foaf:Image" src="http://www.example.com/sites/default/files/styles/large/public/test.png">

Maybe styles or media has changed since the last time I updated though?

JacobSingh’s picture

Looking forward to seeing what's happening here :) I'll be actively reviewing this issue this week, so please don't be shy about posting patches.

rootwork’s picture

Subscribe (and huge +1 on all the hard work above!)

dddbbb’s picture

Subscribing.

I'm only concerned with centering media at the moment but float left/right would obviously be a big bonus.

lelizondo’s picture

Centering should be working by now.

dddbbb’s picture

Do you mean in dev or in the patch above?

discipolo’s picture

for me adding @[style] to Allowed HTML tags made all the difference.

lelizondo’s picture

@dixhuit with the patch above. The topic was discussed here http://drupal.org/node/1084082 but they were merged here.

7wonders’s picture

sub

dddbbb’s picture

The patch in #55 worked great for me. Images now seem to respond to the following CSS to centre images:

#your-content-body img {
  display: block;
  width: auto;
  margin: 0 auto;
}

Any idea if/when this patch will be commited to dev?

EoinPaganus’s picture

I upgraded to the latest releases of styles and media and then applied #1094016: Allow Wysiwyg styles this has worked for me.

If I look at the source I am getting style="float: right; ;;" but I can live with that if the images are in the right place.

I'm using

Media 7.x-1.x-dev (28/4)
Styles 7.x-2.x-dev (12/3)
Wysiwyg 7.x-2.0
CKeditor 3.5.2.6450

lelizondo’s picture

I think the problem with this patch is that although it works, is not an elegant solution, I believe that's why it hasn't been committed by the maintainers and it has this status.

effulgentsia’s picture

Version: 7.x-1.0-beta3 » 7.x-1.x-dev

Does #55 solve something that isn't solved by #1094016-10: Allow Wysiwyg styles alone? If so, can someone please summarize what (or link to the relevant comments)? If not, any objections to closing this as a duplicate? Thanks.

lelizondo’s picture

This is no duplicate. Both patches work together to make this work. The reason for this marked as "needs work" is because of http://drupal.org/node/1043570#comment-4317850

Originally the patch for Styles was here also (see #25), but @oxyc moved the patch where it belongs and I kind of lost track of that issue, but unless a patch in Styles solves this in a very elegant way, I don't think this is a duplicate, specially because this patch removes the markup that prevents images from floating.

effulgentsia’s picture

@lelizondo: are you sure? what markup do you get when you use the latest Media dev snapshot and Styles with just #1094016-10: Allow Wysiwyg styles applied? BTW, please read the top of the Media project page for warnings about using the dev snapshot on a production site.

lelizondo’s picture

I haven't used this patch in a few weeks, but from looking at the code in both patches, without this one, you'll get the field label and some divs that when using a wysiwyg editor will prevent the image from floating.

About the warning, I haven't visited the media project page in a while so I didn't know. Maybe this was solved in another way. I'll test it out.

mikeker’s picture

FileSize
3.04 KB

I hope I'm not further confusing this issue by submitting a patch that takes a different approach to things.

First off, I don't believe that #55 is required to make this work... At least not any more -- it looks like the latest from the 1043570-float_images includes a more abstracted version of what @lelizondo posted, though I couldn't find a post here to that effect.

Also, the crazy number of branches for this project makes it really hard to figure out where to do dev work. The 7.x-1.x branch seems woefully far behind the others which means only the Git-enabled will be able to test these changes.

  • This patch is based on the 1043570-float_images branch with no other patches applied. Hopefully that is a recent enough branch...
  • It does require this patch in Styles. (NOTE: that patch never applied correctly for me (built in CVS?) so I'll post what worked for me on that thread).
  • This patch adds the classes media-image-left or media-image-right to the img tag which gives themers more control over margins around images when floated left vs. right. The V/Hspace options used by CKEditor are deprecated and don't offer the control that margin/padding CSS directives do.
  • The style="float: left/right;" remains on the img tag so that the WYSIWYG editors will pick on the settings. The downside is that themers will not be able to change the float value in CSS, but they couldn't previously so I doubt there will be complaints.
  • I added a small media-image.css file to Media so that the media-image-left/right classes would have some default values. However, because filters are cached, I couldn't find an appropriate place to add drupal_add_css or ['#attached']['css'] so I added it to the modules .info file. See my comments in media.filter.inc, which should be removed before committing...

I think the last item is a bad idea and would appreciate someone with more experience on filter caching and attaching CSS files to chime in with a better way of doing it. Alternatively, we could leave the CSS file out altogether as the style attribute will float the image and themers can add margins as needed.

idflood’s picture

#94 . I don't have time to test the patch now, but I wanted to say the 'style="float: left/right;"' should not cause issue to themes. It can theoretically be changed like that:

.media-image-left{
  float: none !important;
}
Shadlington’s picture

Subbing

jpstrikesback’s picture

subadubdubtenpatchesinatub

mikeker’s picture

Status: Needs work » Needs review

Sorry, meant to bump this to needs review when I posted the patch.

Status: Needs review » Needs work

The last submitted patch, 1043570-classes.patch, failed testing.

thijsvdanker’s picture

subscribe

mikeker’s picture

Status: Needs work » Needs review

The patch fails because the testbot tries to apply it to the 7.x-1.x branch, which is way behind 1043570-float_images branch, which is what the patch is based on. Bumping back to needs review...

mcaden’s picture

#94: 1043570-classes.patch queued for re-testing.

EDIT: Oops, re-queued before realizing it was pointless.

Status: Needs review » Needs work

The last submitted patch, 1043570-classes.patch, failed testing.

lelizondo’s picture

@effulgentsia I finally had time to test this and I'm using the latest dev version of Media and Styles 7.x-2.0-alpha5 and it works.

I didn't not apply the patch for Styles in http://drupal.org/node/1094016#comment-4294582 I don't know if it already has been applied in that version or if it's even needed.

Dates and Versions of the Dev versions
Media 2011-May-19
Styles 7.x-2.0-alpha5

thumb’s picture

@lelizondo, I just installed the versions you list over the patched version, but floats to the right do not work for me.

lelizondo’s picture

@thumb I tested them with Ckeditor and it worked for me. Didn't test with TinyMCE

thumb’s picture

Hrm, yeah, I'm using CKeditor, but right floats are being stripped while left floats worked. I ended up re-applying the patch to get right floats working again. I guess I'll wait for the next release.

rootwork’s picture

Is it likely this patch will be applied to dev soon? I'm just getting a little befuddled by all of the intersecting patches-on-patches, which still are needed, which version of the module to apply them to, etc.

The general feedback seems to be that this patch is (at least partly) successful, and given that we're just talking about a dev and not a full release, it would be nice to see it rolled in to dev for further testing.

lelizondo’s picture

Status: Needs work » Closed (duplicate)

The patch is not needed anymore as long as you have the latest version of Media. At least it wasn't for me. If somebody else disagrees, then please reopen this issue. I'm closing it so nobody is confused anymore.

thumb’s picture

Status: Closed (duplicate) » Active

I'd like to see additional confirmation before closing. Can anyone else confirm?

bryancasler’s picture

How can I confirm this? I have a clean install with media dev running on it and I'd be willing to test it out.

thumb’s picture

Install the module versions previously mentioned by @lelizondo

Dates and Versions of the Dev versions
Media 2011-May-19
Styles 7.x-2.0-alpha5

Try floating images to the left and to the right using CKEditor. Verify that both work.

bryancasler’s picture

Seems to work for both left and right floats.
http://imgur.com/WXuW4

Output

<img alt="" class="media-image" style="float: right; " typeof="foaf:Image" src="http://localhost/sandboxsite1/sites/default/files/styles/large/public/5G4UY.jpg">

UPDATE To get it to float left I had to float it right first.
UPDATE2 Having to float left first seems to only be needed some of the time. Investigating more.

thumb’s picture

Yes, it looks like it works in the editor window, but do the floats work when you save and view the node?

bryancasler’s picture

Yes, but it's acting quirky. I'm updating my post in #113 as I try to figure out what's going on.

lelizondo’s picture

Also, please verify that you can center images. it was also a bug related to this issue but the title doesn't mention it.

stoltoguzzi’s picture

does not work for me
as soon as I save the floats are gone...

lelizondo’s picture

Can you post your filters order?

mikeker’s picture

Using Styles 7.x-2.0-alpha5 (no patches) and a git clone of Media's 7.x-1.x branch (should be equivalent to today's -dev release) I was able to float images left or right and have them appear correctly in both CKEditor and the saved page. I didn't see any of the left-then-right issues reported in #113. I'm using the stock "Full HTML" input format with the Media filter added to the top of the list.

I've posted the addition of media-image-left/right in a new issue as I think it's still a useful addition for theming: #1169192: Add classes to images floated left or right

mikeker’s picture

PS: Sorry to pick nits, but "closed (fixed)" might be a better status for this issue once the fix is verified, since something was changed in the Media/Styles code to support this...

johannesdr’s picture

Hi mikeker. Did you change any other options? Because I can't get it to work.
I used Styles 7.x-2.0-alpha5 and Media 7.x-1.x-dev (2011-May-26 release).
In the CKEditor (3.5.3.6655) the image is floated correctly, but when I save the page the float disappears.
I tested it in filtered HTML and full HTML.
Can anyone else confirm that the latest Media 7.x-1.x-dev release fixes the problem? Or should I also get the dev release of Styles?

Media tag:
[[{"type":"media","view_mode":"media_large","fid":"3","attributes":{"class":"media-image","style":"float: left;","typeof":"foaf:Image"}}]]
Resulting image tag:

<img src="/sites/default/files/styles/large/public/temp-portf3.png" typeof="foaf:Image">
mikeker’s picture

I just tried it on another dev machine and the floats are NOT working... (grumble, grumble...)

The only difference off the top of my head with your config and the one that worked for me is that I was using CKEditor 3.6 (which requires the May 5 or greater -dev release of WYSIWYG to properly recognize the version number). But I just upgraded both of those and it's still not working.

The config I got things working on was a pretty clean setup. I'll try it again later this afternoon and report back...

lelizondo’s picture

@mikeker I was using WYSIWYG dev version 2011/01/23 and CKEditor 3.3.1 when it worked for me.

protools’s picture

Floating images within text has been a requirement for a great number of people on the internet for many years now.

And with this module potentially being the best solution in drupal for inserting images within text (potentially a very large user base) I would say that at least float left and right would be base functionality that people would expect.

People coming to this module from any other wysiwyg image insertion methods would expect this functionality to exist.

i think functionality like this http://drupal.org/files/issues/wysiwyg_imagefield_float.png
ore http://drupal.org/node/342674
must be in core Media module

for and user is too hard upload image in one way, and set floating in another way (Ckeditor ore other ...) Housewife - does not evaluate hard interface ....

guys support me and vote for!

lelizondo’s picture

WYSIWYG Imagefield is probably the easiest way to upload and manipulate images form the user perspective. The only disadvantage is that the images are not fields or nodes.

mikeker’s picture

OK, I just tried everything from a squeaky-clean install and it works. Here's what I did.

From the command line:

$ drush dl
Project drupal (7.2) downloaded to d:/documents/dev/sandbox/drupal-7.2.                                        [success]

Project drupal contains:                                                                                       [success]

 - 3 profiles: testing, standard, minimal
 - 4 themes: stark, seven, garland, bartik
 - 47 modules: drupal_system_listing_incompatible_test, drupal_system_listing_compatible_test, user, update,
trigger, translation, tracker, toolbar, taxonomy, system, syslog, statistics, simpletest, shortcut, search,
rdf, profile, poll, php, path, overlay, openid, node, menu, locale, image, help, forum, filter, file,
field_ui, text, options, number, list, field_sql_storage, field, dblog, dashboard, contextual, contact,
comment, color, book, blog, block, aggregator

$ mv drupal-7.2 drupal7

<...Install stock Drupal7 site via web browser...>

$ cd drupal7/sites

$ drush dl ctools styles wysiwyg-7.x-2.x-dev media-7.x-1.x-dev
Project ctools (7.x-1.0-alpha4) downloaded to d:/documents/dev/sandbox/drupal7/sites/all/modules/ctools.       [success]

Project ctools contains 9 modules: views_content, stylizer, page_manager, ctools_plugin_example, ctools_custom_content,
ctools_ajax_sample, ctools_access_ruleset, bulk_export, ctools.
Project styles (7.x-2.0-alpha5) downloaded to d:/documents/dev/sandbox/drupal7/sites/all/modules/styles.       [success]

Project styles contains 3 modules: styles_ui, file_styles, styles.
Project wysiwyg (7.x-2.x-dev) downloaded to d:/documents/dev/sandbox/drupal7/sites/all/modules/wysiwyg.        [success]

Project media (7.x-1.x-dev) downloaded to d:/documents/dev/sandbox/drupal7/sites/all/modules/media.            [success]

Project media contains 3 modules: media_internet, file_entity, media.

$ drush en styles wysiwyg media
The following extensions will be enabled: styles, wysiwyg, media, ctools, file_entity
Do you really want to continue? (y/n): y
ctools was enabled successfully.                                                                                    [ok]

file_entity was enabled successfully.                                                                               [ok]

media was enabled successfully.                                                                                     [ok]

styles was enabled successfully.                                                                                    [ok]

wysiwyg was enabled successfully.                                                                                   [ok]

$ cd all

$ mkdir libraries

$ cd libraries/

$ wget http://download.cksource.com/CKEditor/CKEditor/CKEditor%203.6/ckeditor_3.6.tar.gz
<...snip...>

$ tar -zxvf ckeditor_3.6.tar.gz
<...snip...>

$ rm ckeditor_3.6.tar.gz

Then from the Web browser:

http://local.sandbox.d7/admin/config/content/wysiwyg
	- Set Full HTML to CKEditor 3.6.0.6902 and Save
	- Click edit to configure and add a few buttons plus the Media Browser and click Save.

http://local.sandbox.d7/admin/config/content/formats
	- Configure the Full HTML format
	- Add the "Converts Media tags to Markup" filter and move it to the top of the list
	- Save Configuration

http://local.sandbox.d7/node/add/page
	- Switch input format to Full HTML
	- Add a title and some text
	- Click the Media browser button and upload an image
	- right click on the image and set Alignment to Right
	- Save.

I end up with a lovely image floated to the right. Resulting HTML is

<img alt="" class="media-image" style="float: right; " typeof="foaf:Image" src="http://local.sandbox.d7/sites/default/files/styles/large/public/_MG_4415.jpg">
protools’s picture

@lelizondo I say: floating must be in core Media module, nothing more

mikeker’s picture

@luk911: I appreciate your passion, but your energy would be better spent testing the configurations @lelizondo or I have specified to ensure that this is fixed. There's obviously some small config option that is causing this to not for for some people. Getting to the bottom of that will ensure floating images will work with Media/WYSIWYG/CKEditor.

evanmwillhite’s picture

Honestly, I can't tell if this will help or not, but I am noticing that images run through a '3rd party' style (Colorbox) are keeping those inline styles intact, but ones run through using a default File Style are not.

protools’s picture

@mikeker I don't want to test something that confuses end-users, it's great that the guys did it, but I want the interface to be truly user Friendly :) must be in core Media module

protools’s picture

see to the facebook, see to the apple ... is all user Friendly

protools’s picture

for whom we ultimately do all this? for housewives! Housewife needs to understand how to do it and how to manage it! and all this should be simple, understandable, it is not for programmers
read http://drupal.org/node/1043570#comment-4521914

yareckon’s picture

luk911, you are confusing the goal (lots of features, easy to use for everyone) with the process (painstakingly testing and improving code). It doesn't help if you hit the issue queue to tell us repeatedly it all has to "just work." When things Just work, it's not because of magic gnomes, or people saying "Fix it!" over and over, it's because of the type of continual participatory refinement taking place in threads like this one.

Media is a new module that tries to achieve the holy grail. Because it does so, it has a lot of bugs. Because it's also the most watched media management solution for Drupal 7, things are moving fast in many different directions which makes things confusing as different user needs pour in willy nilly. There is a definite tradeoff between having a simple, bug free module and a complex featureful one. On top of that, the usebase is split between early adopting hackers, and folks who want things to Just work. It's a tough situation to steer, and folks are going a good job moving the marble forward every day in the issue queue. My hat is off to the commiters on this suite of modules.

luk911, you clearly understand the goal of the module to be refined and wonderful to use, but you're not helping anyone just showing up to preach that it must be bug free or that feature X must work (we all know that).

jherencia’s picture

Maybe this sytles' issue #1170736: Better compatibility with media filter (WYSIWYG related) helps in some way.

protools’s picture

@yareckon unfortunately I'm not programmer, but usability testing is also spending money and resources. I just want to say that efforts could be directed not there? if it is in the core and then will not have this problem, how to tie floating from another editor ...

rootwork’s picture

@luk911 Everyone on this thread agrees that this issue needs to be addressed. Several people are actively trying to address it. If you're not able to contribute to a) addressing it through programming, or b) testing its usability, (or perhaps c) financially funding a programmer's time to fix it more quickly) then there is no need for you to post further on this thread; you can simply follow along and wait for it to be fixed. We all recognize that this is an important issue. Please don't post just to remind us of that again.

rootwork’s picture

@mikeker #126 I just tried your exact steps and got the same successful result. And noting that in my case, the latest version of Styles was alpha6.

I'm going to play around with the install I have that wasn't working before now, and see if I can get it to work.

lelizondo’s picture

I think the best way to test this is with a fresh site, install only the required modules, Media, WYSIWYG and Styles (and any dependencies) with the latest version of CKEditor and possibly TinyMCE and test the following:

1. Float images to the left
2. Float images to the right
3. Center images

Please do not install any other modules that could possibly break this functionality so we can move on.

protools’s picture

@rootwork all trying to solve through integration with other editors using CK Editor ore other ... And I say that this functionality should be independent of them, and is built directly into the module http://drupal.org/files/issues/wysiwyg_imagefield_float.png

protools’s picture

FileSize
30.66 KB
rootwork’s picture

@luk911 Ah, I think I understand what you're saying now. You're pointing out that at the moment, you have to insert the image, and then right-click/control-click it to get to Image Properties, and then set the alignment (and other details). Whereas with the other modules, you can do that when you're uploading an image.

Yes, I think that's a good point, and I agree that ultimately it would make more sense to have that built in to the image-upload interface.

That said, this issue is about fixing the *functionality* of floating things left and right. Until that is fixed, it's unlikely anyone will be working on making the UI more friendly. However, I'd encourage you to see if there's a feature request for this already (and add a supportive comment) or open a new issue for that so you can track its development and offer feedback.

rootwork’s picture

This is now working on the site I originally experienced the issue on. It has a whole bunch of modules that probably make it a bad candidate for testing -- but for reference, I will say that it started working after I:

- upgraded Styles to the latest (a6)
- upgraded CKEditor to the latest (3.6)
- made this tiny patch to WYSIWYG to allow it to work with CKEditor 3.6

In retrospect, I should have tested it after the Styles upgrade, before I messed with CKEditor. Sorry about that. But my suggestion to others would be to start with upgrading Styles and see if that fixes it.

johannesdr’s picture

I can also confirm that I got it working on a clean install with
styles 7.x-2.0-alpha6, wysiwyg-7.x-2.x-dev, media-7.x-1.x-dev and CKEditor 3.6

thumb’s picture

Success, at least so far. Just installed Styles alpha 6, running the latest Media development version, and not exactly a clean install. Left and right floats are working for me now without the patch.

thumb’s picture

Ugh, double post. Sorry!
Success, at least so far. Just installed Styles alpha 6, running the latest Media development version, and not exactly a clean install. Left and right floats are working for me now without the patch.

rvilar’s picture

subscribing

rootwork’s picture

I'm afraid Styles alpha7 has broken this. It was working with alpha6; I installed alpha7 and it all stopped working.

Can someone else confirm that it's broken after upgrading to a7? (I recommend not on a production site...)

rootwork’s picture

Project: D7 Media » Styles
Version: 7.x-1.x-dev » 7.x-2.0-alpha7

I downgraded it to alpha6 and it fixed it. So I'm pretty sure alpha7 breaks it. I did a diff between the versions and tried commenting out different parts of what was added to see if I could identify what did it, but I couldn't seem to (short of commenting out all of the new changes, which doesn't identify anything).

I'm moving this to the Styles queue for now so we can try to get the problem in alpha7 addressed.

aaron’s picture

this fixes it, at least as far as styles is concerned. tested against media 7.x-1.0-alpha4 and 7.x-1.x.

aaron’s picture

Project: Styles » D7 Media
Version: 7.x-2.0-alpha7 » 7.x-1.x-dev

setting back to the media module, in case this needs more work when not using styles. the patch at #149 has been committed to the styles module. please open another issue against that module if needed. thanks!

bidaking88’s picture

Status: Active » Needs review

media.filter.inc_.float_.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, styles-honor-overrides-1043570-149.patch, failed testing.

protools’s picture

i open new issue for build in floating to interface http://drupal.org/node/1171802#comment-4587394

Agence Web CoherActio’s picture

subscribing

lsolesen’s picture

+1

bryancasler’s picture

Now that we're getting the modal working, bump!

Dave Reid’s picture

Bumping is still considered rude.

mikeker’s picture

Status: Needs work » Fixed

Based on #150, this is fixed... No?

bryancasler’s picture

I wanted to confirm that I am able to float images in (WYSIWYG - CKEditor) that have been inserted via the media module.

Example output

[[{"type":"media","view_mode":"media_small","fid":"22","attributes":{"alt":"","class":"media-image  media-image-right","height":"100","style":"float: right; width: 75px; height: 100px; ","typeof":"foaf:Image","width":"75"}}]]
bryancasler’s picture

[deleted]

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

RobW’s picture

What issue/ patch provided the shortcode additions in #159? I believe it's separate to any work from this thread. Adding both a "media-image-right" class and an inline style "float:right;" isn't the best front end behavior, and I'd like to fix it.

mikeker’s picture

@RobW: I believe you'll need to the code checked into Media as of comment #150 in this thread. If you use Media 2.x anytime after #1169192: Add classes to images floated left or right was committed, you'll get the media-image-left/right classes.

The reason both the class and the inline float is needed is so that some WYISWYG editors behave properly (including CKEditor, as of when I wrote that patch). They rely on the inline float to push the image left or right -- even with using the theme's CSS for the editor, I couldn't get it to use floats associated with the media-image-left/right classes.

Agreed, not ideal, but best for the general use scenario.

RobW’s picture

@Mikeker, thanks for the info. I'll see if I can fix the WYSIWYG class problem so that we can discard the inline styles.