enhanced IE event handling

[mokhet]

For every events we want to manipulate, we always have to check if HTMLArea.is_ie or not to take the correct event object. (by the way, it may be better to do ev?ev:window.event instead of is_ie?window.event:ev, but that's not the point here)

I think we could simulate the DOM behavior for IE if we change a little bit the function _addEvent as following

HTMLArea._addEvent = function(el, evname, func)
{
  el['xinha_evt' + evname + func] = function() { func(window.event); }
  el.attachEvent("on" + evname, el['xinha_evt' + evname + func]);
  HTMLArea._eventFlushers.push([el, evname, el['xinha_evt' + evname + func]]);
};
HTMLArea._removeEvent = function(el, evname, func)
{
  el.detachEvent("on" + evname, el['xinha_evt' + evname + func]);
  el['xinha_evt' + evname + func] = null;
};

This way, i think we would never have to check if we have the event object or not. Here is a sample of what we have to do, checking if is_ie to get the correct object :

HTMLArea._addEvent(el, "mousedown", function (ev) {
  if (obj.enabled) with (HTMLArea) {
    _addClass(el, "buttonActive");
    _removeClass(el, "buttonPressed");
    _stopEvent(is_ie ? window.event : ev);
  }
});

but simply

HTMLArea._addEvent(el, "mousedown", function (ev) {
  if (obj.enabled) with (HTMLArea) {
    _addClass(el, "buttonActive");
    _removeClass(el, "buttonPressed");
    _stopEvent(ev); // <--- MODIFICATION HERE, NO TEST, WE KNOW WE HAVE OUR EVENT
  }
});

This way, the event object will always be correctly populated if we are IE or not and we will not have to ponder again about this boring crossbrowser stuff inside Xinha events.

I dont have tested it yet but as you may have noticed i'm actually focused on reducing the maximum of tests inside xinha i can, and i think this one can be a good one. What do you think ?

[mokhet]

I am using this way of passing event argument in one of my Xinha implementation and have *no* problem at all. I think this is a good thing to do before we going further to enhance the event manipulation in Xinha.

[gogo]

mokhet, feel free to commit your change if it doesn't cause any problems.

[mokhet]

Wow, a long time ago and i had forgot this ticket. Yes, i will submit my changes in a sec since they are working great.

However, i have almost finished another way to handle events, a clever one i would say but since it's not working as intended yet, i will propose this update much later :)

[mokhet]

The patch i wanted to introduce is not enough (see ticket:825 and ticket:30 for more explanation)

So i ended up integrating my event system (which is based on HTMLArea and yahoo events) and here is the patch. It's 957 lines long (full of comments) so i'll attach it in a moment.

HTMLArea._addEvent and HTMLArea.addDom0Event are still there (and removeEvent also obviously), because i dont wanted the patch to break the plugins, but everything in the core of this patch is now using the new HTMLArea.Events.add i've introduced.

Here is why it is better than our actual events system

• only 1 method to add a DOM2 or IE listener or add / prepend a DOM0 one
• control of the execution scope
• synchronize event object inside handler function
• synchronize stop ( HTMLArea.Events.stopPropagation(e) and HTMLArea.Events.preventDefault(e); )
• can provide an arbitrary object to the handler function

/**
 * Add a listener to an element
 * @param {string | HTMLElement} e (Element)   Element ID or the reference
 * @param {string | array}       T (Type)      Event type to add or indexed array of event types to add
 * @param {function}             F (Function)  Callback function
 * @param {object}               S (Scope)     Execution scope (optional) default to the element reference
 * @param {object}               B (Bonus)     Bonus object provided with the callback (optional) default to null
 * @param {boolean|string}       D (forceDom0) true if DOM0 model is forced, 'prepend' if DOM0 model is forced and must be prepended to the existings listeners of the same name (optional) default to false
 * @return {boolean} true if the binding is successfull
 * @public
 */
HTMLArea.Events.add = function(e, T, F, S, B, D)

so to add an event to a SPAN tag with the execution scope (the variable this inside the handler) set to the editor, it is

var handler = function(e)
{
  // e is the object event even for IE, no need for e = HTMLArea.is_ie ? window.event : e
  // this is the editor instance of Xinha
  // to stop, HTMLArea.Events.stop(e);
}
var span = document.createElement('spam');
HTMLArea.Events.add(span, 'click', handler, editor);

If we need to give some more information to our handler function, it just needs to add the object we want to provide

var handler = function(e, B)
{
  // B is the object myCUSTOM
  // e is the object event even for IE, no need for e = HTMLArea.is_ie ? window.event : e
  // this is the editor instance of Xinha
  // to stop, HTMLArea.Events.stop(e);
}
var span = document.createElement('spam');
var myCUSTOM = {Anything:'Can',be:'here',from:'object',to:'integer',or:'array',anything:'really'};
HTMLArea.Events.add(span, 'click', handler, editor, myCUSTOM);

And if we needed the same but in DOM0 model, the next parameter must be set to true

var handler = function(e, B)
{
  // B is the object myCUSTOM
  // e is the object event even for IE, no need for e = HTMLArea.is_ie ? window.event : e
  // this is the editor instance of Xinha
  // to stop, since it is a DOM0 handler, we must return FALSE to stop
}
var span = document.createElement('spam');
var myCUSTOM = {Anything:'Can',be:'here',from:'object',to:'integer',or:'array',anything:'really'};
HTMLArea.Events.add(span, 'click', handler, editor, myCUSTOM, true);

And if we needed to prepend the DOM0 handler, instead of true, we must use 'prepend'

HTMLArea.Events.add(span, 'click', handler, editor, myCUSTOM, 'prepend');

The execution scope can be set to any variable, to set it on the span element, it's easy :

HTMLArea.Events.add(span, 'click', handler, span, myCUSTOM, 'prepend');

The working example is at http://mokhet.com/xinha/examples/basic_example.php

Can someone review the patch and see if it's ok to submit or not at all. Especially gogo i'll would like to hear about the patch since this is a big move, but of course i'll like to hear from anyone. But i'm convinced it's the direction to go.

[mokhet]

oh, and i forgot, HTMLArea.Events.add() and HTMLArea.Events.remove() are able to use multiple listeners for one element. Instead of providing a string event type, it needs an array :

HTMLArea.Events.add(element, ['mousedown','click'], handler);

[gogo]

To be honest, I'm not a fan of ridding ourselves of inline functions (and thus closures). That's because I'm a bit of an idealist (or maybe purist) I guess, I tend to think that one shouldn't buckle from using the features of a language just because a certain implementation has problems. Especially when those features make it nicer to code.

I also like the addDom0 syntax, because I'm lazy, and it's less typing. It also makes it clearer IMHO that you are adding a dom-0 event when you use that, instead of adding yet-another-argument to your Events.add() method which has lots of arguments already. But that's not so much an issue, becuase those syntaxes can be retained using your framework behind the scenes.

I'd much prefer if you used proper variable names for arguments, infact, if I had a veto (I don't), I'd use it until those abominations were fixed ;-) It's much harder to quickly see what arguments a method takes if they are all single-letter names.

