My module has a page that has a form that appears via the Popups module. This works by getting the page with the form via XMLHttpRequest and then puplling the form out of the HTML and pasting that in to the existing page. This means that when the Wysiwyg API module is scanning the form and modifying it to trigger the JavaScript code that adds the editor, the JavaScript files and Drupal behaviours' settings added drupal_add_js have no effect, so the editor is not activated.

I imagine the same problem would affect text areas added to the page via the AHAH machinery.

I have worked around this by adding code that creates a fake form and calls wysiwyg_process_form directly in order to get the code added to the page.

Testing Matrix

Editor IE8 IE9 FF11 Win Chrome Win Safari OSX FF11 OSX Chrome OSX
ckeditor Y Y Y Y Y Y
tinymce Y Y Y Y Y Y
CommentFileSizeAuthor
#217 wysiwyg-lazy-init.217.patch12.84 KBTwoD
#217 interdiff-356480-213.txt2.36 KBTwoD
#213 wysiwyg.lazy-init.213.patch11.65 KBsun
#213 interdiff.txt7.16 KBsun
#202 wysiwyg-lazy-init-scripts.356480.202.patch11.23 KBTwoD
#202 interdiff-lazy-189-202.txt10.7 KBTwoD
#202 interdiff-lazy-197-202.txt4.58 KBTwoD
#197 wysiwyg.lazy-kiss.196.patch10.26 KBsun
#197 interdiff.txt1.17 KBsun
#189 wysiwyg-globals-callback.356480.189.patch9.24 KBTwoD
#194 wysiwyg.lazy-kiss.194.patch9.72 KBsun
#193 wysiwyg-globals-callback.356480.193.patch11.15 KBzhangtaihao
#188 lazy_wysiwyg_followup-356480-187.patch1.93 KBzhangtaihao
#180 Custom content panels30.01 KBlord_of_freaks
#178 lazy_wysiwyg-356480-178.patch4.35 KBwatchdog
#172 lazy_wysiwyg-356480-172-with-ctools.patch4.62 KBzhangtaihao
#168 lazy_wysiwyg-356480-168.patch4.32 KBzhangtaihao
#168 lazy_wysiwyg-356480-168-with-ctools.patch4.66 KBzhangtaihao
#167 lazy_wysiwyg-356480-167.patch8.4 KBMiSc
#150 lazy_wysiwyg-356480-150.patch7.68 KBwwhurley
#137 lazy_wysiwyg-356480-137.patch7.96 KBmradcliffe
#122 test_wysiwyg.zip2.4 KBidflood
#121 lazy_wysiwyg-356480-121.patch7.99 KBidflood
#115 test_wysiwyg-7.x-1.x.tar_.gz1.03 KBmradcliffe
#114 tiny-beforesave.jpg150.38 KBAnonymous (not verified)
#114 tiny-aftersave.jpg773.52 KBAnonymous (not verified)
#109 356480-by-zhangtaihao-Shawn_Smiley-sun.-Lazy-load-ed-p0.patch7.17 KBpopulist
#91 0001-356480-by-zhangtaihao-Shawn_Smiley-sun.-Lazy-load-ed.patch8.31 KBmradcliffe
#87 only-one-ckeditor-toolbar.png25 KBmradcliffe
#87 duplicate-ckeditor-toolbars.png31.46 KBmradcliffe
#68 lazyloadeditors-356480-68.patch8.27 KBzhangtaihao
#39 lazyloadeditors-356480-39.patch8.67 KBshawn_smiley
#29 wysiwyg-ajax-29.patch9.16 KBshawn_smiley
#28 wysiwyg-ajax-28.patch9.58 KBshawn_smiley
#26 wysiwyg.ajax_.26.patch9.27 KBshawn_smiley
#17 wysiwyg-HEAD.ajax_.17.patch7.33 KBsun
#16 wysiwyg-HEAD.ajax_.16.patch5.97 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: Need a way to activate WYSIWYG api when the form does not exist yet » Load Wysiwyg/libraries for AHAH forms

Yes, I experienced this issue as well, and we have yet to find a proper solution for it.

Currently, I'm doing the following in a custom form_alter() implementation to load all libraries for the Panels 2 page content (display editor) page:

  // Panels support.
  if (module_exists('panels') && $form_id == 'panels_edit_display') {
    // Load potential editor.
    foreach (filter_formats() as $format => $object) {
      if ($profile = wysiwyg_get_profile($format)) {
        wysiwyg_load_editor($profile);
        wysiwyg_add_plugin_settings($profile);
        wysiwyg_add_editor_settings($profile, 'advanced');
      }
    }
  }

I.e.: All libraries are loaded upfront on the executing page, without knowing whether the libraries will be needed at all.

Lacking a better idea, I thought of introducing a wysiwyg_load_all() function or similar for other modules that just does that.

effulgentsia’s picture

In case it's helpful, you may be interested in checking out the ajax_load module and/or the combination of ahah_response, ahah_page_storage, ahah_script_ensurer, and ahah_style_ensurer.

Both ajax_load, and the combination of the 4 ahah_* modules, try to address this general limitation to using AJAX/AHAH. At some point, I hope the developers of ajax_load and I can collaborate on how to improve/integrate these modules, as well as work towards solving the issues in core for drupal 7.

dkruglyak’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev

Note, there is an issue about WYSIWIG-enabling Panels: http://drupal.org/node/235764

@sun, I think your code may need to be updated to the latest Panels 3.

greg.harvey’s picture

Having a similar issue - got an AHAH call on the node form that replaces the body field (edit-body-wrapper) and when the field reloads the WYSIWYG no longer works on the node body. I think is because I don't seem to be able to use the core node_body_field() function to replace the form field, so I'm reduced to rebuilding them myself - probably some hooks not invoked or something like that?

But this will be a recurring problem I think, because there doesn't seem to be any elegant way to manipulate the node body field with AHAH right now, which is a shame. =(

Edit: my related issue, if anyone is interested: #527622: WYSIWYG editors disappear after node body rebuild and switching filter does not bring them back

ruloweb’s picture

Suscribing :).

Danny_Joris’s picture

subscribing

wmostrey’s picture

You might want to try the ajax module which contains an ajax_wysiwyg module that fixes this behavior in some cases.

intelligentdesigner’s picture

Wow, I have a large flat patch on the side of my head after banging it into this brick wall for daaaaaaays.

In the end, whaddaya know, there's a glitch in TinyMce where it doesn't recognise an ahah request as completed, and therefore can't initialise the editor properly.

After your callback, add a line of js before you call attach behaviors like so...

                $.ajax({
                    url:      url,
                    dataType: 'json',
                    success:  function (response) {
                                $(target).html(response.data);
                    },
                    error:    function (xmlhttp) {
                                alert('An error occured: ' + xmlhttp.status);
                    },
                    complete: function(){
                                tinymce.dom.Event.domLoaded = 1;
                                Drupal.attachBehaviors($(target));
                    }
jseffel’s picture

I've the same problem with textareas which have a "Add more" button. The first time I hit the "Add more" button a textarea via TinyMCE appears but it is frozen, the second time another textarea with TintMCE appears - the previously added textarea becomes unlocked and the newly added is frozen. I've tried TinyMCE 3.3.9 and 3.3.9.2. I have ajax_wysiwyg a go but with no luck.

Do I need to perform some magic in hook_form_alter() ?

wmostrey’s picture

I wrote down the solution I came up with: The Wysiwyg and CCK multiple value fields.

In short: you can fix most issues by installing the Wysiwyg API CCK Integration module, which itself depends on three other modules: JS Alter, jQuery Form Update and jQuery AOP.

If you still have issues then the only solution is to use the FCKeditor plug-in for the Wysiwyg module. It's the only one that doesn't have any issues with multi-value cck fields. (So yes, even CKEditor has issues.)

sun’s picture

Title: Load Wysiwyg/libraries for AHAH forms » Lazy-load editors
Version: 6.x-2.x-dev » 7.x-2.x-dev

This can only be fixed in Drupal 7.

bensnyder’s picture

to people more intelligent than me on this:

Are there some simple steps that can be included here to add in template.php or something to get this functional for D7? how difficult is this? I'm a bit out of my realm. Please provide a step by step if possible.

Thanks in advance!

TwoD’s picture

No, there are no simple steps to get this to work, which is part why we've waited for D7 to arrive.
Wysiwyg needs to be redesigned a bit for this to work as now it loads all the necessary files right away and it wouldn't know how/when to load them at a later stage. D7 also has a later version of jQuery which supplies the events we need, but the clientside code also expects all files to be there already so it doesn't react to those events yet.

thekevinday’s picture

subscribe

wrburgess’s picture

subscribe

sun’s picture

Status: Active » Needs work
FileSize
5.97 KB

ok, I officially don't get this:

  1. none.js is correctly loaded and appears in Drupal.wysiwyg.editor.attach[params.editor]
  2. but ckeditor is loaded, but then again, not loaded at all

To understand what I mean, you need:

  • admin_devel module, shipped with admin_menu
  • enable wysiwyg_test module via drush -y en wysiwyg_test
  • Enable CKEditor for a second (not the default) text format
  • Apply this patch
  • Disable browser caching, this is AJAX, cowboys! ;)
sun’s picture

FileSize
7.33 KB

Same as before, but this one loads something. :) Well, no editor yet, but it looks promising ;)

EliteMonk’s picture

subscribing

bensnyder’s picture

sun, your time spent on this is much appreciated, thanks!

bensnyder’s picture

bump bump :)

shags_j’s picture

subscribe

bryancasler’s picture

subscribe

joostvdl’s picture

subscribe

wOOge’s picture

subscribing

shawn_smiley’s picture

I've been looking into this some today and I've been able to get CKEditor to load/display (though not perfectly).

The main issue that I've found is that when the editor is being loaded via AJAX the basePath isn't getting set before the editor loads and thus the skin and some related files are being loaded from an invalid path.

I've been able to get the editor to display by adding the following line to the top of "wysiwyg.init.js".

  window.CKEDITOR_BASEPATH = 'http://test.localhost/sites/all/libraries/ckeditor/';

I'll continue working on this today and hopefully will be able to submit a patch in the next day or so to help move things forward.

shawn_smiley’s picture

FileSize
9.27 KB

Here is an updated version of Sun's patch in #17 that moves things forward a bit. We at least now have CKEditor displaying.

This patch adds two things to Sun's patch:
1. In ckeditor.inc, I add a new setting value that specifies the name of the global variable CKEditor looks at for the basePath.
2. In wysiwyg.init.js, I added some code that scans the settings array for all enabled editors and sets the global basePath variable if defined.

These are the remaining issues that I'm aware of at the moment:

  • The enable/disable rich-text link does not work correctly. The none.js file doesn't appear to be loading.
  • When used with Panels, we seem to loose all of the content in the wysiwyg when the form is submitted unless you first disable the wysiwyg before submitting.
shawn_smiley’s picture

Just a quick update on my progress. I have CKEditor working with the Panels module, though there are still a couple issues to work out.

The main issue I solved today is that I had to change the order of the form submit handlers so that the WYSIWYG submit handler fires before the Panels/CTools submit handler. This solved the problem with the WYSIWYG content not being saved.

The remaining issues to resolve include:

  • Enable/disable Rich Text Link not working correctly
  • wysiwyg doesn't load in Panels if you close the overlay and then reopen it

I'm hoping to submit an updated patch in the next day or two.

shawn_smiley’s picture

Status: Needs work » Needs review
FileSize
9.58 KB

Attached is a fully functional patch that allows CKEditor to work with Panels or in theory any other AJAX loaded text area. I haven't tried using any other editor yet, so there may be issues there that still need to be addressed.

