Tinymce.module has not yet been upgraded to jQuery. It uses some dated javascript approaches. Particularly, the use of document.write conflicts with AJAX use, see this issue: http://drupal.org/node/113151.

Attached is a patch to minimally upgrade the module to jQuery. We generalize and move some of the inline calls to an include file, tinymce.js, meaning that we don't have to repeat as much js code. Some data are passed as settings through drupal_add_js() of type 'setting'. The I didn't want to wade into a full rewrite so stuck to the basics.

The toggle links are appended dynamically rather than through document.write. They could probably use some css to make them look like links.

There are some hard-coded tests in the code for the image_assist module. I adapted them but haven't tested with image_assist.

CommentFileSizeAuthor
#11 tinymce-jquery_4.patch29.32 KBnedjo
#9 tinymce-jquery_3.patch28.32 KBnedjo
#8 tinymce-jquery_2.patch29.27 KBnedjo
#7 ibrowser_tar_gz.doc6.98 KBluti
#6 font-select.png6.28 KBnedjo
#4 tinymce-jquery_1.patch30.28 KBnedjo
#1 tinymce-jquery_0.patch16.33 KBnedjo
tinymce-jquery.patch6.67 KBnedjo

Comments

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new tinymce-jquery_0.patch16.33 KB

Here is a much more complete patch, including a full rewrite of the javascript in the module. All configuration data are now passed through the Drupal.settings object--gone is all the inline javascript.

Previously we had a different call to tinyMCE.init() for each textarea. This was a workaround to allow different tinymce controls on the same page, e.g., simple and advanced.

Instead, we now call tinyMCE.init() not for every textarea, but rather for each distinct theme. Then the actual attachment of the control is done through a direct call to the tinyMCE mceAddControl method. As well as being cleaner, this opens the way to dynamically attaching controls to AJAX-loaded content.

Previously textareas were customized through a workaround--a call to a theme function that didn't actually theme. I've moved that to where it is arguably more appropriate, a hook to allow modules to customize how tinymce attaches to their textareas.

There's one piece that I've dropped: the hard-coded img_assist handling. If this patch is applied, though, I'll be happy to contribute to a clean implementation, which would go in img_assist module.

kreynen’s picture

kreynen commented

I will definitely check this out, but the fact that you've patched the 2007-Feb-07 release may prove problematic as the Moxie developers are coming back on board to maintain that release. The code you updated was drupal-id's last contribution to the module and I'm not sure it's been tested to anyone's comfort level. I'm guessing that the 5.1 release will look more like the Moxie module soon and that Steve and Allie will be more concerned with a stable, issue free release than in implementing a JQuery approach.

If this JQuery friendly approach provides more flexibility with a per role/per content type configuration that supports custom .js files, I'd be glad to add it to the 5.2 release (coming soon) because that's the direction we want to go with that branch.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

The code I patched differs little if at all from the moxie module (which I also experimented with patching, with the same results). This patch could easily be adpated for any version based on the original tinymce module.

I've been thinking a bit more about approaches and will open a couple of new issues with suggestions.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new tinymce-jquery_1.patch30.28 KB

Here is a polished patch that significantly improves the tinymce code in several key areas.

I've incorporated http://drupal.org/node/119862. We support the following element properties for textareas:

#wysiwyg
boolean, should wysiwyg editors attach to this element, default TRUE
#wysiwyg_style
style or theme to apply, default 'advanced'

So any contrib module can control the WYSIWYG behaviour of its text areas through the forms api. For Drupal core textfields, we retain the handling in tinymce.module, setting $element['#wysiwyg'] = FALSE; to prevent attachment to given elements and $element['#wysiwyg_style'] = 'simple'; to trigger the simple rather than advanced theme.

I've carefully refined the jQuery js file, streamlining and improving in several areas. The main change is that the function Drupal.tinymceAttach() now executes on document.ready. This is possible thanks to tinyMCE's strict_loading_mode, which triggers DOM methods instead of the default document.write. As a significant side benefit, this means that we can call this same function to attach tinyMCE controls at any time, including after we've loaded new content through AJAX/AHAH.

luti’s picture

luti commented

I've just applied a second patch, and generally it seems to work (I also like the field appearance more than with a previous version of this patch).