[mokhet]

I understand the preference about keeping closure, and i could probably rewrite the patch to keep them, but the problem i am encoutering is "anonymous methods" and events. Once set, we cant remove them anymore (until the end of the page or by arbitrary removing every events set on the element) since they are anonymous methods and so, we dont have any reference to remove them anymore. And that's a problem for #30.

For addDom0 syntax, yeah we can have an helper method :)

HTMLArea.addDom0 = function(element, type, funct, scope, arbitraryObject)
{
 return HTMLArea.Events.add(element, type, funct, scope, arbitraryObject, true);
};

HTMLArea.prependDom0 = function(element, type, funct, scope, arbitraryObject)
{
 return HTMLArea.Events.add(element, type, funct, scope, arbitraryObject, 'prepend');
};

I apologize for the variable names, i'll attach another patch with proper variable names as soon as possible. Sorry, since i had commented it a lot i though it would be enough to understand. Very sorry, i took my framework and integrated it into Xinha without thinking about proper variable names, it's this way because i'm minimizing all my javascript. sorry again.

[gogo]

I'm not sure how making the lambda (anon) functions named gives you a reference to them that you can free, surely the handle you need to free is where it's attached to the element, and you always must know that, because you attached it.

Can you perhaps provide an example, maybe I'm just being dense.

[mokhet]

same patch with proper variables name

[mokhet]

I attached the same patch with proper variable name.

No you do not always know the reference of the function and the handle is not enough to correctly remove the listener.

For instance, let's say i have my own submit DOM0 handler (with only 1 listener, which is an alert) on the form and 2 editors : editor1 and editor2

<form onsubmit="alert('my own submit');">
<textarea id="editor1"></textarea>
<textarea id="editor2"></textarea>
</form>

