Incongruence of Normal and Parameterized Handlers

[lawrence-dol]

@leeoniya

I am opening this as a new issue because I want others to be able to find it easily, and I don't want to confuse the issue with the recent enhancements to event handling (#235). So this should be food for thought (I would have opened a discussion item, if discussions were enabled). I should note up front that I don't really have a big dog in this fight, since I don't use this and with recent changes I can now live with simply always using parameterized handlers.

There remain four ways in which DOMVM parameterized event handlers (henceforth p-handlers) are inconsistent with JavaScript event handlers (henceforth e-handlers):

1. The this for e-handlers is the current target; for p-handlers it's the current vm.
2. The first argument for e-handlers is the event; for p-handlers the event argument is after whatever arguments were attached in the handler declaration.
3. The only argument for e-handlers is the event; for p-handlers DOMVM implicitly adds node, vm, and vm.data.
4. The onevent handlers are not called for e-handlers, and they are for p-handlers.

My issue with these differences is that none are technically necessary, and the differences create additional cognitive load and code fragility for me. Given that there's no longer a performance penalty for one or the other, I feel the differences are no longer technically justified. Note that I am not intending to speak to compatibility concerns -- that alone might render the entire discussion moot.

When I am deciding between an e-handler and a p-handler the only factor in view is whether or not I need to bind context arguments. Often, as the design shifts, and especially when I am first developing a component, a specific handler will change from normal to parameterized, or vice versa. Doing so makes the existing handler code broken in subtle ways, and it's easy to forget one of them, which becomes a runtime error, often a fatal one, that occurs only when that event handler is actually triggered. Painful to test and easy to miss.

The cognitive load increase is related, but applies to every handler I write -- is it an e-handler or p-handler; does it have arguments of evt,vm,nod,dta or just evt. Will the onevent handler be called or not (if I am using an auto-redraw system this could be critical). For programmers accustomed to this, I imagine having this be the current target in one context and the vm in another would be a constant source of frustration.

As reflected in my recent pull request these issues could now be easily addressed, and though doing so would break compatibility a case could be mounted that these are bugs.

The changes would be:

1. Bind this to currentTarget.
2. Always use the exec function.
3. Bind added arguments after the event, so instead of (...,evt,node,vm,data) use evt,...,node,vm,data).

The argument for #1 is purely consistency with JavaScript in general. The this value is always current-target, for every event handler I see in every JavaScript context.

The argument for #2 is that the extra arguments do no harm and consistency within DOMVM code is better than inconsistency. This is the one that is the least important; there seems to be some defense for only adding the DOMVM arguments to p-handlers, so I lean toward doing this, but not strongly.

The argument for #3 is that the event is the "fundamental" JavaScript argument, the explicitly declared arguments are context-specific, and node, vm, and data are implicit DOMVM "value-added" arguments. The event argument is always there, and it's always first, for every event handler I see in every JavaScript context -- the precedent for this is overwhelming. The explicit added arguments specified in the declaration seem to be naturally expected next, then finally the implicit DOMVM arguments.

As I said, food for thought; and in my opinion worth a major version bump and a breaking change to remove this constant source of friction.

[leeoniya]

generally i have no issues with all you points, though it would have to be a v4 bump, which also implies that it might be worth evaluating what other breaking changes / paper-cuts should be fixed to ride along.

The argument for #3 is that the event is the "fundamental" JavaScript argument, the explicitly declared arguments are context-specific, and node, vm, and data are implicit DOMVM "value-added" arguments. The event argument is always there, and it's always first, for every event handler I see in every JavaScript context -- the precedent for this is overwhelming. The explicit added arguments specified in the declaration seem to be naturally expected next, then finally the implicit DOMVM arguments.

when i first decided on the argument order for p-handlers, i thought very hard about this, and it wasn't an easy decision. the main reason for making things the way they are is because i wanted to be able to have generic functions that can be triggered by an event but do not have to be triggered by an event, and therefore do not need e in their signature. for example:

let count = 0;

function incrementBy(amount) {
  count += amount;
}

incrementBy(1);

