When inserted into the WYSIWYG, links to files are duplicated when text surrounding them is manipulated

I have had several very annoying issues with WYSIWYG (CKEditor 3.6.6.1.7696) similar to the above:

• some or all content after the linked file is removed or duplicated
• inserting 2 different files one after the other sometimes shows 2 duplicates of the same file
• inserting a line break < br > removes the file

Is there an update coming out soon as it sometimes takes me up to half an hour just to edit a page while trying to tip-toe around the markup problems (along with other File Entity and/or Media bugs e.g having to rebuild permissions after each file upload)

In addition, if I use the Media selector tool to insert an image (Original view), and then use the Image tool to add a class, this is totally ignored.

As an update, I saw the alpha3 version is having some of the same problems and was wondering whether it might be related to the < ! - - hidden HTML tags inserted with the file data? sometimes these are not removed when a file link is deleted

Another update as I have been struggling with various pages on my website which feature file links.
I had a page which had an image at the top. I removed file links further down the page and when I tried to add them again, some text before and after the file had been deleted (but not all!). At a certain point, the page would display ok, but when I went to edit it and click 'Source', the text would be removed again. After I removed the < !--MEDIA-WRAPPER-START-1-- > before the top image (I had to capture the page content from a SQL backup) I can now edit the page normally again.
So I must conclude that the problem lies in these hidden html comments...
???

Sorry not sure how to find this out exactly. Script links at head of homepage read
/misc/jquery.js?v=1.4.4
misc/ui/jquery.ui.core.min.js?v=1.8.7</script>
misc/jquery.once.js?v=1.2
Does that help?

I'm having this issue with content duplication on 7.x-2.0-alpha3 without jquery_update, with ckeditor.

What is happening for us is that an editor adds and then moves or removes media in the wysiwyg or by clicking 'View Source'. When they do this, they sometimes leave behind a <!--MEDIA-WRAPPER-END-1--> tag. With further editing they can get into a situation in which they have a START-1 and END-1 tag pair lower down.

As a result, media.filter.js replacePlaceholderWithToken finds that first orphaned end tag and that later start tag and calls them a pair. It never validates that the start tag should be before the end tag. This results in odd content duplication. Each time the editor switches back and forth between WYSIWYG and source a further duplication is added on.

We need to harden the token replacement against these kind of scenarios (I'm sure there are others). Here's a patch that will remove an extraneous end tag if its position comes prior to the start tag.

I was seeing this issue as well, using nearly the same steps as Mirabuck in the original post. What i noticed in my case is that the containing element for the files was not yet exited. So in our case it was a span containing the file tokens and replace. When the original containing element was altered it resulted in messing up the token replace the next time the wysiwyg window was rendered and then items were duplicated often quite a few times. We could get around this by:

1) Inserting
2) Hitting return
3) backspacing
4) Hitting return once again

This would place you outside the containing element and allow for alteration of the content without duplication.

Patch #13 Seems to work. I haven't extensively tested it but it appears to clean up the duplicate images I was having on a couple of pages.

I think #13 might be a slightly different issue. The original issue here is reproducible without clicking "View Source" (edit: or without moving/removing media items).

The root cause of the issue is basically described in https://dev.ckeditor.com/ticket/6955. CKEditor assumes that when you start a new line you want the styling from the old line to continue onto it (as long as your cursor is within the styling block when you start the new line) and it treats the <span> from the embedded media document the same way.

It is reproducible in the latest Media 7.x-2.x code, but I ran into it in a much older version and wrote a patch for that in #2028231-4: Embedded documents in the WYSIWYG can be very hard to delete (since it is closely related to the patch I already had running from that issue, and reuses some of the same code). It might be possible to look at that for inspiration on how to fix this in Media 7.x-2.x, although the current patch there still has some issues (and mainly only works in Firefox).

