Improve Media's WYSIWYG Macro handling

All very valid points. Also, I don't think we need to ensure the sourceMap unless we're actually using it.

Not sure if this has been discussed before, or if IE8 compatibility is not something media plans to support, but the usage of getElementsByClassName() in this patch (and #2067063: Wysiwyg integration is broken) isn't supported in IE8 and currently causes us issues with our rich text editor. You can use querySelectorAll(), instead, which fixes things, but I'm wondering why we are even using any of this, when we have jquery's selectors power?

It fixes the weird problem with missing alt and title attributes but can not align images left or right. It always shows "float: none"

@rteijeiro re #6 -- Are you sure you're applying this patch against the latest 7.x-2.x dev branch? If so, can you provide you OS version and browser version and which wysiwyg library you're using?

@alexkb re #5 -- Thanks for the x-browser compatibility update. We do need to support IE8, so that's an important piece to consider. Earlier attempts at this in #2067063: Wysiwyg integration is broken used jQuery but the order in which attributes were returned was unpredictable across different browsers and made a match and replace virtually impossible for one or more browsers. Getting a clean nodemap and then swapping out nodes was not only super efficient but solved the x-browser issues we were facing, until now with IE8.

I'll take another stab at this tonight and see if there's another even more x-browser compatible approach. Parsing html with a regex is always a bad idea, so we should avoid that at all costs...

I went back and revisited this code briefly this evening. It looks like simply replacing the getElementsByClassName method with querySelectorAll works for everything including IE8. I found one other IE8 peculiarity with looping and change a for..in loop to a for..a;b;c; loop. Also, it occurred to me that in the event of a JSON parsing error, something should be returned or else the editor could potentially be empty.

I think we definitely need to support IE8. It should be noted however that IE7 and below do not support the querySelectorAll method, thus wysiwyg integration with Media 7.x-2.x would not be supported in IE8 and below. How far back do we need to go here? With IE11 released for Windows 7 today and Windows XP support officially ending in April, I'm wondering how much hair-pulling we should be spending to support Internet Explorer versions older than IE8?

I personally am 100% comfortable with only supporting as far back as IE8 here.

I think we definitely need to support IE8. It should be noted however that IE7 and below do not support the querySelectorAll method, thus wysiwyg integration with Media 7.x-2.x would not be supported in IE8 and below

I just want to clarify here that this means not supporting content editors using IE7. Visitors browsing on IE7 would still see the markup fine.

Right; good point. This update would not affect front-end rendering at all and is only limited to content editors working in IE7 (...or below ::shudder::).

I just want to clarify here that this means not supporting content editors using IE7. Visitors browsing on IE7 would still see the markup fine.

Sure, that's how I understood it. I still think it's fine.

8: media-improved-macro-handling-2126755-8.patch queued for re-testing.

seems like everybody agrees on dropping ie7 support for content editing.
Lets see, if the patch still applies.

Re-rolled for latest dev.

Hi, I've been following this issue and doing a bit of testing with the patch and I still get an issue if I add multiple links to the same file. Why would anyone do that? I don't know but it's possible.

In my media library I have several files (in this case .doc but I don't think that's relevant). If I add those in to a new page with no other content, just a link on each line and put at least one of them in twice the links get written in fine but if I then change the WYSIWYG view to "plain text" the links are all displayed as media tags but the first one that is a repeat has the wrong ID. When I then immediately change back to WYSIWYG view they are converted back into links except that some are not. So far there have always been two broken ones, next to each other, one of which is the repeated one.
When the page is saved and viewed it shows proper links again, i.e. there are no media tags showing but, of the ones that showed in the editor incorrectly as media tags at least one will be linking to the wrong file and usually they will both be linked to the same file and showing the same link text even though that's not how they were originally added.

Some screenies

Chasing HEAD

Note, as of this commit: http://drupalcode.org/project/media.git/commit/7f322d9 you will need to have the new submodule 'Media Wysiwyg' enabled.

EDIT: The new 'Media Wysiwyg' module should enable itself by running update after upgrading to the latest dev snapshot.

I make a patch 1 month ago but no there was no issue. Running out of time, i hadn't submit it (it's bad i know).
Here is my patch for this issue. It works for my cases to surround macro by link for example, no more conflict due to

