Add Drupal plugin support for FCKeditor

Sorry, Image Assist is currently supported via TinyMCE only. We are working on FCKeditor integration already, but that still requires a lot of work. We will use this issue for further coordination.

Subscribing.

Starting point.

Drupal plugins almost working! :)

I'll not be able to work on this until the weekend, so if anyone wants to take over, please assign the issue to yourself.

Moving to Wysiwyg API's queue.

Subscribing.

I'm currently digging into this issue, so I'll assign it to me.
I've only been able to make the img_assist button open the dialog so far, but nothing is visible yet because the throbber is still there.
Image Assist is currently hard-coded to include a JavaScript file for tinymce, and I'm a bit unsure on how to proceed.

If I've understood the final vision correctly, editor plugins should not need to worry about which editor is actually invoking it. So, Image Assist will require some modifications in parallel to this wysiwyg patch as it's not yet editor independent. And that's where I'm stuck at the moment, trying to figure out how much needs to be changed/hacked in Image Assist, before I can continue here.

I'm concentrating on the Break plugin instead and making progress.
My first concern was getting plugin-specific stylesheets into the editor area, which had to be done after it had loaded.
This seems to be working, but at the moment it requires an extra iteration over all available "Drupal plugins" in FCKeditor's FCKeditor_OnComplete special event function to insert them.

Second task was to attach event listeners to FCKeditor which should fire the attach/detach methods for each plugin, to keep data consistent when switching between filter modes.
I'm fiddling around with the various FCKeditor events available to see which are most fit for changing the contents when plugins are attached/detached, without causing infinite loops...

I can't make a good patch with the updates yet as I've applied some other patches to ease testing, but maybe tomorrow.

Note that we are passing the "local" FCKeditor instance as 'instance' to Drupal.wysiwyg.editor.instance.fckeditor.addPlugin(), so we have full access to its configuration. (I left those "DEBUG" lines exactly for this reason)

Still no patch, sorry. But I think I've got a good implementation for attaching/detatching plugins now.

I set up event listeners for FCK_STATUS_COMPLETE, which only happens after the editor is done loading, and it's safe to modify the contents.

As FCKeditor does not have any events to tell when it has been removed, or even a function call to do so, I decided to simply call instance.SetStatus(FCK_STATUS_NOTLOADED) and then instance.UpdateLinkedField() whenever the editor is detached. An OnStatusChange event will fire on the first call, which each plugin captures and does its thing on.
The later call makes sure any final modifications from the plugins are put into the textarea.

I noticed that setting the editor area's stylesheets in FCKeditor_OnComplete was not such a good idea. They will be deleted each time someone does a SetHTML etc, because the entire inner editor area document is recreated then. It would be possible to set up an event listener for OnAfterSetHTML and then re-inject the stylesheets, but I didn't like that for some reason.
I think it was because it's not possible to call the function to dynamically inject stylesheets until after the FCKeditorAPI is initialized, at which point we already need the stylesheets to be there.
So I now inject them into the settings in Drupal.wysiwyg.editor.attach.fckeditor, by appending each plugin's file to FCKinstance.Config.EditorAreaCSS. That way FCK will always include the plugin css, no matter when or how many times the editor area is recreated.