But, I am observing an annoyance. I have a default setting disabled (I've tried it as administrator) and am turning TinyMCE on only when I need it. So, when I go to an edit view and turn on TinyMCE for the first time (no matter if it is a teaser or a content), in FontSelect and styleSelect fields appeat at the top of list entries (in any of respected fields):
{$lang_theme_block}, {$lang_theme_paragraph}, {$lang_theme_address}, {$lang_theme_pre}, {$lang_theme_h1}, ...
{$lang_theme_fontdefault}, below are font names as expected
{$lang_theme_font_size}, below are sizes listed as expected
{$lang_theme_style_select}, below I don't have any entries

If I turn it off and back on, lists are generated properly. If I turn on TinyMCE in any other field on this page, lists are OK immediately.

It seems to me something is not initialized properly at the beginning...

I also see you are mentioning 2 textarea properties - would it make sense to give certain users (with right privileges, at least administrators) a possibility to set them on a per textarea basis? For example, I would like to provide it to users in a message body field (to make their life easier when enter not only text), but not in an address field (to keep a simple few-lines text field, which is only occasionally used, reasonably small and in line with other user data fields). But, this might rather be another topic, or?

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new font-select.png6.28 KB

Thanks for the review.

I can't reproduce the issue with selects. With my latest patch applied, attached is a screenshot of the styles select when I have just enabled tinymce for the first time on a page.

Allowing user setting of particular text fields might be useful. As you suggest, it's not something I'd want to include in this already large patch. I suspect it's part of a more general problem. The same could be said of other form-related behaviours, collapsible fieldsets, etc., so a general solution would be preferable to creating a custom UI just for tinymce behaviours.

luti’s picture

luti commented
StatusFileSize
new ibrowser_tar_gz.doc6.98 KB

I've noticed the behavior as described above doesn't happen for my adminsitrator profile anymore (after clearing cache etc.), so it might be that it was something not related to a patch (it happened in Firefox).

Anyway, I am noticing now a more serious issue - for anonymous user (similar profile, just a few buttons less) in MSIE 7 I am getting a blank page and the following is logged:
array_key_exists() [<a href='function.array-key-exists'>function.array-key-exists</a>]: The second argument should be either an array or an object in /var/www/html/new.gewiss.si/sites/all/modules/tinymce/tinymce.module on line 153.

It might be different settings (don't believe, nothing much different but a few plugins less), MSIE 7, but I suspect it is a fact that here TinyMCE is enabled by default (while before it was disabled and I've enabled it later...). The errormessage doesn't mean anything to me, and I wouldn't test all posssible combinations if it does to someone. Otherwise, I can try, but not today...

Besides, I am noticing a plugin ibrowser (I had to modify it a bit from the last verison found) doesn't work anymore after this last patch (with a first patch version named tinymce-jquery.patch it worked well). What could be the reason?

I am attaching 3 important files (the rest is as I've found it...), if anyone is willing to take a look, or use them... ;-) A file ibrowser_tar_gz.doc has to be renamed into ibrowser.tar.gz first, of course.

TinyMCE plugin settings (plugin_reg.php) are:

  $plugins['ibrowser'] = array();
  $plugins['ibrowser']['theme_advanced_buttons2'] = array('ibrowser');

What I find strange is a link, which is for other buttons like:
javascript:tinyMCE.execInstanceCommand('mce_module_0','mceModuleName');
but for my ibrowser module it seems to be:
http://<site_URL>/tinyMCE.execInstanceCommand('mce_editor_0','mceBrowseImage');return%20false;

I know it is not ImageBrowser module about here, but is seems the last patch is breaking something what worked before pretty well (even if I am confident it is a right way to proceed, probably the reason is ImageBrowser code is too obsolete alltogether already...) and maybe it would be better to be done providing as much as possible a backward compatibility. I've tried to adapt a editor_plugin.js code of ImageBrowser so much that a link was displayed as with the other plugins and managed also to get the window open and do some actions in it, but the save or cancel buttons didn't work, and after closing the window in a hard way the document was frozen... After some tries I am giving up, and reverted everything back to the state before this recent update.

And, yes, I know there is much more capable IMCE module, but I found some features I don't like (I just need to upload some images and select the right one, but don't need a complete file storage access, and in particular not for all of the other users...), and my version of ImageBrowser allows me to get exactly what I need (browse and manipulate images on server - including upload or delete - and easily find what I need).

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new tinymce-jquery_2.patch29.27 KB

Thanks again for your testing and review!

The array error puzzles me. I can't reproduce it. In the code, I'm testing for the existance of an #attributes property and, if it's not already set, creating it as an array. Then I test that array for the 'class' key. I would expect this error only if the #attributes property exists, but isn't an array. So my first guess is that you have a module enabled that is incorrectly setting a non-array #attributes property for a textarea.

Here is a polished patch

Okay, that was a rash claim given that I had neglected to test at all in IE ;)