.

I hope this could help to make media_wysiwyg better.

Files shouldn't be hidden. My mistake

8: media-improved-macro-handling-2126755-8.patch queued for re-testing.

The patch in #19 fixed the issue for me.

+1 to patch #19

Thanks heaps!

Changing status based on recent comments.

Since this patch, the media file_entity field data stored in the macro seems to be getting encoded if it contains any markup. I've been trying to debug the issue, but haven't had much success yet.

Per #39, this still needs work.

Also it seems that markup fields are not editable - adding an image and fillling out the field, then editing the image and updating the field results in the original text being left in the output.

*edit* the particular use case is a Caption field that uses a simple WYSIWYG to allow italics.

Suspect #2153851: When inserted into the WYSIWYG, links to files are duplicated when text surrounding them is manipulated may be a duplicate of this or at least related.

Scratch that. The issue above seems to be related to an issue with the current code and jQuery 1.7. Seems to be fine when I downgrade to jQuery 1.5.

I tried updating to dev, updating the DB, and applying patch #19. The patch applies, but I still cannot align images in the wysiwyg. I am using CKEditor 4.2.2 in the latest version of Chrome.

Using media alpha-3, my tags looked like:

[[{"fid":"262629","view_mode":"default","fields":{"format":"default","field_file_image_alt_text[und][0][value]":"","field_file_image_title_text[und][0][value]":"","field_tags[und]":"","field_folder[und]":"_none"},"type":"media","attributes":{"class":"media-element file-default","style":"float: right;"}}]]

(note the style: float: right)

However, that attribute was not present in the IMG tag after node save, so no image alignment was present.

Now, after switching to media dev plus #19, I don't even get the style: float: right.

[[{"fid":"265584","view_mode":"default","fields":{"format":"default","field_file_image_alt_text[und][0][value]":"","field_file_image_title_text[und][0][value]":"","field_tags[und]":"","field_folder[und]":"_none"},"type":"media","attributes":{"class":"media-element file-default"}}]]

In my wysiwyg preview, after selecting the image and clicking the right align button, the image appears to be aligned right, but again, there is no attribute attached to IMG after the node is saved. This is very confusing, given that the code above doesn't change at all when I select the image and click the align right button.

I know that my input filters are not stripping the style, because I can add an image with a hard URL and have no problems with any alignment I choose.

For those using #19 successfully - am I doing something wrong here?

@sheldonkreger - I'm not sure why your setup isn't happy with the patch in #19. I just double-checked it with my test setup and it appears to be working as expected. The only thing I can think of is that CKEditor 4.2+ isn't fully supported yet with Drupal Wysiwyg. See https://drupal.org/comment/7448570#comment-7448570 and #1956778: Ckeditor 4.1 ACF. It may be that Media will need an update to accommodate changes within CKEditor itself. We've been running 4.0.3 for a while now, and even though the Media integration still has a few kinks here and there, the general Wysiwyg <-> CKEditor compatibility seems to be quite stable; I'm not able to reproduce your issue with this setup. It may also be possible that there is a browser conflict in the mix -- what browser/version/os are you using?

@alexkb - I'm relatively sure we have to encode the data-file_info to avoid collisions between markup and json, especially when there is markup within the json. It may be as simple as needing to un-encode it properly on the retrieval/rendering end. Can you elaborate on what you're seeing versus what you're expecting to see?

@samhassell, you may be experiencing this issue #2126697: Wysiwyg -- Alt and Title fields require some special handling.. Can you try the patch there any see if that solves it for you?

FYI: I just created #2164823: Fix for replacePlaceholderWithToken in media_wysiwyg.filter.js because there were some browser related issue in the placeholder replacement code.

It looks like, for some reason, my configuration (database) is disallowing 'style' . . . and it only starts being blocked out by the JS once I update to dev of Media, which has the new media_wysiyg module. Very frustrating, considering I allow the style attribute in every setting I can find in the UI. Good times, debugging this in Chrome's JS debugger using breakpoints inside media_wysiwyg.filter.js, and eventually discovered that the JS actually strips out disallowed attributes, based on *yet another* array of allowed attributes for the wysiwyg.

