When CKEditor library is used with any download configuration that does not include the widget plugin, and the ckeditor version is greater than 4.0, the media_ckeditor plugin in wysiwyg_plugins in media_wysiwyg submodule improperly converts media elements to and from tokens when the editor binds and unbinds. This is for implementations using the ckeditor module in conjunction with media and the ckeditor library.

In the media_ckeditor plugin an approach is attempted to wrap media elements with a custom element. This was introduced here: https://drupal.org/comment/7766639#comment-7766639 This approach causes elements (documents) to be stripped out and lost in prepareDataForSourceMode.

By the end of the above issue a decision was made to break ckeditor module support into media_wysiwyg submodule, and in the process integrate ckeditor module token handling more closely with media's implementation for wysiwyg module. From my debugging, it looks to me like the mediawrapper approach is no longer valid because the filtering happens upstream in media_wysiwyg.filter.js. Functions replaceTokenWithPlaceholder and replacePlaceholderWithToken handle all the tokens in the editor and so any custom wrappers would have to be implemented there, or those functions would need to be copied, extended, and utilized in the media_ckeditor plugin. Much work is currently ongoing to improve macro handling, which covers converting tokens in media_wysiwyg.

Proposal

Remove custom wrapper from media_ckeditor for configurations that do not contain the ckeditor library widgets plugin but do contain a version of ckeditor library greater than 4.0. Also, investigate the use of the custom wrapper for ckeditor library configurations with the widgets plugin and determine if this approach still makes sense there. If it does not, remove it as well. This will fix bugs and utilize efforts underway elsewhere in media_wysiwyg to improve working with media elements and tokens.

CommentFileSizeAuthor
#18 media-ckeditor4-media-plugin-2177893-18.patch4.53 KBLittleRedHen
#14 interdiff.txt527 bytesSpadXIII
#14 media-ckeditor4-media-plugin-2177893-14.patch5.15 KBSpadXIII
#13 interdiff.txt638 bytesdrclaw
#13 media-ckeditor4-media-plugin-2177893-13.patch5.78 KBdrclaw
#12 interdiff.txt767 bytesdrclaw
#12 media-ckeditor4-media-plugin-2177893-12.patch5.86 KBdrclaw
#11 interdiff.txt3.6 KBdrclaw
#11 media-ckeditor4-media-plugin-2177893-11.patch5.1 KBdrclaw
#4 interdiff.txt841 bytesjoelpittet
#4 2177893-4-ckeditor4-media-plugin.patch3.24 KBjoelpittet
#2 media-ckeditor-remove-mediawrapper-2177893-2.patch2.92 KBkevinchampion
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kevinchampion’s picture

kevinchampion CreditAttribution: kevinchampion commented

My configuration is described by the following make file:


; Contributed modules.
projects[ckeditor][version] = "1.x-dev"
projects[ckeditor][type] = "module"
projects[ckeditor][subdir] = "contrib"
projects[ckeditor][download][type] = "git"
projects[ckeditor][download][revision] = "57245a9"
projects[ckeditor][download][branch] = "7.x-1.x"
; Integration with Media 2.x
; http://drupal.org/node/1504696
projects[ckeditor][patch][1504696] = "http://drupal.org/files/issues/ckeditor-accomodate-latest-media-changes-2159403-6.patch"
projects[ckeditor][patch][1649464] = "http://drupal.org/files/issues/ckeditor-hook_into_media_admin-1649464-8141819_0.patch"
projects[file_entity][version] = "2.x-dev"
projects[file_entity][type] = "module"
projects[file_entity][subdir] = "contrib"
projects[file_entity][download][type] = "git"
projects[file_entity][download][revision] = "e80b223"
projects[file_entity][download][branch] = "7.x-2.x"
projects[media][version] = "2.x-dev"
projects[media][type] = "module"
projects[media][subdir] = "contrib"
projects[media][download][type] = "git"
projects[media][download][revision] = "c3cda2b"
projects[media][download][branch] = "7.x-2.x"
projects[media][patch][2126755] = "http://drupal.org/files/issues/media-wysiwyg-improve-our-macro-handling-2126755-58.patch"