Turns out that trying to dynamically initialize using strict_loading_mode breaks completely in IE6. I guess I should have looked at the documentation first, which says this is Gecko-specific. "This option will force TinyMCE to load script using a DOM insert method, instead of document.write, on Gecko browsers." http://wiki.moxiecode.com/index.php/TinyMCE:Configuration/strict_loading...).

So back to initializing before the page has fully loaded. That's okay, we can still dynamically attach to textareas based on class selectors. We're just limited to the set of configurations already initialized.

I'm hoping that the IE path errors noted in the review came from the use of strict_loading_mode or the post-load initialization, and will now politely disappear.

Other changes in this version:

  • Config objects need to be cloned before being passed to tinyMCE.init(). Otherwise, they're passed by reference and can't be reused (they're turned into whatever tinyMCE.settings is).
  • Whenever we toggle, we reset the configuration options first. This is needed in order to ensure that the tinyMCE control that's attached has the appropriate settings. I assume that this fixes a bug in previous versions of the module. Unless we dynamically change the tinyMCE settings when attaching controls, the controls will have the last-set settings, rather than the ones they were originally set with.

Tested in IE6 and Firefox 1.5.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new tinymce-jquery_3.patch28.32 KB

Previous version mistakenly loaded tinymce files on every page. This reinstates loading them only if needed.

Note: This patch also opens the way to integrating manually coded tinymce config files. They can be added like this.

1. Write the include file like this:


Drupal.settings.tinymce.configs.push({
  theme: 'sometheme',
  ...
});

2. Add the file as follows (in tinymce_footer() before the line that adds tinymce.js):


drupal_add_js('path/to/file.js', 'footer');

So, after applying this patch, we can easily add in integration for manual files as, e.g., overrides.

ufku’s picture

ufku commented

i have tested the latest patch.

- It works nice with uncompressed tiny_mce.js.

- I couldnt make it work with the compressor. (tested superficially. tried old and new versions of the compressor)

- Chameleon themes have problems(js files are not included):
This is an issue of footer hook, which I'm familiar from the project IMCE. The problem is that if the footer in template file is processed after the header, the javascript files added with drupal_add_js in footer hook are not written to head. it becomes too late for them. The only way to safely include js files from footer hook is to return them as html output. However, then they are written to the closure of the theme file. And this closure thing is another headache that it is usually omitted by theme developers and many users are reporting issues about it even though it is documented in install.txt
I'm still looking for a way to get rid of it. That's why i recently have proposed an alternative way to integrate IMCE into tinymce and fckeditor.
What i'm saying is that, footer hook may ruin all your good efforts.

i hope the review helps.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
StatusFileSize
new tinymce-jquery_4.patch29.32 KB

Thanks for the heads-up about the use of hook_footer().

I've changed the implementation so we don't need hook_footer(). Instead, we output the needed files once, then, for each theme, add it's configuration data once through drupal_add_js() of type 'inline'. The only difference in the output is that, whereas before the settings were all passed as part of the Drupal.settings object, now that object is set with an empty Drupal.settings.tinymce.configs object, then elements are added to that object through inline calls, e.g., :

<script type="text/javascript">Drupal.settings.tinymce.configs["advanced"] = { "mode": "none", "theme": "advanced", ... };</script>

Again, this could easily be extended to allow manually-coded custom overrides. They would look exactly like this example, except that they'd be external rather than inline scripts.

The only additional change in this version of the patch is that I've added a test to ensure the requested theme is installed. If not, we default to 'advanced'.

Now regarding the compressor, I've not used it so I have no immediate idea of what's going wrong. Can you by chance confirm it was working before this patch on the same install? I'll install and test it and see if I can find the issue.

luti’s picture

luti commented

As much as I can see, the last patch (3) works fine. Thank you, nedjo. It stays now in use as it is (so patched), and I will report any issues that could pop up (hopefully not though).

About a compressor, I've had so many issues with it already in the past (advanced image window, advanced link window, some troubles more later, but don't remember well details...), so I am avoiding it since a few months ago (also for my production site 4.7). So, don't blame this patch for any issues, compressor didn't work well even witout it, not for version 4.7 as well as not for version 5.

Maybe it would be nice to provide some more details about the integration of manual files for amateurs like me - what we can do with them, and how exactly they have to be structured. For example, I've had some issues in the past opening windows in Firefox, which were of the wrong size, so I had to change an option about that in the tiny_mce_src.js / tiny_mce.js file manually. Could this be done this way?

luti’s picture

luti commented

Oooops, new version while I was typing my previous post. I'll try it right now... ;-)

luti’s picture

luti commented