$ drush vget wysiwyg_allowed_attributes
media_wysiwyg_wysiwyg_allowed_attributes: array (
  0 => 'class',
)

Anyway, it gets stripped out by a function starting on line 153 of media_wysiwyg.filter.js

    /**
     * Extract the file info from a WYSIWYG placeholder element as JSON.
     *
     * @param element (jQuery object)
     *    A media element with attached serialized file info.
     */
    extract_file_info: function (element) {
      var file_json = $.data(element, 'file_info') || element.data('file_info'),
        file_info,
        value;

      try {
        file_info = JSON.parse(decodeURIComponent(file_json));
      }
      catch (err) {
        file_info = null;
      }

      if (file_info) {
        file_info.attributes = {};

        // Extract whitelisted attributes.
        $.each(Drupal.settings.media.wysiwyg_allowed_attributes, function(i, a) {
          if (value = element.attr(a)) {
            file_info.attributes[a] = value;
          }
        });
        delete(file_info.attributes['data-file_info']);
      }

      return file_info;
    },

I'm not sure how or where this was set before I updated, but the media_wysiwyg module actually has a default list (which includes style).

<?php
function _media_wysiwyg_wysiwyg_allowed_attributes_default() {
return array(
'alt',
'title',
'height',
'width',
'hspace',
'vspace',
'border',
'align',
'style',
'class',
'id',
'usemap',
'data-picture-group',
'data-picture-align',
);
}
?
>

For some reason, my particular site (database) overrides this default, hence my bug.

Anyway, please disregard my prior comment about this not working. I'm going to find a way to set this variable and will report back.

@kaidjohnson yeah [2126697] doesn't address this particular issue - which is specific to fields containing HTML. With the patch in #19 applied it works for normal text fields.

"field_caption[und][0][value]":"

test 1

","field_caption[und][0][format]":"summary_html"},

in the token will never be updated.

Is it better to follow this up in the other issue?

(Interestingly, i also have a date field which keeps getting reset to 1970 in the edit screen, but seems to work fine in the macro)

Edit:
it does apply. clash with another media patch. I am sorry for the confusion.

Patch in #19 applied cleanly to the latest 2.x-dev and fixed the synchronisation between wysiwyg changes and the media token.

I can also confirm that #19 applies to 2.x-dev and fixes the issue.

Adding related issue as per my comments in #37.

Issue queue didn't set the related issue.

@samhassell - thanks for the update. I believe I understand your issue, and I don't believe it's specific to this thread or #2126697: Wysiwyg -- Alt and Title fields require some special handling.. I created a new issue for it specifically - #2170759: File fields that use WYSIWYG instead of plain text don't get updated. - and proposed a patch. Let me know (on that thread) if it does the job or if I'm still misunderstanding.

@das-peter re #36 - Thanks for that update! Much cleaner cross-browser solution for that piece of this update; rolled it into this proposed patch.

@alexkb re #29 - If you could provide some clarification about the issue you're experiencing, it would be much appreciated.

Thanks all for the continued testing and feedback!

Blargh; same tests run cleanly locally. Looks like something's up with the Media testbot again...

@alexkb - I believe the latest patch in #45 solves the encoding issue. It looks like the javascript was parsing htmlentities, and then not parsing them back. The updated approach avoids the issue altogether. Let me know if that works for you.

Hey Kai, you are 100% correct: your #45 patch + the patch from #2170759: File fields that use WYSIWYG instead of plain text don't get updated. solves all our problems.

To handle old incorrectly escaped/encoded captions entered prior to these patches, I think we'll use the hook_media_wysiwyg_token_to_markup_alter() hook to re-parse them before rendering on the front end.

Thanks heaps for your work on this!

@alexkb - glad it works! We should probably implement hook_update_N() to handle the escaped/encoded captions so others with a legacy format don't run into issues. Can you provide a few samples of what your captions look like now (old incorrectly esacped strings)? I don't think I have any samples of what you're working with to put together an update script. And/or, post your parsing script you're using with hook_media_wysiwyg_token_to_markup_alter() that is working for you or roll it into this patch so we can incorporate a safe update path for this update. Thanks!