; Libraries.
libraries[ckeditor][download][type] = "file"
libraries[ckeditor][download][url] = "http://download.cksource.com/CKEditor/CKEditor/CKEditor%204.3.1/ckeditor_4.3.1_full.zip"
libraries[ckeditor][directory_name] = "ckeditor"

Update: removed patch which wasn't applying cleanly

; External plugin declarations are redundant.
; http://drupal.org/comment/8284591#comment-8284591
projects[ckeditor][patch][2158741] = "http://drupal.org/files/issues/ckeditor-remove-external-plugin-declarations-1-alt.patch"
kevinchampion’s picture

kevinchampion CreditAttribution: kevinchampion commented
Status: Active » Needs review
FileSize
media-ckeditor-remove-mediawrapper-2177893-2.patch2.92 KB

This patch removes the mediawrapper usage when ckeditor library does not inlcude the widgets plugin and is greater than version 4.0.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

This is so good. Gave it a test and it works great.

Read through the patch and other than the nice little formatting cleanups everything looks very straight forward.

Setup:

  • media 7.x-2.0-alpha3+39-dev
  • media_wysiwyg
  • mediaelement 7.x-1.2+2-dev
  • file_entity 7.x-2.0-alpha3+10-dev
  • ctools 7.x-1.3+4-dev
  • ckeditor module 7.x-1.13+12-dev
  • CKEditor library 4.3.2
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Reviewed & tested by the community » Needs review
FileSize
2177893-4-ckeditor4-media-plugin.patch3.24 KB
interdiff.txt841 bytes

Probably a bit hasty, may need a few more eyeballs... anyways here's a small patch to clean the code a bit more.

PI_Ron’s picture

PI_Ron CreditAttribution: PI_Ron commented

I'm still unable to successfully embed files via media browser using above configuration. The file browser also does not close on submit and you have to click invisible close button in top right.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@PI_Ron that doesn't sound sound at all like it's related to this issue, maybe considering another?

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Just an update this still helped me out with the latest dev:

  • ctools 7.x-1.4
  • file_entity 7.x-2.0-alpha3+13-dev
  • media 7.x-2.0-alpha3+51-dev
  • media_wysiwyg
  • mediaelement 7.x-1.2+2-dev
  • ckeditor module 7.x-1.13+12-dev
  • CKEditor library 4.3.3 full

With this patch in #4:#2177893-4: Custom wrapper breaks tokens with CKEditor >=4.0 without widget plugin and #2159403-23: Make CKEditor plugin system modular and clean applied to ckeditor module.

One thing that drove me nuts was forgetting in my full text format to enable "Convert Media tags to markup"

Also for what it's worth I've got ACF disabled on CKeditor. I've got the plugin " Plugin for inserting images from Drupal media module " and "Plugin for inserting Drupal embeded media " turned on but may not be needed. And added "config.allowedContent = true;" to my Custom JavaScript configuration

Hope that helps anybody wandering down this path.

kevinchampion’s picture

kevinchampion CreditAttribution: kevinchampion commented

With so many outstanding issues and little movement happening, and with how many different configurations and interchanging parts there are, it's really difficult to track a working setup. For instance, with media alone I'm now running this build:

projects[media][version] = "2.x-dev"
projects[media][type] = "module"
projects[media][subdir] = "contrib"
projects[media][download][type] = "git"
projects[media][download][branch] = "7.x-2.x"
; Picking a commit from latest dev past 2.0-alpha3 as of 2014/01/21 so we can
; avoid new development breaking functionality on new builds until we're ready
; to advance the version or commit forward.
projects[media][download][revision] = "2f828ea761103c49197a50aaeac9b98a350a559b"
projects[media][patch][2126755] = "https://github.com/kevinchampion/imagex_patches/raw/update-media-patch/contrib/media/media-wysiwyg-improve-our-macro-handling-2126755-78.patch"
projects[media][patch][2177893] = "https://raw.github.com/kevinchampion/imagex_patches/update-media-patch/contrib/media/media-ckeditor-remove-mediawrapper-2177893-4.patch"
projects[media][patch][0] = "https://github.com/kevinchampion/imagex_patches/raw/update-media-patch/contrib/media/media-ckeditor-bind-unbind-stripping.patch"

