Popup does not launch for editing existing image

I tested with TinyMCE 3.3b2 with Wysiwyg 5.x-2.x-dev on Drupal 5.21 using FF 3.6 (Ubuntu) and it seems to work fine there as well.
I enabled Wysiwyg, IMCE (latest stable), IMCE Wysiwyg bridge (latest stable) and added Image, Advanced Image and IMCE to the toolbar. I then uploaded a new image, saved the node, edited it again, clicked the image (toolbar button got highlighted), opened the Image dialog and the data I had entered before was still there.

Is the toolbar Image button highlighted in your case? Do you get any errors when the page is loaded or when pressing the button?
The code dealing with this is part of TinyMCE's internal selection handling and not something which would normally be affected by Wysiwyg module.

If you however only added the images with Wysiwyg 2.0, editing them again won't work due to a bug in the TinyMCE implementation layer which added the class 'mceItem' to every image. That part where the class was added has been fixed, but it isn't removed on images which already have it. The class is used to mark TinyMCE's placeholder images internally and should normally not be part of the output. Manually removing the class on images (or re-adding the image with 2.x-dev) will work.

Could you please confirm that your issue only happens when the image was added using 2.0?

From a discussion I just had with sun:
We tag the teaser break image with drupal-content, so the tinyMCE implementatin can add mceItem to prevent the toolbar button from being lit. But we only leave behind <!--break--> in the final source, and so should other plugins.
Well, unless they deal with images which should be possible to edit ;)
tinymce/jscripts/tiny_mce/tiny_mce_src.js line 6671: // Remove internal mceItemXX classes when content is extracted from the editor
Technically, 'mceItem' should be a prefix, which is why it isn't found and removed by the RegExp on the next lines: v = v.replace(/\s?mceItem\w+\s?/g. '');

If I get this right, then the following should fail:

1. Have the two default formats, Filtered HTML + Full HTML.
2. Enable TinyMCE for Full HTML, with image and advimage plugins.
3. Enter the following into the textarea in Filtered HTML:
   

   <img src="/misc/druplicon.png" class="mceItem" />
   

4. Switch to Full HTML. The "Image" button will not highlight (but it should).

Confirmed.
Btw, the advimage plugin is not required for this behavior, just the image plugin.

One can also reproduce it with a single input format if you first disable Rich text editing, paste the above image tag and then enable the editor again. The image will not be editable as TinyMCE now treats it as an internal placeholder.

So are we sure that "mceItem" will never ever appear in the resulting markup when submitting a form? And that we can absolutely safely remove all instances of "mceItem" in the markup initially?

If we are, then the only thing to consider is whether we want to do this on the client-side or the server-side.

I'm very sure that 'mceItem' (as a single word inside a class attribute) will never appear in content generated by TinyMCE.
Since having the class there won't affect the content in any way (except for when inside the editor), I don't think we need to be very aggressive about removing it. I.e, we don't need to screen every node, comment or other place where markup content can appear to get rid of it. We could instead let it be a semi-automated process taking place whenever the content is edited again.

Doing it on the server-side is probably more complex as we're not already doing manipulations on field values. It would be enough to run some kind of client-side filter on what goes into TinyMCE before it is attached to a field. Maybe something like field.value = field.value.replace(/class=(['"].*?)\dmceItem\d(.*?['"])/ig, 'class=$1$2'); in tinymce-#.js' attach()?

In mysql you can use UPDATE table SET column = replace(column, 'mceItem', ''), which would work if you know where the contents are stored.
Note that you might have other classes on the images as well, so replacing just 'mceItem' with '' would be safest. (If you're worried about leaving behind empty class attributes, just purge those with another replace query.)

Running queries like that in an update hook (or a similar place) would probably only catch a fraction of all the places content passed through an editor could be stored in. We'd be spending a lot of time updating the update hook to screen new locations, rather than putting in a less intrusive 'filter' which focuses on fixing the issue just before it would appear, regardless of where the data ends up being stored. (Running queries like that would also take a loooong time on super sized sites).

I'd prefer a method where we don't touch content which doesn't need to be touched, leaving the class there until the content needs to be edited again won't hurt anyone.

Just thought I would chime in that I've been having this same problem ever since I've been using wysiwyg and I'm using Drupal 6. I have not tried the most recent dev version though and I would prefer not to... If this has been fixed in D6 then it would be really nice for it to be committed to the stable release.

Thanks!

Excellent! I came here looking for a fix on this and notice that a 2.1 has been released and that fixed my problems!

Thanks!

@deggertsen, the cause of the problem is fixed in 2.1 so new images won't have this problem, but images inserted with 2.0 may still have the 'mceItem' class which needs to be removed manually. Removing and re-inserting the image will of course work as well. What we're discussing here is how to remove that class automatically in a simple but still effective way.

@TwoD Thanks for the clarification

Should the version be updated to 6.x? I'm seeing the problem there, too.

I'm going to upgrade to 2.1, and use the Scanner module to get rid of the offending tags in existing nodes. I'd prefer a "fix on edit" solution, but my users can't wait for it.

Yes, the version should be bumped as this applies to all versions if images were added with a pre 2.1 release. D6 or D7 doesn't matter, the relevant code is the same.

I like the fix-when-edited approach. It's non evasive and gets the job done. I need a fix on this ASAP too, so I tried to patch tinymce-3.js with some of the code above with no luck. I don't really know if I'm on the right track here. Can someone with more expertise take a look?

This will do the trick. I had made a typo in the RegExp above (\b vs \d).

Thanks! This works perfectly for me. :-)

