Hi guys,

I worked on YUI Plugin API support to make plugins that use Wysiwyg API work.
It still needs work and is a little rough, but that's an encouraging first step !

Here is an a patch to be applied against the last 6.x-2.x dev snapshot.

Comments

StatusFileSize
new6.42 KB

Forgot to remove some console.log() debugging.

Status:Active» Needs work

+++ wysiwyg_new/editors/js/yui.js 2009-10-26 02:46:52.000000000 +0100
@@ -1,13 +1,43 @@
+ * Initialize editor instances.
+ */
+Drupal.wysiwyg.editor.init.yui = function(settings) {
+  // Initialize editor configurations.
+};
+
+/**

No need to add this if it does nothing.

+++ wysiwyg_new/editors/js/yui.js 2009-10-26 02:46:52.000000000 +0100
@@ -1,13 +1,43 @@
+   for (var plugin in Drupal.settings.wysiwyg.plugins[params.format].drupal) {
+   console.log(Drupal.settings.wysiwyg.plugins[params.format].drupal[plugin]);
+   if (Drupal.settings.wysiwyg.plugins[params.format].drupal[plugin].css) {

Debug code.

+++ wysiwyg_new/editors/js/yui.js 2009-10-26 02:46:52.000000000 +0100
@@ -1,13 +1,43 @@
+  // Create a group for extra plugins.
+ editor.toolbar.addButtonGroup({ group: 'plugins', buttons: [] });

We wish to handle button grouping/sorting the in a generic way for all editors, see #277954: Allow to sort editor buttons.

+++ wysiwyg_new/editors/js/yui.js 2009-10-26 02:46:52.000000000 +0100
@@ -30,9 +60,57 @@ Drupal.wysiwyg.editor.detach.yui = funct
+    editor.toolbar.addButtonToGroup(button, 'plugins');
+
+    var buttons = editor.toolbar.getButtons();
+    var lastButton = buttons[buttons.length-1];
+ $(lastButton._button).parent().css('background', 'transparent url('+settings.icon+') no-repeat center');
+

See button grouping/sorting above. Is the plugin button guaranteed to be the last one?

Indents are off all over the place, use two spaces per indent.

The attach() and detach() plugin methods need to be called whenever content is set to or gotten from the editor, but you did say it needed work so I assume you knew that.

Keep it up! =)

This review is powered by Dreditor.

Thanks for the review TwoD.

I am aware that this is rough and needs work, I just wanted to show some progress on this.
I'll keep working on this and will post another patch soon.

I am making some progress on this, though I have difficulties to catch "onSetContent" and "onGetContent" events since there are no such events for YUI :(. I managed to implement this through different events, it does the trick but it is not as clean and straightforward as it could be. I'll digg some more.

About the grouping/sorting thing, I understand this is still a todo ? Sun speaks about some work in progress http://drupal.org/node/277954#comment-1054151. Is this still to be considered ? I may be able to help on this, but I don't want to duplicate any effort.

Status:Needs work» Needs review
StatusFileSize
new9.32 KB

Here we go, another patch !

Fixed in this patch :
- Overall cleanup.
- Implementation of attach and detach methods for plugins (that was not an easy one...) and isNode.
- Correction of a bug in Drupal.wysiwyg.editor.detach.yui where instance.saveHTML() was called although instance.destroy() already calls it. This caused a javascript bug and content was not saved at all.

No one ? :/

I haven't yet had time to actually test this but here's a review of the code itself.
I should have used a computer with larger screen for this, sorry if something got duplicated or looks malplaced.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -2,12 +2,37 @@
+        settings.extracss = settings.extracss+' @import "'+Drupal.settings.wysiwyg.plugins[params.format].drupal[plugin].css+'"; ';

Needs spaces around + operators.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -2,12 +2,37 @@
+    editor.on('toolbarLoaded', function() {
+      // Create a group for extra plugins.
+      editor.toolbar.addButtonGroup({ group: 'plugins', buttons: [] });
......
+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+    editor.toolbar.addButtonToGroup(button, 'plugins');

Like I said above, button grouping will be handled the same for all editors. This will probably mean the buttons in the settings are already sorted and grouped the way they should be when handed over to the serverside implementations, their job will be to translate these groupings into a format suitable for the editor initialization on the client. The client basically just sends the complete list of buttons to the editor init call along with the other settings. If the button commands are implemented by Drupal plugins the corresponding commands should either have been previously defined or they are also stored with the settings, depending on how the editor prefers it.

In the case of FCKeditor, all plugins are loaded during the processing of the custom configuration file, before the editor is attached to the textarea. Buttons/commands are created but not added to any toolbar as their final position in the toolbar is up to the user, and thus part of the settings. (Creating a GUI to edit that is mostly what the button-ordering issue is about.)

The buttons should be treated just like any other button and added to Drupal.settings.wysiwyg.configs.yui.format#.toolbar.buttons.0.buttons via wysiwyg_yui_settings on the server.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+    var lastButton = buttons[buttons.length-1];

Seems a bit risky if the above is changed and the last button is something else. In any case, missing spaces around - operator.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+    $(lastButton._button).parent().css('background', 'transparent url('+settings.icon+') no-repeat center');

Also needs spaces around + operators.
When not inserting buttons using this method, I think you should still be able to style them all by using the selectors mentioned on http://developer.yahoo.com/yui/examples/editor/icon_editor.html instead.
The generated css code using those selectors could probably be dynamically added to the document head as each plugin is processed.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+    editor.toolbar.on(plugin+'Click', function(e) {

Missing spaces.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+      editor._createCurrentElement('span');

Please document why this new span is needed (if creating a span is what that method does), or why an internal method is called? Just a short note is enough.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+      e.target.setEditorHTML(e.target._getEditorHTML());

Will this cause an extra update of the editor contents? Can it be avoided?

+    editor.on('afterSetEditorHTML', function(e) {
+      if (typeof Drupal.wysiwyg.plugins[plugin].attach == 'function') {
+        e.content = Drupal.wysiwyg.plugins[plugin].attach(e.content, pluginSettings, instanceId);
+        e.content = Drupal.wysiwyg.editor.instance.yui.prepareContent(e.content);
+      }
+    });
+
+    editor.on('afterGetEditorHTML', function(e) {
+      if (typeof Drupal.wysiwyg.plugins[plugin].detach == 'function') {
+        e.content = Drupal.wysiwyg.plugins[plugin].detach(e.content, pluginSettings, instanceId);
+      }
+    });

Would it be possible to do something similar to the FCKeditor/CKEditor implementations where all plugins are iterated over in a single 'afterSetEditorHTML'/'afterGetEditorHTML' event handler? It eliminated quite a lot of flicker and delays in those editors. Could these event handlers be refactored out completely? See below.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+        if (button && Drupal.wysiwyg.plugins[plugin].isNode(e.target._getSelectedElement())) {

'button' is always defined here.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+YAHOO.widget.Editor.prototype._setEditorHTML = YAHOO.widget.Editor.prototype.setEditorHTML;
+YAHOO.widget.Editor.prototype._getEditorHTML = YAHOO.widget.Editor.prototype.getEditorHTML;

Just in case, maybe we should store the 'old' methods somewhere other than in the original object. Less risk of interfering with any other code doing the same. Probably not critical though.

+++ ../wysiwyg/editors/js/yui.js 2009-10-27 20:34:35.000000000 +0100
@@ -28,11 +53,103 @@ Drupal.wysiwyg.editor.detach.yui = funct
+YAHOO.widget.Editor.prototype.setEditorHTML = function(incomingHTML) {
+  var params = { type: 'afterSetEditorHTML', target: this, content: incomingHTML };
+  this.fireEvent('afterSetEditorHTML', params);
+  this._setEditorHTML(params.content);
+};
+
+YAHOO.widget.Editor.prototype.getEditorHTML = function() {
+  var params = { type: 'afterGetEditorHTML', target: this, content: this._getEditorHTML() };
+  this.fireEvent('afterGetEditorHTML', params);
+  return params.content;
+}

Why not loop through all the plugins for the current instance here? Might save some time and cause less update-flicker if many plugins are used. It must only loop through the plugins belonging to that instance/config. These wrapper definitions could probably be moved inside the attach method. That way you get easy access to the configs via the scope chain, but it must be changed to override only a single editor instance or it will get messy.

Missing a semicolon on the last line.

+++ ../wysiwyg/editors/yui.inc 2009-10-26 02:54:06.000000000 +0100
@@ -1,5 +1,5 @@
-// $Id: yui.inc,v 1.5 2009/06/09 00:18:12 sun Exp $
+// $Id: yui.inc,v 1.6.2.1 2009/10/23 03:00:56 twod Exp $

That should not be there. Edited one version and diffed against another?

+++ ../wysiwyg/editors/yui.inc 2009-10-26 02:54:06.000000000 +0100
@@ -69,7 +77,7 @@ function wysiwyg_yui_version($editor) {
-    if (preg_match('@version:\s([0-9\.]+)$@', $line, $version)) {
+    if (preg_match('@version:\s([0-9\.]+)@', $line, $version)) {

This was fixed in #380586: YUI editor: Version detection not working.

+++ ../wysiwyg/editors/yui.inc 2009-10-26 02:54:06.000000000 +0100
@@ -187,7 +195,9 @@ function wysiwyg_yui_settings($editor, $
+        if ($plugin == 'default') {
+        $buttons[] = wysiwyg_yui_button_setting($editor, $plugin, $button, $extra);
+        }

Why the new if statement? I'm guessing this is because the buttons are added to the toolbar via JS. Please see previous comments about that.

This review is powered by Dreditor.

Status:Needs review» Needs work

Whops.

Title:YUI Plugin API supportPlugin API support for YUI Editor
Category:task» feature
Status:Needs work» Needs review
StatusFileSize
new8.12 KB

Re-rolled against CVS. Fixed a couple of coding style issues. Would be cool to get this done.

Thanks sun.

I gave up on this for now, but I may give it another try some time.

The main issues with the current implementation are :

1/

In the case of FCKeditor, all plugins are loaded during the processing of the custom configuration file, before the editor is attached to the textarea

The problem is that with the YUI implementation we have to set up buttons before calling the render() method which actually builds the toolbar. So it is challenging to have the same approach than with other editors. That also explains the implementation of afterSetEditorHTML and afterGetEditorHTML on behalf of plugins.

2/

Like I said above, button grouping will be handled the same for all editors.

At the time I tried to implement this, there was still no group handling. I don't really know if this has been implemented right now, but without it being implmented, I had to use grouping to make buttons actually work. I don't remember why I had to do this, but I remember I had to. Anyway, if grouping works, it could be useless.

3/

+YAHOO.widget.Editor.prototype._setEditorHTML = YAHOO.widget.Editor.prototype.setEditorHTML;
+YAHOO.widget.Editor.prototype._getEditorHTML = YAHOO.widget.Editor.prototype.getEditorHTML;

We have to override YAHOO.widget.Editor.prototype.setEditorHTML to be able to fire the afterSetEditorHTML and afterGetEditorHTML events. But that's fine I guess.

Just a quick note: Button grouping and sorting is still not implemented and probably won't be until 3.x. However, based on my earlier work on YUI editor integration, I know that YUI Editor more or less "requires" its buttons to be grouped. IIRC, we implemented quite a hack to make YUI work at all via pseudo-button-groups. This does not mean we can or should implement custom button grouping for YUI, but it may mean that we might need to make the hack even uglier. ;)

StatusFileSize
new7.53 KB

I worked on this, it's cleaner and addresses most issues.
But at the moment, there is a bug, content is not saved on form submission, and I did not succeed to track the bug down for now. Anyone ? I'm using YUI 2.0.8.

Btw, what about the version number in function wysiwyg_yui_editor() ? :

    'versions' => array(
      '2.7.0' => array(
        'js files' => array('yui.js'),
      ),
    ),

StatusFileSize
new7.91 KB

This is driving me crazy...

Fixed an issue with the overriding of getEditorHTML and setEditorHTML (obviously, that could not work).

The bug is still here. I found that instance.destroy() causes the error, and while trying to debug this, I noticed that the detach() method seems to be called multiple time which seems to cause the instance.destroy() method to throw an error.

Is it normal that the destroy() detach() method is called more than once ?

StatusFileSize
new7.91 KB

A little correction, but bug is still here.

StatusFileSize
new8.27 KB

Okay, corrected a little oddity and detach method should work fine now, but I am unable to make the Teaser break plugin work with any editor or with no editor at all, content always strips <!--break-->, even in Full HTML.
I must be missing something here.

+++ editors/js/yui.js 8 Feb 2010 12:49:35 -0000
@@ -1,13 +1,61 @@
+Drupal.wysiwyg.editor.init.yui = function(settings) {
+  // Initialize editor configurations.
+  for (var format in settings) {
+    // Create plugins buttons.
+    if (Drupal.settings.wysiwyg.plugins[format]) {
+      for (var plugin in Drupal.settings.wysiwyg.plugins[format].drupal) {
+        Drupal.wysiwyg.editor.instance.yui.addPlugin(plugin, Drupal.settings.wysiwyg.plugins[format].drupal[plugin], Drupal.settings.wysiwyg.plugins.drupal[plugin]);
+      }
+    }
...
+ * Since buttons must be added before the editor is rendered, we add plugins
+ * buttons on attach event rather than in init.
...
   var editor = new YAHOO.widget.Editor(params.field, settings);
...
   editor.render();

Is there a particular reason for why we have to add those plugins in .init() instead of .attach()?

I've read that comment, but as of now, it simply tells me that the code from .init() could be moved before the .render() call or if required also before the new editor instantiation.

+++ editors/js/yui.js 8 Feb 2010 12:49:35 -0000
@@ -1,13 +1,61 @@
+  var _setEditorHTML = editor.setEditorHTML;
+  editor.setEditorHTML = function(incomingHTML) {
...
+  var _getEditorHTML = editor.getEditorHTML;
+  editor.getEditorHTML = function() {

Can we clone those methods on the editor object, i.e. editor._setEditorHTML(), so we don't lose access to them in this local scope?

+++ editors/js/yui.js 8 Feb 2010 12:49:35 -0000
@@ -18,17 +66,89 @@ Drupal.wysiwyg.editor.attach.yui = funct
+      YAHOO.widget.EditorInfo._instances[params.field] = null;
...
+        YAHOO.widget.EditorInfo._instances[e] = null;

Shouldn't we use delete here?

Powered by Dreditor.

Thanks for the review sun.

Is there a particular reason for why we have to add those plugins in .init() instead of .attach()?

I've implemented that this way to stick other editors implementations, but yes, we could move code from init() to attach(). I thought TwoD said it would be better, but now that I'm looking for a quote I realize i may be wrong. So definitely makes more sense and would be more straightforward.

Can we clone those methods on the editor object, i.e. editor._setEditorHTML(), so we don't lose access to them in this local scope?

From comment #7 :

Just in case, maybe we should store the 'old' methods somewhere other than in the original object. Less risk of interfering with any other code doing the same. Probably not critical though.

In the first patches, I modified the prototype object directly, but I thought this was still true for the object instance and did not want to pollute the object. But I'm fine with cloning the methods on the original object.

Shouldn't we use delete here?

Okay.

Do you have any idea about my problems around the Teaser break comment being striped as mentioned in #13 ?

StatusFileSize
new7.64 KB

Here is an updated patch modified according to sun's review.

I "namespaced" the getEditorHTML as editor.WYSIWYGGetEditorHTML in case another plugin tries to hijack this, and put Drupal.wysiwyg.editor.instance.yui.addPlugin back in attach(). We have to pass the editor instance as an additional parameter though.

I'm too tired to fully review this at the moment but I'll do it first thing tomorrow. From what I can tell at first glance, it's quite an improvement over the first patch.

@TwoD: Ok then I'm waiting for your review. If you've got any clue on the Teaser break problem (probably something I overlooked somewhere), it would be great.

It turns out YUI eats all errors thrown inside its event handlers. The only way I was able to debug this was to turn on "break on all errors" in Firebug, and use the non-minified editor sources to track things down. Breaking on all errors is a bit buggy tho, as it sometimes kills the browser entierly, so this has taken some time...

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -2,12 +2,45 @@
+  editor.setEditorHTML = function(incomingHTML) {
+    var params = { type: 'WYSIWYG.setEditorHTML', target: this, content: incomingHTML };

content should be e.content

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -2,12 +2,45 @@
+  editor.getEditorHTML = function() {
+    var params = { type: 'WYSIWYG.getEditorHTML', target: this, content: this.WYSIWYGGetEditorHTML() };

content should be e.content

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -2,12 +2,45 @@
+      Drupal.wysiwyg.editor.instance.yui.addPlugin(plugin, editor, Drupal.settings.wysiwyg.plugins[params.format].drupal[plugin], Drupal.settings.wysiwyg.plugins.drupal[plugin]);

Drupal.wysiwyg.editor.instance.yui.addPlugin should be Drupal.wysiwyg.instances[params.field].addPlugin or this will point to the wrong object inside addPlugin.
This is also true for all calls to insert, prepareContent, getContent and other methods on the instance.

When a new instance of the abstraction layer is created, everything below Drupal.wysiwyg.editor.instance.editorName is copied, merged with the format-specific parameters (that's what adds the .field property) and put in Drupal.wysiwyg.instances[params.field aka instanceId].

Drupal.wysiwyg.editors.editorName acts as the 'definition'

I've noticed we do this wrong in other implementations as well, and I'll create an issue about fixing that.

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -2,12 +2,45 @@
@@ -18,17 +51,85 @@ Drupal.wysiwyg.editor.attach.yui = funct
+Drupal.wysiwyg.editor.instance.yui = {
+  plugins: [],

the plugins array seems to be a leftover as it's not used anywhere.

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -18,17 +51,85 @@ Drupal.wysiwyg.editor.attach.yui = funct
-    var instance = YAHOO.widget.EditorInfo.getEditorById(params.field);
+    var instance = YAHOO.widget.EditorInfo._instances[params.field];

I often prefer using the editor's own API when grabbing its instance over going directly to where it's stored. That way there's less for us to update if the editor changes this internally. This case is an exception tho as we need to delete the actual object from the array and deleting a variable referencing it doesn't seem to help, so I'm ok with this change but would prefer if we used getEditorById when we can.

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -18,17 +51,85 @@ Drupal.wysiwyg.editor.attach.yui = funct
+      editor._createCurrentElement('span');
+      var data = { format: 'html', node: editor.currentElement[0], content: $(editor.currentElement[0]).html() };

This adds <span></span> to the contents each time a plugin is invoked. And it'll pass them an empty html string.
Why can't we use _getSelectedElement() here as well? Ideally, we should use the same method of retrieving the element passed to isNode() and invoke().
On a side-note: .html() will cause XHTML vs HTML issues, we need to remember to change this as part of #510552: Invalid XHTML: missing trailing slashes, absolute urls and uppercase tags.
var selectedElement = editor._getSelectedElement();
....node: selectedElement, content: $(selectedElement).html()

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -18,17 +51,85 @@ Drupal.wysiwyg.editor.attach.yui = funct
+    editor.on('editorContentLoaded', function (e) {
+      e.target.setEditorHTML(e.target._getEditorHTML());
+    });

Move this to attach() or it'll register the event handler for each plugin.
e.target._getEditorHTML() should be e.target.WYSIWYGGetEditorHTML().

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -18,17 +51,85 @@ Drupal.wysiwyg.editor.attach.yui = funct
+        e.content = Drupal.wysiwyg.editor.instance.yui.prepareContent(e.content);

See above on invoking instance methods.

Do you have any idea about my problems around the Teaser break comment being striped as mentioned in #13 ?

The break comment wasn't really stripped as far as I could tell, there was just no placeholder created for it because the teaser break code didn't get the correct contents sent to it (e.contents vs contents), and the _getEditorHTML() call failed.

Is it normal that the detach() method is called more than once ?

No, it shouldn't be called more than once. I don't see that behavior.

In #7 I suggested that you loop through all the plugins in the same event handler, to sort of 'bulk' process them. It would save some event processing overhead. On second thought, I don't think we need the custom events at all. Why not just process all plugins in the overridden set/get HTML methods? That's basically what the FCKeditor and CKEditor implementations do.

I took the liberty of modifying the patch based on the above changes to cut down the roundtrip time.

I also created a second version of it with some additional changes of my own, I'm posting it as a separate/alternative patch to be able to compare the approaches easier.
I removed the custom events and put the plugin processing directly in the get/set overrides, which also allows for direct acces to the scope of attach(). The references to the old set/get methods can be stored in the scope of attach() - no way anyone from the outside can overwrite them there - and the overriding functions form a closure which will keep the old methods alive even after attach() returns.
All of the setup code which only needs to be run once is now in the attach() method. It still invokes addPlugin to hook up the plugin buttons to the click handlers. This code could also be moved to attach(), but I guess there's a 'tradition' of putting the button-handling code there.

I'm a bit biased towards the approach in the -alt patch as it avoids the editor's own event system when we don't really need it, but I want sun's opinion on this as well.

Powered by Dreditor.

content should be e.content

No, the second arguments should be an anonymous object containing the event properties. It's when catching the event we should use the event object.

From http://developer.yahoo.com/yui/docs/EventProvider.html#method_fireEvent :

p_type <string> the type, or name of the event
arguments <Object*> an arbitrary set of parameters to pass to the handler.

Drupal.wysiwyg.editor.instance.yui.addPlugin should be Drupal.wysiwyg.instances[params.field].addPlugin

Yes, makes sense, but I just did as in other editors implementations.

I often prefer using the editor's own API when grabbing its instance over going directly to where it's stored

-    var instance = YAHOO.widget.EditorInfo.getEditorById(params.field);
+    var instance = YAHOO.widget.EditorInfo._instances[params.field];

The problem is that we need to access the object directly to be able to delete it with :

delete YAHOO.widget.EditorInfo._instances[params.field];

So that when detach() is called again, the instance object does not exist and instance.destroy(); is not called. But if detach() is not called twice anymore, this should not be useful anymore. Otherwise, an error is thrown when submitting the form.

This adds <span></span> to the contents each time a plugin is invoked. And it'll pass them an empty html string.

It's a long time since I did this, but if I remember well, if nothing is selected, editor.currentElement and _getSelectedElement() will be empty. I can't remember if there is a reason why I used different methods for isNode() and invoke().

But I saw this implementation on several Yahoo examples :

if (!this._isElement(el, 'body')) {
    Dom.setStyle(el, 'background-color', value);
    this._selectNode(el);
    exec = false;
} else {
    this._createCurrentElement('span', { backgroundColor: value });
    this._selectNode(this.currentElement[0]);
    exec = false;
}

So we probably should do this too.

[Deleted here : Sorry, I read too fast, forget this part.]

No, it shouldn't be called more than once. I don't see that behavior.

Adding a simple alert() in the detach method showed it was called twice.

About the using custom events instead of looping through all plugins, I did this to stick to the other editors behaviors and to be consistent with other plugin methods calls. And I thought it could be useful to have these events fired anyway.

In your alternative patch :

+  var oldGetEditorHTML = editor.getEditorHTML;

I implemented it this way in #15 but changed it according to sun's review :

Can we clone those methods on the editor object, i.e. editor._setEditorHTML(), so we don't lose access to them in this local scope?

Btw, I'm fine with calling plugin methods directly in getEditorHTML / setEditorHTML instead of using events, let's see what sun thinks about it.

All of the setup code which only needs to be run once is now in the attach() method. It still invokes addPlugin to hook up the plugin buttons to the click handlers. This code could also be moved to attach(), but I guess there's a 'tradition' of putting the button-handling code there.

I think you meant "This code could also be moved to init()" ? In that case, we can't since buttons must be added before editor.render(); and we need the editor instance to already exist.

Just a quick note on the last question (don't have more time right now): .init() was originally introduced for TinyMCE 2 only, due to it not being able to instantiate editor instances after the DOM has been loaded. Most (or perhaps even all) other editors should ideally be able to be instantiated on-the-fly, whenever an editor is actually needed/requested/attached. Especially with regard to lazy-loading via AJAX and similar scenarios. Therefore, we should try to avoid executing code in .init(). However, I didn't look at the actual code flow in detail yet, so I cannot tell whether this editor requires us to use .init(), in which case it would be ok.

+++ editors/js/yui.js 8 Feb 2010 14:11:16 -0000
@@ -18,17 +51,85 @@ Drupal.wysiwyg.editor.attach.yui = funct
+        e.content = Drupal.wysiwyg.plugins[plugin].attach(content, pluginSettings, instanceId);

Sorry, I commented the wrong lines. This is where content should have been e.content, in the event handler.

Yes, makes sense, but I just did as in other editors implementations.

Yeah, I know, I wrote some of the other implementations so you probably followed my bad example. ;) This system is a bit of a mess and we will replace it with a single editor object for 3.x. We just need to get the functionality in before refactoring.

The problem is that we need to access the object directly to be able to delete it with :
...

Yes, that's why I said I agree with your change to not using getEditorById in the detach() method.

It's a long time since I did this, but if I remember well, if nothing is selected, editor.currentElement and _getSelectedElement() will be empty. I can't remember if there is a reason why I used different methods for isNode() and invoke().

When I tested without anything selected or an empty field, _getSelectedElement() returned the body of the iframe. Looking at the code, it seems that the only possible outcome where the returned element is NULL is if the iframe body is undefined (it falls back to the body if no other element is selected).
Just to be sure, I also installed Safari via Wine and tested the patches using _getSelectedElement there as well, seems to work fine. Judging by the description of the method; I think it's to be used when you need to know where something was replaced (often done using the queryCommand insertHTML) and need a reference to the new element(s). Since we only pass that node in to be able to know what is going to be replaced (and maybe read values for populating a dialog, see img_assist's Drupal plugin), and that Drupal plugins go via Wysiwyg's API to insert stuff and don't get anything returned, I think were safe with _selectedElement().

The YUI documentation is pretty good at commenting stuff, but not at documenting where, when and why to use specific methods, which is a general problem with open source projects I guess. :/

Adding a simple alert() in the detach method showed it was called twice.

Well, I didn't test that part before making the -mod patch, but it didn't happen afterwards. I tested with breakpoints and they were only hit once.

+  var oldGetEditorHTML = editor.getEditorHTML;

I implemented it this way in #15 but changed it according to sun's review :

Ah, yes I see that now. I don't know what we'd need those references outside of attach() tho.

I think you meant "This code could also be moved to init()" ? In that case, we can't since buttons must be added before editor.render(); and we need the editor instance to already exist.

No, I mean attach(). The addPlugin code from the -alt patch:

Drupal.wysiwyg.editor.detach.yui = function(context, params) {
  addPlugin: function (plugin, settings, pluginSettings) {
    if (typeof Drupal.wysiwyg.plugins[plugin] != 'object') {
      return;
    }
    var editor = YAHOO.widget.EditorInfo.getEditorById(this.field);
    var button = editor.toolbar.getButtonByValue(plugin);
    $(button._button).parent().css('background', 'transparent url(' + settings.icon + ') no-repeat center');
    // 'this' will reference the toolbar while inside the event handler.
    var instanceId = this.field;
    editor.toolbar.on(plugin + 'Click', function (e) {
      var selectedElement = editor._getSelectedElement();
      // @todo Using .html() will cause XTHML vs HTML conflicts.
      var data = { format: 'html', node: selectedElement, content: $(selectedElement).html() };
      Drupal.wysiwyg.plugins[plugin].invoke(data, pluginSettings, instanceId);
    });
  },
  prepareContent: function (content) {
  ...

could just be lifted from there and replace the call to addPlugin(), something like:
Drupal.wysiwyg.editor.attach.yui = function(context, params, settings) {
  ....
  // Attach editor.
  var editor = new YAHOO.widget.Editor(params.field, settings);
  ....
  // Load plugins.
  editor.on('toolbarLoaded', function() {
    // Load Drupal plugins.
    for (var plugin in Drupal.settings.wysiwyg.plugins[params.format].drupal) {
      if (typeof Drupal.wysiwyg.plugins[plugin] != 'object') {
        return;
      }
      var button = editor.toolbar.getButtonByValue(plugin);
      var pluginSettings = Drupal.settings.wysiwyg.plugins.drupal[plugin];
      var buttonSettings = Drupal.settings.wysiwyg.plugins[params.format].drupal[plugin];
      $(button._button).parent().css('background', 'transparent url(' + buttonSettings.icon + ') no-repeat center');
      editor.toolbar.on(plugin + 'Click', function (e) {
       var selectedElement = editor._getSelectedElement();
       // @todo Using .html() will cause XTHML vs HTML conflicts.
       var data = { format: 'html', node: selectedElement, content: $(selectedElement).html() };
       Drupal.wysiwyg.plugins[plugin].invoke(data, pluginSettings, params.field);
     });
    }
  });
  editor.render();
};

The editor instance has been created, and the (real buttons definitions are sent in the settings to YAHOO.widget.Editor), then we register the toolbarLoaded handler and finally call render(). That will fire the toolbarLoaded event which in turn will fix the icon and register the click-handler for each button. The code flow is the same as before, we just "inlined" the code in addPlugin() and changed parts of it to use the variables already in the scope chain rather than taking function arguments.
I've tested the above code in FF, IE and Safari and they can all handle it equally well.

I think it becomes easier to follow the code this way instead of having to jump back and forth between the things happening once on attach, and those which can happen at any time in Drupal.wysiwyg.instance.editor.yui.

Powered by Dreditor.

Ah, your comment formatting is much better now ;)

e.content = Drupal.wysiwyg.plugins[plugin].attach(content, pluginSettings, instanceId);

Yeah, I missed that one when I decided to use events finally.

This system is a bit of a mess and we will replace it with a single editor object for 3.x. We just need to get the functionality in before refactoring.

This 3.x branch will really be one step above !

Yes, that's why I said I agree with your change to not using getEditorById in the detach() method.

Oh sorry, I overlooked what you said in your previous comment. Ok then.

I think were safe with _selectedElement().

TBH, I don't feel like looking at this again, so if you concluded it is fine, let's do this. We should keep this in mind in case a problem around this pops up.

Well, I didn't test that part before making the -mod patch, but it didn't happen afterwards. I tested with breakpoints and they were only hit once.

In fact I did not use an alert but breakpoints in Firebug too. If it works with your last patch, then I guess it's okay, I may try this again though, just curious.

+ var oldGetEditorHTML = editor.getEditorHTML;
Ah, yes I see that now. I don't know what we'd need those references outside of attach() tho.

Either way is fine to me. Let's see why sun advised to do do.

About my implementation of plugins loading through addPlugin, once again, I followed the logic from other editors, putting all plugin methods (isNode, prepareContent, etc.) in the addPlugin method in the instance object.

It feels a little different from other editors implementations in your patch, but if you prefer this way, it's your call I guess. NP for me anyway.

Yeah I missed a slash in a closing blockquote at the top and everything got messed up hehe

Yes, doing as much of the setup as possible in attach() is a bit different than how it has been done in some of the other implementations. I feel that the way it's done there (jumping to the Drupal.wysiwyg.instance object for some things when they don't really need to be there) has limitations and makes the flow much less natural, so I'm hoping writing the new plugin api implementations this way will make it clearer for people who want to contribute with new editor implementations. I'd like to restructure the existing implementations this way too, but we're too close to a new release to start moving around old code and "disturbing the peace". ;)

I think the addPlugin method was originally meant to be called by Wysiwyg itself after initializing the editor, so each implementation would not have to keep track of which plugins are enabled for it, but it turned out that when editors expect to have event listeners etc registered and when they're ready to set up the plugin differs too much. It would probably be better to have a Drupal.wysiwyg.getDrupalPlugins(format) method or similar to call when they're ready. One of the many things I hope to test for 3.x! =)

Interesting plans for 3.x, I'll be glad to help if I've got some time.

Status:Needs review» Reviewed & tested by the community

This patch looks 99% ready to me. I'm tempted to commit a 99% ready patch, unless there are any known major blockers.

Version:6.x-2.x-dev» 7.x-2.x-dev
Status:Reviewed & tested by the community» Fixed
StatusFileSize
new7.52 KB

Based on the latest comments, the -alt patch still seems to be the better one.

Thanks for reporting, reviewing, and testing! Committed to 7.x-2.x. :)

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Nice !

Status:Fixed» Closed (fixed)

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

I've ported this to 6.x-2.x, including the patches from #1617706: YUI Editor does not work when no plugins are enabled.