Of particular note is the the last patch which is now more like a glue patch than anything. It cleans up some logic, removes some legacy code, and fixes some problems I was having with file documents on editor unbind/bind.

https://github.com/kevinchampion/imagex_patches/blob/update-media-patch/...

Things have gotten to the point that I'm finding it hard to tease out particular fixes and create distinct issues and patches for them because there are so many patch dependencies.

@joelpittet is right too, that there are a lot of UI configurations between the text format and the ckeditor profile that can cause things to break. Items that have tripped me up are:

  • make sure advanced content filter is disabled in ckeditor proflie
  • make sure to disable media embed plugin
  • make sure the media token filter in your text format is enabled and listed as the first/topmost filter
  • be sure to check and audit permissions, being unable to submit media browser could be problem there
  • make sure to check for console errors when hitting that submit button, it could be problems with jquery_update or some other reason causing javascript to break on the page

It goes on and on...

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB commented

Thanks kevinchampion, the follwing patch was my missing link:
https://github.com/kevinchampion/imagex_patches/raw/update-media-patch/c...

I now got everything to work with the following patches:

  • sites/all/modules/contrib/ckeditor/ckeditor-accomodate-latest-media-changes-2159403-39.patch
  • sites/all/modules/contrib/media/2177893-4-ckeditor4-media-plugin.patch
  • sites/all/modules/contrib/media/media-wysiwyg-improve-our-macro-handling-2126755-92.patch
  • sites/all/modules/contrib/media/media-ckeditor-bind-unbind-stripping.patch

Hope this helps someone else fix Media work with CKeditor.

hefox’s picture

hefox CreditAttribution: hefox commented

Patch is being used on a project with multiple testers/developers and has not presented any problems yet and has fixed the issue.

drclaw’s picture

drclaw CreditAttribution: drclaw commented
FileSize
media-ckeditor4-media-plugin-2177893-11.patch5.1 KB
interdiff.txt3.6 KB

There was some question of whether or not the custom mediawrapper is even needed when there is widget support. With #2126755: Improve Media's WYSIWYG Macro handling now committed, I don't believe that it is. Attached patch is an initial attempt. For now, I've only removed the wrapper when there's widget support. I didn't want to mess with the legacy wrapper support handling and I don't have time to test on a ckeditor instance without widget support.

Here's my current setup:

drclaw’s picture

drclaw CreditAttribution: drclaw commented
FileSize
media-ckeditor4-media-plugin-2177893-12.patch5.86 KB
interdiff.txt767 bytes

Last patch is bunk. Use this one instead.

drclaw’s picture

drclaw CreditAttribution: drclaw commented
FileSize
media-ckeditor4-media-plugin-2177893-13.patch5.78 KB
interdiff.txt638 bytes

Aaaaaaand third time's a charm. :/

SpadXIII’s picture

SpadXIII CreditAttribution: SpadXIII commented
Related issues: +#2328493: Async behavior of wysiwyg plugin attach/detach leads to some media tags not being replaced in time
FileSize
media-ckeditor4-media-plugin-2177893-14.patch5.15 KB
interdiff.txt527 bytes

Just rerolled last patch for the latest dev. The first part was already merged in https://www.drupal.org/node/2328493

I didn't review the patch in depth, but it seems to fix my problems with ckeditor 4.2.2 and 4.4.6. So I'll leave this on needs review.

izmeez’s picture

izmeez CreditAttribution: izmeez commented

Thank you so much. This is the patch I needed to get ckeditor module to embed media into body.

aDarkling’s picture

aDarkling CreditAttribution: aDarkling commented
Status: Needs review » Reviewed & tested by the community

Worked for me too!

I still have a problem with multiple -tag content being inserted incorrectly, but I think that its outside the scope of this patch. Stepped through the code & the problem only occurs after it hits ckeditorInstance.insertElement.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Project: D7 Media » Media CKEditor
Version: 7.x-2.x-dev » 7.x-1.x-dev
Component: Media WYSIWYG » Code
Status: Reviewed & tested by the community » Needs work