now let's say we want this to be called from a button onclick with an amount of 10:

el("button", {onclick: [incrementBy, 10]})

if we change the invocation signature to always include the event at the front, you can no longer do this elegantly. and must create wrappers that remove e from the front or change the signature of the pure incrementBy to accept an event it doesnt need/use. so now you need:

function handleClick(e, amount) {
  incrementBy(amount);
}

el("button", {onclick: [handleClick, 10]})

this was the motivation for the current param order. in my experience, the event itself is often not needed, so having to strip it from the front of the array every time is a nuisance, more of a nuisance than simply accessing it from a second implicit argument, when necessary.

[leeoniya]

if you prefer it to work the other way, then it should be easy to add a global config setting that will modify this behavior in your app without having to do a v4 bump.

[lawrence-dol]

the main reason for making things the way they are is because i wanted to be able to have generic functions that can be triggered by an event but do not have to be triggered by an event, and therefore do not need e in their signature.

That's a perfectly valid argument, and makes a great deal of sense. Given this concrete use-case, I admit to having trouble articulating a compelling argument against the current argument (pun intended) order.

I am in the habit of making explicit functions for event handlers based on the general advice to not use functions as callbacks unless they are designed for that specific callback. I've been burned a few times by unexpected behavior from optional, typically omitted arguments, especially as I was learning JavaScript a few years back.

It does seem to me that having these direct uses might be a ticking time-bomb in an app, given that when used as an event handler the function is receiving:

function incrementBy(amount,evt,node,vm,data) {
  count += amount;
}

It seems only a matter of time before it's legitimately enhanced with an additional argument and every event invocation is broken, an (admittedly contrived) example being:

function incrementBy(amount,base) {
  if (typeof(amount)==="string") { amount = parseInt(amount,base||10); }
  count += amount;
}

if you prefer it to work the other way, then it should be easy to add a global config setting that will modify this behavior in your app without having to do a v4 bump.

Actually, this kind of thing can never be a global setting. Doing so immediately breaks code from other sources (even within the same team) that expect the other behavior.

Supposing both your argument and my counter argument hold, then my list is reduced to:

1. The this for e-handlers is the current target; for p-handlers it's the current vm.
2. The first argument for e-handlers is the event; for p-handlers the event argument is after whatever arguments were attached in the handler declaration.
3. The only argument for e-handlers is the event; for p-handlers DOMVM implicitly adds node, vm, and vm.data.
4. The onevent handlers are not called for e-handlers, and they are for p-handlers.

Striking (2) on your argument (use-case for prefixing arguments), and (3) on mine (not arbitrarily adding arguments to an event handler).

I still think there's a case for (1), especially, and (4). Though backward-compatibility argues against (1), and I have mixed feelings about (4).

[leeoniya]

let's leave this open. i agree with a lot of your points.

[lawrence-dol]

Proposal

I now think with near certainty that the next version should be rolled out with at least these two breaking changes:

1. Event is the first argument to all handlers and all handlers receive nod,vm as well.
2. The return value of all handlers should be ignored and defaultPrevented never set.

Point 1

To achieve use-case consistency, either (a) the invocation of a "normal" handler in handle() should test the return for false and explicitly prevent default; or (b) parameterized handlers should not do this.

Point 2

Event handlers are special enough that app functions should not be directly wired to handle events. For this reason, I am now firmly convinced that the event argument should always always be the first in the list, resulting in evtHandler(evt,additional-args...,node,vm) for all DOMVM event handlers.

What I am saying, now, in response to "i wanted to be able to have generic functions that can be triggered by an event but do not have to be triggered by an event", is that given the nature of JS event handlers which is a permanent feature of JS at this point, it's a really bad practice to use functions not explicitly designed to be an event handler as event handlers. It is very brittle.

Context

This last week I have been tracking down and fixing problems with event handlers in a complex app. The symptom of the problem was that some events were also being handled by the browser, causing unexpected scrolling issues. The process of doing this has underscored to me the special nature of event handlers, and the fact that "normal code" should not be directly attached as an event handler.