This patch is rolled against the Feb 25 dev release of WYSIWYG.

I'm sure there are some things that could be implemented better and I'm certainly willing to continue making refinements to the patch as needed.

Please give it a try and post the results of your testing here.

Here is a quick summary of the enhancements made to the patch in comment #17.

editors/ckeditor.inc

  • Added a new setting named "global_basepath_var"

wysiwyg.init.js

  • Added code to check for the existence of the setting "global_basepath_var" and to use that to set an appropriate global path variable for the enabled editors.

wysiwyg.js

  • Added routine to re-order the event handler bindings so that the wysiwyg handlers always fire before any other handlers that had been previously attached.
  • Added additional check and event handler to the CTools overlay close link to unbind the wysiwyg.
shawn_smiley’s picture

FileSize
9.16 KB

Here is a modified version of the patch in #28 that doesn't have absolute paths to the files so that it can be directly applied to the wysiwyg module.

bryancasler’s picture

Any update on this?

shawn_smiley’s picture

I'm waiting for some feedback from the community on the patch and to have some other folks test it out. The patch works with CKEditor in the D7 project I'm working on right now, but I expect that it will need some more refinement before it can be merged into the main dev tree.

thekevinday’s picture

Okay, I just did a quick test using the views module where I first had this issue. (Reference: http://drupal.org/node/1027064)

The module applied without issues against the mentioned -dev branch.

When I tried to edit a text editor in the view module, the text area appeared.
Prior to this patch, the text area did not appear.

However, clicking the submit or even cancel button causes some sort of ajax error such that the page turns into a single line of ajax jargon.

The other issue I have had is that my particular editor is not showing up in the views.
I personally do not care about it appearing here, but this may or may not be intended by the lazy-load process.

chrisschaub’s picture

Hey @Shawn_Smiley, thanks for submitting the patch. It does allow wysiwyg to load ckeditor in panels, great. The data doesn't save, however. I'm using drupal7+bartik+panels... and your patch. Saves without the ckeditor. Also, you have a lot of debug statements in the patch that cause the javascript to fail unless you have something to support that function, throws lots of js errors. Thanks!

chrisschaub’s picture

Just to follow up on #29, it's all working except for the saving. I can't get the changes to stick.

beyond67’s picture

subscribe

chrisschaub’s picture

Has anybody else tried #29?

shawn_smiley’s picture

Thanks for the feedback thekevinday and schaub123. I'll try to find some time to investigate these issues over the weekend.

Related to content not saving, one of the challenges I found was that I have to re-arrange the order of the event handlers when the overlay window is submitted or closed. Otherwise the form was being processed before the WYSIWYG had a chance to update the underlying form field with the updated content.

chrisschaub’s picture

Hey Shawn_Smiley, what module are you using to handle the $.debug calls in the javascript? Firebug for drupal? Debug isn't released for dp7 yet.

Thanks.

shawn_smiley’s picture

@schaub123

I think something got lost in the patch generation so that we don't have the $.debug() function defined any longer. It was just a wrapper around Firebugs Console.debug() statement if I remember correctly.

Here is an updated patch that removes all of the debug statements.

Can you give me the exact versions of Drupal, contrib modules, and CKEditor that you're using so that I can try to replicate the issue?

Also, if you click the "Disable Rich-text" link before saving, does the correct HTML display in the text field?

It's working correctly for me with the following configuration (everything is latest version):

  • Drupal 7.x-dev (2011-Mar-21)
  • Administration menu 7.x-3.0-rc1
  • Chaos tool suite 7.x-1.0-alpha3
  • Devel 7.x-1.0
  • Libraries API 7.x-1.x-dev (2011-Feb-24)
  • Panels 7.x-3.0-alpha3
  • CKEditor 3.5.1
  • WYSIWYG 7.x-dev (from git as of today)
  • Views 7.x-3.0-alpha1

@thekevinday

I also wasn't able to reproduce your issue with the above configuration. I was able to enter and format the global header text using the WYSIWYG.

sun’s picture

$.debug() is provided by the admin_devel module, which ships with admin_menu.

chrisschaub’s picture

Ok, here's the issue with #39. Turn off the "disable rich text" link in the wysiwyg setttings and things stop working. I think it's because you are counting the handlers +2. So, you are expecting that folks will have the disable rich text link enabled. That's why it wasn't working for me. Is there a way to get it to not require that option being checked? Thanks, cool stuff!

shawn_smiley’s picture

Ah, excellent information. I'm sure I can come up with a better check before resetting the event handlers based on this info. I'll try to get an updated patch rolled out on Friday or at the latest Monday.

chrisschaub’s picture

Fyi, something else got changed in patch #39 that causes the window not to show on re-edit. Patch #29 was better, and I just manually removed the debug. Nevermind, #39 loads the editor, just no save.

shawn_smiley’s picture

Can you give me some exact steps to reproduce the issues including which browser you're using?

I just did the following this morning:

  • Created a new branch of the WYSIWYG module and applied path 39 using the "git apply" command
  • Updated my WYSIWYG settings to hide the "Enable/Disable" link
  • Created a new panel node
  • Added "New custom content" to one of the panel regions using the Filtered HTML format and clicked Finish
  • Edited the content I just added to the panel to confirm that the WYSIWYG editor still displays and contains the content.
  • Tested clicking the "Close Window" link and "Cancel" button followed re-editing to confirm that the WYSIWYG displays and has the correct content.
  • Saved the node and verified that the correct content is displayed.

I've done these tests using Firefox 3.6.10 on OS X 10.6.

thekevinday’s picture

#39 works for me without issues.
I am using the feb-25 dev release.

chrisschaub’s picture

I am using everything the same as you in #39. The only difference is that I'm using alpha4 of ctools. Tried firefox 3.6 on OSX 10.6. I couldn't use git apply, complained about whitespace. Used patch -p1 instead, but it said it patched all files cleanly. Using stock bartik with seven as admin theme in latest drupal 7 from dev.

chrisschaub’s picture

Shawn_Smiley. Just downgraded to ctools alpha3, no dice. Are you using the bartik theme and the stock admin menus?

chrisschaub’s picture

Found it. It was jquery_update. Disabled it and all is now working. Not sure what would cause that, but jquery_update was causing the issue. Patch #39 is working.

shawn_smiley’s picture

Thanks for the update schaub123. I haven't been running the jquery_update module, but I'll get it installed and see if I can track down where the conflict is at.

shawn_smiley’s picture

I've done some investigation of the issue related to jquery_update. It appears that the object structure returned when querying existing events on an object has changed between jquery 1.4.x and 1.5.x. Which is causing the WYSIWYG event binding to break when it's loaded in a ctools overlay.

I've implemented a hack of an update to confirm the problem and potential solution, but I want to come up with a cleaner implementation before submitting a patch.

Does anyone know of a clean way of getting a list of events bound to an object that works in both jQuery 1.4 and 1.5?

Currently I'm doing the following: event_list = $("selector").instance.data().events;

The problem with jQuery 1.5 is that $("selector").instance.data() returns an intermediate object with an instance specific name so you have to do $("selector").instance.data().jquery2358723598237542.events to get a list of bound events. The name of the object "jquery2358723598237542" changes with every page request so I ended up having to loop over the properties of the data() object to find the correct item to query.

loop54’s picture

hello guys!
1st, i'm new in drupal.
last time i work at my own website hosted by hostmonster.com.
my problem: in panel-> content it dont will be show wysiwyg editors.
my question: how can i patch with "lazyloadeditors-356480-39.patch"#39 or "wysiwyg-ajax-29.patch" #29?
my configuration:
drupal 6
ctools 6x18
panels 6x39
devel 6x123
wysiwyg 6x23
full pack of editors...
again i host my site at hostmonster, here no git programm and it seems will be difficult to install.
"git apply" command -only one way?
thanks

shawn_smiley’s picture

Hi loop54 and welcome to the Drupal community.

Unfortunately this patch only works with the Drupal 7 version of WYSIWYG right now. See comments 11 and 13 above for more detail.

zilverdistel’s picture

subscribing

The patch in #39 is working with the following configuration:

Update information last refreshed: Tue, 04/12/2011 - 09:20

Drupal core: 7.0
Chaos tool suite: 7.x-1.0-alpha4
Libraries API: 7.x-1.0
Panels: 7.x-3.0-alpha3
Wysiwyg: From git

CKeditor: 3.5.3

I also tested with TinyMCE (3.4.2-jquery), but it didn't work. The editor simply didn't show up.

Again with ckeditor 3.5.3, I tested IMCE (7.x-1.3) and IMCE wysiwyg bridge (7.x-1.x-dev) which also seemed to work. I'm able to add images to my custom content in panels with ckeditor.

Thanx for the patch, Shawn!

Just for the sake of completeness: the patch could only be applied to the git master branch of wysiwyg. It failed on the dev release.

luckystrikerin’s picture

great works #39 works for me as well (wysiwyg 7.x.-2.0)

update: looks like it works. Unfortunately it works not always and the content pasted via wysiwyg is not beeing saved!!!!

Any ideas?

zilverdistel’s picture

@luckystrikerin: I have the impression this doesn't work with all configurations, can you post your module versions?

luckystrikerin’s picture

Drupal: 7.0
Chaos tool suite: 7.x-1.0-alpha4
Libraries API: 7.x-1.0
Panels: 7.x-3.0-alpha3
Wysiwyg: 7.x.-2.0
CKEditor 3.5.2.6450

After this patch I could at least see the wysiwyg editor sometimes, before I always got a white place with nothing. But the content I inserted via ckeditor is not beeing saved.

zilverdistel’s picture

@luckystrikerin: I was not able to apply the patch to Wysiwyg: 7.x.-2.0. It generated failures (1 out 5) when I tried (even on the dev version, which I think is weird). In my case, it only applied correctly on the git version, with git apply ...

It's the master branch: http://drupal.org/node/181465/git-instructions/master.

luckystrikerin’s picture

@zilverdistel: thanks for your help. Its working now with the git version.

rwohleb’s picture

Any update on the jQuery 1.5 issue? I can confirm that the patch in #39 works with wysiwyg loading in a ctools modal (privatemsg send form in this case) with jQuery 1.4, but not 1.5.

jeffschuler’s picture

Patch in #39 is working for me with CKEditor but not with TinyMCE.

Using:

  • Drupal 7.0
  • Latest WYSIWYG 7.x-2.x-dev from git
  • CKEditor 3.6
  • Panels 7.x-3.0-alpha3
  • CTools 7.x-1.0-alpha4

Seeing the same issue with JQuery Update.

sun’s picture

Not sure if relevant, but referencing nevertheless detail about CKEditor base path in duplicate issue #1067792: Editor not showing on Views Global text area - m.lang.contextmenu is undefined

jmones’s picture

subscribe

bensnyder’s picture

sub

ryne.andal’s picture

sub

cangeceiro’s picture

patch in #39 also worked for me

alex.skrypnyk’s picture

#39 worked for 7.2, but not master. weird.
However, changes are still not saved.

zhangtaihao’s picture

I've fixed #39 (for at least CKEditor and TinyMCE) by correcting the logic that inserts Wysiwyg's event handlers before CTools's. Also, TinyMCE is now manually instructed to load by telling it DOM is loaded.

Granted, TinyMCE still doesn't work that well with Panels IPE, but I suspect it's IPE's fault (see #69).

TwoD’s picture

This is starting to look really good!
Though, it does reveal a bug in Core: #1287368: Drupal.settings.ajaxPageState.css gets overwritten.

grossmann’s picture

sub

Vandalf’s picture

subscribing

franzkewd’s picture

Subscribe

tanitani’s picture

Subscribing.

greg.harvey’s picture

@ the last two posters, you realise there's a "Follow" button in the top right of every issue page now so you don't have to type "Subscribe" in a post to get alerts? Right now it'll say "Following" because you were auto-subscribed, but before that (on any other issue) it's a big green "Follow" button in the right sidebar.

See: http://drupal.org/node/1306444

bryancasler’s picture

What's holding up the patch from #68 getting committed?

fonant’s picture

Patch in #68 plus patch from #1287368-7: Drupal.settings.ajaxPageState.css gets overwritten work nicely to get CKeditor in the Panels custom content popup in my Drupal 7.10 site.

TwoD’s picture

@animelion, not much. A final review of #68 should be able to RTBC it now that #1287368-25: Drupal.settings.ajaxPageState.css gets overwritten is RTBC, and it can get committed when problems caused Core won't cause a flood of bug reports/comments about this fix not working properly. (Without the Core patch, some editors will look really crappy when lazy-loaded by, for example, the Panels' Pane-editor due to missing stylesheets.)

That might still happen considering not everyone will also update to the latest Core when updating Wysiwyg. But at least then we can close those issues with a quick "Please try with the latest Core release first, then re-open this issue."-message. Hopefully, that won't happen so often, as anyone about to create such an issue should be reading this existing issue and comment first. ;)

joelstein’s picture

I applied the patch in #68, and now CKEditor works in Panels Panes. Marvelous. However, I noticed that on line 23 and 32-34 of the patch, some commented-out code was added. It should be removed, unless it was meant to be un-commented. FYI.

smartango’s picture

is related to http://drupal.org/node/1391034 ?

I applied patch in #68 and still wysiwyg editor is not loaded via ajax ( TinyMCE )

EDITED: TinyMCE

smartango’s picture

sun code in #1 in form creation load wysiwig toolbar twice (actually cause the call of wysiwyg editor twice)

smartango’s picture

#80 happen when filter format with wysiwyg profile activated is the first (default) for the form element, moving to second position, thus activating it by changing drop down, do not cause problem

TwoD’s picture

@smartango, http://drupal.org/node/1391034 does not lead anywhere. Wrong nid?

The editors won't always be loaded via AJAX when the #68 patch and the D7 patch from #1287368-40: Drupal.settings.ajaxPageState.css gets overwritten are added (it's been committed to D8), but it should make it possible to use them in the Panels editor and other places where textareas with format selectors are loaded via AJAX.

