Parameterized Event Handler Invocation

[l-cornelius-dol]

I have found two problems a problem with parameterized event handling, demonstrated in this fiddle.

First, the global and vm onevent handlers are invoked multiple times, once for each parameterized handler found.

Second, parameterized event handlers are invoked in top-down order, from the outermost handler to the innermost, which is the opposite of normal onxxx event handlers.

Also, I also note that the vm and global handlers are invoked regardless of whether an element-specific handler has returned false -- this may or may not be desirable/defensible, but it absolutely should be clearly documented (it might be, already, but I don't recall seeing it).

function AppView(vm) {
    return function(vm) {
        return ve("div", {
            onkeydown: bindVm(keyDown1, vm),
            }, [ 
            ve("div.key-target", {
            	tabindex: 0,
                onkeydown: bindVm(keyDown2, vm),
                }, [ 
                ve("div", "Handler 1: "+message1),
                ve("div", "Handler 2: "+message2),
                ]),
            ]);
        };
    }

function bindVm(hdl,vm) {
    return [ hdl,vm ];
    //return (evt) => { if(hdl(vm,evt)===false) { evt.stopPropagation(); evt.preventDefault(); } }
    }

As demonstrated, keyDown1 is invoked before keyDown2. But if the bindVm return is changed, invocation order will be the expected 2, then 1.

This is hugely significant in terms of logically cooperating handlers; it's very common to want to handle an event only if a more specific handler has not already done so (as the fiddle demonstrates). This totally blindsided me, and renders parameterized event handlers of no practical use.

If it cannot be changed for reasons of compatibility, I believe a global, and per-vm option is needed to reverse this. But I, personally, would consider this a bug.

The problem seems to be in ancestEvDefs, using unshift instead of push (which is also less efficient):

    function ancestEvDefs(type, node) {
        var ontype = "on" + type, evDef, attrs, evDefs = [];

        while (node) {
            if (attrs = node.attrs) {
                if ((evDef = attrs[ontype]) && isArr(evDef))
                    { evDefs.unshift(evDef); }
            }
            node = node.parent;
        }

        return evDefs;
    }

[leeoniya]

sorry, didnt see this till now! please tag me directly in issues so they show up in my sidebar. github's ui for notifications is shit.

i'll try to take a look at this this weekend and let's see what makes sense to do.

you're right that the synthetic event system used for parameterized handlers in domvm is incomplete. i dont think i ever had plans to add the necessary complexity to mimic the real thing.

(from memory, without looking back at the code yet..it's been years):

parameterized handlers are bound using single, capturing-phase listeners at the document level, so they will always fire before any handlers bound directly via on* functions, which are bubbling-phase e.g. [1]. (this may have been the reason for using unshift). this way, if you have 10k rows with an array handler on each, it does not need to actually bind 10k real listeners, but just one at the top, and pass through the params stored in the vnode array at invocation time.

if you need to have complex handling of the same events at different parts of the tree with cancellation, etc, i think the simple answer is to just use directly-bound events instead of using parameterized/capturing/simulated ones.

PPS: Are you interested in my going over the code and improving odd constructs like this? Noting, of course, this specific construct would not exist because the vm and global handlers should not be invoked here like this.

i think if we can get away with some relatively simple changes that are both more logical and no less performant, then i'll be happy to review a PR. there are some event tests [2] and adding some extra test cases that are resolved would be helpful.

[1] https://jsfiddle.net/8f9x6qu0/
[2] https://github.com/domvm/domvm/blob/master/test/src/events.js

[l-cornelius-dol]

parameterized handlers are bound using single, capturing-phase listeners, so they will always fire before any handlers bound directly via on* functions

I can see how you might have reasoned that being a capture-phase listener, they should fire in capture-phase order, and I am arguing that this is a mistake. Container-then-contained order is rarely desirable and in 25 years of writing GUIs and GUI toolkits, I've never actually needed to execute handlers in that order. Indeed, I believe JS is the first GUI system that I've encountered that even has the option, and there I would consider capture-phase to be an IE implementation bug (I think that's where it first arose).

As it stands now parameterized handlers will always fire either before or after all onxxx handlers, you couldn't do otherwise. But the handlers themselves should fire in the same order that normal onxxx handlers would have, contained before container.

After thinking about it, currently one clearly can't reliably mix normal handlers and parameterized handlers in the same GUI. There is, therefore, a case to be made that DOMVM should treat all onxxx handlers as parameterized even if there is a slight performance cost to doing so; it's probably balanced out be avoiding many extra lambdas at lower levels. Once this fire-order is fixed, I certainly plan to always use [handler] regardless of arguments, to avoid a fire-order trap with mixing the two kinds and I would vastly prefer no to need to remember to express all handlers as an array. Certainly for me, this choice has always seemed an arbitrary source of confusion (and I think it explains a number of "odd bugs" I have had). Indeed, it was making this switch over in some existing that alerted me to this out of order problem to begin with.

Further, it seems to me that they should use a View listener not a Document listener. When it comes to mixing components from different sources, especially from non-DOMVM components, I would need a nested component's handlers to be invoked before any of its container's handlers. Say I use a pure-JS calendar component, it's crucial that all of it's key handlers get first priority and keys only bubble up to my DOMVM handlers if they were not handled by the calendar. Conversely, say my DOMVM component is used by a React program; then I need be sure that DOMVM's handlers have all been invoked before any of the React handlers (and had the opportunity to preventDefault and stopPropagation. The inner to outer order becomes critical.

Otherwise one simply can't intermingle DOMVM with any other system, not even pure-JS components (because event handlers will be invoked in illogical, and ultimately conflicting, orders).

Finally, View listeners then must be bubble-phase listeners, otherwise each view is also out of order with respect to nested views (see my argument below that they should be bubble-phase listeners).

While technically a breaking change, synthetic event handlers are arguably completely broken as it sits, and these changes merely fix them. I've stewed on this for a couple of weeks now and I find myself forced to conclude that since there are times I need arguments bound to event handlers, and because we need interop with at least pure JS and possibly with some existing React code, the only alternative would be to fork DOMVM since I can't think of any way to fix this without involving the internals of the code.

if you have 10k rows with an array handler on each, it does not need to actually bind 10k real listeners, but just one at the top, and pass through the params stored in the vnode array at invocation time.

This advantage must be preserved.

> the global and vm onevent handlers are invoked multiple times, once for each parameterized handler found.

We must not overlook this bug in the detail of invoking event handlers.

---

So in summation I think that event handling must:

1. Invoke handlers in innermost-to-outermost order.
2. Treat all onxxx handlers as paramterized so all handlers have the correct order with respect to each other in the hierarchy. (*)
3. Use bubbling listeners attached to either their view or, at highest, the mount-point view, not the document. (**)
4. Fire the view-level onevent handlers after the handlers they contain, but before any higher level views. (***)
5. Fire global onevent handler at the Document level, after all other handlers. (***)

(*) The programmer can use addEventListener (and the entailed hooks) to escape this structure in the exceedingly rare event they absolutely need to create a capture-phase event handler. (I suppose I could live with this distinction, if there is a compatibility or significant performance reason to leave it as is.)

(**) So DOMVM components can properly interop with other systems, including independently mounted DOMVM views (for which we actually have a concrete use-case for customer supplied components).

(***) I retract my comment about onevent invocations; I can see that the intent is to invoke after every (parameterized) event. But they should fired be after every onxxx event as well, further strengthening the case to remove the distinction between onxxx: handler and onxxx: [handler]. Again, I can live with the distinction between normal and parameterized events, if needs be.

[l-cornelius-dol]

On the use of stopPropagation...

        if (out1 === false || out2 === false || out3 === false) {
            e.preventDefault();
==>         e.stopPropagation();
            return false;
        }

In my experience this is rarely correct and safe to use without causing unintended interop problems. In general, an event handler can never know that a higher level handler does not even need to see an event. This kind of handling in other contexts has resulted in things like shift states being locked on because some event handler prevented a higher level handler from seeing the key-up which turned the state off.

I strongly advise that returning false only do preventDefault allowing higher level handlers to know if the event has already been handled. In the exceedingly rare event that propagation should actually be halted, let the programmer do that explicitly.

However invocation must test whether stop propagation has been set and respect it, of course, and all event handler invocation must stop, including view and global invocation (which the above code does not do).

https://css-tricks.com/dangers-stopping-event-propagation/

[l-cornelius-dol]

I'm going to take a swing at a PR.

[leeoniya]

i'm on a phone, so have not read everything, but want to make sure your pr isnt wasted effort.

i dont think removing native handlers is a good idea. we need a way to stay close to the dom (principle of least astonishment), and making native handlers do magic behind the scenes should be avoided. i'm open to changing how parameterized handlers are delegated, and just using bubbling instead of capturing listeners.

[l-cornelius-dol]

i dont think removing native handlers is a good idea.

I had a feeling you'd say that, and I don't necessarily disagree. Which is why I subsequently annotated my main "solution" comment in that vein.

So scratch (2), above, and having already scratched (4) & (5):

1. Invoke handlers in innermost-to-outermost order.
2. Use bubbling listeners[1] attached to either their view or, at highest, the mount-point view[2], not the document.

[1] Bubbling so parameterized handlers fire event after normal handlers, so embedding non-DOMVM components in DOMVM is possible (else DOMVM handlers preempt the components handlers).
[2] View listeners so DOMVM components can be embedded inside other non-DOMVM components/systems (else DOMVM handlers preempt, if capture phase, or are preempted by, if bubble phase, all other handlers).

[l-cornelius-dol]

(I deleted my comment about the odd construct, because it was a result of distribution generation handling of the feature flags, not something in the source code. Sorry.)

[l-cornelius-dol]

Digging into what's needed to have a per-View event handler registry, instead of a per-Document one...

It seems to me that it might be a net gain for all nodes to have an immediate reference to their nearest VM (thus obviating the need for getVm() to walk the hierarchy).

Obviously this can't be node.vm which is used extensively to differentiate a view node from an element node. But perhaps all nodes could have an nvm value, being the nearest vm?

(Similarly, if the decision is made to use the mounted VM for event listeners, then it would be mvm, for "mounted VM", though with no other utility? Both nvm and mvm might have benefits that I am unaware of, which make adding both worth the cost.)

[l-cornelius-dol]

Not able to make progress on a PR. Where/when/how are the build.js and tests run?

[leeoniya]

But perhaps all nodes could have an nvm value, being the nearest vm?

not sure. it potentially means more complex inter-node bookeeping to do between redraws. let's try without it first and profile.

Where/when/how are the build.js and tests run?

to build you need to install node: https://nodejs.org/en/

then in the repo dir open a terminal / console and...

1. run npm i to download and install the build dependencies
2. run npm run build to build the /dist bundles when you make changes

you can open https://github.com/domvm/domvm/blob/master/test/index.html in a browser to run all the tests.

[l-cornelius-dol]

Node added a package-lock.json file to the repo, after following the directions above. What do you want me to do with that?

Also, node reports some package vulnerabilities after auditing. Do you want me to also update the build packages to resolve these?

[leeoniya]

plz leave as-is. i'll do the tooling updates afterwards.

[l-cornelius-dol]

@leeoniya : The master branch has a number of tests failing for streams, before making any changes, and present for 3.4.12. How do you want me to proceed?

(My changes pass all tests, but I have yet to verify that they actually change event handlers as desired.)

[leeoniya]

if any unrelated tests fail prior to your changes, ignore them for now.

verify that your changes do what you want and still pass the existing event handler tests, unless those need to be changed in some logical manner. open a PR when everything you need works as you expect and you've added extra tests to show what failed before but works after your changes.

[l-cornelius-dol]

@leeoniya : Pull request completed.

Updated fiddle showing the invocation order and interaction with native listeners: https://jsfiddle.net/LawrenceDol/usqrgf84/

BTW, I was not clear on whether the listeners will now need to be removed (I confess I almost never use add/remove listener). It seems to me that the remove of any listeners would have to be done when the element is discarded -- and the onxxx flags could be used to remove any that had been added.

[l-cornelius-dol]

@leeoniya

My new test fiddle unearths another fault. With nested views, the outer view's onevent is invoked with the inner view's vm argument. That seems wrong.

Furthermore, the this and currentTarget values are logically incorrect; they should reflect the element which originally had the onxxx: [ ... ], but will in fact be the Document (currently) or the View element (with my pull request implemented).

[l-cornelius-dol]

@leeoniya

I really have to question whether any of this complication is necessary. If a parameterized handler simply binds a listener on the actual element then the entire complexity just disappears, with handle in patchEvent.js being simply:

function handle(e) {
    var nod = e.target._node;

    if (nod != null) {
        var dfn = nod.attrs[e.type];
        exec(dfn[0],dfn.slice(1),e,nod);
    }
}

[leeoniya]

being auto-delegated is a key aspect of these, so it obviates 10k bindings. if you're okay with direct bindings then just:

1. use on* functions
2. save the param array in the vnode's data, e.g. {onclick: clickHandler, _data: {onclick: ["a","b"]}}
3. then in the handler just access the data via e.target._node.data

[l-cornelius-dol]

@leeoniya

if you're okay with direct bindings then just...

OK, I'll retract that comment, although the ergonomics are much worse. I think all other considerations stand.

[leeoniya]

although the ergonomics are much worse

agreed.

TBH, browsers have changed a lot since domvm was written. perhaps the overhead of a reasonable amount of bindings (let's say 2k) is not that great, and we can remove the delegation.

can you bench the difference in memory and JS cost between the two variants and maybe there's a path forward here.

one concern is still being able to trigger global and vm-level onevent functions. i'm not sure how this would be done without also creating 2k wrappers around the supplied handlers. i think we'd need to keep a global Map of seen handlers and wrap them once, followed by re-use during binding.

[leeoniya]

here are timings for 10 runs of the raw dom code, one set of runs with addEventListener commented:

<!doctype html>
<html>
  <head>
    <title>Test</title>
  </head>
  <body>
    <script>
      function onclick() {}

      console.time("time");

      for (let i = 0; i < 2000; i++) {
        let div = document.createElement("div");
        div.textContent = i;
        div.addEventListener("click", onclick);
        document.body.appendChild(div);
      }

      console.timeEnd("time");
    </script>
  </body>
</html>

without listeners (same as single delegated):

with 2k listeners:

interestingly, the memory metrics look identical. if you take into account both Scripting and System timing differences, it's about 2x as expensive to have directly-bound listeners.

upping this to 10k elements, the results are the same (about 2x), so this looks like the lower bound of the additional cost.

also, creating 10k wrappers does not seem to have much impact:

<!doctype html>
<html>
  <head>
    <title>Test</title>
  </head>
  <body>
    <script>
      function wrapCick(fn) {
        return () => {
          console.log('!');
          fn();
        };
      }

      function onclick() {}

      console.time("time");

      for (let i = 0; i < 10000; i++) {
        let div = document.createElement("div");
        div.textContent = i;
        div.addEventListener("click", wrapCick(onclick));
        document.body.appendChild(div);
      }

      console.timeEnd("time");
    </script>
  </body>
</html>

[l-cornelius-dol]

@leeoniya

i'm not sure how this would be done without also creating 2k wrappers around the supplied handlers. i think we'd need to keep a global Map of seen handlers and wrap them once, followed by re-use during binding.

There's no need for wrappers, per se. Each listener directly invokes handle(e), which in turn invokes the app handler, vm.onevent, and domvm.onevent.

All handlers are so invoked, so there should be little to no overhead above pure native handlers. Indeed, I would simply set the onxxx attribute of the Element, and not use addEventListener. And I while I would inline away the exec function, my earlier "reformed" handler(e) function remains substantially:

function handle(e) {
    var nod = e.target._node;

    if (nod != null) {
        var dfn = nod.attrs[e.type];
        exec(dfn[0],dfn.slice(1),e,nod);
    }
}

I do not expect that binding the same function to multiple elements should have a material impact on memory use; it's just a pointer, when all is said an done, and null takes the same 8 bytes as 0x0102030405060708. The key is that a new function is not created for each binding.

Of course reusing a DOM element will need to wipe an onxxx bindings, by iterating the attributes of the old node.

PS: Without an x-axis scale, I'm not seeing a "2x more expensive"?
PPS: I'm not convinced the extra complexity is worth the performance gain.
PPPS: One would also need to compare the savings in DOMVM from not having the extra code and work to have this super-special event handling. It might well be swings-and-roundabouts here with creating the DOM vs. reusing it.
PPPPS: You'd need to compare onxxx, not addEventListener. Again, an onxxx handler is nothing more than a pointer in the DOM, which already exists.

[leeoniya]

PS: Without an x-axis scale, I'm not seeing a "2x more expensive"?

there is no x axis. look at the timings in the summary.

if we can keep the overhead to within what's above, i'm okay with removing the delegation/simulation. go ahead and implement the non-delegated strategy for parameterized listeners in another PR and i'll take a look 👍

[leeoniya]

PS, you dont need to tag me in every comment. just the first issue or PR comment.

[l-cornelius-dol]

I'm done with work for the day, so I'll work on the pull request now. Do you have a preference for basing it on master vs. the 3.4.12 commit?

[leeoniya]

Do you have a preference for basing it on master vs. the 3.4.12 commit?

always master

[l-cornelius-dol]

See pull request #235 and alternative pull request #236.