When the 2 editors are generated (editorX.generate()), one anonymous listener is added for each editor

// Set up event listeners for saving the iframe content to the textarea
if ( textarea.form )
{
  // onsubmit get the HTMLArea content and update original textarea.
  HTMLArea.prependDom0Event(
    this._textArea.form,
    'submit',
    function()
    {
      editor._textArea.value = editor.outwardHtml(editor.getHTML());
      return true;
    }
  );

Once the page is loaded and everything is generated, there is still 1 submit DOM0 handler on the form but there is now 3 listeners waiting to be called : * the anonymous onsubmit defined by editor2, * the anonymous onsubmit defined by editor1 * and the original one (the alert here)

And now it is impossible to degenerate one single editor and correctly free the memory (remove listeners and nullify everything) without reloading the whole page. We cant just nullify the whole onsubmit.

With this patch i propose, yes it is not anymore inline functions. But we have a reference to the functions and so, we are able to remove them easily

// Set up event listeners for saving the iframe content to the textarea
if ( textarea.form )
{
  // onsubmit get the HTMLArea content and update original textarea.
  HTMLArea.Events.add(textarea.form, 'submit', HTMLArea.onsubmit_form, this, textarea, 'prepend');

...

HTMLArea.onsubmit_form = function(evt, arbitraryObject)
{
  arbitraryObject.value = this.outwardHtml(this.getHTML());
  return true;
};

...

HTMLArea.prototype.dispose = function()
{
  HTMLArea.Events.remove(this.textarea.form, 'submit', HTMLArea.onsubmit_form);
}

And the same occurs for every others events on buttons, on statusbar element, on iframe, on document, etc. Yes we can just dont care about it and wait for the garbage to remove all thoses references when the browser will be closed. However, i think it's our best interest to do it properly by ourself instead of waiting for the page to unload. Especially since there is a solution.

[mokhet]

As said in ticket #825, a branch as been created where i'll commit my updates concerning this event update proposition and a disposer method.

http://svn.xinha.python-hosting.com/branches/mokhet

All my tests for this branch will be at http://mokhet.com/xinha/branch_events/

[gogo]

I must still be missing something...

This happens for each textarea right?

  // onsubmit get the HTMLArea content and update original textarea.
  HTMLArea.Events.add(textarea.form, 'submit', HTMLArea.onsubmit_form, this, textarea, 'prepend');

And this happens when you unload a single Xinha, right?

HTMLArea.prototype.dispose = function()
{
  HTMLArea.Events.remove(this.textarea.form, 'submit', HTMLArea.onsubmit_form);
}

So how does dispose know which of those HTMLArea.onsubmit_form's you want to dispose?

Every textarea has (most likely) the same form object, so as far as I can see that dispose removes the submit event refercing HTMLArea.onsubmit_form from the textarea's form, but because more than one textarea uses that form, you don't know which is being removed (or it removes them all).

[mokhet]

Humm, at first didn't see the problem but after trying to explain myself I think I see your point, and I hope I'm not screwed. You are pointing the fact that we may remove a wrong listener even if they have the same reference, right ?

with editor1

HTMLArea.Events.add(textarea.form, 'submit', HTMLArea.onsubmit_form, this, textarea, 'prepend');

the first listener is set on the form for onsubmit with the scope editor1

with editor2, the same is added

HTMLArea.Events.add(textarea.form, 'submit', HTMLArea.onsubmit_form, this, textarea, 'prepend');

a second listener (same reference) is set on the same form for the same onsubmit with the scope editor2

And yes you are right, grrrr ;-), when removing the listener for let's say editor1, there is a chance to remove the listener set from editor2. I see your point now. And in fact the wrong listener is removed because the scope defined in the add method was not taken into account for the remove. I'll update the HTMLArea.Events.remove method to take care of the scope, and i think it should then fix the problem, right ? Or do you see more issues with the system ?

[mokhet]

I have tested the situation and you are damn right. Without the scope, the remove method is randomly removing the wrong listener. http://mokhet.com/xinha/branch_events/examples/listeners2.html with changeset:542 shows it.

So i updated the remove method ( changeset:543 )to take care of the execution scope and now it seems to work as I expect, or at least as I think I expect for the moment. You make me have a big doubt about my ability to have the system work errorless.

Lesson to me : do not say your boss you may be able to accomplish something before actually having finished it especially when you think you can :-)

[mokhet]

is the changeset:548 (and especially the changeset:546) sounds better ? It is using back the inline function, so there's no risk to mistake the listener we remove.