Committed to all branches.
Thanks for testing, reviewing and commenting.

This fix will soon be in the -dev snapshots and part of the next stable release.

Thanks all!

+++ editors/js/tinymce-2.js	13 Jun 2010 15:42:14 -0000
@@ -48,6 +48,11 @@ Drupal.wysiwyg.editor.attach.tinymce = f
+  // #715228: Remove extra mceItem class added by Wysiwyg < v2.1.

+++ editors/js/tinymce-3.js	13 Jun 2010 15:42:14 -0000
@@ -64,6 +64,11 @@ Drupal.wysiwyg.editor.attach.tinymce = f
+  // #715228: Remove extra mceItem class added by Wysiwyg < v2.1.

Sorry for having to re-open this issue, but

1) we do not put d.o issues into code comments, unless we are talking about highly controversial issues with 100+ replies that can't be remotely summarized in a few words.

2) the code comment does not explain WHY this highly suspicious operation is done.

Powered by Dreditor.

Better like this or too long?
Ignore the leading space on the last line...

+++ editors/js/tinymce-3.js	26 Sep 2010 13:38:29 -0000
@@ -65,7 +65,9 @@ Drupal.wysiwyg.editor.attach.tinymce = f
+  // Remove extra mceItem class. TinyMCE uses it internally for placeholders
+  // only, but Wysiwyg added them where TinyMCE didn't expect them before v2.1,
+  //  preventing images from being edited until the class was removed.

"Remove TinyMCE's internal mceItem class, which was incorrectly added to submitted content by Wysiwyg <2.2. TinyMCE only temporarily adds the class for placeholder elements. If preemptively set, the class prevents (native) editor plugins from gaining an active [?] state, so we have to manually remove it prior to attaching the editor. This is done on the client-side instead of the server-side, as Wysiwyg has no way to figure out where content is stored, and the class only affects editing."

Quite a novel, yes. ;) However, whenever we add such code (which likely triggers a "WTF?!" for people reading the code), we have to carefully explain the WTF. =)

That said, since this bug only affects existing installations, this makes me think about the possibility of adding a backwards compatibility add-on module, or alternatively, passing additional BC options into the editor attach/detach settings.

Lastly, wouldn't it make sense to do a $field.val().indexOf('mceItem') != -1 first?

Powered by Dreditor.

The bug itself was fixed before 2.1 was released (#400482-19: editor.instance.prepareContent() breaks editor's native markup handling) and that version only applies the class where needed so I changed it back to 2.1 in the comment.

I've been thinking long the same lines for backwards compatibility between 3.x and 2.x, like a redirection layer from the old API to the new one (like GNU/Linux->WINE->Win95), but for this it would be overkill as it's just the content being "corrected on-the-fly". No module ever relied on that class being incorrectly added by Wysiwyg AFAIK, and only TinyMCE suffered from it.

I thought about adding a check for "mceItem" before the RegExp but decided against it. An .indexOf() call would have to search through the entire string to know if the class was added and then the RegExp would search through it again to replace all ocurrances, but only where "mceItem" is between two word boundries. indexOf() would trigger on "amceItems" while the RegExp would search again but replace nothing. We'd still need to search it all if the string is huge, and then I'd prefer if it happened just once.
Note: I haven't actually benchmarked this, but I'm assuming the RegExp search [and possible replaces] isn't slower than an indexOf() search by several orders of magnitude.

Thanks!

+++ editors/js/tinymce-2.js	26 Sep 2010 16:34:37 -0000
@@ -49,7 +49,13 @@ Drupal.wysiwyg.editor.attach.tinymce = f
   $field.val($field.val().replace(/class=(['"].*?)\bmceItem\b(.*?['"])/ig, 'class=$1$2'));

Would it make sense to additionally replace the two instances of .*? with [^>]* ? I think that would make the regex rock solid and prevent us from potentially incorrectly removing "mceItem" from a blog post's text content, for example ;)

Powered by Dreditor.

You mean change /class=(['"].*?)\bmceItem\b(.*?['"])/ig to /class=(['"][^>]*)\bmceItem\b([^>]*['"])/ig?
I think that would in the end be less accurate as > and < can't be used in class attributes so they should never appear anyway.
(Note that it's all wrapped in /class=(['"]...['"]) so the smallest possible match is class="mceItem" [or class='mceItem']).
The question mark makes sure .* is non-greedy and returns the smallest possible match, meaning .*? won't include strings matched by later parts of the pattern.

To strengthen it a bit more given <, > and other special characters won't appear in class attributes, we could do /class=(['"][\w\s]*?)\bmceItem\b([\w\s]*?['"])/ig. That would still catch the class in <em>class="mceItem"</em> in the first part of this message though.

To make it pretty much bulletproof, /(<.+?\sclass=['"][\w\s]*?)\bmceItem\b([\w\s]*?['"].*?>)/ig. (Note the parenthesis have moved to capture the whole starting tag.)
The smallest possible match would then be <p class="mceItem">, which is cut down to just <p class="">.
That should not match anything in non-markup content, if it contains the correct HTML entities.

"Reinforced" RegExp as per last paragraph in #23, and slight adjustment of line lengths in comment.
Confirmed that the RegExp now only catches instances in actual markup, not text content, as long as the proper entities are used for < and >.

With the risk of this being reopened (and me being slapped across the fingers) again, the patch in #24 has been committed to all branches.

The new RegExp is working really good for me, especially when writing about the mceItem class itself, like Sun said. :)