Also the tinymce-jquery_4.patch patched TinyMCE seems to work fine (as much as I've tried it until now).

luti’s picture

luti commented

Maybe I was a bit too quick with my previous feedback. I've noticed later my Firefox is using more than 300MB of memory... Testing more, I've realized everytime I open my ibrowser plugin window (which now seems to work fine) memory consumption rises up immediately for another approx. 10MB, which are not freed even after I close the window.

Probably it has not much to do with this patches, but I rather post it here - just in case nedjo can find some new idea for any further improvements. Theoretically I can even live with this issue, since I am not using this plugin much - so, to close Firefox from time to time (to recover memory) also shouldn't be such a problem... ;-)

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

Hmm, I'd like to be able to pin down this memory issue before we apply this. I don't know much about testing for memory leaks. If you have time, could you test with an unpatched version and see if the results are the same, to help determine if the issue is new with this patch?

I did have one odd experience while working on this patch that might be related. I was IIRC navigating away from a page with the tinymce control on it. While the new page was loading, firebug started filling up with error messages about jQuery not being defined. These messages kept piling up after the new page loaded, and even after I'd closed the window (they started up in the next active window). I could only stop them by exiting Firefox and then relaunching it. I've tried to trigger this again but not been able to.

luti’s picture

luti commented

nedjo, no problem with your patches. I've tried with unpatched version, and it behaves exactly the same way, so it is obviously an issue of the ibrowser plugin (most probably I've introduced it when I was doing adaptations to work at least as ti is working now...). Sorry to bother you.

kreynen’s picture

kreynen commented

LUTi actually did find some problems with the patch, but posted them as issues with the 5.x-1.x-dev release. LUTi reported issues with advanced link and a strange error in webform that do not seem to be a problem in the 5.1 release.

m3avrck’s picture

m3avrck commented

Nedjo, this is a great patch! I really like where this is going and I had very similar ideas for doing the same with jQuery and Drupal 5.

A few thoughts:

- array_key_exists() isset() should be faster... $element['#attributes']['class']
- reference to simple, advanced themes -- what about custom? I know custom ones work but this is a bit unclear...
- merge the #wysiwyg and #wysiwg_style elements into one:
#wysiwyg = 'simple', 'advanced', FALSE ... etc, less elements to mess with :-)

Not sure I full get the cloning part -- or why it's needed, ping me to chat about that.

I say go ahead and commit this to HEAD. We can polish it up, fix the bugs that have been pointed out, fix a few other things, then merge all of that back into DRUPAL-5, tag an official release, and go from there :-)

luti’s picture

luti commented

nedjo, before including the latest patch above, check this issue: http://drupal.org/node/121789, please. With the latest published 5.x-1.x-dev version (2007-Feb-26; seems to be without those patches) I don't have any such problem as described there.

m3avrck’s picture

m3avrck commented

My suggested switch should clear up that issue with array_key_exists().

m3avrck’s picture

m3avrck commented

Nedjo, I've been cleaning up DRUPAL-5 branch and I think this is ready to go in, prob needs another look through.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

Because this patch rewrote substantial parts of the module, and because several patches have been applied since then, the patch fails in 7 out of 11 hunks when applied to the DRUPAL-5 branch.

I'd like to see this go in and think it was close to ready, but haven't found time to refresh it.

m3avrck, if you're able and willing to refresh it, that would be great. Otherwise let me know and I'll try to get to it. It's going to be tricky to determine which of the changes made to the module since this patch was written are still needed.

mounte’s picture

mounte commented

any progress on this?

Irrow’s picture

Irrow commented

I'm also curious what the progress is here, as I'm eagerly awaiting integration of WYSIWYG and activeedit.. unfortunately I'm quite clueless when it comes to javascript so I can't help out.

tanc’s picture

tanc
he/him
Primary language English
Location Italy
commented

Are there any advances in this area?

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

At this point I'm not keen to take on a major long term role in TinyMCE module maintenance, and would hesitate to see this scope of a change applied unless there was someone in place to carry it forward. So I'm not likely to repeat the considerable work of making this patch (much has changed since I had this working).

It would be better in any case to have a general approach that multiple rich text plugins could use. Possibly in the new wysiwyg module, http://drupal.org/project/wysiwyg ?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

nedjo, I'd be more than happy to join forces with you in the Wysiwyg project. After all, the current TinyMCE module is not that big, and your patched code here could be used as a starting point in Wysiwyg.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Project: TinyMCE » Wysiwyg
Version: master » 5.x-1.x-dev
Status: Needs review » Fixed

I'm done. The TinyMCE project is poorly maintained. How can this awesome patch be in the queue for ages?

Moving over to Wysiwyg queue.
Re-rolled and committed patch.

Works great, improves performance, and fixes compatibility with jQuery > 1.2.1.

Good job, Nedjo! :)

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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