David, I think you are correct in your assessment. I have related this issue to issue #2028231. I tried the latest patch from 2028231 and it appears to have resolved the root issue with some minor bugs. For those of you who are struggling with this issue and are looking for a starting point for Media 7.x-2.x, I would recommend checking out the related issue.

Related issue:
Embedding Videos in WYSIWYG Leads to Duplicate Embeds

I've found a very quick (non permanent) fix to this for those that cannot apply the patch.

Once you have created the media link, hit 'enter' to place your cursor on a new line.

Then hit the 'Media browser' button again and straight away close down the window that pops up using the 'X' in the top right hand corner.

You should now be able to enter normal text that will not be overridden by the media module.

Hope that helps someone.

@Stephen Ollman, if you are experiencing this issue, your media, wysiwyg plugin options and text format configuration is likely incorrect OR you are using an incorrect configuration on a previous release of media instead of a current release with the correct configuration as specified in the recipe https://www.drupal.org/node/2843391 or the media_dev distro.

To review a working configuration you can run the media_dev distro on simplytest.me by clicking this link:
http://simplytest.me/project/media_dev/7.x-2.0-beta5

OR/AND if you prefer using the media_ckeditor module with the ckeditor module with the ckeditor library instead of the wysiwyg module with the ckeditor library then review the media recipe here:
https://www.drupal.org/node/2843391

@Joseph, I gave the http://simplytest.me/project/media_dev/7.x-2.0-beta5 a try and got the same result.

1. Add new content - Basic Page
2. Using the Media Browser tool, uploaded a PDF
3. Select the PDF using the 'Default' as the display type.
4. Once showing in the editor, hit enter for a new line
5. Typed in some normal plain text
6. Saved the page

The resulting output is two identical links to the PDF.

Am I missing a configuration component?

Hmm, I just tested in simplytest.me and could not reproduce given your steps.

What browser are you using? what version?

I am testing on Firefox 52.2.0 64bit on Linux.

Are you using Internet Explorer by chance? or Microsoft Edge? Or maybe Safari?

Maybe try with Chrome or Firefox, see if this helps.

Thanks @Joseph for continuing to try and help with this.

I can confirm that I'm using FF 55.0.3 and Chrome 60.0.3112.113 and not IE or Edge.

Both Chrome and FF end in the exact same output result following the steps above.

That is the link to the PDF is duplicated with the line of plain text after the initial media link being replaced by a copy of the media link output.

Original source code should look like this:

<p>[[{"fid":"5","view_mode":"default","fields":{"format":"default","alignment":""},"link_text":"test-document.pdf","type":"media","field_deltas":{"1":{"format":"default","alignment":""}},"attributes":{"class":"media-element file-default","data-delta":"1"}}]]</p>
<p>Some plain text here.</p>

But the output is actually:

<p>[[{"fid":"5","view_mode":"default","fields":{"format":"default","alignment":""},"link_text":"test-document.pdf","type":"media","field_deltas":{"1":{"format":"default","alignment":""},"2":{"format":"default","alignment":""}},"attributes":{"class":"media-element file-default","data-delta":"2"}}]]</p>
<p>[[{"fid":"5","view_mode":"default","fields":{"format":"default","alignment":""},"link_text":null,"type":"media","field_deltas":{"1":{"format":"default","alignment":""},"2":{"format":"default","alignment":""}},"attributes":{"class":"media-element file-default","data-delta":"2"}}]]</p>

test-document.pdf
test-document.pdf

This is achieved by simply adding a media asset link using the 'Media browser' toolbar button, then pressing enter for a new line and adding some plain text. Then without doing anything else, just save the node.

Again thanks for looking into it. I'm getting the same output with the latest version of the Media module and associated contributed modules as well.

@Joseph, when using the Simplytest, should I be enabling any other modules or making any configuration changes?

At the moment I'm leaving everything as default.

Also, can anyone point me in the right direction of where the above patch should be applied.

hmm, maybe this is a bug in the newer firefox and chrome.

Firefox 52.x here has no problem. I tested again, it looks good.

