I've got a patch for this. It looks like CKeditor is happy enough having the plain text of the HTML that it inserts when you add an image. That is to say, this works perfectly if you send the raw Only local images are allowed. tag rather than the [[STUFF]] on detach.

CommentFileSizeAuthor
#14 wysiwyg-media-tagmap.patch1001 bytesjames.elliott
#5 wysiwyg-media.patch1.88 KBjames.elliott
#2 wysiwyg-media.patch1.71 KBjames.elliott
WYSIWYG.patch700 bytesjames.elliott

Comments

JacobSingh’s picture

JacobSingh commented
Status: Needs review » Needs work

See my comments in the JIRA issue.

-J

james.elliott’s picture

james.elliott commented
StatusFileSize
new wysiwyg-media.patch1.71 KB

New patch attached.

So the problem ended up being that during insertMediaFile() we add the image HTML to the Drupal.settings.media.tagmap using its stringified JSON object as the index. When we recreate this tag to get the HTML during attach() the tag doesn't match up to the index in the tagmap. The order of the attributes from the image node was different so the strings weren't matching.

So I changed createTag() so that it would put the attributes of the image node into an array and then sort the array alphabetically by attribute name. And I was no longer getting the tag when the WYSIWYG was re-enabled, I was getting nothing.

That's when I noticed that attach() was using stripdivs() on the img tag, which didn't have any divs to strip. The $('img', $(formattedMedia)) was returning nothing because the img tag didn't have an img tag inside it. So I had stripdivs() check to see if the formattedMedia was an img tag before it tried to strip divs from it. This seems much safer to me.

Now we should be able to turn off and on WYSIWYG to our hearts content while not losing any images.

james.elliott’s picture

james.elliott commented
Status: Needs work » Needs review

forgettingToChangeStatus++

JacobSingh’s picture

JacobSingh commented

Looks reasonable as hacky as this stuff was to begin with. Consider making the sorting routine a separate method just for clarity instead of a closure.

You could add it to jQuery for kicks, or just chuck it onto the wysiwyg object. If you want, you can also just namespace it as part of media. There is a namespace() function (see core.js).

Best,
J

james.elliott’s picture

james.elliott commented
StatusFileSize
new wysiwyg-media.patch1.88 KB

New patch, fewer anonymous closures.

JacobSingh’s picture

JacobSingh commented 
Index: javascript/wysiwyg-media.js
=========================================================
--- javascript/wysiwyg-media.js	(revision 1.7)
+++ javascript/wysiwyg-media.js	Fri Jul 23 17:28:34 EDT 2010
@@ -94,6 +94,11 @@
    * @return HTML of <img> tag inside formattedMedia
    */
   stripDivs: function (formattedMedia) {
+    // Check to see if the image tag has divs to strip
+    if ($(formattedMedia).is('img')) {
+      return this.outerHTML($(formattedMedia));
+    }
+    // This will fail if we pass the img tag without anything wrapping it, like we do when re-enabling WYSIWYG
     return $('<div>').append( $('img', $(formattedMedia)).eq(0).clone() ).html();
   },

I'm a little out of touch here, but in what case does the 2nd return happen vs the 1st?

james.elliott’s picture

james.elliott commented

The first happens when an image has been inserted and then Rich Text editing is toggled off and then on. At this point, the $(formattedMedia) is just an img tag. So in order to get the HTML for it, when send it to the outerHTML function which wraps it in a div and then asks for the HTML content of that div, which is the full HTML of the img tag.

The second happens when an image is first inserted by the popup. It is wrapped in all of the various divs from the formatter chosen in the popup and they are stripped off by this function so that only the raw img tag is inserted.

This is actually a very bad method, as it isn't general by any means. It fails with any other type of media. I'm also not entirely sure WHY we feel the need to rip off the divs output by the media formatter.

I'll try to reroll this patch sometime this weekend.

JacobSingh’s picture

JacobSingh commented

We have to rip the divs off because if the divs are there, and the user puts their cursor after the image, they will be typing inside the divs. This will cause whatever they put there to be essentially deleted. Also, there is a bug when you add multiple images in a row I think where it wraps the next one inside the divs (same issue really). This causes the 2nd one to never get transformed back. I'm told that in the latest version of ckeditor, you can mark an entire piece of the DOM as non-editable. If we did this, it would be possible.

james.elliott’s picture

james.elliott commented
Status: Needs review » Fixed

Point taken, we'll have to refactor later when CKeditor allows us to mark things as uneditable so that they don't get text nodes within them.

For now, this solution works fantastically at fixing this issue.

Status: Fixed » Closed (fixed)

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

tsvenson’s picture

tsvenson commented
Status: Closed (fixed) » Active

This problem seems to have reoccurred again. Using latest HEAD with D7b1 and CKEditor 3.4.1.

It happens both when I disable and enable the editor and when just using the Source button in CKEditor to view the HTML.

Simply click the Source button to view the HTML, then click it again to go back and [[stuff]] is shown instead of the image.

tsvenson’s picture

tsvenson commented

This problem seems to be rather random. Today I can switch between source code as well as enable/disable the editor and the photo reappeared correctly, but if I scale the image in CKEditor and then view source and switch back I get [[stuff]].

If I save the node with the scaled image it will one again reappear correctly, until I scale it again.

james.elliott’s picture

james.elliott commented

The problem is that we use a json of all the image attributes as the key for the media items in the WYSIWYG. This key doesn't get updated when you alter the attributes using the WYSIWYG. So if you disable/enable the WYSIWYG without saving, the keys don't match because the attribute values don't.

I'm working on a solution for this problem now.

james.elliott’s picture

james.elliott commented
Status: Active » Needs review
StatusFileSize
new wysiwyg-media-tagmap.patch1001 bytes

Status: Needs review » Needs work

The last submitted patch, wysiwyg-media-tagmap.patch, failed testing.

james.elliott’s picture

james.elliott commented

Bleh, I blame patches from SmartCVS.

JacobSingh’s picture

JacobSingh commented

Then how is it being stored? I don't understand... It seems to me this would store the raw stuff in the DB.

-J

james.elliott’s picture

james.elliott commented

Nothing is being stored. This is specifically when you turn off and on the WYSIWYG without loading the page. The problem is that once you've altered the image in the WYSIWYG it doesn't match the tagmap, so won't reconstruct itself when you turn the WYSIWYG back on.

So when the WYSIWYG is turning off (detach) I rebuild the tagmap, adding any new items to it. That way when the WYSIWYG gets turned back on (attach) it will find the index and insert the correct HTML.

If you save the node and then go to edit again, this will not happen, because the tagmap is rebuilt in PHP and passed to the WYSIWYG via the Drupal.settings object.

JacobSingh’s picture

JacobSingh commented

But isn't detatch the same place where we turn it to a tag for storing in the DB? Are you sure that the Only local images are allowed. doesn't get stored in the DB w/ this patch?

-J

james.elliott’s picture

james.elliott commented

Yes, detach is where the WYSIWYG converts it's content into plain text/markup for storing in the DB. But this patch doesn't alter that behavior. It just adds to a javascript object so you can enable/disable the WYSIWYG to your heart's content without ever saving the node. And not have your images remain as the plain text tag when the WYSIWYG is turned on.

JacobSingh’s picture

JacobSingh commented
Status: Needs work » Reviewed & tested by the community
james.elliott’s picture

james.elliott commented
Status: Reviewed & tested by the community » Fixed

Committed

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein commented

Related issue: #2028253: If you change a file's information with the WYSIWYG disabled, re-enabling the WYSIWYG loses the file