nicEdit doesn't remove its submit event handler

Bumping this to 7.x-2.x as that's where we apply patches first and backport later. (This patch should apply to both 7.x and 6.x but I've only tested with 7.x yet.)

With this patch I try to clean up after the editor so submit handlers for each editor instance aren't fired at all once they've been removed. This could potentially remove other leftover event handlers too.
The reason I do this temporary call interception the attach() method rather than once in an init() method, and leaving it there, is that this way we should be able to detach just the handlers related to a certain instance. I think that could make things easier if there are multiple forms on a page and some are submitted via AJAX (which can also trigger submit handlers).

Like I said above, I've only had time to test this in IE (8) yet.

Nice catch, without the false FF throws an "not enough arguments"-exception.

FF 3, Opera 11, Safari 5 all seem fine with this change too so I'd say it's ready to go.
I did make one syntax change tho, since it made things a bit more readable I split the inlined condition to an if-else.

+++ editors/js/nicedit.js
@@ -4,12 +4,37 @@
+  // Intercept submit handlers so we can unbind them later.
+  var oldAddEvent = bkLib.addEvent,
...
+  // Restore the addEvent helper function and register our unbinder.
+  bkLib.addEvent = oldAddEvent;

In some weeks/months from now, everyone will have to read study this code and the library's code to figure out _why_ this was done, why it was done as a hack (no pun intended, seems to be the only way right now).

Code comments should always state the non-obvious, everything that cannot be read out of the code, which we do know now, but tend to forget later. Comments should primarily be about *why* something is done, and should also answer the immediate question of "WTF why on earth this way?" :)

+++ editors/js/nicedit.js
@@ -4,12 +4,37 @@
+  bkLib.addEvent = function(obj, type, fn) {
+   if (type == 'submit') {

Wrong indentation here.

+++ editors/js/nicedit.js
@@ -4,12 +4,37 @@
+      'obj' : obj,
+      'fn' : fn

There shouldn't be a space after the property names.

+++ editors/js/nicedit.js
@@ -4,12 +4,37 @@
+    while (handlerInfo = submitHandlers.pop()) {
+      if (handlerInfo.obj.removeEventListener) {
+        handlerInfo.obj.removeEventListener('submit', handlerInfo.fn, false);
+      }
+      else {
+        handlerInfo.obj.detachEvent('onsubmit', handlerInfo.fn);
+      }
+    }

mmm... is there any reason why we're not using our addEvent override to simply skip the submit handler events in question?

In other words: why do we add the handlers to a stack if we're going to remove them anyway? On form submit, Wysiwyg should be triggering the detach methods anyway, no?

Powered by Dreditor.

Ok, I'll try to expand the comment, but I'm having a hard time trying to describe things with words that would make sense unless you do already know the editor pretty well...

The reason I kept the submit handlers in there until after the editor has been removed was that they seemed to be executed before Wysiwyg's handler, and I'm a bit paranoid when it comes to ripping out a function call the editor expects to be made just before the page gets unloaded. For example; TinyMCE and others are very careful to remove certain objects and references to not cause memory leaks that would persist across page loads. Removing those handlers before they've run will lead to problems unless we do all of the internal cleanup calls manually, something that could change between minor versions as browser specific bugs are fixed.
Hence, if this editor decides to do something similar, we'd have a "future proof" (as much as it can be at this point) solution where the editor cleans up most of its internal stuff on its own before we go in and remove the handler.
But, I just noticed the editor's handler actually got invoked after Wysiwyg's detach handler - and its only purpose is to save the content to the textarea, something we already do indirectly via the .remove() call - which does indeed make it pointless to keep that stack of handlers around... Considering its last official release was over two years ago, I'm not expecting it to change much anway (if it does change, it'll probably be a huge rewrite - which would require an implementation rewrite as well).

Enough with my ramblings now, I'm too tired to type more so I'll just attach the revised patch and await your response.

Much better :)

I definitely see your perspective, @TwoD :) If I would have spent only half the time that you've worked within these libraries, then I guess I could see the underlying reasonings a bit more clear.

However, we should try hard to make sure that all of our code is easy to understand and follow for newcomers and other developers, who might not have looked into any of these libraries at all yet. They should be able to see and grok what Wysiwyg is doing under the hood, and in edge-cases like this, get to know the reasons for why Wysiwyg has to override and effectively hack an editor's regular behavior to make it work for Drupal. Clean and solid code comments are especially important when it comes to larger, cross-editor API changes; i.e., when (hopefully more) other people need to touch and change existing code to adapt it for something new. That's essentially the only way to ensure that others are able to contribute in the first place and to prevent having to (personally) explain certain code parts, which might be potentially irrelevant for a different, actual task at hand.

Therefore, writing clean code with solid code comments is the only way to solve our topmost problem and reach our ultimate goal: Getting more contributors involved in Wysiwyg. :)

+++ editors/js/nicedit.js
@@ -4,6 +4,15 @@
+  // Intercept and ignore submit handlers or they will revert changes made
+  // since the instance was removed, and they can't be referenced once set.
+  // The same operations are performed when the instance is removed anyway.

The last minor tweak I'd recommend prior to commit is to replace "removed" with Wysiwyg lingo; i.e., "detached" where appropriate.

The "and they can't be removed once set" part reads a bit confusing/out of context, although I think I understand what is meant. Turning this part into an own sentence might already help; e.g., "NicEdit's submit handlers are anonymous and cannot be unbound."

Lastly, it wouldn't hurt to additionally state the most recent finding for future reference; i.e.: Testing showed that NicEdit's submit handlers are invoked after Wysiwyg's.

Powered by Dreditor.

oy, cross-post with #6. Apparently, the issue comment in #6 can almost be used 1:1 as inline code comment right above the line that restores the handler. It's actually a perfect example. ;) If we'd commit the patch in #6, people would have to look up this drupal.org issue to figure out why NicEdit's handler needs to be restored after attaching the editor instance.

Thanks tacituseu! That explains why I saw the handlers getting fired in my earlier tests.

Hope you don't mind me changing the wording a bit.

Thanks! :)

Committed to all relevant branches, -dev snapshots will be updated within 12 hours and this will be in the next official release.
Thank you for patching, testing and reviewing!