simplytest.me (expires in 23 hours from now)
https://d2b9s.ply.st/node/1

screenshot:

source:

<p>[[{"fid":"5","view_mode":"default","fields":{"format":"default","alignment":""},"link_text":"formulaire_dassurance_petits_et_grands_chiro.pdf","type":"media","field_deltas":{"5":{"format":"default","alignment":""}},"attributes":{"class":"media-element file-default","data-delta":"5"}}]]</p

@Joseph, if you edit that page and a straight after the last media item (formulaire_dassurance_petits_et_grands_chiro.pdf) hit enter, type some plain text and then save the node, do you see the plain text or a duplicate of the 'formulaire_dassurance_petits_et_grands_chiro.pdf' link?

I just logged in to your https://d2b9s.ply.st/node/1, edited the node, pressed enter and typed some plain text and got the duplication.

So maybe it is an issue with FF version, but it's also happening in Chrome.

ya ok I was able to reproduce , this time I'm using Firefox 55.0.2 (64 bit)

I wasn't able to reproduce with Firefox 52.x

this might be related to nuances between javascript engine versions.
might explain why this issue hasn't been updated for 3 years

Just letting you know that the issue is also present with the latest versions of IE11 and Safari.

Thanks again for checking and confirming the issue.

Ok, here is a patch you might want to try, I haven't tested it though so I can't say if it will work or not.
However after looking at the patch 12 for 7.x-2.0-alpha3 , I did figure out which function is involved , the filename and module changed, but the same function appears to be relevant.

See patch:

if you want, fiddle around with this function and see if you can get it to behave. Test out my patch, if it doesn't work then maybe try something yourself.

here's the alpha3 version of that function with patch 12 applied:

    replacePlaceholderWithToken: function(content) {
      var tagmap = Drupal.media.filter.ensure_tagmap();
      var i = 1;
      for (var macro in tagmap) {
        var startTag = Drupal.media.filter.getWrapperStart(i), endTag = Drupal.media.filter.getWrapperEnd(i);
        var startPos = content.indexOf(startTag), endPos = content.indexOf(endTag);
        if (startPos !== -1 && endPos !== -1) {
          if (startPos > endPos) { // Something got messed up. Clean it up.
            content = content.substr(0, endPos) + content.substr(endPos + (new String(endTag)).length);
            startPos = content.indexOf(startTag);
            endPos = content.indexOf(endTag);
          }
          // If the placeholder wrappers are empty, remove the macro too.
          if (endPos - startPos - startTag.length === 0) {
            macro = '';
          }
          content = content.substr(0, startPos) + macro + content.substr(endPos + (new String(endTag)).length);
        }
        i++;
      }
      return content;
    },

however , so much has changed since alpha3 , it's not even recognizable code.

the patch 12 added this part:

+          if (startPos > endPos) { // Something got messed up. Clean it up.
+            content = content.substr(0, endPos) + content.substr(endPos + (new String(endTag)).length);
+            startPos = content.indexOf(startTag);
+            endPos = content.indexOf(endTag);
+          }

however again, lots has changed. a new patch will probably not look much like this above code.

+ if (startPos > endPos) { // Something got messed up. Clean it up.
+ content = content.substr(0, endPos) + content.substr(endPos + (new String(endTag)).length);
+ startPos = content.indexOf(startTag);
+ endPos = content.indexOf(endTag);
+ }

I'm must admit I'm now a little confused with the reference to 'alpha3'. What does this refer to? The Media module?

I'm using version 7.x-2.11 of the 'Media' module and the code for the 'modules/media_wysiwyg/js/media_wysiwyg.filter.js' file is very different.

Current code in 7.x-2.11 looks like this:

replacePlaceholderWithToken: function(content) {
      Drupal.media.filter.ensure_tagmap();

      // Locate and process all the media placeholders in the WYSIWYG content.
      var contentElements = $('<div/>');  // TODO: once baseline jQuery is 1.8+, switch to using $.parseHTML(content)
      contentElements.get(0).innerHTML = content;
      var mediaElements = contentElements.find('.media-element');
      if (mediaElements) {
        $(mediaElements).each(function (i) {
          // Attempt to derive a JSON macro representation of the media placeholder.
          // Note: Drupal 7 ships with JQuery 1.4.4, which allows $(this).attr('outerHTML') to retrieve the eement's HTML,
          // but many sites use JQuery update to increate this to 1.6+, which insists on $(this).prop('outerHTML).
          // Until the minimum jQuery is >= 1.6, we need to do this the old-school way.
          // See http://stackoverflow.com/questions/2419749/get-selected-elements-outer-html
          var markup = $(this).get(0).outerHTML;
          if (markup === undefined) {
            // Browser does not support outerHTML DOM property.  Use the more expensive clone method instead.
            markup = $(this).clone().wrap('<div>').parent().html();
          }
                  
          var macro = Drupal.media.filter.create_macro($(markup));
          if (macro) {
            // Replace the placeholder with the macro in the parsed content.
            // (Can't just replace the string section, because the outerHTML may be subtly different,
            // depending on the browser. Parsing tends to convert <img/> to <img>, for instance.)
            Drupal.settings.tagmap[macro] = markup;
            $(this).replaceWith(macro);
          }
        });
        content = $(contentElements).html();
      }

      return content;
    },

Applying the patch @ #33 didn't resolve the issue.

From the code snippet I show above, this seems to be the offending piece of code that causes the duplication.

$(this).replaceWith(macro);

Any thoughts on what this code is meant to be doing?

Commenting out this code removes the duplication and the content is rendered correctly. Although the plain text component is getting wrapped in a <span> tag.

// Element is not an html tag. Surround it in a span element so we can
// pass the file attributes.
html = '<span>' + html + '</span>';   

Hi @Stephen Ollman, great detective work,
To answer your question, alpha3 is a release tag of a really old release of 7.x-2.x called 7.x-2.0-alpha3

So ya, I'm not exactly sure what this:
$(this).replaceWith(macro);
is doing, or if it is doing what it was originally intended to do.

Perhaps try to put insert multiple different images, see if things are broken when that line of code is removed?
Some testing /debugging will be needed to really get to the bottom of this. Keep up the great work, maybe we can get this fixed once and for all.

removing useless patch file

4. Hit the enter key and type some random text.

The "enter" event triggers a new paragraph containing duplicated media file token

<p>
    <span class="media-element file-default" data-delta="1" data-fid="********" data-media-element="1">
        <span class="file-size"></span>
    </span>
    <br>
</p>

Digging deeper, CKEditor defines the enter key (cursor in a tag) as such a behavior by default - the user can continue with the same styling and attributes when pressing the enter key to insert new lines.

For example, you are in a span and press the enter key to insert new lines, CKEditor replicates the span.

Steps to reproduce

1. Go to https://ckeditor.com/ckeditor-4/
2. Move your cursor between the link "Simply delicious newsletter" and full stop (.)
3. Press the enter key
4. Open console to inspect the paragraph
5. You will find an empty "a" tag with same attributes from the "Simply delicious newsletter" link

<p>
    <a data-cke-saved-href="https://ckeditor.com/ckeditor-4/#" href="https://ckeditor.com/ckeditor-4/#"></a>
    , sharing stories about traveling and cooking, as well as tips on how to run a successful blog.
</p>

Resetting to "needs work" since joseph.olstad does not work and has been removed/hidden by himself.

Following up on Joseph Zhao's findings that the behavior is caused by TinyMCE itself, which is duplicating the styling of the span just before the cursor when an enter is pressed, causing TinyMCE to create a new span with the same fid and according classes. The media_wysiwyg JS is then replacing those spans with Media tokens, and that's the final problem. But the root cause is in TinyMCE.

