Improving CKEditor integration

This is really interesting. I read through most of the issue summary but when I got to the part about not being able to use the Caption feature of Image2, I was disappointed.

I've been working on a ckeditor_image2 module (https://www.drupal.org/sandbox/jwilson3/2454995), and would really like to see the ability to import an image into a page with Media module, and have the ability to edit the Caption in the wysiwyg editor.

From your summary, it sounded like it is technically possible to have a widget inside a widget (aka a media image widget inside the image2 figure/figcaption widget). What are the technical challenges to overcome to get this to work?

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

Thanks for this! I spent two days trying to fix the Uncaught RangeError: Maximum call stack size exceeded error and this patch fixed it and got the editor working. It sounds like the patch will allow for a lot of other great features as well, but for now I'm just delighted about the bugfix.

This needs to be rerolled after #2177893: Custom wrapper breaks tokens with CKEditor >=4.0 without widget plugin.

#10 definitely helps a lot, and it applies cleanly to 7.x-2.0-alpha1. It makes sure that an inserted picture stays with a "widget" box in CKEditor when editing content that was previously saved. Without #10 the widget box is gone after editing, and this is a problem when displaying files with the Picture module. The patch provides a "mediabox" dialog for CKEditor which I can't make work. Probably this has to do with the Picture module since I tracked things down as far as line 119 of mediabox.js, which says

      // Currently only supporting these options for images.
      if (widget.element.getName() != 'img') {

, but updating that conditional to allow for picture elements left things broken. An unfortunate workaround is to disable the dialog in the theme's ckeditor.config.js:

CKEDITOR.on( 'dialogDefinition', function( ev ) {
  var dialogName = ev.data.name;
  var dialogDefinition = ev.data.definition;

  if (dialogName == 'mediabox') {
    var contents = dialogDefinition.getContents( 'info' );
    contents.remove( 'height' );
    contents.remove( 'width' );
    contents.remove( 'align' );
    contents.remove( 'elementStyle' );
    contents.elements = [
      {
        type: 'html',
        html: '<p>To edit this image please insert it again and then delete the old one.</p>'
      }
    ];
    dialogDefinition.onShow  = function(){};
  }
});

In #2620648: Extra p tags inserted when editing content with embedded images. I explain that a line can be added to make sure the widget is inline with the "picture" element, preventing extra "p" from getting inserted.

Added one line to patch in #10 to make inserted picture element wrapped in an inline widget element. Solves #2620648: Extra p tags inserted when editing content with embedded images.. Applies against 7.x-2.0-alpha1.

applied 2513454-17-improving-ckeditor-integration.patch to version = "7.x-2.0-alpha1"
now i get "Uncaught TypeError: Cannot read property 'align' of undefined" in the console log, body text is not loading.

Because I need media and media_ckeditor for a current project, I've been posting a lot of small patches to this issue queue, many of which I suspect are improvements that may have been contained in this rewrite. I think it would be good if we could turn this into a "meta-issue" and have the code changes actually be in separate issues. Otherwise I think this ticket will be stuck always needing a re-roll.

To start, if anyone could list the improvements/fixes this patch provides, I can at least try and point to separate patches that might duplicate the feature.

A new patch which apply against 7.x-2.0-alpha2.

It is committed to 7.x-2.x dev
please test the latest 7.x-2.x dev to confirm

Can anyone summarize what features and/or fixes this patch provides?

@brockfanning this commit is not in a release (only in dev). as for a summary, from what the description says this patch makes use of CKEditor's API rather Media's JS library when capable. I'd consider this commit a high risk commit, this is why I have no immediate plans to push a release with this.

Right now I'm going to be heavily putting 7.x-2.0-alpha2 release through its paces which doesn't have this commit. going to run some more tests on 7.x-2.x dev before making anything out of it.

I tested the latest dev last night for a few hours and I'm running into issues related to this 'mediabox'.

For example, if I copy paste a media token from one node to a new one into the "Source". And flip back to wysiwyg mode I get an error on ".getOuterHtml();" because wrapElement returns null on this code:

   widgetWrapper = editor.widgets.wrapElement(
                    new CKEDITOR.dom.element.createFromHtml(
                      '<span class="media-element media-element-token">' + media + '</span>',
                      editor.document
                    ),
                    'mediabox').getOuterHtml();

I'm not sure what mediabox is supposed to be but it's broken.

@ptsimard: Does that procedure work without this patch? Say, if you go back to commit ffa8be3cd1c38efa8932dc02b333df3a6d877e19
I have a feeling that it may still give you an error (though possibly a different one).

@joseph.olstad: I think it's going to be tough to test this without knowing what exactly it's supposed to do. From the code of the patch it seems to be largely new functionality, but with a few scattered changes here and there. Since the issue description was written, media_ckeditor has split out from Media and has implemented Ckeditor's Widget API. So I feel like this is in need of an updated explanation, preferable detailing the problem it is solving and the changes that it makes.

Based on @ptsimard tests We should probably rollback the commit. Maybe a new branch or project for the major changes if at all.
Thx

reverted commit 1d8c47a0f

The two issues I had which this patch was solving are:

• Ability to resize the inserted image
• Ability to float left or right the embedded image

I am sure the floating left and right functionality can be added by implementing hook_media_wysiwyg_format_form_prepare_alter() then adding some template preprocess. The thing I like about the approach of the patch it's using almost the same idea as if you were embedding the image through wysiwyg and it much user friendly.

@ptsimard can you tell us the version of ckeditor library, media module and ckeditor module you are using? May be it might help to reproduce the error.

All this patch did for me was failing at a different step in the process. Before It was failing at something liek TagName function or somesuch. Sorry I'm not in front of this code right now and can't be more specific.

Regardless of this patch or not I was getting errors before when copying a token in source mode (or renaming a file id in a media token, to test) switching back out of source mode and the whole wysiwyg would be blank.

@nikathorne: I'm using the latest ckeditor, I think 4.5.11 via the CDN and associated widget/line utils plugins. Also the latest Media beta. AlsoI'm testing with files, not just images. Are you using the image2 plugin or something when you say "Ability to resize the inserted image"?"

@joseph: Please don't be too quick on the commit and or revert buttons. We need to discuss the issues before making big sweeping changes.

Maybe my setup is wrong but in general the whole media + media_ckeditor setup is broken for me. So very very brittle and unreliable.

@nikathone : without using this patch I seem to vaguely recall successfully testing float right or left the embedded image in a vanilla install the other day on simplytest.me using the latest releases of media , media_ckeditor , ckeditor , multiforms and file_entity , d7 core 7.51 . I did not need this patch to do that from my memory, however I would like to run some more tests to figure out exact configuration I used.

@ptsimard

here is some documentation for setting this up, may help out

Setting up WYSIWYG and CKEditor to include Media Browser / Embed Media button

@joseph ... that documentation has been updated in 2014... and reference ckeditor 3.x so not very helpful. Setting this properly is a lot more involved that what that guides mentions. It also mentions installing the WYSIWYG module.. Which I'm not using. I'm using the standalone ckeditor module. This guide predates the media and media_ckeditor split.

The last comment refers to unzipping everything.. are you saying that you can't use it with default way of using CDN from ckeditor module?

Sorry if I sound harsh. I'm just very salty about this whole broken D7 media experience.

@ptsimard, see this comment 5 months ago

https://www.drupal.org/node/1882314#comment-11287061

kingfisher64 commented 5 months ago

Download and install https://www.drupal.org/project/media_ckeditor

1) configuration > content authoring > text formats > enable filter (convert media tags to markup) > save.
2) configuration > content authoring > ckeditor > full or advanced > edit > editor appearance, drag turquoise button which is what we have just enabled in the filter into toolbar.

