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.
Comment | File | Size | Author |
---|---|---|---|
#149 | styles-honor-overrides-1043570-149.patch | 2.55 KB | aaron |
#140 | media_float.jpg | 30.66 KB | protools |
#94 | 1043570-classes.patch | 3.04 KB | mikeker |
#66 | filterorder.png | 8.54 KB | lnovell30 |
#65 | Textformats.png | 23.48 KB | lnovell30 |
Comments
Comment #2
Jackinloadup CreditAttribution: Jackinloadup commentedThis 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!
Comment #3
rnyberg CreditAttribution: rnyberg commentedI 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! :)
Comment #4
matrobin CreditAttribution: matrobin commentedIn 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:
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:
Comment #5
travismiller CreditAttribution: travismiller commentedI 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!
Comment #6
cocoloco CreditAttribution: cocoloco commentedThanks 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.
Comment #7
ogi CreditAttribution: ogi commentedsubscribe
Comment #8
traceelements CreditAttribution: traceelements commentedSubscribing...
Comment #9
Kreativs CreditAttribution: Kreativs commentedsubscribe
Comment #10
JacobSingh CreditAttribution: JacobSingh commentedI 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...
Comment #11
rnyberg CreditAttribution: rnyberg commentedYou 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. ^^
Comment #12
aaron CreditAttribution: aaron commentedcrud, i thought i had all this working in v2 of styles. i'll need to take another look. thanks for the work towards this!
Comment #13
discipolo CreditAttribution: discipolo commentedWhat 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.
Comment #14
Mac Ryan CreditAttribution: Mac Ryan commentedsubscribe
Comment #15
grandcat CreditAttribution: grandcat commentedsubscribe
Comment #16
kirkofalltrades CreditAttribution: kirkofalltrades commentedSubscribed. 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.
Comment #17
mcaden CreditAttribution: mcaden commentedsubscribe
Comment #18
AlanAtLarge CreditAttribution: AlanAtLarge commentedSubscribing
Comment #19
Bios Element CreditAttribution: Bios Element commentedI 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.
Comment #20
bryancasler CreditAttribution: bryancasler commentedsubscribe
Comment #21
baronmunchowsen CreditAttribution: baronmunchowsen commentedI 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.
Comment #22
bryancasler CreditAttribution: bryancasler commentedComment #23
aaron CreditAttribution: aaron commentedyes, #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.
Comment #24
camdarley CreditAttribution: camdarley commented(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.
Comment #25
lelizondo CreditAttribution: lelizondo commentedHere 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:
Comment #26
lelizondo CreditAttribution: lelizondo commented@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.
Comment #27
camdarley CreditAttribution: camdarley commentedThat's what i thought, so I bring my issue back there: http://drupal.org/node/1084082
Comment #29
lelizondo CreditAttribution: lelizondo commentedI think the patch will fail because is from another module, but I don't really know how the automated tests work.
Comment #30
oxyc CreditAttribution: oxyc commentedI added the filter to allow the
display
attribute as well, this is because TinyMCE usesdisplay: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.
Comment #31
lelizondo CreditAttribution: lelizondo commented@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.
Comment #32
justafishThis is working for me, except when I turn the images into links and then they display as blocks on separate lines
Comment #33
JacobSingh CreditAttribution: JacobSingh commentedI fixed the branch in the git instructions. I think that happened by default since we moved to the 7.x branch.
Best,
Jacob
Comment #34
mcaden CreditAttribution: mcaden commentedI applied #30 and it seems to work great except that my image gets put inside divs.
Is there a patch I'm missing or have I set something up wrong?
EDIT: This is on media dev and styles dev.
Comment #35
oxyc CreditAttribution: oxyc commentedCurrently 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
Comment #36
mcaden CreditAttribution: mcaden commentedWhy 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?
Comment #37
JulioGuedes CreditAttribution: JulioGuedes commentedComment #38
justafishpatch in #30 is missing a comma after 'display'
Comment #39
mcaden CreditAttribution: mcaden commentedOh yeah, I forgot I intended to mention that in my comment #36.
Comment #40
lelizondo CreditAttribution: lelizondo commentedHere is the patch with the error in #38 fixed.
Comment #41
JacobSingh CreditAttribution: JacobSingh commentedWhere is the corresponding issue in the filestyles queue which makes this work?
Comment #42
lelizondo CreditAttribution: lelizondo commented@JacobSingh here it is: http://drupal.org/node/1094016#comment-4214802
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #44
DerekAhmedzai CreditAttribution: DerekAhmedzai commentedThe 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
Adding the semi-colon seems to fix it.
Comment #45
JacobSingh CreditAttribution: JacobSingh commentedSorry 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.
Comment #46
mcaden CreditAttribution: mcaden commentedI'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.
Comment #47
lelizondo CreditAttribution: lelizondo commentedActually, 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.
Comment #48
mcaden CreditAttribution: mcaden commentedI 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.
Comment #49
lelizondo CreditAttribution: lelizondo commentedPerhaps 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
Comment #50
mcaden CreditAttribution: mcaden commentedNvm, 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.
Comment #51
mcaden CreditAttribution: mcaden commentedGot it.
In function 'media_get_file_without_label' in file 'medie.filter.inc' add the following line right before the return:
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.
Comment #52
lelizondo CreditAttribution: lelizondo commentedCan 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
Comment #53
mcaden CreditAttribution: mcaden commentedWhen I use the text align center button it centers it just fine after my fix in #51.
Comment #54
lelizondo CreditAttribution: lelizondo commentedGreat. I'll create a patch then.
Comment #55
lelizondo CreditAttribution: lelizondo commentedHere 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.
Comment #56
Thomas Bosviel CreditAttribution: Thomas Bosviel commentedsubscribe
Comment #57
tomgf CreditAttribution: tomgf commentedsubscribing
Comment #58
camdarley CreditAttribution: camdarley commentedI 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 fixedComment #59
lelizondo CreditAttribution: lelizondo commenteddo you have the filters exactly as I said?
Comment #60
mcaden CreditAttribution: mcaden commented@camdarley - got a site you can show? I've got multiple sites and this fixes all.
Comment #61
anavarreSubscribing
Comment #62
thumb CreditAttribution: thumb commentedSubscribe
Comment #63
lnovell30 CreditAttribution: lnovell30 commentedI 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!
Comment #64
lelizondo CreditAttribution: lelizondo commentedCan you post how are your filters ordered?
Comment #65
lnovell30 CreditAttribution: lnovell30 commentedThanks 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!
Comment #66
lnovell30 CreditAttribution: lnovell30 commentedForgot to add the actual input filter order
Comment #67
lnovell30 CreditAttribution: lnovell30 commentedOk.
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.
Comment #68
mcaden CreditAttribution: mcaden commentedAre you using the patch from #1094016: Allow Wysiwyg styles?
Comment #69
idflood CreditAttribution: idflood commentedsubscribe
Comment #70
tim.plunkettsub
Comment #71
lelizondo CreditAttribution: lelizondo commentedcan we mark this as RTBC?
Comment #72
tim.plunkettIf you have to ask, the answer is probably no :)
Especially since the last patch was yours...
I'll test this tomorrow.
Comment #73
aaron CreditAttribution: aaron commentedthe 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!
Comment #74
anthonyR CreditAttribution: anthonyR commentedSubscribe
Comment #75
tim.plunkettThis 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.Comment #76
roborn CreditAttribution: roborn commentedsubscribe
Comment #77
lelizondo CreditAttribution: lelizondo commented@tim.plunkett then I guess this needs work.
Comment #78
mcaden CreditAttribution: mcaden commentedhmm...not what I get at all. I get:
Maybe styles or media has changed since the last time I updated though?
Comment #79
JacobSingh CreditAttribution: JacobSingh commentedLooking 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.
Comment #80
rootworkSubscribe (and huge +1 on all the hard work above!)
Comment #81
dddbbb CreditAttribution: dddbbb commentedSubscribing.
I'm only concerned with centering media at the moment but float left/right would obviously be a big bonus.
Comment #82
lelizondo CreditAttribution: lelizondo commentedCentering should be working by now.
Comment #83
dddbbb CreditAttribution: dddbbb commentedDo you mean in dev or in the patch above?
Comment #84
discipolo CreditAttribution: discipolo commentedfor me adding @[style] to Allowed HTML tags made all the difference.
Comment #85
lelizondo CreditAttribution: lelizondo commented@dixhuit with the patch above. The topic was discussed here http://drupal.org/node/1084082 but they were merged here.
Comment #86
7wonders CreditAttribution: 7wonders commentedsub
Comment #87
dddbbb CreditAttribution: dddbbb commentedThe patch in #55 worked great for me. Images now seem to respond to the following CSS to centre images:
Any idea if/when this patch will be commited to dev?
Comment #88
EoinPaganus CreditAttribution: EoinPaganus commentedI 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
Comment #89
lelizondo CreditAttribution: lelizondo commentedI 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.
Comment #90
effulgentsia CreditAttribution: effulgentsia commentedDoes #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.
Comment #91
lelizondo CreditAttribution: lelizondo commentedThis 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.
Comment #92
effulgentsia CreditAttribution: effulgentsia commented@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.
Comment #93
lelizondo CreditAttribution: lelizondo commentedI 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.
Comment #94
mikeker CreditAttribution: mikeker commentedI 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.
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 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.
Comment #95
idflood CreditAttribution: idflood commented#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:
Comment #96
Shadlington CreditAttribution: Shadlington commentedSubbing
Comment #97
jpstrikesback CreditAttribution: jpstrikesback commentedsubadubdubtenpatchesinatub
Comment #98
mikeker CreditAttribution: mikeker commentedSorry, meant to bump this to needs review when I posted the patch.
Comment #100
thijsvdanker CreditAttribution: thijsvdanker commentedsubscribe
Comment #101
mikeker CreditAttribution: mikeker commentedThe 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...
Comment #102
mcaden CreditAttribution: mcaden commented#94: 1043570-classes.patch queued for re-testing.
EDIT: Oops, re-queued before realizing it was pointless.
Comment #104
lelizondo CreditAttribution: lelizondo commented@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
Comment #105
thumb CreditAttribution: thumb commented@lelizondo, I just installed the versions you list over the patched version, but floats to the right do not work for me.
Comment #106
lelizondo CreditAttribution: lelizondo commented@thumb I tested them with Ckeditor and it worked for me. Didn't test with TinyMCE
Comment #107
thumb CreditAttribution: thumb commentedHrm, 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.
Comment #108
rootworkIs 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.
Comment #109
lelizondo CreditAttribution: lelizondo commentedThe 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.
Comment #110
thumb CreditAttribution: thumb commentedI'd like to see additional confirmation before closing. Can anyone else confirm?
Comment #111
bryancasler CreditAttribution: bryancasler commentedHow can I confirm this? I have a clean install with media dev running on it and I'd be willing to test it out.
Comment #112
thumb CreditAttribution: thumb commentedInstall the module versions previously mentioned by @lelizondo
Try floating images to the left and to the right using CKEditor. Verify that both work.
Comment #113
bryancasler CreditAttribution: bryancasler commentedSeems to work for both left and right floats.
http://imgur.com/WXuW4
Output
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.
Comment #114
thumb CreditAttribution: thumb commentedYes, it looks like it works in the editor window, but do the floats work when you save and view the node?
Comment #115
bryancasler CreditAttribution: bryancasler commentedYes, but it's acting quirky. I'm updating my post in #113 as I try to figure out what's going on.
Comment #116
lelizondo CreditAttribution: lelizondo commentedAlso, please verify that you can center images. it was also a bug related to this issue but the title doesn't mention it.
Comment #117
stoltoguzzi CreditAttribution: stoltoguzzi commenteddoes not work for me
as soon as I save the floats are gone...
Comment #118
lelizondo CreditAttribution: lelizondo commentedCan you post your filters order?
Comment #119
mikeker CreditAttribution: mikeker commentedUsing 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
Comment #120
mikeker CreditAttribution: mikeker commentedPS: 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...
Comment #121
johannesdr CreditAttribution: johannesdr commentedHi 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:
Comment #122
mikeker CreditAttribution: mikeker commentedI 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...
Comment #123
lelizondo CreditAttribution: lelizondo commented@mikeker I was using WYSIWYG dev version 2011/01/23 and CKEditor 3.3.1 when it worked for me.
Comment #124
protools CreditAttribution: protools commentedFloating 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!
Comment #125
lelizondo CreditAttribution: lelizondo commentedWYSIWYG 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.
Comment #126
mikeker CreditAttribution: mikeker commentedOK, I just tried everything from a squeaky-clean install and it works. Here's what I did.
From the command line:
Then from the Web browser:
I end up with a lovely image floated to the right. Resulting HTML is
Comment #127
protools CreditAttribution: protools commented@lelizondo I say: floating must be in core Media module, nothing more
Comment #128
mikeker CreditAttribution: mikeker commented@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.
Comment #129
evanmwillhite CreditAttribution: evanmwillhite commentedHonestly, 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.
Comment #130
protools CreditAttribution: protools commented@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
Comment #131
protools CreditAttribution: protools commentedsee to the facebook, see to the apple ... is all user Friendly
Comment #132
protools CreditAttribution: protools commentedfor 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
Comment #133
yareckon CreditAttribution: yareckon commentedluk911, 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).
Comment #134
jherencia CreditAttribution: jherencia commentedMaybe this sytles' issue #1170736: Better compatibility with media filter (WYSIWYG related) helps in some way.
Comment #135
protools CreditAttribution: protools commented@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 ...
Comment #136
rootwork@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.
Comment #137
rootwork@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.
Comment #138
lelizondo CreditAttribution: lelizondo commentedI 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.
Comment #139
protools CreditAttribution: protools commented@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
Comment #140
protools CreditAttribution: protools commentedComment #141
rootwork@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.
Comment #142
rootworkThis 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.
Comment #143
johannesdr CreditAttribution: johannesdr commentedI 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
Comment #144
thumb CreditAttribution: thumb commentedSuccess, 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.
Comment #145
thumb CreditAttribution: thumb commentedUgh, 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.
Comment #146
rvilarsubscribing
Comment #147
rootworkI'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...)
Comment #148
rootworkI 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.
Comment #149
aaron CreditAttribution: aaron commentedthis fixes it, at least as far as styles is concerned. tested against media 7.x-1.0-alpha4 and 7.x-1.x.
Comment #150
aaron CreditAttribution: aaron commentedsetting 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!
Comment #151
bidaking88 CreditAttribution: bidaking88 commentedmedia.filter.inc_.float_.patch queued for re-testing.
Comment #153
protools CreditAttribution: protools commentedi open new issue for build in floating to interface http://drupal.org/node/1171802#comment-4587394
Comment #154
Agence Web CoherActio CreditAttribution: Agence Web CoherActio commentedsubscribing
Comment #155
lsolesen CreditAttribution: lsolesen commented+1
Comment #156
bryancasler CreditAttribution: bryancasler commentedNow that we're getting the modal working, bump!
Comment #157
Dave ReidBumping is still considered rude.
Comment #158
mikeker CreditAttribution: mikeker commentedBased on #150, this is fixed... No?
Comment #159
bryancasler CreditAttribution: bryancasler commentedI wanted to confirm that I am able to float images in (WYSIWYG - CKEditor) that have been inserted via the media module.
Example output
Comment #160
bryancasler CreditAttribution: bryancasler commented[deleted]Comment #162
RobW CreditAttribution: RobW commentedWhat 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.
Comment #163
mikeker CreditAttribution: mikeker commented@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.
Comment #164
RobW CreditAttribution: RobW commented@Mikeker, thanks for the info. I'll see if I can fix the WYSIWYG class problem so that we can discard the inline styles.