We are deprecating the ckeditor integration and moving it to a separate module.

LittleRedHen’s picture

LittleRedHen CreditAttribution: LittleRedHen commented
Related issues: +#2364559: Link / Icon to media document is stripped out by ckeditor before save or when switching to markup view
FileSize
media-ckeditor4-media-plugin-2177893-18.patch4.53 KB

Re-rolled the patch from #14 against the dev branch of the new ckeditor_media module.

This patch contains the fix for the problems with formatting being stripped that was discussed at https://www.drupal.org/node/2364559

rooby’s picture

rooby CreditAttribution: rooby commented
Status: Needs work » Needs review
tanc’s picture

tanc
he/him
Primary language English
Location Italy
CreditAttribution: tanc at Agile Collective commented

Patch applies cleanly but it seems like plugin.js isn't being used somehow. I can see that its loaded by the browser but putting console.logs in there doesn't produce any output. I still see issues with media tokens not being saved as rendered elements rather than tokens in the database.

tanc’s picture

tanc
he/him
Primary language English
Location Italy
CreditAttribution: tanc at Agile Collective commented

Apologies, plugin.js is definitely being loaded. I needed to remove the button from ckeditor and then re-add it for it to start working (I think). Anyway, I've still got the problem of tokens only working the one time. Adding an image through the media button and then swapping between wysiwyg mode and plain text mode fails to re-add the token in wysiwyg mode. Instead the rendered html is kept.

I can see that swapping to plain text mode the first time triggers the downcast function and the token is correctly generated. Then when swapping to wysiwyg mode the rendered html is added in upcast. Finally swapping back to plain text does not create a token successfully.

tanc’s picture

tanc
he/him
Primary language English
Location Italy
CreditAttribution: tanc at Agile Collective commented

Hmm, just realised this issue is with ckeditor without widget plugin support. I've been testing with widget plugin.

tanc’s picture

tanc
he/him
Primary language English
Location Italy
CreditAttribution: tanc at Agile Collective commented
Status: Needs review » Needs work

Right! Using ckeditor without the widget plugin does work successfully. So my conclusion is that the patch makes ckeditor work nicely with media tokens as long as you don't include the widget plugin in your ckeditor library. Looks like there is some work to be done to get the widget plugin working. Should this be in a different issue? Marking this as needs work for now.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
mkhamash’s picture

mkhamash CreditAttribution: mkhamash at Vardot commented

Yes @tanc the ckeditor work great with current latest dev version from media +media_ckeditor, without the widget ckeditor plugin, and without this patch, adding the ckeditor widget plugin will break media tokens and convert them to img tags when switching to source view or just edit them form image ckeditor plugin, and the current patch does not solve the problem.

rooby’s picture

rooby CreditAttribution: rooby commented

I have had the same experience.

Running latest recommended media & media_ckeditor along with latest ckeditor dev...

When I use no patches and ckeditor + widget plugin I end up with "too much recursion" errors as per #2420765: Media filter causes "too much recursion" JS error in CKEditor after updating to latest dev.
When I remove the widget plugin and add this patch it seems to work okay, with images transitioning correctly between tokens and HTML.

Devin Carlson’s picture

Devin Carlson CreditAttribution: Devin Carlson as a volunteer commented
Status: Needs work » Fixed

Let's work on improving the Widget API support in a separate issue.

I tested #18 with the latest HEAD of Media, File Entity and CKEditor in combination with the latest CKEditor library, with and without the Widget API available. The patch improved up/downcasting of the token and didn't cause any separate issues.

Committed to Media CKEditor 7.x-2.x.

Status: Fixed » Closed (fixed)

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

rooby’s picture

rooby CreditAttribution: rooby commented

So do we have a follow up issue as mentioned in #27?

I'm using latest dev and tokens are changed to markup but they still never change back.

This is a pretty critical bug.

[edit] I see it does work reasonably well without the widget plugin, which is a bit of an issue since the README tells you to use the widget plugin.