So, I debugged (the unminified version of) tinymce.js and the JS event 'keydown' is triggering the duplication. More specifically this code:

            editor.on('keydown', function(evt) {
                if (evt.keyCode == 13) {
                    if (handleEnterKey(evt) !== false) {
                        evt.preventDefault();
                    }
                }
            });

Which is ultimately ending up in createNewBlock (via handleEnterKey & insertNewBlockAfter). That function has to my surprise a very simple solution, since the code duplicating the span is this:

                    // Clone any parent styles
                    if (settings.keep_styles !== false) {
                        do {
                            if (textInlineElements[node.nodeName]) {
                                // Never clone a caret containers
                                if (node.id == '_mce_caret') {
                                    continue;
                                }

                                clonedNode = node.cloneNode(false);
                                dom.setAttrib(clonedNode, 'id', ''); // Remove ID since it needs to be document unique

                                if (block.hasChildNodes()) {
                                    clonedNode.appendChild(block.firstChild);
                                    block.appendChild(clonedNode);
                                } else {
                                    caretNode = clonedNode;
                                    block.appendChild(clonedNode);
                                }
                            }
                        } while ((node = node.parentNode) && node != editableRoot);
                    }

So, simply having 'keep_styles' in your settings will solve this!

Patch attached to set that setting from the media_wysiwyg module.

PS: The drawback of this is obviously that styles are not cloned to new paragraphs (like in Outlook or Word for example), but hey, I rather have this :-)

Minor tweak to the patch, replacing the default hook_wysiwyg_editor_settings_alter comment/documentation.

And also noting that for another editor, which has the same style cloning behavior, for example CKEditor, the patch needs to be extended. But since I already spent more than a day trying to find the code in TinyMCE responsible for that, I'm not going to search for it in CKEditor.

I haven't yet tried the above patch or reviewed it, appears to be a related issue open as well.

Appreciate reviews of the patch, thanks!

CKEditor the same thing is happening. Files embedded in bulleted list all getting duplicate file id.

@minnur which (if any) of the ones above is working for you?

Also, what version of media are you using?

have you tried upgrading to media 4.x (in a sandbox first of course, not on a production system) did a 4.x upgrade help?

@minnur & @joseph.olstad: possibly CKEditor also has a setting by which you can control whether it is duplicating styles. So if anybody could debug/trace that JS to find out where the duplicating is done exactly, we might find that setting (or trying his/her luck googling for said setting).

This issue is still occurring with the latest version of media 7.x-4.x-dev and CKEditor 4.9.2, even after deleting and recreating the wysiwyg profile and enabling "Ensure that embedded Media tags are not contained in paragraphs" in the text format as suggested in the related issue https://www.drupal.org/project/media/issues/2386133#comment-12614693

@49kyoder, I have tried that but it did not solve the issue. My suggestion to solve this issue is posted at Embedding Media in WYSIWYG Leads to Duplicate Embeds. Really appreciate feedback on that suggestion.

Patch does not work for Media module alone. Duplication issue still presents.

The patch is intended for those using the TinyMCE as a library using the wysiwyg module, media module and the media_wysiwyg (submodule of media), according to the patch writer it does not work for those using CKEditor with media, it is for TinyMCE.

For CKEditor, see the explanation here by David Rothstein: #2153851-17: When inserted into the WYSIWYG, links to files are duplicated when text surrounding them is manipulated

see suggestion also #2153851-48: When inserted into the WYSIWYG, links to files are duplicated when text surrounding them is manipulated

Recent versions of media have resolved most of peoples concerns and is compatible with entity translation, multilingual and various advanced configurations. Due to the high volume of inactive and most often irrelevant issues we are Closing this as (outdated). If for whatever reason this issue is important to you AND you still have issues after checking the media recipe documentation, then let us know and we will review your concerns.

Otherwise, see the recipe documentation for how to configure media and for troubleshooting tips OR refer to the media_dev distribution if you want to see a working media setup.

As mentioned, feel free to make some noise in this issue if you still feel it is important to you or someone else.

Thanks,

Media team