You (Sun) mention that the argument "instance" (which is a reference to the editing area window, but we can't get an actual editor instance object via it as the API is not initialized) is passed to Drupal.wysiwyg.editor.fckeditor.addPlugin, and that it could be used to access the configuration. I tried that for EditorAreaCSS, and it didn't work for the reason outlined below.
I'm also not so sure adding an extra argument for just FCKeditor is such a good idea, it makes it inconsistent with the other editors.

So I've been thinking about the current settings implementation a bit.

From the FCKeditor docs:

Configurations Loading Precedence

When overriding configurations, the following steps are taken:

1. The configurations in the main configuration file (fckconfig.js) are loaded.
2. The configurations are overridden by the settings in the custom configuration file (if provided).
3. The configurations are finally overridden by the settings done inline in the editor page, except for the "CustomConfigurationsPath", which is set right after step 1.

You don't need to include all configuration options in your custom file, just those you want to change. Your file will "override" the default one.

When FCKinstance.ReplaceTextarea() is called FCK stores everything set to FCKinstance.Config in a hidden field in the main page.
There is NO direct reference available to the FCKinstance object after this as it goes out of scope, but we really shouldn't need it anymore.
The only way to get a new reference to the "real" FCKeditor object is via the FCKeditorAPI, but that's not initialized until after everything below.

Then it loads the actual editing area iframe along with the default fckconfig.js.

Then it parses the hidden field into FCKConfig.PageConfig, with the exception of CustomConfigurationsPath.

The file from CustomConfigurationsPath is loaded, fckeditor.config.js in our case, to make step 2 above possible.

Then it re-inserts everything from FCKConfig.PageConfig into FCKConfig, to make step 3 above possible.

This makes it impossible to set say EditorAreaCSS from fckeditor.config.js or from within Drupal.wysiwyg.editor.instance.fckeditor.addPlugin. It was set in Drupal.settings.wysiwyg.configs.fckeditor['input-format'], which was put in FCKConfig.PageConfig.EditorAreaCSS and that will override whatever I put in FCKConfig.EditorAreaCSS.

Making changes to FCKConfig.PageConfig sounds like an ugly hack to me as it means we're actually overriding properties set directly on FCKInstance.

I'm trying to figure out a better way to do this, but work and things at home are taking up too much time at the moment...

Sorry if this is all a bit confusing. I just want to get a discussion going but we don't seem to be online at the same time in #drupal-wysiwyg.

In general, what you have outlined here was my reasoning to invoke

Drupal.wysiwyg.editor.instance.fckeditor.addPlugin()

from within fckeditor.config.js, so we have the full editor instance available. I manually stepped through the FCKeditor API, and exactly the way how it

- processes the .Config values into the hidden input form field,
- extracts them into the local FCK.Config instance in the iframe (which supports only simple data types, no arrays and stuff),
- and finally overloads that configuration with what has been defined in the custom configuration file,

makes it impossible to dynamically adjust the local editor instance in the iframe as well as dynamically registering plugins at runtime.

That said, I do not worry so much about different function signatures in Drupal.wysiwyg.editor.instance.fckeditor.addPlugin() - because this is rather a private function of Wysiwyg API. It is (and should only be) used by the editor implementation itself. At least, I cannot see how any other module or JavaScript could invoke it - I mean properly, at the right time, with the proper data, etc... We could move the additional 'instance' argument to the end though.

Here's finally a patch. Most things are working as expected, but I didn't include the dialog stuff yet. Postponing that until the rest is working.
I've moved a few things around but I don't have time to list all the reasons for it now. There are some comments in the code though.

Ideas are welcome as I've run into somewhat of a dead end on updating content modified by plugins because of an issue with running many GetXHTML/SetHTML calls in short time.

Update since #12 (...-4.patch)

I've got some updates to the patch in #12 based on what Sun and I discussed about some time ago.
Those are in wysiwyg-HEAD.fckeditor-drupal-plugins-4.patch. Still nothing on dialogs as there are a few things which need to be discussed first. Plugins not using dialogs (like Teaser Break) should work now.
I've only been able to test them with a single editor instance running yet so I need some help with that.

I've noticed a bit of a hiccup when the editor loads in some circumstances. If I close Firefox (3) with some tabs open, one of them containing the editor, and then restart it I'll get two messages about missing toolbars for img_assist and break. But I can't reproduce this when simply opening/closing a single tab, reloading the page with cache off or anything like that. Just when the browser opens. The editor loads fine but without those two buttons. If I refresh the page, they're back. (Problem still exists on the alternate approaches below.)

Alternative approach no 1, group plugin calls (...-alternative.patch)

Because of the problems with calling SetData multiple times in a row (the editor area has no time to reload) I altered the code around a bit to group calls to the plugin attach/detach methods to do a single SetData after they've all run. This works great so far and completely eliminates the problems I had. There's still a small risk of the same problem coming back if a native plugin decides to call SetData when the editor has just loaded, but I think we can risk it.
I also needed another approach to shutting the editor down, as setting FCK_STATUS_NOTLOADED could lead to the editor restarting in some cases. So I created a custom status code for that. Seems to be working, but I figured I could use some input on it, so I didn't include that in ...plugins-4.patch. It feels like a less risky process as there's a guarantee that nothing in FCKeditor will react to that specific state.
This testcase can be found in wysiwyg-HEAD.fckeditor-drupal-plugins-alternative.patch and also includes the relevant changes since #12.

Alternative approach no 2, Data Processors (...-dataprocessors.patch)

Sun mentioned that the FCKeditor module used a "Document Processor" (FCKeditor object) to implement the Image Assist plugin. The pros are that their code will instantly work when switching between Source/Wysiwyg mode inside FCKeditor, and it does not contain a SetData call as it works directly on the DOM. The cons are that the Drupal plugins know nothing about the DOM and work only on an XHTML string, while the Document Processors are only handed the Wysiwyg area DOM. FCKeditor has also already been escaping comments, scripts and anything else it does not want to run inside the editor area, meaning we would need to replicate all the work FCKeditor does to serialize all this back to a string, let the plugins do their thing, escape the 'bad stuff' and then recreate the DOM again.

But I later found out that FCKeditor also has a class called a Data Processor, which is what does the conversion to/from XHTML in calls to Set/GetData. This can be overridden, and we can use it to let the plugins do their thing and let the old Data Processor do the conversion before or after that. The beauty of it is that switching between Source/Wysiwyg mode will work out of the box for all plugins altering the source.
I implemented it in the config file for now as it was the easiest thing to do.
This patch is built on ...-plugins-4.patch as well and can be found in wysiwyg-HEAD.fckeditor-drupal-plugins-dataprocessor.patch.

I personally like the dataprocessor approach best, as it is the most safe thing to do. (Note, the two later patches are just quick tests/proposals and have had very little actual testing.)

Committed, erm, a mixture of those patches, but only the parts that did not change so far, to all 2.x branches.

An event handler was left out from the ...-4 patch.
Inserting teaser breaks will work, but those already in the contents will not show up, nor will the inserted placeholder image be converted back to the break-comment when the editor is detached.
I'll fix this in a patch tomorrow, and reroll the other alternatives against the current HEAD, it's getting too late over here to do it now.

Does / will this issue resolve missing support for IMCE + IMCE Wysiwyg API bridge is installed?

Currently it seems that wysiwyg.module @ 6.x-1.1 fails to load the plugins offered via IMCE Wysiwyg API bridge because of this check:

  // Assume that this editor does not support neither native external plugins
  // nor Drupal plugins if it does not provide a callback.
  if (!(isset($editor['plugin settings callback']) && function_exists($editor['plugin settings callback']))) {
    return;
  }
 

In plugins/fckeditor.inc there is no plugin settings callback set for the stable version.

@xurizaemon: Good catch. Yes, this change is already in the latest development snapshot since today. So imce_wysiwyg module will work with it.

After much consideration and pulling of my hair, I think we should absolutely go for the Data Processor approach.

The aim for these patches have been to do plugin attachment and detachment in an as solid way as possible, until this whole procedure gets a much needed rewrite.
The previous commit made sure that actually pressing a button and inserting something in FCKeditor worked. Now we needed to make sure that 'something' was
either parsed into a tag or placeholder depending on if the editor is being detached or attached. Users should for example never see the img-tag placeholder for Teaser Break appear when they view the source in the editor or turn it off, they should just see the "break" comment.

I had to make some more changes than expected since #13 I discovered a few problems with running multiple editor instances on the same page, and I could optimize away a few things thanks to JavaScript's way of allowing access to the scope a function was declared in. I also moved everything into fckeditor-2.6.js as a result of a discussion with Sun after the last commit.

The quick hack (...plugins-5.patch)

Using SetData multiple times in a row to insert plugin generated XHTML does not work because it's an asynchronous call and suffers from race conditions/timing issues.
This version is a straight forward attempt to insert the plugin-generated XHTML straight into FCKeditor's Iframe/Wysiwyg area instead. It works but is NOT recommended as it prevents the editor from escaping

Grouped plugin calls (...plugins-5-alternative.patch)

Data Processor approach (...plugins-5-dataprocessor.patch)

Wow! Looks promising. I'll try to spare some time ASAP to test this.

subscribing

sub

Sorry, no time for testing yet. But I wanted to do a quick review.

   for (var instanceName in instances) {
+    var instanceName = instances[instanceName].Name;

This looks a bit overloaded. The editor's own .Name is not the same as our params.field?

+  catchingEvents : {},

This property could use a better name, maybe something with "is*". Some JSDoc above it would not hurt either.

+    /**
+     * Create a custom Data Processor to wrap the default one and let the plugins modify the editor contents.
+     * Only do this once!
+     */
+    if (!this.catchingEvents[instance.FCK.Name]) {
+      var pluginsDataProcessor = function() {};

So we add one more dataProcessor for each added plugin? Could we also add just one data processor and invoke all of our Drupal plugins to attach/detach? Or was there a reason for not doing so? If there was, the code should explain it.

Thanks!

sub

This looks a bit overloaded. The editor's own .Name is not the same as our params.field?

The editor instance is given the exact name we pass in the first argument to new FCKeditor() which is params.field. I looked this up beforehand just to be sure. They use it to keep track of which instance belongs to which textarea, so I think we're fine doing the same.

+ catchingEvents : {},This property could use a better name, maybe something with "is*". Some JSDoc above it would not hurt either.

Yeah, I tried to figure out a better name for that but I thought I had put an "is" on there. Considering it's used to check if the instance has been given the DataProcessor, how about just "hasDataProcessor"?

+    /**
+     * Create a custom Data Processor to wrap the default one and let the plugins modify the editor contents.
+     * Only do this once!
+     */
+    if (!this.catchingEvents[instance.FCK.Name]) {
+      var pluginsDataProcessor = function() {};

So we add one more dataProcessor for each added plugin? Could we also add just one data processor and invoke all of our Drupal plugins to attach/detach? Or was there a reason for not doing so? If there was, the code should explain it.

That is exactly what happens, hence the "Only do this once!" comment. The !catchingEvents[instance.FCK.Name] check is to make sure it only runs when the first plugin is added (note, not attached). If no Drupal plugins are used, the DataProcessor will not be wrapped by us at all. At the bottom of addPlugin is

+      instance.FCK.DataProcessor = new pluginsDataProcessor();
+      this.catchingEvents[instance.FCK.Name]  = true;
+    }

I first had this code in fckeditor.config.js as it is only supposed to run once for each editor instance, and each editor instance will open the config file in their own frame. As a result of our discussion about using that config file I moved it into fckeditor-2.6.js. Since there is no place in the code which only runs once and has access to FCK's internals, except for the addPlugin function, I put it there.

I thought about making a new method on Drupal.wysiwyg.editor.instance.fckeditor and call it once from fckeditor.config.js, before the plugins are added to wrap the DataProcessor, but then I still had to make changes to that file. I guess that function call would not be something to worry about in future versions since we could just let the function be empty in fckeditor-2.X if it's not needed anymore. I was just unsure about where to draw the line on config file changes, so I opted to avoid it.

Yeah, a new method would be the way to go then. Maybe just "init" or simply "attach". (Takes us a step closer to the overhaul we discussed)

I guess this would eliminate the catchingEvents property then? If not, your proposal of "hasDataProcessor" sounds very much better. I wonder whether this could be a Wysiwyg API default editor instance property for all editors, so maybe a not so FCKeditor-ish name might even more sense.

Regarding instanceName: Then I don't really understand why we are re-defining the variable? Does FCKeditor itself use an indexed array for its instance registry?

Regarding instanceName: Then I don't really understand why we are re-defining the variable? Does FCKeditor itself use an indexed array for its instance registry?

Oh, I just read your previous post again. I thought you wondered why I had used instance.FCK.Name, which I do in a few places. I did not see that the variable instanceName was declared twice. That second declaration should be removed completely as it was part of something I tested earlier when I had move that code somewhere else.

Back to the other things. I continued testing the reroll of the DataProcessor-patch and noticed I had made a logical error in my use of the scope chain to get the pluginSettings variable for the attach and detach calls. The variable is available, but since JavaScript passes objects by reference, the contents will always be those of the last call to addPlugin.

I'll change those calls to use Drupal.settings.wysiwyg.plugins.drupal[plugin] instead. (Fixed in patch below.)

@TwoD: Why are you active in the queues but not in IRC? :) This rocks!

Turned out that IE6 also strips comments in TinyMCE when an editor is detached.

Committed with some coding-style and documentation fixes. Please have a look at it, so we can commit further patches faster 8)

Patch and comments look good to me.
Would be very helpful with comments from people other than Sun and me as well. It's very easy to stare yourself blind on code hehe.

Could the IE problems be what's reported in #404532: TinyMCE/FCKeditor: Teaser break vanishes in IE6/7?

(I'm at work atm, so no IRC access...)