I'm not sure if this is a bug or the intended functionality, but when I select text and then click on the IMCE button and select a file, instead of creating a link and using the selected text as link text, it replaces the text with the filename.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mkesicki’s picture

Status: Active » Needs review
FileSize
1.87 KB

I added small patch to improve this behaviour. Now is as @glowsinthedark wanted. I also added full path to url (protocol + host) created by IMCE.
Please check this patch.

Jon Betts’s picture

@michal_cksource - Thank you very much for this!! I've done a few quick tests and for the most part it is working great and is a huge improvement. I haven't tested exhaustively, but I noticed a few things.

If I select some text and click on the IMCE button to select a pdf, the link works as expected. The selected text becomes the link text and the selected file becomes an href for the a tag.

However, when I select a jpg, the image is placed in the editor as opposed to being linked to the text. There are many times when, for instance in a pressroom, one may wish to make available a series of high resolution image of logos, head shots of high mucky-mucks, etc. It appears that the code is looking only for text-based content and linking that.

I'm not sure if this is possible or if it makes sense in all cases, but it seems that if text is selected, then the intention is to link the selected file to the text, no matter what the filetype. If no text is selected, then it could be assumed that the intention is to place the selected file inline the content.

mkesicki’s picture

@glowsinthedark I think that what you wrote in http://drupal.org/node/1198068#comment-4648066 is not what most users expect in that case. When you select a text and insert image in that place you expect that image shows not link. Desktop editors like Open Office do it also as it works now. This is no problem to do it this as you wish, but I have to discuss this topic with our developers. But IMHO this can be confusing for many users.

mkesicki’s picture

@glowsinthedark I created small patch for your needs. Please test it.

Jon Betts’s picture

OK, sounds good! But just to further clarify and as you point out, others may do it differently, but:

1. Selecting text before placing an image seems like an extra step as the selected text has little to do with the image (unless it is text like [insert image here], in which case the text could just be deleted and the cursor position used for placement. Or perhaps one could use it for the alt text, though I haven't determined if that is the way it works currently). Otherwise, I'm typically using the cursor position for image placement. Haven't tried selecting text and then clicking on the image placement button to se what happens. Will give it a whirl.

2. There are going to be times when one is going to want to link to an image for downloading purposes. Currently, this isn't possible without resorting to the link button. It seems like if there is a clear demarkation (select text for link, insert cursor for placing) then it wouldn't be confusing as long as it is consistent.

The advantage to the link and image buttons is you've made it clear how you want to handle the insertion of the selected content based on which button you've clicked. You don't have the clear intent with a single button, but you can mimic intent based on whether you've selected something or not before clicking it. Perhaps there is a better way to do that though.

Thanks for looking into this!

Jon Betts’s picture

Thanks. I tested this (patch #4) out and it doesn't seems to do anything different from the other patch above (#1). If I select text in order to attach an image or any other file for downloading, it inserts the image and blows away the text, but works as expected for non-image files, that is links the selected file. I'll disable the button to avoid client confusion until I hear back from you. I'd be curious as to the use case for having to select text in order to place an image. It just doesn't work that way for non-open source projects (keeping in mind of course we aren't creating a word processor, but a WYSIWYG interface for manipulating HTML, unlike Open Office).

jcisio’s picture

I support #5 even I haven't used IMCE for links, because it sounds good.

If I want to modify alt text or other things for images, I use the Image button (then use Browse Server to open IMCE).

@glowsinthedark: for Hi-Res images, I inserted them directly into the editor, then I use an input filter to replace them by an imagecache (full text-width) version linking to the full version. I can also use smaller imagecache preset by setting class/align in my images, then the filter recognizes it automatically. Of course all those need some coding.

mkesicki’s picture

I think we can add support to http://drupal.org/node/1198068#comment-4648066. So when user select text and adds image with IMCE , image will be linked to text. So for every kind of files it will behave the same. This should works in patch http://drupal.org/node/1198068#comment-4648330, so please check it. If this patch doesn't works as expected I will fix it in free time.
For more advanced text and image processing the way which @jcisio choose in http://drupal.org/node/1198068#comment-4659196 sounds reasonable.

Jon Betts’s picture

@jcisio I do the same thing you suggest in cases where I want a thumbnail, but there are cases where there are multiple versions of an image, etc. where having a thumbnail for each wouldn't be practical or visually interesting.

You bring up an interesting point about alt text. I'm not sure about current behavior, but it might be a good idea to include alt="" on a placed image, if it isn't already. There really isn't a time when you wouldn't want some form of alt tag included with an image.

@michal_cksource thanks, that is great news! I'll try the patch out again. I'm applying the patches manually so I may have missed something in my initial tests.

Jon Betts’s picture

OK, I just retested the patch in #4 on the current (6/22/2011) dev version (7.x-1.x-dev) and it appears to work as requested. Thanks a lot for this!

Also, regarding my comment about the alt tag above, I noticed during testing that the filename is included by default when placing images which seems reasonable so please disregard my comment about it in #9.

Thanks again everyone for your help with this!

dczepierga’s picture

Status: Needs review » Fixed

Ok so i commit patch from #4 to GIT, pls check the last DEV version of CKEditor module.

Greetings

Jon Betts’s picture

Status: Fixed » Needs review

I just checked the last DEV version which appears to include the changes; however, it appears to be broken.

Installed CKEditor DEV version (6/28).

1. Select text in the editor and click IMCE button
2. Double-click on any file in the IMCE browse window. Nothing happens.
3. Click the insert file button once and a # is added to the URL in the browser and nothing happens. Click on the insert file button again and the window closes but there is no linked file.

IMCE works fine if link and image buttons are used.

Anyone else experiencing the same thing?

mkesicki’s picture

Status: Needs review » Postponed (maintainer needs more info)

Hi @glowsinthedark ,
I checked latest dev version and I can't reproduce your issue. Please chceck in firebug console (Firefox add-on) if there are some errors . You can also check in firebug net tab if all request are correct.

dczepierga’s picture

I check this with last DEV version od CKEditor module (2011-Jun-28) and IMCE 7.x-1.4 and for me everything works fine.

Greetings

Jon Betts’s picture

I'm getting the error:
text = sel.getSelectedText();
is not a function.

This is from the latest dev version but I'm downloading it from d.o, not git. message was in firebug console.

mkesicki’s picture

@glowsinthedark, which browser and version of CKEditor library do you use ?
If your version of CKEditor library is < 3.6.1 please try update to it and check again. This behaviour one more time please.

Jon Betts’s picture

Status: Postponed (maintainer needs more info) » Fixed

@michal_cksource that was it! Thanks! I had tested this on a dev server that had 3.6.1 version and it worked fine, but when I moved it over to another server to test, I hadn't yet updated to the latest version of CKEditor. Once I did that, it worked flawlessly. Thanks again.

So to clarify, this feature works under the following circumstances to date:

CKEditor library >= 3.6.1
CKEditor Module 7.x-1.x-dev (6/28)
IMCE 7.x-1.4

mkesicki’s picture

Status: Fixed » Postponed (maintainer needs more info)

@glowsinthedark can you test this in IE6 (sic!). Because I have some problem with it.
Please write me if you have some errors in IE6 or this feature just doesn't work (my case).

mkesicki’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
3.04 KB

I added patch to fix problem with IE6 and to works properly (without bugs) on modules with CKEditor library
earlier than 3.6.1. Please review it and give a note if it works as it should.

Jon Betts’s picture

I downloaded 2011-Jul-04 dev version, applied the patch and tested using ckeditor 3.5.1 and 3.6.1. It didn't work with the older version, worked fine with latest. In other words, it placed the image even if text was selected on the older version. I'll look into it more later. I don't have a good way to test the IE 6 functionality, but I may be able to later tonight.

mkesicki’s picture

This will work (as you wish) only with ckeditor >= 3.6.1. Because it's use some future added in latest CKEditor library.
Using latest CKEditor library is always good idea , because of its new features and bug fixes.
In my latest post I thinking about that it works without errors in earlier versions of CKEditor library, but the link to image will be not inserted in place of text (image will replace selected text).
Hope this clarify how it looks.

Jon Betts’s picture

OK, thanks for clarifying. I wasn't sure if it was supposed to work the same between versions. I've updated all of my libraries and kept an old one around for testing purposes. I didn't notice any errors when using the most recent patch with either version. I still haven't had a chance to test in IE (one of the luxuries with working with Drupal is not having to use IE 6 ;-)) I will test soon.

Jon Betts’s picture

Status: Needs review » Needs work

@michal_cksource ack! This one got away from me, sorry! I finally was able to test in IE and it doesn't work at all. selecting text and then selecting an image replaces the text with the image as before. Additionally, if I select text and then link a pdf using the IMCE button, it replacers the text with the file name. It works fine using other browsers mentioned above. I'm using VMWare on a Mac and the OS is Windows...XP (Pro!). I flipped the status to needs work in case i.e. 6 support is a requirement.

mkesicki’s picture

@glowsinthedark I have just tested this on Windows XP and IE6 and it works as expected (create link to image/file like with Selected Text as place to click ). Maybe someone else will test this ?
Do you have any javascript errors ? Please test latest DEV version.

mkesicki’s picture

Status: Needs work » Closed (fixed)
mkesicki’s picture

Title: Confusing behavior with IMCE button implimentation » Confusing behavior with IMCE button implementation