Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#30 | wysiwyg.yui-pluginapi.18-alt-rerolled.patch | 7.52 KB | sun |
#21 | wysiwyg-DRUPAL-6--2.yui-pluginapi.18-mod.patch | 7.47 KB | TwoD |
#21 | wysiwyg-DRUPAL-6--2.yui-pluginapi.18-alt.patch | 8.02 KB | TwoD |
#18 | wysiwyg-DRUPAL-6--2.yui-pluginapi.18.patch | 7.64 KB | jide |
#15 | wysiwyg-DRUPAL-6--2.yui-pluginapi.15.patch | 8.27 KB | jide |
Comments
Comment #1
jide CreditAttribution: jide commentedForgot to remove some console.log() debugging.
Comment #2
TwoDNo need to add this if it does nothing.
Debug code.
We wish to handle button grouping/sorting the in a generic way for all editors, see #277954: Allow to sort editor buttons.
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.
Comment #3
jide CreditAttribution: jide commentedThanks 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.
Comment #4
jide CreditAttribution: jide commentedI 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.
Comment #5
jide CreditAttribution: jide commentedHere 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.
Comment #6
jide CreditAttribution: jide commentedNo one ? :/
Comment #7
TwoDI 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.
Needs spaces around + operators.
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.
Seems a bit risky if the above is changed and the last button is something else. In any case, missing spaces around - operator.
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.
Missing spaces.
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.
Will this cause an extra update of the editor contents? Can it be avoided?
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.
'button' is always defined here.
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.
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.
That should not be there. Edited one version and diffed against another?
This was fixed in #380586: YUI editor: Version detection not working.
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.
Comment #8
TwoDWhops.
Comment #9
sunRe-rolled against CVS. Fixed a couple of coding style issues. Would be cool to get this done.
Comment #10
jide CreditAttribution: jide commentedThanks 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/
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/
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/
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.
Comment #11
sunJust 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. ;)
Comment #12
jide CreditAttribution: jide commentedI 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() ? :
Comment #13
jide CreditAttribution: jide commentedThis 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 ?Comment #14
jide CreditAttribution: jide commentedA little correction, but bug is still here.
Comment #15
jide CreditAttribution: jide commentedOkay, 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.
Comment #16
sunIs 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.
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?
Shouldn't we use delete here?
Powered by Dreditor.
Comment #17
jide CreditAttribution: jide commentedThanks for the review sun.
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.
From comment #7 :
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.
Okay.
Do you have any idea about my problems around the Teaser break comment being striped as mentioned in #13 ?
Comment #18
jide CreditAttribution: jide commentedHere 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.
Comment #19
TwoDI'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.
Comment #20
jide CreditAttribution: jide commented@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.
Comment #21
TwoDIt 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...
content
should bee.content
content
should bee.content
Drupal.wysiwyg.editor.instance.yui.addPlugin
should beDrupal.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 inDrupal.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.
the plugins array seems to be a leftover as it's not used anywhere.
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.
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.
Move this to attach() or it'll register the event handler for each plugin.
e.target._getEditorHTML()
should bee.target.WYSIWYGGetEditorHTML()
.See above on invoking instance methods.
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.
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.
Comment #22
jide CreditAttribution: jide commentedNo, 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 :
Yes, makes sense, but I just did as in other editors implementations.
The problem is that we need to access the object directly to be able to delete it with :
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.
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 :
So we probably should do this too.
[Deleted here : Sorry, I read too fast, forget this part.]
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 :
I implemented it this way in #15 but changed it according to sun's review :
Comment #23
jide CreditAttribution: jide commentedBtw, I'm fine with calling plugin methods directly in getEditorHTML / setEditorHTML instead of using events, let's see what sun thinks about it.
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.
Comment #24
sunJust 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.
Comment #25
TwoDSorry, I commented the wrong lines. This is where content should have been e.content, in the event handler.
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.
Yes, that's why I said I agree with your change to not using getEditorById in the detach() method.
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. :/
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.
Ah, yes I see that now. I don't know what we'd need those references outside of attach() tho.
No, I mean attach(). The addPlugin code from the -alt patch:
could just be lifted from there and replace the call to addPlugin(), something like:
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.
Comment #26
jide CreditAttribution: jide commentedAh, your comment formatting is much better now ;)
Yeah, I missed that one when I decided to use events finally.
This 3.x branch will really be one step above !
Oh sorry, I overlooked what you said in your previous comment. Ok then.
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.
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.
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.
Comment #27
TwoDYeah 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! =)
Comment #28
jide CreditAttribution: jide commentedInteresting plans for 3.x, I'll be glad to help if I've got some time.
Comment #29
sunThis patch looks 99% ready to me. I'm tempted to commit a 99% ready patch, unless there are any known major blockers.
Comment #30
sunBased 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.
Comment #31
jide CreditAttribution: jide commentedNice !
Comment #33
TwoDI've ported this to 6.x-2.x, including the patches from #1617706: YUI Editor does not work when no plugins are enabled.