The crux of the issue was inconsistent handling of the return value. For onxxx handlers the return value of false (not falsey) causes the event to prevent default handling. This is not the case with event listeners (add/removeEventListener) where the listener is explicitly documented to have no return value. I was previously unaware of the distinction.

In theory, DOMVM should behave congruently with onxxx semantics (since that's what they appear to be using), which means it's current treatment of the return value for parameterized handlers is correct. But for non-parameterized handlers the return value false is ignored instead of cancelling the event.

But JS never specified (AFAICT) the return value behavior; it was just a de facto standard of browsers from early times. Worse this de facto standard return value is terribly counter intuitive. Even worse still, it's my understanding that this cancellation may not be true of all events, but only the majority of events established prior to standardization; certainly it's not true of events that can only be used via a listener.

@leeoniya : This has become a HUGE deal for me as I detest brittle coding practices.

One (of several) mistakes I had was a function that returned true or false to indicate whether the requested action was actually performed (false indicating that the action was not currently valid and was not done). Without realizing the consequence, I hooked it up as an event handler with the effect that not only was the event was cancelled when the action was not performed, but when it was the browser also did default handling.

A second mistake I made was designing a key mapper which set defaultPrevented if the return value was explicitly true, and coding a bunch of key handlers masquerading as event handlers, which returned inverted values if actually directly hooked up as an event handler, and which returned values in direct conflict with any other event handler. Since most of my event handlers were explicitly setting defaultPrevented and returning undefined, this only became apparent when I set about making all my event handlers prefixed with evt to designate them as such. Really, really UGLY.

This experience leads me to conclude, surely, one very important thing. It is never a good idea to directly hook up normal application code as an event handler. Event handlers should always be functions that receive an event, return undefined, and call Event.defaultPrevented if they actually perform the "definitive" action for the event in question. And this is regardless of using listeners or handlers because most programmers will encounter codebases that use one or the other in places. Of this I am now absolutely convinced, where previously I heard the advice but did not understand the significance.

So now, if I need to attach an app function which is not explicitly an event handler I'll always do one of these:

function evtMouseClick(evt) {
    listSelectionMade();
    evt.preventDefault();
    }

~ or ~

let evtHandler = (evt) => { listSelectionMade(); evt.preventDefault(); }; // note curlies not parens

One of the things I did to resolve things at the application level was to create two simple utility functions which I could apply in the onxxx array to indicate explicitly whether I was handling an event, or merely observing it.

exported(evtHandler,"evtHandler");
function evtHandler(fnc,...args) {
    let evt = args[args.length-4] || args[0]; // DOMVM || standard

    if(!genU.isFunc(fnc) || !(evt instanceof Event)) {
        if(DEVELOPER) { console.error("Bad args: `evtHandler`.",fnc,args); }
        return;
        }
    if(!evt.preventDefault) {
        fnc(...args);
        evt.preventDefault();
        }
    }

exported(evtObserver,"evtHandler");
function evtObserver(fnc,...args) {
    let evt = args[args.length-4] || args[0]; // DOMVM || standard

    if(!genU.isFunc(fnc) || !(evt instanceof Event)) {
        if(DEVELOPER) { console.error("Bad args: `evtObserver`.",fnc,args); }
        return;
        }
    fnc(...args);
    }

Used like this:

onclick: [evtHandler,itemSelected,itm]
onmouseenter: [evtObserver,highlightOn,itm]
onmouseexit : [evtObserver,highlightOff,itm]

let evtMouseEnter = evtHandler.bind(null,highlightOn,itm);
...
onmouseenter: evtMouseEnter

But note the awkward uncertainty about which event argument is actually the evt; either index 0 or length-4, depending on whether a normal or parameterized hookup was used.

These are simple to use, efficient, and as a happy side-effect provide a two isolated points in the app which all events pass through. Using evtHandler also means that all module functions are isolated from consideration of defaultPrevented in an intuitive way (the assumption being if it has a "handler", its handled unless something deeper in the tree has already handled it). This has resulted in clear, self-documenting, unsurprising code at very little cost in either effort or footprint -- for me that's a solid win.