I think they have simply removed it from x2 branch of media.

@joseph:

2) configuration > content authoring > ckeditor > full or advanced > edit > editor appearance, drag turquoise button which is what we have just enabled in the filter into toolbar.

So I did not see this step... I have a diferent Add Media button (not turquoise). So I went there and added the MediaEmbed turquoise button to the toolbar. But it never shows up on any edit page. I cleared cache a zillion time. Doesn't show up.

So maybe it's related to #2567275: Media plugin is not able to detect widget plugin registration properly?

I'll update the documentation soon, I'm working on switching one if my clients sites wysiwyg from TinyMCE to ckeditor and we're using media and we'll need media_ckeditor. I'm not sure how far I'll get friday though.

As for that other issue, I just queued the patch for #2567275: Media plugin is not able to detect widget plugin registration properly for testing

@ptsimard

actually I did notice what you described on a wetkit distro client, their site is heavily customized now, but still has TinyMCE and wetkit_wysiwyg , I tried uninstalling wetkit_wysiwyg and I could not get the widget to go in, I'm thinking there's some other problem.

I'll run some more tests on http://simplytest.me but on a vanilla D7 , the other tests I ran seemed to work ok the other day but I don't recall adding the media widget to fields. Not sure if this is related to this issue or not.

Followup soon.