45: media-improved-macro-handling-2126755-45.patch queued for re-testing.

@kaidjohnson - I was just looking into this a little more. Since we're no longer encoding the media fields now, it doesn't look like there will be a need to adjust the previously inserted images as they're the same.

We were getting fatal js errors if the body field didn't contain any embedded media elements. The side affect to this js error, was that the body field was getting emptied, and we couldn't save pages (drupal's form validation stopped this).

Anyway, attached is a small edit to @kaidjohnson's #45 patch to fix this.

This update fixes a small bug around the 'Disable rich-text' link. If, for some reason there are no '.media-element' elements in the content, patch #45 throws an error. This issue is especially highlighted when an image field uses rich-text format. EDIT: @alexkb - timing! I think we uploaded our patches at nearly the same time. Looks like we were on the same page for the fix on this piece, too.

This update also addresses one of the last major concerns I've had with the macro handling - namely that the file_info is stored in the DOM via the data-file_info attribute. This method is extremely inefficient, as we're JSON stringifying and then encoding on top of that then injecting it into the DOM. As an alternative, this update creates a dataMap to store the data in, keyed by file id, which is attached to the element in the DOM via the data-fid attribute. No more encoding/decoding or JSON stringifying, just store the data and retrieve it by key.

It looks like all media testbot test are on hold right now; not sure when that will get cleared up, but let me know if this patch works for you!

Thanks!

UPDATE : @alexkb, I have no idea how this update managed to remove your patch... I could have sworn that I was removing the patch I had named #54 that I uploaded just after yours. Sorry for the confusion there. Didn't mean to step on that patch like that...

I was having issues with Firefox as related in #2075123: Media browser inserting HTML tag rather than media tag and this patch works great!

There is an error if attributes fields (title or alt) have have text with " character.
If this fields have " character, this is not escaped in attributes array and JSON can't parse it. (array malformed).

Following patch resolve this.

// EDIT: do not test #57 patch. Sorry for that

#57 is totally crap. I make it with old branch. Shame on me.

This patch should be better and resolve " issue.

This patch is part of the #1day1patch initiative.

I've been directed to this issue from Comment #148 of issue #2067063: Wysiwyg integration is broken.

I'm still getting the same age old problem of alt and title attributes being stripped from the [[media]] filter after rendering. Note that the data *is* there inside the [[media]] macro, and looks like this when I disable rich text:

[[{"fid":"1295","view_mode":"file_styles_square_thumbnail_fancybox","fields":{"format":"file_styles_square_thumbnail_fancybox","field_file_image_alt_text[und][0][value]":"My image alternate text value","field_file_image_title_text[und][0][value]":"My image title text value"},"type":"media","attributes":{"height":"125","width":"125","class":"media-element file-file-styles-square-thumbnail-fancybox"}}]]

I would have expected the alt/title attributes to show up in the attributes section, next to height, width, class, etc, but you guys know a lot more than me about how this macro works.

The HTML produced at rendering time is:

<a href="http://example.com/files/_event_thumb/3_article_photo.jpg" title="" class="fancybox" data-fancybox-group="gallery-page"><img typeof="foaf:Image" src="http://example.com/files/styles/square_thumbnail_fancybox/public/_event_thumb/3_article_photo.jpg?itok=vLlTvNgc" width="125" height="125" alt="" title=""></a>

Versions:

media-7.x-2.x-dev (alpha3+39-dev as of 2014-01-20) + patch in #58
file_entity-7.x-2.x-dev (alpha3+10-dev as of 2014-01-16)
wysiwyg-7.x-2.x-dev (snapshot packaged on 2014-01-22)
ctools-7.x-1.3

Does this look right? Am I missing something?

@jwilson3 - What is the output of drush vget media_wysiwyg_wysiwyg_allowed_attributes for you? You may have a legacy setting that is stripping your attributes. @see #2168291: Make 'media_wysiwyg_wysiwyg_allowed_attributes' configurable from UI

Also, you may be running up against alt/title attribute syncing problems. @see #2126697: Wysiwyg -- Alt and Title fields require some special handling.

@kaidjohnson. Wow, those are some great pointers there. It turned out my problem was the later of the two (I didn't have the allowed_attributes variable set in my installation)

I'd like to point out that the patch in #2126697: Wysiwyg -- Alt and Title fields require some special handling. conflicts with the patch here because they both modify two of the same js files in such a way that when you apply one, the other must be applied by hand due to various hunks being rejected on the modules/media_wysiwyg/js/media_wysiwyg.filter.js file.

In my case, I was able to end up just using #2126697 and I unapplied the patch here, and it solved my immediate issues.

I've 1++'d the RTBC there, in the hopes that that can get in and pave the way for re-rolling this one.

Thanks for the work thus far!

A customer ran into this issue where if you have multiple identical macros in body, the JS only replaces the first occurrence and it'll leave plain text macros in WYSIWYG when the node is edited. This happens when the database body field is read and macros are replaced with HTML (replaceTokenWithPlaceholder). The leftover macros are never read back either, because they've become HTML escaped.

The problem is that the macro with multiple occurrences in body is only once in the tagmap array that's looped through. This is how it should work since we can replace all occurrences of it, but unfortunately I found out that the macro to markup replacement code only replaces the first occurrence. String.replace() could be done "globally" where it replaces all occurrences, but would require the "g" RegExp option. For that to work, we'd need to use new RegExp(macro, "g"). Also, for that to work, macro would need to be escaped for RegExp.

What I found on Stack Overflow is you can first split the string (macro) and then join the returned array back using the replacement (markup) as its glue. It has been tested to be faster than RegExp on Chrome and a bit slower on most browsers. But that's without any escape code which macros would need with RegExp. However, I have no idea about real world performance using longer strings, probably doesn't make a visible difference anyway. http://stackoverflow.com/a/1145525

A
content = content.replace(macro, markup);
B
content = content.split(macro).join(markup);

I added my suggestion on top of #58 and this one includes both so it should work on the latest dev. (2f828ea)

Oops, botched it. Used old variable name "macro" instead of "match". Here's the fixed patch. Previous comment still applies.

I'd like to test this patch, but it does not apply cleanly with the latest dev. Can an updated patch be rolled?

I'll do that soon. I'd also like to add #2139461 to the next patch I create. It's a very simple fix, but it's tedious to keep that issue in sync with this one (can't apply it on top of this), because it's too deep in the same code.

Re-rolled with rejected hunks in Drupal.media.filter.replaceTokenWithPlaceholder() and Drupal.media.filter.extract_file_info() with versions from this patch/issue, not the ones introduced in #2126697: Wysiwyg -- Alt and Title fields require some special handling.. This patch also haven't included the bits in #2139461: Can't edit link text for uploaded files (not images), mentioned by oetseli in #65.

The re-roll in #66 dropped some important updates from the replacePlaceholderWithToken() method. Those updates shouldn't simply be dropped; they provide an important update to this issue overall.

Yay! It looks like simpletest support is back up for media patches...

- Re-rolled against the latest HEAD.
- Retained fix from #58
- Retained fix from #63 (excellent!)
- Restored dropped failed hunks from #66
- Ignored adding patch from #2139461: Can't edit link text for uploaded files (not images) to this roll, for the following reason: The scope of that issue is so refined and concise that it would be a shame to have this patch prevent that issue from being committed & fixed. That issue needs a simple re-roll (coming soon) and should be good to go. This patch (or that one) can always be re-rolled and this issue should not attempt to be a one-stop fix-all solution for the media module. This issue is really meant to address the x-browser inconsistencies in the macro swapping, which it has (nearly) accomplished.

Tested patch #76 on Firefox 26 on Mac and IE 10 on Windows. On both the image I upload using CKEDITOR WYSIWYG saves as a media macro. If I edit and save with no changes, the image is still saved as media macro. (Before the patch it would save as img tag). So this patch fixed that issue for me.

Since I am using Filtered HTML text format, the style gets stripped when viewing. So in order for the float left/right of images to work, I need a class defined. No class is available, so I need to use Figure out a way to keep markup output consistent in media embeds to make a small module to provide the classes.

After #2126697: Wysiwyg -- Alt and Title fields require some special handling. was reverted #76 doesn't apply anymore. I think this re-roll will fix things. It's basically Drupal.media.filter.extract_file_info() from before the above mentioned revert with the patch in #76 applied to it.

But, there something fishy with this call in media_wysiwyg.filter.inc:

    drupal_add_js(array('mediaDataMap' => array($file->fid => $data)), 'setting');

The $file->fid => $data mapping has no effect in the client side Drupal.settings.mediaDataMap object. The added $data is indexed chronologically due to the nature of PHP and drupal_array_merge_deep_array(). From drupal/includes/bootstrap.inc:2127:

      // Renumber integer keys as array_merge_recursive() does. Note that PHP
      // automatically converts array keys that are integer strings (e.g., '1')
      // to integers.

This results in a mixed DOM tree before and after enabling rich text editor (ckeditor.module + media_wysiwyg.module) as this one. I've inserted fid=1 and fid=4 into the content:

Before:

After:

Marked #2194741: Media not being added to text, getting false instead as a duplicate.

Before this patch all media items get replaced with the word "false", after this patch all media items disappear completely.

If I apply this patch and add the following hook to one of my modules the media works, which suggests that the content filtering in CKeditor 4.1 is still preventing media from working properly as it filters out the fid attribute.

/**
 * Implements hook_wysiwyg_settings_alter();
 */
function opencrm_kickstart_wysiwyg_editor_settings_alter(&$settings, &$context) {
 if($context['profile']->editor == 'ckeditor') {
   dpm($settings);
   $settings['allowedContent'] = TRUE;
 }
}

nr #78 does not apply anymore with newest DEV, reroll.

@Schnitzel
When you do rerolls of patches, make sure the patch applies from the root of the module you're patching. You patch has sites/all/modules/media/ prefix to all diff url's. This means it will fail when trying to apply using drush make.

I rerolled the patch so it matches this, and it works now with latest 7.x-2.x branch (commit: 9583d89e810312ee76abb5ba9ac9174cecb7d815)

Status set to needs review

just making the title clearer.

I came here because I noticed that media was only replacing the first img tag with teh media token, after applying this patch, i notice both media tags get replaced, horray!

However, now, a new bug seems to have appeared. If you add the SAME media file to a wysiwyg area twice (I know, maybe not the most common occurance?), it appears to not be possible to display them with different view modes. For instance, I placed 2 images, one with Preview and the second one with Default.

Once I hit save, they both come out as Default. Furthermore, going back to the node, now the top one has both file-preview and file-default classes applied.

edit: on second thought... this appears to be a big improvement, maybe this is just a known gotcha of doing it this way.

Moving back to needs review because who actually puts 2 of the same media items on the same node?

Seems likes its working to me FWIW.

I ran into an issue with other modules also using [[..]] in the content (node_embed):

The regex matching all media-tags in Drupal.media.filter function replaceTokenWithPlaceholder was matching too much. It included the node_embed-tag with a media-tag: [[nid:xxx]]</p><p>[[{..media-tag..}]]

I changed the regex to capture everything inside [[ ]] and then in the loop filter out any non-media-tags. Applied this to the latest patch listed here:

var matches = content.match(/\[\[.*?\]\]/g);

if (matches) {
for (var i = 0; i < matches.length; i++) {
  var match = matches[i];
  if (match.indexOf('"type":"media"') == -1) {
    continue;
  }

Other than this, the patch seems to work just fine!
(I applied #78 to media 7.x-2.0-alpha3+36-dev)

@SpadXIII - watch the use of Array.indexOf. Use $.inArray() instead. @see #2108647: Array.indexOf doesn't exist!

@kaidjohnson - in this case, match is a string and not an array. Using $.inArray() won't work for this case.

1 hunk failed on the patch at #88. I've rerolled against 7.x-2.0-alpha3+46-dev. Was able to embed multiple media images incl. the same image twice using Firefox and able to attach/detach the rich-text editor with the token to placeholder & placeholder to token conversions working correctly.

Did some more testing and unfortunately this patch doesn't resolve embedding the same image multiple times. Each embed of the same file ends up with the same settings (e.g. embed image, set caption to instance 1. Embed same image, set caption to instance 2, after node save both images display with caption instance 2).

@jonathan_hunt : isn't that because the caption is saved with the file and not where it's used?

@SpadXIII: The caption is saved with the file but can be overridden locally on each embed via the Media embed filter UI. There probably needs to be per-embed id so that multiple embeds of the same file don't collide, or generate an id by taking document order into account on the fly,

Just to make sure I'm understanding this issue and patch...

Problem (WYSIWYG):
In my case I was using the WYSIWYG editor (CKEditor 3.x) library. When inserting a media item (image in this case) if I clicked the source button or toggled the wysiwyg editor the media token markup was converted to an image tag.

Patch #92:
This seemed to fix the issue I just described.

Problem (CKEditor module)
A similar problem exists when using the CKEditor module but the media token is immediately converted to the img tag. The patch does *not* fix it in this case.

@caschbre #97:

Problem (CKEditor module)
A similar problem exists when using the CKEditor module but the media token is immediately converted to the img tag. The patch does *not* fix it in this case.

I can confirm this problem on a fresh installation with the following setup:

• Drupal 7.27
• Media 2.0-alpha3+80-dev (incl Media WYSIWYG)
• CKEditor module 1.4
• CKEditor library 4.4.0 (also tried with 4.3.5)
• Patch for CKE module: https://drupal.org/node/2159403#comment-8744095
• Patch for Media: #92 from this thread (tried it with and without it)

I've also tried to disable CKE security filter. But no matter, what I tried, the tokens always immediately converted to img tags (having horrible classes like "media-image img__fid__1 img__view_mode__default attr__format__default attr__field_file_image_alt_text[und][0][value]__Original alt attr__field_file_image_title_text[und][0][value]__original title")

Rerolled #92 to apply to the latest dev code.

Probably still needs work based on the above comments, but setting to "needs review" for the testbot.

Closed #2279749: Media in WYSIWYG no longer shows rendered entity as a duplicate of this based on #97 & #99, although in my testing that bug is present on a clean (latest dev) media install and is not a result of this path.

Trying to figure out if it's an existancing issue or due to a patch, but media_wysiwyg_wysiwyg_browser_plugins setting (e.g. show library, etc. tab for wysiwyg button) isn't followed with ckeditor intergration. As far as I can tell nothing for ckeditor adds that setting so thus it's not respected.

We've been using #100 in production for a while now. It works for us.

We are already at 100+ posts on this issue, can I suggest we push this patch through and move the CKeditor issue to a follow up? The title of the issue indicates this is about Media & WYSWIYG.

We're also now using #100 in Panopoly (which uses TinyMCE). Working great for us! :-) I second the suggestion to move the CKEditor issues to another issue so this can get committed finally.

Also tested and approved here.

This patch should be commited as soon as possible (maybe with a new alpha release...), many other issues are probably related (such as this one for instance https://www.drupal.org/node/1792738.

It should be noted that this doesn't help with the Ckeditor module integration, but the issue is currently being worked on over there.

Also chiming in here. Have been using #100 patch for a while now for Web Experience Toolkit and is working with CKEditor 4.2+. Also agree that this patch should be committed as soon as possible.

I have reopened #2279749: Media in WYSIWYG no longer shows rendered entity, which is to address the issues mentioned in #97 & #99.

I downloaded the latest dev version - with this issue patch and experienced that existing media embeds still seem to work, but nodes with images embedded with old macro format lost the images when saving after the upgrade.

Any ways to avvoid disappointing all users (while thanks to you making them happy about a more robust image embedding) ?

Edit: I added a new support issue here: #2320535: Media 7.x-2.0-alpha2 to 7.x-2.x-dev break existing WYSIWYG content

re: #2126755-111: Improve Media's WYSIWYG Macro handling
I manually tested today after upgrading from media 7.x-2.0-unstable7+25-dev. I resaved a node with a media embed and did it in both a panelizer fieldable panel page body field and a regular page body field. In both cases, re-saving didn't break the embed. I even switched to non-rich text and back to rich text to see if that caused a regression.

Please open a new issue with steps and versions from which you are upgrading. Reopening an old issue that has over a hundred comments will only make the water muddy.

Changes related to this issue introduced a new bug when there are multiple WYSIWYG editors on a page. Please see https://www.drupal.org/node/2328493.