I wonder how far i can go on the branch, if it worth it to keep working on it or if it's useless. I mean, i know everyone is on holidays :) actually, but am i going in the good direction or not at all ? The changeset:548 sounds good to me, besides some leaks i've still not managed to correctly remove but that's will go to #30.

gogo, i think the branch i'm working on can be a real enhancement to Xinha, but do you think it worth the work, still in the branch for now, to update the plugins to make them use thoses "enhancements" ? I'm asking gogo first but of course everybody comments and patch :-) are welcome.

[gogo]

Looks much better...

  HTMLArea.Events.add(el, 'change', el_onchange);  
  editor.notifyOn('dispose', function() { HTMLArea.Events.remove(el, 'change', el_onchange); });  

What are your thoughts on perhaps having an event adder in the prototype of the editor which can automatically do the disposal, so it's more like

   editor.addEvent(el, 'change', el_onchange);  

saves some typing, means for less chance of forgetting to dispose an event. I *think* that would also allow anonymous functions to be passed to editor.addEvent() ? Because inside of that method you have the reference to it you need to dispose of it. That said, I'm not adverse to the inline closure functions you've made like el_onchange, since you've already put in the effort I think they can stay.

Small thing, but can

  function el_onmout()  

be el_onmouseout() please (similarly for onmdown).

 	3088	            // eval ???? why not using the following line instead ?  
 	3089	            // if ( !ancestors[k][attrs[ka]] )  

Don't know, what you put there should be fine, so don't know why it was done that way.

      3665	// Why is this is still in the trunk ?  
 	3666	// there's no reference to it anywhere else even in plugins  

It won't be the only disused function, but I'm reluctant to remove them incase somebody is using the functions in a custom button or custom plugin. Perhaps it would be good to take out these unused functions from htmlarea.js and put them into a new deprecated.js file which people can load if they want to use these functions. You are welcome to do that if you feel so inclined, as long as all the xinha core and plugins work without deprecated.js being loaded, I see no problem.

1615 if ( !textarea.form.$XINHA_submit )

While you're making changes, could you change $XINHA_submit to something more in keeping with our variable naming (I know you didn't write it), xinha_submit or something.

I think you're making some really good progress here, so, yea, keep up the good work, the likely hood is that once you're happy with it as working-at-least-as-good-as-the-trunk that you can merge to the trunk, that's the only way we'll get many people to test it, if there's any huge problems can always revert the merge.

[mokhet]

My thoughts about HTMLArea.prototype.addEvent(), well it's just brilliant. Once i've read your message i had to do it, it looked so obvious once you pointed the direction :-)

And yes, with this change, handlers does not need to have a "real" reference anymore to be removed. I can bring back anonymous functions to be closer to trunk source if you want. Should I ?

with HTMLArea.prototype.addEvent, I have also added HTMLArea.prototype.addDom0Event and HTMLArea.prototype.prependDom0Event. I dont think a HTMLArea.prototype.removeEvent is needed - especially since we dont have a way to remove notifier (yet) - but if you think it's needed, perhaps for plugins maybe, I'll implement it.

The update is in changeset:551

this update also has :

• apply patch from ticket #415 (warn when run locally)
• rename some inline functions with better name
• slightly update the form.submit patch from ticket #823

Concerning the unused functions and deprecated.js, I have open ticket #829 and I dont think i'll bother with such things right now. Thoses "noisy" comments in source, are just me annotating code when i'm seeing something weird i'm not working on. Because it's so heavy that's it's easy to get lost (at least me :-) and I forget about all the details. I can stop annotating if you prefer, it's a just a practice i have. Your call.

Thanks for the incentives to keep working this way, i need them. I dont feel very confident will all this javascript :-)

[gogo]

I think removeEvent would be good, for completeness.

It's up to you if you want to revert back to the anonymous functions, I'm fairly easy either way, if the named inline functions are in roughly the same place in the file as the anonymous functions are it should be obvious what changed.

By all means keep on annotating it brings to light problems that should be corrected in thed future, put [mokhet] next to the annotation would be good so we know who mentioned it ;)

[guest]

I'd like to ask some questions regarding event caching and this new garbage collector but don't know where to do that. Should I write my questions here or open a thread in the forum? Please feel free to delete this message if it's in the wrong place.

Thanks!

Marc

[mokhet]

Hi, feel free to question and make any comment concerning this branch (events update) here in this message or in forum. I'll try to give every info i can have. Even if i'm late to commit my last updates.