Patch #21 needs a reroll, hunk #3 is failing on latest dev 7.x-2.x .

patch -p1 < 2513454-21-improving-ckeditor-integration.patch 
patching file js/plugins/media/dialogs/mediabox.js
patching file js/plugins/media/library.js
patching file js/plugins/media/plugin.js
Hunk #2 succeeded at 406 with fuzz 2 (offset 8 lines).
Hunk #3 FAILED at 538.
Hunk #4 succeeded at 686 with fuzz 1 (offset 44 lines).
Hunk #5 succeeded at 747 with fuzz 2 (offset 41 lines).
1 out of 5 hunks FAILED -- saving rejects to file js/plugins/media/plugin.js.rej

Ok, I refactored / rerolled patch #21
This patch is to improve integration with the image 2 plugin and the contrib module ckeditor_image2

I haven't yet tested this patch but I am hoping to get it to work so that we can use the image2 plugin with media_ckeditor .

see patch:

Reviewing patch #42 ,
Getting this javascript error:

TypeError: a.split is not a function

and image2 plugin shows up as 'array'

Needs work.

This patch might work.

using ckeditor latest release 4.6.2 , with this patch 46 for media_ckeditor , using a version of recipe 9, I get this error:

TypeError: a.split is not a function[Learn More]  ckeditor.js:248:103
	CKEDITOR.resourceManager.prototype.addExternal sites/all/libraries/ckeditor/ckeditor.js:248:103
	Drupal.ckeditorLoadPlugins sites/all/modules/ckeditor/includes/ckeditor.utils.js:219:9
	Drupal.ckeditorInit sites/all/modules/ckeditor/includes/ckeditor.utils.js:141:27
	Drupal.ckeditorOn sites/all/modules/ckeditor/includes/ckeditor.utils.js:182:7
	attachCKEditor/< sites/all/modules/ckeditor/includes/ckeditor.utils.js:292:9
	c</<.each misc/jquery.js:34:44
	c</b.prototype.each misc/jquery.js:27:386
	attachCKEditor sites/all/modules/ckeditor/includes/ckeditor.utils.js:285:5
	Drupal.behaviors.ckeditor.attach sites/all/modules/ckeditor/includes/ckeditor.utils.js:346:7

and the ckeditor fails to load.

checking the minimized code on ckeditor.js
return CKEDITOR.getUrl(this.getPath(a)+(d?d.file:this.fileName+".js"))}

so something is wrong with the patch.

Here's a screenshot from the config page, this gives an idea of what the problem manifests as (Array) for form-item-loadPlugins-image2:

/admin/config/content/ckeditor/edit/Advanced

if disable ckeditor_image2 module that the patch is supposed to integrate with, the 'Array' item becomes 'image2'

could be something wrong with the ckeditor_image2 module.

Update:

I ended up using imce to provide image properties .

without any patch just using the media_ckeditor I was able to get image2 plugin to apply to the media browser file entity by clicking 'source' and then going back to 'wysiwyg' mode, however after saving and then new draft and editing the image the media browser could no longer find the file entity .

So I ended up disabling the image2 plugin and for that functionality left and right float of image I'm using the imce plugin with the default image plugin, media browser image properties is not enabled however there's a workaround I'm looking into for that.

so basically my client for now is going to be using imce and the image plugin to do things that currently aren't out of the box with media embed , and media embed for things that imce cannot do.

There's another ticket for media float right and left , so we should followup with that.

Postponning this issue in favour of:

#2842391: better support for float media left and float media right

there's lots of good information in this issue to help with
#2919974: Media_WYSIWYG css is not loading in CKeditor iFrame

Helpful information in this issue, however prefer to continue work in :
#2919974: Media_WYSIWYG css is not loading in CKeditor iFrame
instead

@joseph.olstad whoops, you've closed this as a duplicate of itself!

I'm not sure if you meant to close as a duplicate of #2842391 or of #2919974 since you've mentioned both in your last couple comments.

Thanks, I updated my comment to link to
#2919974: Media_WYSIWYG css is not loading in CKeditor iFrame