Sun's code from #1 was an experiment, it's of no use now as things have changed a lot since 2009.

smartango’s picture

@TwoD I've done a wrong test, #68 works with TinyMCE, css loading is missing but I added it to the initial form.

So, for me #68 is ok

I think http://drupal.org/node/1391034 is the same problem: editor not loaded if node_add il called in ajax callback. Am I wrong?

TwoD’s picture

Ah, now the link works (I got a 404 earlier for some reason).

If the situation in #1391034: wysiwyg is not loaded when used with node_add() in ajax forms works after applying #68, it's the same issue.

comodo1980’s picture

Sorry, I´m reading this post and documentation about patch and i don´t understand where and how i have tu apply this patch. My english is very bad and I suppose it´s a big problem.
I´d like somebody could help me a little, please.

TwoD’s picture

@comodo1980, you apply the patch from #68 to your wysiwyg folder, and the latest patch from #1287368-40: Drupal.settings.ajaxPageState.css gets overwritten to the root Drupal folder. How to apply a patch depends on which OS you have, start at Applying patches with Git, or HowTo: How to apply a patch to a contributed module - Beginner's version (Windows).

mradcliffe’s picture

I'm not sure if this is related to this patch or issue, but when loading in a modal using ckeditor 3.5.2 (after doing something similar to sun's code in #1) there are two ckeditor spans beneath the textarea (thus getting duplicate toolbars). I, then, only get one span if I close the modal and re-opening (after applying/using #947676: Support new CTools event for detaching when the modal closes - this doesn't matter for this issue i'm running into other than it doesn't happen on re-opening the modal).

The only place I found to fix this was in wysiwyg.js line 65, changing an if to an else if. I am not sure what the effects of this are as I am not familiar with Wysiwyg API.

      if ($this.is(':input')) {
        Drupal.wysiwygAttach(context, params[format]);
      }   
      // Attach onChange handlers to input format selector elements.
      // Without this else if I get two duplicate ckeditor toolbars on the FIRST load in a modal, but the second time it's fine.
      // I have no idea why.
      else if ($this.is('select')) {
        $this.change(function() {
          // If not disabled, detach the current and attach a new editor.
          Drupal.wysiwygDetach(context, params[format]);
          format = 'format' + this.value;
          Drupal.wysiwygAttach(context, params[format]);
        }); 
      }   
TwoD’s picture

It sounds like the editor gets attached twice. The change to an "else if" prevents the editor from being attached when there's a format select box present. Your screenshot does not include anything below the area, but I assume there is a selectbox there.

Why are you using code similar to that in Sun's #1? It is not needed if you apply the patch from here, since any Wysiwyg/editor scripts that should get loaded when a format-enabled textarea is created are automatically added by Drupal/Wysiwyg.

Can you try with just the patch from here to Wysiwyg 7.x-2.x-dev, then apply the one from #947676: Support new CTools event for detaching when the modal closes? I'm not sure the last one is needed for D7 since we now support detaching behaviors.

Could you attach or link to the code loading the modal if it doesn't work so we can try to reproduce the issue?

mradcliffe’s picture

Yeah, I'm going to have to try to narrow down and simplify the code to see if I can reproduce it first.

pdcarto’s picture

I followed instructions in #86, cleared cache (important), and it works! Thanks TwoD!!!!

mradcliffe’s picture

#88. Yes, the patch does solve the double issue I was having (instead of having to make that js modification). Patch didn't apply on my 7.x-2.x-dev so I have re-rolled it here.

I did not need to apply #1287368-40: Drupal.settings.ajaxPageState.css gets overwritten to get wysiwyg with ckeditor to load in a ctools modal.

zhangtaihao’s picture

Thanks for rerolling the patch.

Additionally, I think it's finally time to decide what to do with that commented section (per #78).

EDIT

For clarity:

+//  if (typeof Drupal.wysiwyg.editor.initialized.ckeditor == 'undefined') {
+//    Drupal.wysiwyg.editor.init.ckeditor(settings);
+//  }
JulienD’s picture

I followed Twod's instructions in #86, cleared cache, and it works fine!

Thank you !

dieuwe’s picture

Status: Needs review » Reviewed & tested by the community

Patch from #91 works great!

Anybody’s picture

Just tested it and can confirm that #91 works great!

For the German users (and others of course who would like to use it - but blog post is in German), I've provided a temporary patched wysiwyg module download at my blog post:
http://julian.pustkuchen.com/drupal-7-wysiwyg-tinymce-panels-benutzerdef...
which I am using myself.
(As a short help for users who don't know how to patch, of course handle with care, it's just a reviewed patch!! See warning at the end of the blog post.)

mradcliffe’s picture

I am not sure if this is what #43 was running into, but the first submit does not transfer values to form_state. After the form is rebuilt, it works fine. For instance, if you had a button that just saves and returns the form in ctools, then on the next save/submit, the value is saved.

Is anyone seeing this as well? I don't want to knock this out of RTBC if I can't reproduce that issue.

derhasi’s picture

#91 works fine so far for me too.

+1

mradcliffe’s picture

Status: Reviewed & tested by the community » Needs work

Sadly, I'm definitely seeing an issue with core ajaxSubmit and putting ckeditor wysiwyg in a ctools modal. If I add the following javascript to misc/ajax.js line 254, then it works, but without it, ckeditor.js does not update in time. I think this is because Drupal ajax is binding way late in the process.

Does anyone have any tips for solving this bug and getting updateElement() to fire before ajaxSubmit?

      for (instance in CKEDITOR.instances) {
        CKEDITOR.instances[instance].updateElement();
      }
mradcliffe’s picture

This does not seem like the right way to do things:

  $(document).ready(function() {
    window.setInterval(function() {
      for (instance in CKEDITOR.instances) {
        CKEDITOR.instances[instance].updateElement();
      }   
    }, 500);
  }); 
DanielF’s picture

Sorry to break the flow, but most of the above is beyond my technical expertise. I came here from #97 on the Textareas issue. I'm experiencing the exact same problem described there:
Cannot enter text in the text field.
I was hoping someone could indicate to me if any progress on fixing this has been made. I know of the wysiwyg_panels module, but because it's not available on Drupal.org I'm hesitant to use it, and would prefer a fix in the Wysiwyg module itself. Thanks!

mradcliffe’s picture

Try decreasing z-index for those dialogs?

Anonymous’s picture

91 works for fixing the broken CKEditor+WYSIWYG with Panel popups.

mradcliffe’s picture

The ckeditor div.cke_kama is a sibling of modalContent so it's child, div.cke_rcombopanel, is outside of modalContent as well. On OSX Firefox 10 this means that you can't click on any item in the combo box.

zhangtaihao’s picture

@DanielF, @mradcliffe: Perhaps something to do with #905546: modalContent focus?

mradcliffe’s picture

My issue was apparently a Flash issue because I had a flash object on the page and a modal above that with WYSIWYG (ckeditor). Strange bug that also had an effect on Firebug.

kardave’s picture

#91 worked me too. Thanks!

mradcliffe’s picture

Status: Needs work » Needs review

Review comment #92 and patch #91

Edit: I forgot to change this back to needs review after I discovered the issue with Flash.

Jim Kirkpatrick’s picture

#91 works... Thanks!

populist’s picture

Here is a quick reroll of #91 for the -p0 standard. No other changes have been made.

FiNeX’s picture

Patch on #109 works fine

mradcliffe’s picture

Here's a table of testing if anyone else wants to confirm "works for me" status. I have the following tested for ckeditor.

Editor IE8 IE9 FF11 Win Chrome Win Safari OSX FF11 OSX
ckeditor Y Y Y Y Y
tinymce N
?
?
Anonymous’s picture

TinyMCE seems to have an issue with the 91 patch, but CKEditor seems to be lazy loading fine. The problem I found with TinyMCE is that when you hit save in a modal, it loads the AJAX request text into the actual browser window (tested in Chrome) and doesn't redirect back to the Panels interface.

It does seem to save though... if I refresh then the content is changed to whatever I made it in the TinyMCE modal edit window.

mradcliffe’s picture

@112: This might be an issue with either not having the ctools detach behavior.

Edit: I can't reproduce this in a simple ctools modal loading tinymce in Safari Windows or Firefox 11 or IE8. Clicking Save works properly.

mradcliffe’s picture

Issue summary: View changes

Added testing matrix.

mradcliffe’s picture

Issue summary: View changes

Adding some more test data.

Anonymous’s picture

FileSize
773.52 KB
150.38 KB

Here are the screenshots of what I was experiencing in #112, aftersave shows the text loaded inside Chrome.

Anonymous’s picture

Issue summary: View changes

oops, typo.

mradcliffe’s picture

Issue summary: View changes

More manual tests.

mradcliffe’s picture

I'll try in Google Chrome on Windows tomorrow. Need to install it first. Google Chrome in OSX works fine in both panels and in custom ctools modals.

Do you get the same problem with this custom module (it exposes a menu item in the navigation menu)?

mradcliffe’s picture

Issue summary: View changes

More testing, correct heading for chrome OSX.

mradcliffe’s picture

I just tested my test module and in panels with latest Chrome in Windows XP. I could not reproduce the issue you ran into with either tinymce or ckeditor, vilepickle.

Any IE9 testers out there?

Juan C’s picture

Patch #109 fixed mine.

mradcliffe’s picture

Juan C, what browsers did you test the patch in? Can you add any more confirmation for the testing matrix?

TwoD’s picture

I've been trying to debug the TinyMCE related changes from the #109 patch in Chrome (under Ubuntu) by opening the node edit form in a CTools modal dialong - which I originally created to test #947676: Support new CTools event for detaching when the modal closes.

The reason TinyMCE doesn't work seems to be that its theme doesn't get registered properly when its script file is loaded.
For some strange reason, inspecting TinyMCE's ThemeManager right after the theme script registers itself does show that the theme was added to tinymce.ThemeManager.lookup, but when inspecting it from other scopes it's no longer there.
It only happens the first time TinyMCE is rendered. Subsequent calls to the render() method does correctly register the theme so the rest of TinyMCE sees it.

It's really difficult to debug this with the tools in Chrome because it sometimes completely skips over calls into dynamically loaded scripts even if there are breakpoints in there, so one has to rely on console.log() calls...

Juan C’s picture

Tested on OSX: Safari, Firefox, and Chrome.
Update: Using ckeditor

idflood’s picture

I've tested the patch in #109 with panels only and it globally works really well. With ckeditor (3.6.2.7275) everything is perfect, but for TinyMCE (3.5.0.1) I had a this little error when submitting a form.

Uncaught TypeError: Cannot call method 'replace' of undefined
Drupal.wysiwyg.editor.attach.tinymce (content:86)
...

It appears that the Drupal.wysiwyg.editor.attach.tinymce function is called when the modal is closed but the dom element is removed. This is called from Drupal.behaviors.attachWysiwyg, by looking at Drupal.settings.wysiwyg.triggers[this.id].

Simply adding a 'delete' inside the detach function seems to do the trick, but I'm not sure if this can create other issues. So the only difference with patch in #109 is one extra line in Drupal.behaviors.attachWysiwyg.detach:

Drupal.behaviors.attachWysiwyg = {
  ...
    detach: function (context, settings) {
    $('.wysiwyg', context).removeOnce('wysiwyg', function () {
      var params = Drupal.settings.wysiwyg.triggers[this.id];
      Drupal.wysiwygDetach(context, params);
+    delete Drupal.settings.wysiwyg.triggers[this.id];
    });
  ...

I did the test with these browsers:
mac: firefox 12 / opera 11.64 / safari 5.1.5 / chrome 19
windows: ie7 / ie8 / chrome 19

idflood’s picture

FileSize
2.4 KB

I wanted to test the patch from #121 with the custom test module in #115 but the modal content was empty. Here is an updated version. I've tested it with chrome on windows and mac and I had exactly the same error as described in #121. Here too, the "delete" looks enough to fix that.

Summit’s picture

Hi Guys, Anyone have latest wysiwyg with patch for ckeditor already added?
Thanks a lot in advance for posting if you have it.

greetings, Martijn

swentel’s picture

Tested with latest ckeditor - works perfectly. Patch even applies against latest official release. Don't have to time to reroll now.

cangeceiro’s picture

Tested with ckeditor and everything looks good, this also resolves the following issue #1401038: Wysiwyg not showing, Node Form in a Ctools Modal

Andi-D’s picture

Thx for Patch from #121, works fine

Greetings Andi

mradcliffe’s picture

That's good enough for me. I'll roll a new patch.

idflood’s picture

I wanted to quickly reroll the patch since I'm working with this right now but I don't see why it would be required (the patch in #121 still applies cleanly on the 7.x-2.x branch).

mradcliffe’s picture

Status: Needs review » Reviewed & tested by the community

Great.

Stalski’s picture

Status: Reviewed & tested by the community » Needs work

For me, wysiwyg with tinymce loads the editor fine with the patches above (109, 121 however needed to apply them manually on current dev and stable from 19 June).
However links and images do not work:
- The modal is not themed and some controls are not workable
- The inser/edit image link does not work

I'll try with ckeditor as well.

adamdicarlo’s picture

Patch in #121 works awesomely for me with TinyMCE 3.4.8 in Panels, even with the IPE.

Baysaa’s picture

There seems to be a small issue with the paths of external CKEditor plugins after applying patch in #121. I have Linkit module installed which loads a plugin.js from the Linkit module's directory. However on the "Add custom content" modal window of Panels there's a javascript error:

"GET http://www.example.com/sites/all/libraries/ckeditor/_source/plugins/link... 404 (Not Found)"

The plugin.js for linkit should be loaded from sites/all/modules/linkit/editors/ckeditor/plugin.js

Seems like after applying this patch, the paths for external plugins are not respected.

mradcliffe’s picture

Does wysiwyg module support loading plugins from arbitrary directories? I thought all of those were supposed to be in sites/all/libraries?

TwoD’s picture

@mradcliffe, Yes, Wysiwyg supports loading plugins from anywhere (if the editor supports it), as long as hook implementation sets 'internal' => FALSE for native plugins. See wysiwyg.api.php for details.

@Baysaa, are you sure you only applied that patch? It looks like CKEditor is trying to load the source/uncompressed version of the plugin. Does it use the correct path when not lazy-loading the editors, on the node edit form?

Baysaa’s picture

@TwoD looks like another patch made by you has been applied by my colleague developer, which allows editors to be configured to load from unminified source, which probably explains why the editor worked fine just after I applied the patch at #121, but stopped working few days later. Bummer lol, sorry false alarm. I went to the config page of the wysiwyg profile that was having problems, and selected "default" mode from advanced settings (was set to Source by the other developer, so not sure if setting to Source is required for some other functionality .... :( ).

franzkewd’s picture

Can someone update Patch #109 or #121 against the latest dev built (July 2, 2012).
Thanks.

mradcliffe’s picture

Re-rolled patch. I added a serialize option to one of the detaches to match the current code base. Not sure if it's correct at the moment.

TwoD’s picture

@mradcliffe, That does seem correct. If the event triggering a detach will also lead to the editor's DOM elements being removed [and the editor being automatically re-attached later], a 'serialize' trigger should be used.

I haven't had time to verify this, but do we need to explicitly find the CTools modal close button if we get #947676: Support new CTools event for detaching when the modal closes in?
Seems reacting to a custom event sent directly by them is more reliable in the long term than digging into the DOM and hooking into a click event on some element somewhere. What if they need to change how that close button works or move it somewhere else? Could themes change the structure of the modal so our event attaching script would break?

seutje’s picture

It's always advised to make the least tight coupling between your markup and your javascript.
Things like $(this).parent().parent().parent().parent(); are not only unreadable, but also very fragile.
Binding to a custom event would seem like a better option, as it is far less intrusive, less costly in DOM traversal and far more future-proof.
Although I don't fully support the format CTools uses for its custom events, it certainly beats scouring the DOM for a close button and then binding it directly to said button.

heshanlk’s picture

For me issue was the BASE_PATJ and I'm using CKEditor.
http://docs.cksource.com/CKEditor_3.x/Developers_Guide/Specifying_the_Ed...
and adding

window.CKEDITOR_BASEPATH = '/sites/all/libraries/ckeditor/';

on my JS fixed the issue.

Ivanhoe123’s picture

Thank you

***
EDIT: this did not work for me

The issue is back when I open several times WYSIWYG areas - WYSIWYG is loaded only the first time
Also the content is not saved when I hit "Save"; the area is left blank when I edit it later on

mradcliffe’s picture

@TWoD, I've been running with this patch and then I added the code in #947676: Support new CTools event for detaching when the modal closes separately without any issue.

ailgm’s picture

I tried applying the patch in 137 to 7.x-2.1, but couldn't find the spot to apply this change:

@@ -96,6 +119,7 @@ Drupal.behaviors.attachWysiwyg = {
wysiwygs.each(function () {
var params = Drupal.settings.wysiwyg.triggers[this.id];
Drupal.wysiwygDetach(context, params, trigger);
+ delete Drupal.settings.wysiwyg.triggers[this.id];
});
}
};
@@ -113,7 +137,7 @@ Drupal.behaviors.attachWysiwyg = {
* @param params
* An object containing input format parameters.
*/

Suggestions?

mradcliffe’s picture

Usually patches only apply to development versions. You could try the patch in #109, which I think I've applied to 7.x-2.1.

cangeceiro’s picture

I have ran into an issue with this patch when used in conjunction with the media module. To repeat, create a node with a wysiwyg enabled field, and a multivalued media field. Every other time the "Add another item" button is clicked, an Exception is thrown. Remove the try/catch in misc/ajax.js and it reveals that wysiwyg.js is throwing an error.

rachek’s picture

@cangeceiro, I have the same problem. I managed to overcome it by hacking wysiwyg.js and deleting "detach" function (103 line). Seems to work so far, but I haven't tested thoroughly.

TwoD’s picture

What is the exception being thrown?
If you remove the detach function, Wysiwyg won't be able to clean up after the editors, which will make most of them throw additional exceptions when they are to be reattached (because they think instances still exist).

rachek’s picture

@TwoD, in my case for multivalued image field I can successfully upload only one image, and when I click "Upload" to upload another one ajax.js catches an exception and displays an alert box with this text "An error occurred while attempting to process /file/ajax/field_images/und/form-qXOjWBivjUEAnPiMgBtp5sq3RjWK4UXjgTAEAtZ08qU: params is undefined" and image is not uploaded.
As cangeceiro mentioned, if you remove try/catch block in ajax.js (line 239) then in console you can see that exception is being thrown in wysiwyg.js, line 187, "TypeError: params is undefined".
I know that removing detach funtction is not the right thing to do, but somehow for several days after this hack and after using ckeditor quite extensively in Panels, in modal windows without page reloads, I've never seen a single error or exception.

wwhurley’s picture

We're having the same issue as rachek. It appears to be caused by WYSIWYG attempting to detach events on items in Drupal.settings.wysiwyg.triggers that were deleted but never re-created. If I comment out the line:

delete Drupal.settings.wysiwyg.triggers[this.id]

the error goes away. Anyone know why that line is in there and what I might test with it commented out?

wwhurley’s picture

Attached is a patch re-rolled on dev with the delete statement taken out. So far it appears to work with all multi-value items as well as Panels / Panelizer.

TwoD’s picture

The delete statement was there to prevent errors caused by trying to trigger a re-attach on elements which may no longer be available.

The reason the error is thrown is that the trigger data also get deleted during the serialization stage, before we know exactly which elements in a form will be unloaded and need to be re-attached later. The whole form gets sent as the context in the serialization stage, so all triggers get destroyed. After new content has been fetched from the server the unload stage is entered. The triggers should only have been deleted during that stage so triggers belonging to fields which are not about to be replaced are left intact. Wrapping the delete statement in a simple if (trigger == 'unload') statement should have fixed the problem while keeping the trigger data properly pruned.

Only deleting trigger data during unload will however create a secondary issue. If no new settings are returned with the AJAX response Drupal will instead pass the global Drupal.settings object to the detach code during unloads and once again to the re-attach behaviors. This leads to deleting the triggers for existing items in a multi-value field and only attaching editors to the newest field item.

I suppose we have to trust that any new Wysiwyg settings merged into Drupal.settings.wysiwyg.triggers by an AJAX call do not cause any conflicts. Having leftover triggers in Drupal.settings.wysiwyg.triggers should not cause any harm as long as nobody messes with the wysiwyg/wysiwyg-processed classes.

populist’s picture

Thankswhurleyf1 for the re-roll in #150! I applied this to the latest version of Panopoly (TinyMCE) and see this working across a range of use cases involving Panels, Panelizer, and Fieldable Panel Panes.

willieseabrook’s picture

I confirm that at 8 August 2012, #150 works for wysiwyg-7.x-2.x-dev and CKEditor 3.6.3.7474 getting minipanels popup content editor to show a wysiwyg editor

robcarr’s picture

Status: Needs work » Needs review

Just tried the patch at #150 with TinyMCE. Works fine with Panels/custom panes and node editing. Even works with the Panels IPE. More reviews please!

Thanks for the work on the patch

discipolo’s picture

for some reason drush doesnt manage to apply the patch.

afmdsouza’s picture

Patch at #150 worked for me with CKEditor - thanks

afmdsouza’s picture

@discipolo CD to the root of wysiwyg project folder, and apply the patch using -p1 option. Make sure you have the latest dev version of the code, it should work.

chriz001’s picture

im trying to use patch 150 and allow aegir to do my build but i think im having the same issue as discipolo, works fine using a newer version of drush though.

xtfer’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #150 appears to work fine.

idflood’s picture

It looks like I can't reproduce the issue described in #121 with the latest patch, drupal 7.15 and current git version of wysiwyg. Looks good.

vamirbekyan’s picture

I cannot confirm that #150 works with wysiwyg-7.x-2.x-dev and CKEditor 3.6.3.7474. I still see the same issue as desc in #141.
But I see no issue with wysiwyg-7.x-2.x-dev plus #150 and TinyMCE 3.5.6.

Scott M. Sanders’s picture

It works for me -- wysiwyg-7.x-2.x-dev and CKEditor 3.6.4.

Max_Headroom’s picture

Confirmed patch #150 works in text area disappearing in Panels. (Yeah!)

I hope it gets committed soon.

prinds’s picture

The patch in #150 works for me too.

However, I had already patched the wysiwyg module with the patch from this comment http://drupal.org/node/507696#comment-6143656 and I had to reverse that patch before i could get #150 working.

If both patches are applied at once I get the uncaught exception: [CKEDITOR.editor] The instance "edit-field-pane-text-und-0-value" already exists.. This only happens for when I use ckeditor in the panels modal window.

I can't figure out how these patches conflict, but I'm hoping someone can help shed some light on this.

MiSc’s picture

The patch in #150 partly worked for me.
In a modal window (panels, mini panels or page manager), the first time I load it, the editor shows up (ckeditor), if I close it and open it again, the editor does not show.

zhangtaihao’s picture

#165: This is basically the problem with removing the delete statement: it completely screws up Ajax workflows.

EDIT: Okay, never mind. Two slightly unrelated issues are being discussed: when editors are detached/unloaded in general, and how editors should be unloaded for CTools.

MiSc’s picture

Ok, a re-roll with the delete statement.

zhangtaihao’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.66 KB
4.32 KB

Here's a clean re-roll, minus commented code, minus all the crap I did back in #68.

Given #138, it would seem merlinofchaos's solution is much cleaner and more dependable. This patch removes my addition in #68 in favor of #947676: Support new CTools event for detaching when the modal closes.

For reference, problems encountered so far that seem to be resolved include:

  • Error thrown when adding another multi-value field item
  • Wysiwyg editor not showing initially in CTools modal dialog
  • Wysiwyg editor not showing after closing CTools modal dialog and opening a new one with editor

I've also taken the liberty of retaining the original signature of Drupal.wysiwygAttach since adding settings serves no purpose if the function still uses Drupal.settings.

In summary: apply the first patch and #947676: Support new CTools event for detaching when the modal closes to get the full effect (you may need to count some lines).

Alternatively, for the lazy (given the topic of this issue), try the second patch.

EDIT: @MiSc Sorry I didn't manage to post this before you did #167.

MiSc’s picture

Status: Needs review » Reviewed & tested by the community

No problem, I tested your patches, and they work as they should.

robcarr’s picture

Status: Reviewed & tested by the community » Needs work

I just rolled against latest DEV (30 Aug 12) and the patch lazy_wysiwyg-356480-168.patch failed:

patching file wysiwyg.js
Hunk #1 FAILED at 160.
1 out of 1 hunk FAILED -- saving rejects to file wysiwyg.js.rej
robcarr’s picture

Status: Needs work » Reviewed & tested by the community

After a second try, the lazyman's patch lazy_wysiwyg-356480-168-with-ctools.patch rolled cleanly, and works as expected against current DEV. Apologies for negative status change.

Could a few others test lazy_wysiwyg-356480-168-with-ctools.patch rolled by zhangtaihao, then also look at the issue #947676: Support new CTools event for detaching when the modal closes to RTBC that too.

zhangtaihao’s picture

I use the following commands to apply the patches.

git apply -v lazy_wysiwyg-356480-168.patch
git apply -v 0001-947676-by-merlinofchaos-7.x-2.x.patch

Anyway, I've attached another patch that combines lazy_wysiwyg-356480-168.patch and the updated #947676-18: Support new CTools event for detaching when the modal closes.

To apply #168 and the new patch serially, type these commands:

git apply -v lazy_wysiwyg-356480-168.patch
git apply -v wysiwyg_ctools_detach-947676-18.patch
orourkedd’s picture

#172 works! I ran this command from the root of the wysiwyg module:

git apply -v lazy_wysiwyg-356480-172-with-ctools.patch

Its working, but I'm picking up this error in the console after I save in Chrome Version 21.0.1180.82:

Uncaught [CKEDITOR.editor.replace] The element with id or name "edit-body-value" was not found.
zhangtaihao’s picture

I'm not getting the error. I've tested it on Chrome 21.0.1180.89m, Firefox 14.0.1, Firefox 15.0, and IE9.

I'm testing with a content type with a multi-value image field and a multi-value term reference field. Adding new items and uploading new images do not cause any error, nor does a failed validation (e.g. empty title). So, a basic case does not trigger any errors.

Could you provide more details than what I've provided here?

slucero’s picture

The patch in #172 solved the issue for me as well, but I am also getting the same error in Firebug that's reported in #173.

I'm using:
Firefox 14.0.1
CTools 7.x-1.1
WYSIWYG 7.x-2.x-dev
CKEditor 3.6.2 revision 7275

zhangtaihao’s picture

I have now deployed the patch on my live platform (matching the details in #175). No errors to report.

Additionally, I have further upgraded CTools to 7.x-1.2 and still no errors.

I forgot to ask: what action are you taking to trigger the error? I initially assumed you were editing the body field of a node, but then I realized the body of a custom content pane also has "edit-body-value" as the ID.

zhangtaihao’s picture

Also, this looks suspiciously like another module has done something to certain text areas and somehow misplaced the ID.

watchdog’s picture

Here is the patch from #168 adapted to wysiwyg-7.x-2.1

ayalon’s picture

#172 is tested by me and works as expected.

lord_of_freaks’s picture

FileSize
30.01 KB

#178 works great for me:

Drupal version: 7.15
Chaos tool: 7.x-1.2
Panels: 7.x-3.3
Wysiwyg: 7.x-2.1
Library ckeditor: 3.6.4

Testing in node add/edit form and Page manager custom content (see attached image)

Great job!!!

lord_of_freaks’s picture

Status: Reviewed & tested by the community » Patch (to be ported)
zhangtaihao’s picture

Thanks.

Are you sure "patch (to be ported)" is the status you intended? After all, this has yet to be committed.

See Issue queue handbook - Status.

zhangtaihao’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

For maintainer reference, #168 is the candidate for committing to Wysiwyg.

Kars-T’s picture

Wrong issue... sorry

mradcliffe’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

Should be this status.

sun’s picture

Status: Patch (to be ported) » Needs work
+++ b/editors/ckeditor.inc
@@ -185,6 +185,7 @@ function wysiwyg_ckeditor_settings($editor, $config, $theme) {
+    'global_basepath_var' => 'CKEDITOR_BASEPATH',

+++ b/editors/js/ckeditor-3.0.js
@@ -1,6 +1,9 @@
 Drupal.wysiwyg.editor.init.ckeditor = function(settings) {
+  window.CKEDITOR_BASEPATH = settings.global.editorBasePath + '/';
+  CKEDITOR.basePath = window.CKEDITOR_BASEPATH;

Is there any reason why we cannot pass the basepath into CKEditor's constructor options?

+++ b/tests/wysiwyg_test.module
@@ -5,3 +5,46 @@
+function wysiwyg_test_menu() {
+  $items['wysiwyg-test/ajax'] = array(

This is awesome to see! :) testing++

+++ b/wysiwyg.init.js
@@ -6,6 +6,18 @@ Drupal.wysiwyg.editor = Drupal.wysiwyg.editor || { 'init': {}, 'attach': {}, 'de
+          window[format_value.global_basepath_var] = Drupal.settings.wysiwyg.configs[editor_index].global.editorBasePath + '/';

I moved this initialization code into wysiwyg.js.

However, this seems to duplicate the code from init.ckeditor()? Also seems to be editor-specific (especially concerning the trailing slash)...

Any chance to clean this up? :)

That said: 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.

Thanks all!

Let's make sure to clean up those last bits, so we can publish a new release ASAP.

TwoD’s picture

I'm on that, sun. Testing a couple of alternate solutions, will post patches later tonight.

zhangtaihao’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

As Shawn_Smiley pointed out in #25, the base path must be set before URL resources are added (e.g. plugins, styles) so that CKEditor will load them under the correct path.

However, I agree that it can be cleaned up. Here's a follow-up that dispenses with 'global_basepath_var'. This also gets rid of the "editor-specific" initialization code.

Now, obviously, I have no idea if there's an impact on those who decide to use window.CKEDITOR_BASEPATH. Nonetheless, given that CKEditor integration is the only one that uses 'global_basepath_var', the pattern established by the initialization code in the TinyMCE integration, and that the patch has only been committed in the last few hours, directly setting CKEDITOR.basePath seems like a good compromise.

Incidentally, this follow-up also allows #507696-75: Allow individual width/height per field to work without special conflict resolution.

EDIT: Damn. TwoD, sorry about the comment number.

TwoD’s picture

As Shawn_Smiley and zhangtaihao have pointed out, we need to set the base paths quite early for them to have any effect, even before the library files are loaded, which rules out using the Drupal.settings object as it gets built way too late.

I had an idea about creating a separate file for the global variables, much like Drupal Core aggregates JavaScripts, as it is much easier to control in which order script files are added. I tried just adding inline code first, but as noted in the patch comments, those won't work in AJAX calls. (See ajax_render() and drupal_build_js_cache().)

It turns out this works pretty well!
This is just a rough first implementation or proof-of-concept I put together in a few hours, so please try to find out if there are any security concerns or other major issues with this approach.

Btw, the properties set on the tinyMCE object seem useless, they aren't referenced anywhere in the TinyMCE source code, so I removed them. This now uses the same loading technique and global variables used by their own compressor scripts.

zhangtaihao’s picture

Shouldn't it be possible to use drupal_json_encode() to some effect? I get the impression what we're trying to achieve here is really similar to what JSONP prescribes. My gut feeling is that wysiwyg.init.js or something similar could provide a facility to materialize globals and the auto-generated JS files would invoke that function on the JSON object containing global values collected from editor implementations. This would largely eliminate security concerns regarding compiled JS.

This does bring me to another question: is there a reason the globals are added before wysiwyg.init.js? It would seem the initialization script is merely for bootstrapping, so perhaps if it could be added before the globals, my proposition above would work.

Also, in ckeditor.inc, is there a reason that

      'CKEDITOR_BASEPATH' => '"' . base_path() . $editor['library path'] . '/"',

couldn't be:

      'CKEDITOR_BASEPATH' => base_path() . $editor['library path'] . '/',

?

TwoD’s picture

The TinyMCE implementation does use drupal_json_encode(), but only because that looked prettier than typing out the structure of tinyMCEPreInit in several small strings. The CKEditor implementation only needed a simple variable so I felt no need to pass it through anything. I wrote it this way mainly to indicate only variables should be set using this method since there's likely not many other useful things we can do before the libraries are loaded. If the dynamic code was a JSON structure, it couldn't hold any editor-specific logic anyway. It also kept the output and execution paths simple with practically no overhead or loading delay of other scripts. If you find a use case, I won't argue [much] against using a callback though. ;)

We currently have to live with the quirk of not knowing if all editor libraries that will be needed on a page have been loaded yet. So, the globals have to be output as each library is loaded, preventing the use of a single JSON structure holding all globals until the code is somehow refactored to build a registry of needed libraries, before loading them all at once. As it is now, a callback would be called once for each editor with a small JSON object, not that I think that matters much other than in terms of a slight overhead.

The globals are added before wysiwyg.init.js just because I did not need to move it to make this work. Moving wysiwyg.init.js up won't hurt, so it could indeed hold the callback to manipulate a JSON structure.

'CKEDITOR_BASEPATH' => base_path() . $editor['library path'] . '/', would become CKEDITOR_BASEPATH = http://example.com/sites/all/libraries/ckeditor/ in JavaScript, the extra quotes are needed to output it as a string. We don't need the quotes for Booleans. Regular Expressions, or objects structures, so I didn't want to force them in from wysiwyg.module. JSON can't directly handle Regular Expresssions or function references, but this way it's easy to use those as well. Since the $editor object doesn't hold any user input and the profiles with the settings aren't available in the callback, I don't see any obvious security issues there.

zhangtaihao’s picture

I think the issue of CKEDITOR_BASEPATH becoming interpreted as a regular expression is itself a use case for using a JSON structure with a pre-init JS callback.

Also, I'm not suggesting that profile details could be leaked out. I'm merely saying that making 'global settings callback' responsible for producing valid JS code feels rather unreliable. I'd expect whatever invokes 'global settings callback' to deal with JS, while the 'global settings callback' itself only needs to return a valid PHP data structure.

Will be back with a patch.

zhangtaihao’s picture

TwoD, I'm sure you already understand what I'm talking about, but here's the patch to illustrate it anyway.

I don't suppose jQuery is available at array('group' => JS_LIBRARY, 'weight' => -1)? In Drupal.wysiwyg.applyGlobals I've resorted to assigning global variables to window.

sun’s picture

FileSize
9.72 KB

Is there any particular reason for why we cannot keep it KISS?

TwoD’s picture

Status: Needs review » Needs work

@zhangtaihao, yes I understood, but thanks for the clarification.
Btw, you forgot to remove the drupal_json_encode() call in tinymce.inc, serializing the object twice in your patch.
To set a global RegExp value when always passing values through drupal_json_decode(), we'd need a way to indicate to the pre-init script that an object, or a sub-object, in the values needs some pre-processing.
To get someGlobal = /^foo/i;
We'd need the implementation to do:

function wysiwyg_myeditor_global_settings($editor) {
  return array(
    'someGlobal' => array(
      'is_wysiwyg_regexp' => TRUE,
      'pattern' => '^foo',
      'flags' => 'i',
    ),
    'otherGlobal' => TRUE,
  );
}

In JSON:

{
  'someGlobal': {
    'is_wysiwyg_regexp': true,
    'pattern': '^foo',
    'flags': 'i'
  },
  'otherGlobal': true
}

The init function:

Drupal.wysiwyg.applyGlobals = function (globals) {
  for (var name in globals) {
    var val = globals[name];
    if (typeof val == 'object' && val.is_wysiwyg_regexp) {
      val = new RegExp(val.pattern, val.flags);
    }
    // Recurse through sub-objects in case we need an array of RegExps?

    // Set global variable.
    window[name] = val;
  }
}

That does add a bit of complexity, but on the other hand I think we could need something like that preprocessing for our main settings object, to be able to change settings that require their use.

@sun, I don't mind keeping it simple, but I'm afraid your patch is a bit too simple.

+++ b/wysiwyg.moduleundefined
@@ -321,9 +337,19 @@ function wysiwyg_load_editor($profile) {
+        if (!empty($init)) {
+          drupal_add_js($init, array('type' => 'inline') + $default_library_options);

This won't work when the editor is loaded via AJAX. ajax_render() ignores any inline scripts, which is why I had to save it to a file first. (forgot to link to that function too in my comment above, added that) I tried it with inline scripts too, but could only use it as a fallback in case saving a file didn't work, to at least keep editors working when not lazy loaded.

Sun's init function with plain JS output could work really well if first saved to a file. It does put all the responsibility for outputting valid JS on the editor implementation, but those mostly about dealing with the editor specific JS needs anyway, right? If we need an init function for an editor, that editor implementation could output it to the file as well. My main concern is still that init functions are adding complexity to a process which may not be so obvious to anyone who may, or may not, have implemented editor support by hand before.

zhangtaihao’s picture

Status: Needs review » Needs work

Umm... just out of curiosity, how well (or how badly) would creating a JS template work for holding the pre-initialization setup? Something similar is done in Provision (in Aegir) but for Apache config files. The idea, obviously, is to make it somewhat obvious that the editor implementation may provide some pre-initialization JS, but may also use PHP to inject site variables. This gets around the "programming JS with PHP" problem and (partially) eliminates the ambiguity behind an "init callback".

sun’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
10.26 KB

What else can I fix? ;)

TwoD’s picture

@sun, Nice try, but locale_js_alter() expects the key in the scripts array to be an actual URL it can pass to _locale_parse_js_file(). I'm getting warnings about not being able to open the pre-init scripts.

Warning: file_get_contents(window.CKEDITOR_BASEPATH = '/drupal/sites/all/libraries/ckeditor/';): failed to open stream: No such file or directory in _locale_parse_js_file() (line 1482 of /home/henrik/Projects/Drupal/drupal/includes/locale.inc).

@zhangtaihao, I don't know how that would work. What would we put in the template?

zhangtaihao’s picture

Perhaps something like:

ckeditor.init.tpl.php:

window.CKEDITOR_BASEPATH = '<?php echo base_path() . $editor['library path'] . '/'; ?>';

Obviously, the invoker needs to provide $editor as a variable for the template.

I know Twig is on the way, so I'm not sure how best to achieve this without PHP.

TwoD’s picture

So, outputting this (whether to a file or inline), would end up looking something like this?

$pre_init_script = theme('wysiwyg_pre_init_editor', array('editor' => $editor));

Where the wysiwyg_pre_init_editor would probably be a theme function figuring out which template to use based on the editor variable? I haven't looked into how Provision does that part so this is just a guess.

zhangtaihao’s picture

Provision just does eval() straight up (since it doesn't presume Drupal bootstrap).

If the solution is indeed going to use the theme system, then it might be worth defining hook suggestions for declared editors (with the respective implementation file paths as 'file' of the theme hooks). This way, we can get the theme system to call theme_HOOK() or use a template file.

For example:

$pre_init_script = theme(array('wysiwyg_pre_init_editor__' . $name, 'wysiwyg_pre_init_editor'), array('editor' => $editor));

Or is this too complicated?

TwoD’s picture

Status: Needs work » Needs review
FileSize
4.58 KB
10.7 KB
11.23 KB

Here's a merge of #189 and #197 to get something fairly simple working.

It keeps the file generation from my patch and uses the init callbacks from sun's patch. I didn't add anything from #193 since init functions and templates are still being discussed, and yes, taking a detour via the theme layer is starting to look like a lot of additional changes. Not mixing PHP and JS is a noble pursuit, but Drupal's theme layer does move the JS behind more PHP calls, IMHO.

populist’s picture

I tested the patch in #202 with TinyMCE integration of the Media Browser plugin and it will load the JS immediately after a Drupal cache clear, but won't load it on subsequent page loads.

sun’s picture

Category: feature » task
Priority: Normal » Critical
Issue tags: +Release blocker
phi16181’s picture

Everyone, I'm new to patching and I need to add this patch to make our site function properly with panels and wysiwyg. Can I apply the patch to the module locally and then install the module as usual to the server? I ask because we use a shared server and Im dont think I can apply the patch because it is, in fact, shared and access is limited.

Thanks in advance for your input.

TwoD’s picture

@phi16181, you can apply the patch locally on a Wysiwyg 7.x-2.x-dev snapshot and then upload/install the patched module to your server. This patch does not require any database changes and you do not need to recreate any existing editor profiles you may already have. Just be sure to clear the caches after installing.

@populist, I'm going to see if I can reproduce that issue tonight.

zhangtaihao’s picture

Not to downplay the significance of #202, but doesn't the extra layer of abstraction on top of Drupal.wysiwyg.editor.init.* strike anyone as a bit odd?

I'm not up to date on the background behind 'init callback' or 'global settings callback'. I understand that the purpose of delegating the task to PHP is to provide a generic interface for releasing any necessary data to JS. However, given that there is already an init layer (in Drupal.wysiwyg.editor.init.*) and a settings aspect, wouldn't having a whole extra pre-init stage seem a bit extreme?

I am invoking sentiments like @todo Isn't the settings callback sufficient? and @todo Move into (global) editor settings. I thought 'plugin settings callback' had the right idea.

sun’s picture

@zhangtaihao is asking the right questions. Actually, I've asked the same myself. Here's what I concluded:

  1. These global JS [window] variables need to be set up before the editor library is loaded.
  2. There's most probably no actual need to load Wysiwyg's editor support/integration JS file after the main editor library, so it could probably loaded before.
  3. This could inherently mean that any "pre-init" code could be located directly within Wysiwyg's editor support JS file. Which sounds appealing.
  4. However, the library settings (or rather, global variables) we're trying to output depend on Drupal (or more specifically, Drupal.settings), so they cannot be hard-coded into the editor support JS file.
  5. JS settings in Drupal can only write into Drupal.settings, not into the global window, so that ship has sailed. We could rewrite our very own editor support JS file to prepend a dynamic pre-init JS "header", instead of delivering it as-is, but that sounds even more awkward?

What do you think?

zhangtaihao’s picture

These global JS [window] variables need to be set up before the editor library is loaded.

Not exactly. To be pedantic, the precise moment a global variable is required is right before a specific editor library is loaded (or even later in editors like CKEditor, just before loading plugins and styles). What exactly this means for wysiwyg_get_profile() I'm not sure yet.

There's most probably no actual need to load Wysiwyg's editor support/integration JS file after the main editor library, so it could probably loaded before.

Whether integration needs to be loaded after the main library depends on whether the main library is responsible for setting up the global variables and loading the libraries. Since the libraries are loaded on page load (like the main library), the order in which they are loaded makes absolutely no difference to setting up global variables, because in the status quo the global variables need to be loaded before them both.

However, it may be possible to insert all JS with a positive weight so that we can get away with setting up globals in a more flexible fashion.

This could inherently mean that any "pre-init" code could be located directly within Wysiwyg's editor support JS file. Which sounds appealing.

This could work, though the code needs to be located outside any callbacks (e.g. Drupal.wysiwyg.editor.init.*).

However, the library settings (or rather, global variables) we're trying to output depend on Drupal (or more specifically, Drupal.settings), so they cannot be hard-coded into the editor support JS file.

I think TwoD had part of the answer with 'global settings callback'. However, IMO the backend should provide only PHP literals (and arrays, of course); that's actually how 'plugin settings callback' works. In other words, stuff like regular expressions should be omitted entirely (and that's for the integration JS to take care of). For an example, just look at the CKEditor integration callback wysiwyg_ckeditor_plugin_settings().

JS settings in Drupal can only write into Drupal.settings, not into the global window, so that ship has sailed. We could rewrite our very own editor support JS file to prepend a dynamic pre-init JS "header", instead of delivering it as-is, but that sounds even more awkward?

This was also my musing in #201, albeit missing the theme system malarkey. I think adding bits just seems like splitting up the integration scripts into different parts. Before you know it, there will be another split down the line to serve another scenario. I'm thinking pre-initialization is not really worth the trouble, given the vast complexity of Wysiwyg JS is already in the main library and supported integration libraries. "Pre-initialization" should then just be setting some globals.

The exception to this argument is the possible need to support a relatively primitive library that absolutely requires pre-initialization before loading library script. If we do go down the path of global settings callback, then the question will be whether it's worth supporting this kind of library.

TwoD’s picture

@zhangtaihao, I don't see an extra "layer" of init code, more like an additional stage. The naming of these stages is somewhat confusing though, something I hope we can clear up once we know it's working.
What we currently have in the editor integration scripts with the Drupal.wysiwyg.editor.init functions is meant to initialize the Wysiwyg integration, our code. What we need is a way to initialize the editor library, their code.

sun:

1. These global JS [window] variables need to be set up before the editor library is loaded.

zhangtaihao:

Not exactly. To be pedantic, the precise moment a global variable is required is right before a specific editor library is loaded (or even later in editors like CKEditor, just before loading plugins and styles).

CKEditor reads the global variables discussed here as part of declaring its own CKEDITOR global, inside self-executing anonymous function declarations, so there's no way to intercept that without hacking the library.
We can bend the rules a bit and set CKEditor.basePath directly, but that feels like cheating and is most likely not officially supported. I feel it's safest to do it as close as possible to how you'd normally initialize the editor.

sun:

2. There's most probably no actual need to load Wysiwyg's editor support/integration JS file after the main editor library, so it could probably loaded before.

sun:

3. This could inherently mean that any "pre-init" code could be located directly within Wysiwyg's editor support JS file. Which sounds appealing.

zhangtaihao:

This could work, though the code needs to be located outside any callbacks (e.g. Drupal.wysiwyg.editor.init.*).

That would not work unless we move much of what's in the current Drupal.wysiwyg.editor.init.* functions into the attach() methods, but then we won't have access to the settings of more than one profile at a time. (We don't want to access the Drupal.settings global directly, it may no longer be in sync with the document state.)
Some of the things done here also need to be performed before creating any instances, like setting up the onAddEditor and onRemoveEditor handlers on the global TinyMCE object as well as calling tinyMCE.init() on all the profiles-to-be. This is in addition to the problems sun mentioned in points 4 and 5.

To recap:

As I see it, we will always need at least two "init stages". One to set up the environment for the editor library, and one to set up the environment for our integration code.

The libraries often need a global variable or two, as we've already noticed when trying to lazy-load them, that forces us to include something in a script before the library. TinyMCE and CKEditor are good examples of editors firing off their own init code and reading global variables (CKEDITOR_BASEPATH, CKEDITOR_GETURL, tinyMCEPreInit) as the first thing in their scripts, before any function calls are made to their APIs. Thus we need to place our injected globals somewhere before all the libraries.
There's no need to specifically place each of our init snippets before the library that needs them, as long as no library uses the exact same globals. All I've seen so far prefix their globals with the editor name, or are so ignorant about other scripts existing on the document that they are nearly impossible to integrate in a practical way anyway.
At this point not even Drupal's API is initialized, so we're in a relatively primitive environment and should only do what is absolutely necessary.
That was our first init stage, the "Library initialization", if you will. This is what we're currently missing.

The second stage would be when the libraries are done setting up their own globals and have made their APIs available.
As as been noted, not all libraries play nice, or expect their editor instances to be replaced with instances of other editors at any given time. Therefore we may need to bind custom code to an editor's event handlers, or even override some of its methods, to achieve clean detaching and re-attaching of editors. This can often only be done once the library is fully loaded and its API is available, and before any instances have actually been created. If we try to do it after instances have been created, each instance may have its own "unreachable" copies of event handlers, or they've already created more damage (dangling event handlers or references, style changes, etc) than we can reliably repair. Some editors, like TinyMCE and CKEditor, also require us to load any plugin we may need later into a global plugin manager. We currently do all that in one batch in the Drupal.wysiwyg.editor.init functions too because it's easy to know if we've already loaded them or not by means of simple loops. That could perhaps be moved out to the attach() methods, if keeping track of loaded plugins across instances does not become too troublesome.
And that was of course the second init stage, the "Integration initialization", if you will.

A third step could be the attach() methods, the "Editor [instance] initialization".

If you want to see it in terms of layers:

  1. Integration A (Prep the 3rd party library)
  2. Library
  3. Integration B (Prep our integration library and "fix" the 3rd party library)
  4. Create instance
  5. Interact with instance
  6. Destroy instance

I think we pretty much agree that far, no? The details of the method in which Integration A is being output is of lesser concern as long as it results in window[A_GLOBAL] = "Some value";

Thinking a few steps ahead...
We currently have at least one editor which could perhaps benefit from allowing logic in the "Library initialization" part, markItUp.
Once Wysiwyg has also been refactored to not actually load any libraries until all formats have been processed, so we know exactly what's in the editor profiles we need to build, it would be nice to pass all the profiles to the 'init' callback' in #202. That would allow us to pre-process and partially parse markup sets downloaded by the user, merge them with the settings generated by Wysiwyg's GUI (the buttons/plugin part would be disabled since markup sets would override those) and have working function references or inlined callbacks in that code. If we allow just JSON output, that would not be possible, or at least very very much harder. Well... That's just something I'd like to try one day, as all of it could be contained inside the markItUp integration and not have to pollute the core. Perhaps the markItUp integration could be split out into its own module if it gets really big.

That leads me to the API part of this. We know a lot about the editors we currently support within Wysiwyg itself, but we know very little about other editors that other modules may support by depending on Wysiwyg. If we allow fully functional JavaScript to be placed before the loading of the editor library then perhaps that enables the use of more editors we've not considered before, or deeper integration with editors partially supported by other modules?

Just a though, feel free to shoot it down. ;)

TwoD’s picture

@populist, the issue you are seeing is caused by #841794: Cache wysiwyg_load_includes(), please follow that issue for progress updates.

zhangtaihao’s picture

Okay, fair enough. I suppose this workflow (i.e. #210) should be documented in detail in the module once the implementation is complete so that there's less chance of other modules abusing the Integration A (Prep the 3rd party library) step.

It's a shame the attach behavior couldn't reliably load the external libraries on its own...

sun’s picture

FileSize
7.16 KB
11.65 KB

I reverted a couple of changes that appeared unnecessary to me, given the inline-JS-file-writing direction.

I also removed the 'baseHref' setting from CKEditor (again), because it is not used anywhere -- was there any particular reason to keep it?

I was not able to locate any public docs on tinyMCEPreInit anywhere on the net. Are we (ab)using some undocumented and potentially unreliable functionality? Or is someone able to fill in some equally elaborate docs for TinyMCE's init callback? (I am sure that this extensive reasoning will be unknown and needed in the future, so I'd like to stress on that a bit.)

Aside from that, it seems we're really close here. Would love to get this off the table ASAP :)

zhangtaihao’s picture

Status: Needs review » Reviewed & tested by the community

Works beautifully.

There are generally two occurrences of tinyMCEPreInit in the TinyMCE source as of 3.4.x and 3.5.x:

classes/adapter/jquery/jquery.tinymce.js:

// Setup tinyMCEPreInit object this will later be used by the TinyMCE
// core script to locate other resources like CSS files, dialogs etc
// You can also predefined a tinyMCEPreInit object and then it will use that instead
win.tinyMCEPreInit = win.tinyMCEPreInit || {
  base : base,
  suffix : suffix,
  query : query
};

classes/tinymce.js:

// TinyMCE .NET webcontrol might be setting the values for TinyMCE
if (win.tinyMCEPreInit) {
  t.suffix = tinyMCEPreInit.suffix;
  t.baseURL = tinyMCEPreInit.base;
  t.query = tinyMCEPreInit.query;
  return;
}

Apparently, tinyMCEPreInit is used in some sort of ASP.NET control (though there is not much activity). There are a few other controls if you search for them, but none of them seem to use tinyMCEPreInit.

Nevertheless, it seems we are using tinyMCEPreInit in much the same way as the ASP.NET control. If we do go on using it, though, we might need to interact with TinyMCE maintainers to ensure they don't remove it if the .NET control goes out of fashion.

P.S. Now that I think about it, using 'init callback' and inlining JS in PHP seems like to reasonably good deterrent for overly complicated pre-initialization code.

P.P.S. I thought I'd look through TinyMCE repositories, and indeed there is a compressor that uses tinyMCEPreInit to serialize critical environment variables into the compressed script.

TwoD’s picture

Status: Reviewed & tested by the community » Needs review

I first found out about tinyMCEPreInit in tiny_mce_dev.js and noted that this global structure was used by all versions of TinyMCE.

I then checked the compressors and found out how they use it, like in the official tiny_mce_gzip.php. It is also checked and set by the Dev variant of TinyMCE 3 to load the source version of the editor and its plugins.

Digging back, I found this was added in TinyMCE 3.0b2, while TinyMCE 2 needed tinyMCE.baseURL, tinyMCE.srcMode> and tinyMCE.gzipMode set directly on the instance.

The documentation I could find other than the code itself was the replies in their forum telling devs to use this method (confirmed by spocke): Dynamic loading of tinyMCE - a bug in tinyMCE?, True dynamic loading of TinyMCE, Dynamically load tiny_mce.js after window.load.

Sorry about the baseHref thing, was a miss on my part.

I've noticed a few minor issues with the latest patch and I'll post an update when I get back home.
Will also fix the baseURL/srcMode for TinyMCE2, if we're not going to drop support of it completely. It doesn't fully support being dynamically loaded and tries to load plugin files that don't exist (uses 'undefined' as the path), but it still works ok.

EDIT: crossposted with zhangtaihao... I think all the compressors use tinyMCEPreInit.

sun’s picture

Just quick: Let's complete the docs for tinymce's init callback, and let's completely ignore tinymce2 - I agree that we should straight-out remove support for 2, I seriously doubt anyone is using it anymore today.

EDIT: To clarify, let's ignore tinymce2, but remove it later; separate issue.

TwoD’s picture

Ok. I've only removed the things related to the global "format" from the TinyMCE 2 integration, as it no longer exists and I don't like the dangling references.

I basically copied the comment from the CKEditor init callback, good enough?

I removed the note about onPreInit as we can't use it for this. I tried setting the baseURL/baseURI properties using the method mentioned in that link. It won't work when calling tinyMCE.init(...) from within Drupal.wysiwyg.editor.init.tinymce because that call never actually creates any instances and thus doesn't fire the preInit event. When I tried to use the same method from within the attach() method, it was too late to affect where plugin files were loaded from and all paths broke in AJAXed forms.

We're fine removing the tinyMCE.init() call completely, btw. All it actually achieves is loading the all the plugin files before we actually need an editor instance. When the init call is removed, most files aren't loaded until the editor is actually needed, reducing the page load time when TinyMCE isn't active by default.
I've tested this change in FF, Chrome and IE8 with both native (internal & external) and drupal plugins, but feel free to drop it if the comment above Drupal.wysiwyg.editor.init.tinymce is still valid and it happens to break in IE6, if anyone cares. ;)

I modified tinyMCEPreInit.suffix as well to better match the variant/adapter combinations that are actually possible to load, since there's no '_dev', '_jquery' or '_jquery_src' suffixed files available other than each variant's or adapter's main file. TinyMCE always overrides the specified suffix internally though, but that might change in the future.

sun’s picture

Status: Needs review » Fixed

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.

Special thanks to @zhangtaihao & @TwoD! Awesome collaboration! (btw, @zhangtaihao, are you working towards becoming co-maintainer, by coincidence? :))

Hey you! Yes, YOU 121 followers! :)

This has been quite a drastic change. It would be awesome if you could download + test the new development snapshot (in <=12 hours from now) on your sites to double-check that we didn't break anything, and report back on this issue if something seemingly related to this is broken — for any other regressions but also positive confirmations on #1340052: New Wysiwyg 7.x-2.2 Release? - that would be great, thanks!

We'll wait with the new release for 2-3 more days.

zhangtaihao’s picture

I'd love to co-maintain Wysiwyg. When might I find you on IRC to chat about it?

Status: Fixed » Closed (fixed)

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

TwoD’s picture

Sorry for poking an old issue, but RE @sun in #213: I think I found why we should not have changed the 'baseHref' setting, its default value may not always work: #1861174: 7.2.2 and 7.2.2dev-Version break CKEditor 3.6.2

cgutteridge’s picture

Version: 7.x-2.x-dev » 7.x-2.2
Status: Closed (fixed) » Needs work

Hi. I've just upgraded to wysiwyg-2.2 on drupal 7.22 with up to date panels (7.x-3.3) and panels-in-place editor (7.x-3.3). I get the issue that editing panels in place fails to bring up the wysiwyg editor.

I can resolve it by downgrading to wysiwyg-2.1 and applying the patch from comment 91 above: https://drupal.org/node/356480#comment-5494486

When it fails on v2.2 I get a whole bunch of javascript 404 errors which may offer some clue:

[09:55:02.460] POST http://ecs1.totl.net/panels/ajax/ipe/edit-pane/panelizer%3Anode%3A289%3Apage_manager/6569 [HTTP/1.1 200 OK 647ms]
[09:55:02.461] GET http://ecs1.totl.net/sites/all/modules/ctools/images/icon-close-window.png [HTTP/1.1 200 OK 152ms]
[09:55:02.463] GET http://ecs1.totl.net/sites/all/modules/panels/images/bg-shade-light.png [HTTP/1.1 200 OK 158ms]
[09:55:02.464] GET http://ecs1.totl.net/sites/all/modules/panels/images/sprite.png [HTTP/1.1 200 OK 201ms]
[09:55:02.465] GET http://ecs1.totl.net/sites/all/modules/ctools/images/throbber.gif [HTTP/1.1 200 OK 177ms]
[09:55:02.466] GET http://ecs1.totl.net/misc/throbber.gif [HTTP/1.1 200 OK 180ms]
[09:55:03.054] Unknown property '-moz-border-radius-topleft'.  Declaration dropped. @ http://ecs1.totl.net/sites/ecs1.totl.net/files/css/css_T2xtdxNnkH6D9_X6y3SHP5Xyg08cL5x5hF1OCkOqXxw.css:1
[09:55:03.054] Unknown property '-moz-border-radius-bottomleft'.  Declaration dropped. @ http://ecs1.totl.net/sites/ecs1.totl.net/files/css/css_T2xtdxNnkH6D9_X6y3SHP5Xyg08cL5x5hF1OCkOqXxw.css:1
[09:55:03.054] Unknown property '-moz-border-radius-topright'.  Declaration dropped. @ http://ecs1.totl.net/sites/ecs1.totl.net/files/css/css_T2xtdxNnkH6D9_X6y3SHP5Xyg08cL5x5hF1OCkOqXxw.css:1
[09:55:03.054] Unknown property '-moz-border-radius-bottomright'.  Declaration dropped. @ http://ecs1.totl.net/sites/ecs1.totl.net/files/css/css_T2xtdxNnkH6D9_X6y3SHP5Xyg08cL5x5hF1OCkOqXxw.css:1
[09:55:03.054] Unknown property '-moz-border-radius'.  Declaration dropped. @ http://ecs1.totl.net/sites/ecs1.totl.net/files/css/css_T2xtdxNnkH6D9_X6y3SHP5Xyg08cL5x5hF1OCkOqXxw.css:1
[09:55:03.054] Error in parsing value for 'word-break'.  Declaration dropped. @ http://ecs1.totl.net/sites/ecs1.totl.net/files/css/css_T2xtdxNnkH6D9_X6y3SHP5Xyg08cL5x5hF1OCkOqXxw.css:1
[09:55:03.054] Unknown property 'zoom'.  Declaration dropped. @ http://ecs1.totl.net/sites/ecs1.totl.net/files/css/css_T2xtdxNnkH6D9_X6y3SHP5Xyg08cL5x5hF1OCkOqXxw.css:3
[09:55:03.054] Unknown property 'zoom'.  Declaration dropped. @ http://ecs1.totl.net/sites/ecs1.totl.net/files/css/css_T2xtdxNnkH6D9_X6y3SHP5Xyg08cL5x5hF1OCkOqXxw.css:4
[09:55:03.157] GET http://ecs1.totl.net/sites/all/libraries/tinymce/jscripts/tiny_mce/tiny_mce.js?mqdsj5 [HTTP/1.1 304 Not Modified 40ms]
[09:55:03.489] GET http://ecs1.totl.net/sites/ecs1.totl.net/files/js/js_loJJMZ-yLpCFnc5Htc8xgIMfdfNbw5E30CdocHWSiaM.js [HTTP/1.1 304 Not Modified 44ms]
[09:55:03.600] GET http://ecs1.totl.net/misc/help.png [HTTP/1.1 200 OK 178ms]
[09:55:03.602] GET http://ecs1.totl.net/misc/menu-collapsed.png [HTTP/1.1 200 OK 168ms]
[09:55:03.603] GET http://ecs1.totl.net/misc/grippie.png [HTTP/1.1 200 OK 181ms]
[09:55:03.605] GET http://ecs1.totl.net/sites/all/modules/ctools/images/arrow-active.png [HTTP/1.1 200 OK 188ms]
[09:55:03.607] GET http://ecs1.totl.net/node//langs/en.js [HTTP/1.1 404 Not Found 2379ms]
[09:55:03.608] GET http://ecs1.totl.net/node//themes/advanced/editor_template.js [HTTP/1.1 404 Not Found 2843ms]
[09:55:03.610] GET http://ecs1.totl.net/node//plugins/advimage/editor_plugin.js [HTTP/1.1 404 Not Found 2716ms]
[09:55:03.611] GET http://ecs1.totl.net/node//plugins/advlink/editor_plugin.js [HTTP/1.1 404 Not Found 2849ms]
[09:55:03.612] GET http://ecs1.totl.net/node//plugins/autosave/editor_plugin.js [HTTP/1.1 404 Not Found 2837ms]
[09:55:03.613] GET http://ecs1.totl.net/node//plugins/contextmenu/editor_plugin.js [HTTP/1.1 404 Not Found 2711ms]
[09:55:03.613] GET http://ecs1.totl.net/node//plugins/fullscreen/editor_plugin.js [HTTP/1.1 404 Not Found 3916ms]
[09:55:03.614] GET http://ecs1.totl.net/node//plugins/paste/editor_plugin.js [HTTP/1.1 404 Not Found 3916ms]
[09:55:03.615] GET http://ecs1.totl.net/node//plugins/preview/editor_plugin.js [HTTP/1.1 404 Not Found 3917ms]
[09:55:03.616] GET http://ecs1.totl.net/node//plugins/searchreplace/editor_plugin.js [HTTP/1.1 404 Not Found 3918ms]
[09:55:03.617] GET http://ecs1.totl.net/node//plugins/style/editor_plugin.js [HTTP/1.1 404 Not Found 4529ms]
[09:55:03.618] GET http://ecs1.totl.net/node//plugins/table/editor_plugin.js [HTTP/1.1 404 Not Found 5043ms]
[09:55:03.619] GET http://ecs1.totl.net/node//plugins/xhtmlxtras/editor_plugin.js [HTTP/1.1 404 Not Found 5041ms]
[09:55:03.620] GET http://ecs1.totl.net/node//plugins/advlist/editor_plugin.js [HTTP/1.1 404 Not Found 5045ms]
[09:55:03.620] GET http://ecs1.totl.net/node//plugins/lists/editor_plugin.js [HTTP/1.1 404 Not Found 5041ms]
[09:55:05.908] Failed to load: http://ecs1.totl.net/node//langs/en.js
[09:55:06.237] Failed to load: http://ecs1.totl.net/node//plugins/contextmenu/editor_plugin.js
[09:55:06.247] Failed to load: http://ecs1.totl.net/node//plugins/advimage/editor_plugin.js
[09:55:06.365] Failed to load: http://ecs1.totl.net/node//plugins/autosave/editor_plugin.js
[09:55:06.388] Failed to load: http://ecs1.totl.net/node//themes/advanced/editor_template.js
[09:55:06.394] Failed to load: http://ecs1.totl.net/node//plugins/advlink/editor_plugin.js
[09:55:07.445] Failed to load: http://ecs1.totl.net/node//plugins/paste/editor_plugin.js
[09:55:07.451] Failed to load: http://ecs1.totl.net/node//plugins/fullscreen/editor_plugin.js
[09:55:07.454] Failed to load: http://ecs1.totl.net/node//plugins/preview/editor_plugin.js
[09:55:07.459] Failed to load: http://ecs1.totl.net/node//plugins/searchreplace/editor_plugin.js
[09:55:08.058] Failed to load: http://ecs1.totl.net/node//plugins/style/editor_plugin.js
[09:55:08.610] Failed to load: http://ecs1.totl.net/node//plugins/lists/editor_plugin.js
[09:55:08.615] Failed to load: http://ecs1.totl.net/node//plugins/xhtmlxtras/editor_plugin.js
[09:55:08.619] Failed to load: http://ecs1.totl.net/node//plugins/table/editor_plugin.js
[09:55:08.622] Failed to load: http://ecs1.totl.net/node//plugins/advlist/editor_plugin.js
TwoD’s picture

Status: Needs work » Closed (fixed)

It looks like the basepaths haven't been set correctly. Those are set by generated scripts located in sites/default/files/js/wysiwyg and named after the editor and the hash of their contents.

It looks like you have JavaScript Aggregation enabled. Please try to update to 7.x-2.x-dev. It contains a fix for a problem happening when Drupal tries to include the generated files in the aggregated script file on some servers.

TwoD’s picture

Backported to 6.x-2.x (to become 6.x-2.5) to keep the latest versions more in sync. A few changes made because of the API differences between D6 and D7, but the functionality remains exactly the same.

Commit: 3b7d969

Note: This does not necessarily mean Lazy loading will work out of the box in D6, or with the same requirements as in D7. It just means I added support for, and implementations of, the 'init callback' property for TinyMCE and CKEditor so they are more likely to work in AJAX situations. D6 is still a lot different from D7 and this does nothing extra to compensate for version differences in Core or contrib modules, but it's a small helping piece of the puzzle in case other D6 modules are able to lazy-load edit forms.

TwoD’s picture

Issue summary: View changes

Tested for myself on a base install with Chrome, drupal 7, wysiwyg patched, wysiwyg_test, and panels. Cannot reproduce chrome issue reported by user.