event.stopPropagation() doesn't stop event propagating to view

[georeith]

Calling event.stopPropagation() does not stop an event propagating to the view.

item.on('mousedown', function(event) {
    event.stopPropagation();
    console.log('item');
});
view.on('mousedown', function(event) {
   console.log('view');
});

Also the event on the view has the target set as the view, although the original target was the item, is this done on purpose?

Edit: The issue is here:

function emitMouseEvent(obj, type, event, point, prevPoint, stopItem) {
    var target = obj,
        prevented = false,
        mouseEvent;

    function emit(obj, type) {
        if (obj.responds(type)) {
            if (!mouseEvent) {
                mouseEvent = new MouseEvent(type, event, point, target,
                        prevPoint ? point.subtract(prevPoint) : null);
            }
            mouseEvent.id = point;
            if (obj.emit(type, mouseEvent)) {
                called = true;
                if (mouseEvent.prevented)
                    prevented = true;
                if (mouseEvent.stopped)
                    return true;
            }
        } else {
            var fallback = fallbacks[type];
            if (fallback)
                return emit(obj, fallback);
        }
    }

    while (obj && obj !== stopItem) {
        if (emit(obj, type))
            break;
        obj = obj._parent;
    }
    return prevented;
}

function emitMouseEvents(view, item, type, event, point, prevPoint) {
    view._project.removeOn(type);
    called = false;
    return (dragItem && emitMouseEvent(dragItem, type, event, point,
                prevPoint)
        || item && item !== dragItem && !item.isDescendant(dragItem)
            && emitMouseEvent(item, fallbacks[type] || type, event, point,
                prevPoint, dragItem)
        || emitMouseEvent(view, type, event, point, prevPoint));
}

Propagation to view seems to be handled by emitMouseEvents if the first two OR statements are false. However propagation detection is in emitMouseEvent and uses a closured MouseEvent so the view receives a different one with the wrong target and without the stopped property.

[lehni]

I agree that this is a bug. Regarding the wrong target on view, could you clarify what you expect to receive?

[georeith]

@lehni I would expect to see the highest z-index Item or View at the event point, so it can still be view sometimes (if it isn't a propagated event), the same way it is supposed to behave in your code but doesn't because its outside of the closure.

[lehni]

At the moment, for reasons of performance I only hit-test for items if there are any item events installed... This would break that. It's intentional, not a problem of closures. The event on view never receives another target than the view, I believe...

[georeith]

@lehni Sorry I should of said the highest one with a listener. Its not an event on view though its an event on item that propagated to view... if an event propagates from one item to another it stores the original item it propagated from, why should view be different?

If you bound the event to something then you know what it was as you had a reference to it at bind time, the real useful information is where did that event come from.

[georeith]

The issue is that emitMouseEvent(view, type, event, point, prevPoint)); handled the propagation when it should of been handled by the original emitMouseEvent and still contain a reference to the original target.

It should be the same event that propagates and an events target never changes is the real point.

[georeith]

That should handle the propagation:

while (obj && obj !== stopItem) {
        if (emit(obj, type))
            break;
        obj = obj._parent || obj.view;
}

But I'm not entirely sure what's going on in the big boolean return statement in emitMouseEvents I think I'd do more harm than good touching that as I'm not sure what issues you are fighting against.

So it looks like the first OR ensures that all mouse events whilst the mouse is down fire against the moused down item, the second for converting draggable events on non draggable items (not sure why stopItem === dragItem) and the last plain old view. Is that correct?

[lehni]

I have already fixed your issue, I just haven't pushed it yet. There is nothing wrong with the propagation the way it's currently written, there was just a mix-up between preventing the default and stopping propagation. There are good reasons for the current separation of items and view (handled by that boolean statement), the lack of a need to hit-test for items when all you want is an event listener on the view being one of them, which is why I am passing the view as the target, not running hit-tests when you most probably never need them (or can run your own if you do).

There is no should or shouldn't, really. This is not the DOM, we can make up our own rules. People expect all kinds of things, though. There is never going to be one solution that makes everybody happy.

[lehni]

PS: Your proposal only works if you actually hit an item. If there is none, then the view would not be receiving the event either.

[lehni]

PPS: If you are curious about the code, I'd recommend looking a the sources, which are full of explaining comments, e.g.: https://github.com/paperjs/paper.js/blob/develop/src/view/View.js#L1197

[lehni]

Reopening, since there are open questions still:

• Should the event on view contain the hit-item as target as well?
• event.preventDefault() in mousedown does not prevent mousedrag events on the view, but does so on items.

[georeith]

@lehni Internally it makes sense to differentiate the view and items but as a user a common interface would be far more useful I think. To me the view is just a part of the scene hierarchy, why should it behave any different events wise?

What benefit does target provide if it is just the item the event is firing on? You could closure that into your event callback. It does solve the currently impossible (well not impossible but the solution is beyond awful) that is an event propagates to view and I want to know A if the event was propagated and B where it began.

Ah I was looking at the prebuilt/module source and it had comments stripped. Didn't realise it had all that.

My proposal is only for when you land on an item. You still need alternative code for a 0-match hit-test which I believe should work just as it does now.

[lehni]

I think you should give the code another go since my fix above. It does exactly what you expect it to. The only difference right now is target.

[lehni]

I don't want to run hit-tests that people might not even use, which is why there currently is the optimization that counts the amount of mouse events installed on items, and only runs hit-tests when there are items that expect events to be called on them. This is a good thing, and works really well (see View._itemEvents). Your proposal would render this useless, and would result in hit-tests being run all the time. I could add a getter to the event object that is passed to events on view for event.target that would run the hit-test once it's requested by the user. I considered that before but rejected it as it will add more code. All that said to explain that there is a good reason why things are implemented the way they currently are.

[georeith]

@lehni I have just installed it and it is working great. The target isn't something I am using now (I used to for the reason it wasn't working correctly).

I don't understand why my proposal needs you to run additional hit-tests? All I'm proposing is that if an event was propagated by something else then keep that as the target even when it hits view, you've already done the hit test otherwise you couldn't of fired the event to propagate anyway. Right now view always receives view as the target even if it wasn't the original receiver.

[lehni]

For the simple reason that if I am not installing any events on items and only on the view, no hit-tests are run at all at the moment, the actual item target doesn't have to be determined, and that's a good thing. If you want the event target, you'd have to run a hit-test to find out. This could be masked behind a event.target getter if required, but so far there was no need for it.

This is what I've been trying to explain in all my comments here: This is the reason why things are implemented the way they are, and that's by design, and not a bug.

[georeith]

@lehni but thats fine if there are no events on any items then the target would be the view because the event didn't propagate. I think you are misunderstanding what I'm suggesting. There is no need for additional hit-tests you just set the target as the first thing that emits and then ensure it never changes.

My issue is that there is an event on an item if you trigger an event on that when the event also triggers on view the target should be the original item and not the view.

[lehni]

No, I don't think so... But I think it's wrong that you get a different target on view events based on whether you have item events installed or not.

[georeith]

@lehni You just get whatever triggered the first emit. No need for any additional hit-tests. If you can call obj.emit then you already know what the object is. Your code already does this but because you create a new closure you lose this information when emitting the view event.

You aren't getting a different target because there's no events installed, you would also get the view as a target if you had events installed and you didn't emit them. Its got nothing to do with what is installed but what is emitting.

[lehni]

You are wrong. This has nothing to do with closures. You make wrong assumptions about the code I've written, and I'm getting tired of trying to explain it to you.

[georeith]

@lehni I'm not assuming.emit contains a closured target from its wrapper emitMouseEvent anything it emits will emit a mouseEvent with the target as target no?

Right now it doesn't propagate to the view... so when you call emitMouseEvent again for the view it no longer has reference to the same target as everything else. Why should the view be any different than the other events? It doesn't make any sense.

var target = obj,
        prevented = false,
        mouseEvent;

    function emit(obj, type) {
        if (obj.responds(type)) {
            if (!mouseEvent) {
                mouseEvent = new MouseEvent(type, event, point, target,
                        prevPoint ? point.subtract(prevPoint) : null);
            }
            mouseEvent.id = point;
            if (obj.emit(type, mouseEvent)) {
                called = true;
                if (mouseEvent.prevented)
                    prevented = true;
                if (mouseEvent.stopped)
                    return true;
            }
        } else {
            var fallback = fallbacks[type];
            if (fallback)
                return emit(obj, fallback);
        }
    }

var target is closured for all emit functions that spawn from this emitMouseEvent in the code above... but because you don't handle the view as just another layer of propagation (which it is) it does not receive the same target as everything else. What possible reason does the view need to have a different target to these?

I think you think I'm talking about the hit-item which I am not. I am saying that target should always be the first thing the event triggers on. As it already is for everything else except the view.

[lehni]

There really is no need to explain the code to me, I've written it, I know what it does. And all of it is intentional : )

What I am saying is is this:

• When you have no events installed on any of the items in the project, then none of the event counters is increased in View#_itemEvents. (Perhaps study the code that relates to that structure)
• In _handleMouseEvent(), View#_itemEvents is used to determine if a hit-test should be run (stored in hitItems). Only if any of the items is expecting an event, the hit-test is run. If none is expecting an event, it doesn't mean there is no item under the mouse. It just means no events expects the hit-test result.

From here the rest should make sense.

[georeith]

@lehni but I'm not talking about whats under the mouse:

• If there is a square under the mouse and it has no listener but the view does then the target is the view.
• If there is a square under the mouse and it has a listener then it is the target.

The target is the first item, you've already done the hitTest because you've already called obj.emit on it. All I'm saying is that you extend how it currently works for child -> parent to treat view like a parent and not its own separate event.

The whole issue is contained with emitMouseEvents.

To explain with code if you took:

function emitMouseEvent(obj, type, event, point, prevPoint, stopItem) {}

and made it:

function emitMouseEvent(obj, target, type, event, point, prevPoint, stopItem) {}

then you would have:

 return (dragItem && emitMouseEvent(dragItem, dragItem, type, event, point,
                prevPoint)
        || item && item !== dragItem && !item.isDescendant(dragItem)
            && emitMouseEvent(item, item, fallbacks[type] || type, event, point,
                prevPoint, dragItem)
        || emitMouseEvent(view, dragItem || item || view, type, event, point, prevPoint));

and it would work as expected. But I expect you want it to write it more succinctly than that.

[lehni]

And I'm saying that the target shouldn't be different based on whether an item has an event installed or not. We're looping.

[georeith]

@lehni But what purpose does always having the view as a target serve? You haven't told me this.

Nothing else treats events like this, it doesn't do anything but force me to write edge cases.

Why do Items get different targets. It's nothing to do with whether events are installed but whether the event propagated. I don't understand why the event has to be different when it reaches the view... that is the weirder behaviour.

What if I want to put catch-all listeners on the view that do things if certain types of elements triggered stuff (a circle reacted to a click event... now do this). Impossible if the target is always the view. If I bind an event listener to the view I already know that it was emitted by the view. I don't need target to do that, it's not the purpose of target.

[lehni]

An API that changes its behavior based on whether some / any item (not even the item under the mouse, necessarily) has an event installed or not, is erratic and to be avoided at all cost. But that's what your proposal would do. As soon as any item has an event, the hit-test is run and the target item is found, whether it has the event installed or not.

The discussion whether view as the target of view events makes sense or not is a separate one, and I said that there is a way to make target work differently (through the accessor), but I didn't see the need for it and decided it didn't merit the added code. If you want, you can just think of view events as not having a target currently, and be fine with that. Or you can argue it should have a target and it should be an item or the view if there is no item under the mouse, and that would require new code, as your proposal wouldn't solve that either in all situations.

[georeith]

@lehni it already changes its behaviour... the current behaviour is the erratic one. Currently the definition of target is different just because of what the event is emitted on.

Perhaps the code is wrong but the principle isn't.

HTML events are erratic? They don't do that, target does not always equal body when you have a listener on the body.

[lehni]

Here we loop again.

[georeith]

@lehni Can you tell me the definition of target? My version is well defined the other is erratic. Please describe it and hopefully you will see.

My definition: The first object to receive an emitted event.

[lehni]

Dude, whatever.

[elliotbonneville]

I think what @georeith is saying is that an event should retain the original target even after bubbling up through the view. target is already determined and stored -- it's just dropped when the event hits the view. It seems to make more sense to keep that information. Do you have a reason not to?

@lehni Lol, just reread a bit and came across this, which sort of answers my question.

An API that changes its behavior based on wether some / any item (not even the item under the mouse, necessarily) has an event installed or not, is erratic and to be avoided at all cost.

Let me ask a different question instead. What do you mean by erratic? It seems that it would be more consistent to always return the target of an event, regardless of what the event propagates through.

[lehni]

I thought I explained this enough times now, but I have a feeling my explanations are being ignored, which is rather irritating, as explaining things takes time away from other things I could be focusing my attention on instead. Also, I can't imagine a situation where this is ever the right tone:

Please describe it and hopefully you will see.

But fine, I'll explain one last time why I think the proposal in its current state is erratic:

1. Imagine this scenario:
   • A view with one item in the center
   • The item has no event handler installed
   • The view has a mousedown handler installed
   • User clicks in the center of the view, view receives the event with itself as target, because no hit-test is executed (none of the items have event handlers installed)
2. The same scenario as 1. with these changes:
   • The item has a mousedown handler installed that doesn't stop propagation
   • User clicks in the center of the view, hit-test is run because items have event handlers, center item is found and receives the event, the event bubbles up, view receives the center item as target
   • -> Behavior of view event changes based on the presence of event handlers on items.
3. The same scenario as 1. with these changes:
   • A second item is introduced at the border of the view
   • This second item has a mousedown handler installed
   • User clicks in the center of the view, hit-test is run because items have event handlers, center item is found but does not receive the event because it has no handlers installed, the event bubbles up, view receives the center item as target
   • -> Behavior of view event changes based on the presence of one item outside the reach of the mouse that receives events

This is what I think is an erratic API, and hence to be avoided. Feel free to prove otherwise, but please note that I never said that I think it is good that the current view handlers only receive the view as the target. This was simply the left-over of an optimization, and I already made suggestions as to how this could be improved, in a way that is not causing the issues outlined above.

[elliotbonneville]

Thank you for taking the time to write another verbose explanation, @lehni. I'm sorry if you felt your earlier explanations were being ignored -- I was just having difficulty understanding them. I'm new to the codebase. : )

Now, the first two scenarios make sense, but I'm curious as to why in the third scenario you say that target would be the center item. It seems to me that in this case, the view should still receive itself as the target, because there are no valid item targets (items that have an event listener installed) that would pass the hit-test, right? Or is there nothing in place right now that filters the results of the hit-test to check for event listeners?

Edit: this has diverged from the original issue. Would you prefer if I opened a new one for this discussion?

[lehni]

@elliotbonneville I'm not explaining how it should work, I'm just trying to explain how the change proposed by @georeith would work, and why I think that would be problematic. I don't think we need a new issue.

[lehni]

To clarify a possible confusion here: In HTML, when I install a mousedown event on the document, event.target is not pointing to the element that has the handler installed (the document in this case), it's the element that's actually under the mouse when I click. I am not sure where the assumption comes from that in paper.js, event.target should point to the item with the handler installed:

https://jsfiddle.net/kwhu7LkL/

[georeith]

@lehni Either version is good. That is just a suggestion so that an additional hit-test is not required, my concern is that event.target being the item that is currently being dispatched isn't useful and that as it currently stands it follows no clear pattern and view should not be treated any differently to any other event dispatcher.

The code I provided was meant more as an explanation of what I meant. I'm sure it's riddled with bugs, its more to show that a hit-test wasn't needed as we already have references to the item, whichever item it is.

If you want to go the hit-test route that is also a good way. Like you said this isn't the DOM so it is up to you but my only concern is that whatever event.target is it needs to follow a clear pattern no matter what the event is emitted on.

Please don't take our discussion personally, I apologise if you feel that way my only intentions are to maintain the integrity of the library.

[lehni]

@georeith I'm not taking it personal, I'm just getting a bit frustrated as you still somehow seem oblivious to what I have been trying to explain for the past dozen of comments or so, and I am about to run out of ways to explain it: If event.target is to be consistent in the way it is in HTML (or any other way really), then additional hit-tests are required in the situations where no item has any event handlers installed. I've tried to explain that in so many ways, I've provided example scenarios illustrating what I am trying to say, and I've provided an HTML example to compare it with, but you seem to be ignoring all this and stating that both versions are fine. I would say the opposite: Neither version is fine, and if the consistency of event.target is important then there is a solution, but neither the current state nor your proposal will solve that.

I have a solution in my head already, so I'll stop discussing this further and will spend my time implementing it instead.

But I'd recommend to consider your style of discussing these things on Github. I know what closures are and how they work. I know what the code does that I wrote myself. To suggest otherwise feels condescending and doesn't set a good tone in an open-source project, where the participation is based on generosity, passion in the subject and a willingness to donate time to a shared goal.

[georeith]

@lehni I realise that and was offering an alternate solution to the HTML spec. If you don't want to implement it that's fine, it's not been clear to me that you don't want that definition for target so much as you think I'm suggesting we implement the HTML spec.

I think you are being unfair and some of your comments come across as irate where no offence has been intended. I'm not suggesting you don't know how things work, I wouldn't even be discussing this with you if I didn't value your opinion. There is no way for me to respond to something like this if I disagree that you wouldn't take offensively:

You are wrong. This has nothing to do with closures. You make wrong assumptions about the code I've written, and I'm getting tired of trying to explain it to you.

Please understand I am also just trying to help and an open-source community is about discussion and having multiple eyes on something.

Regardless this is becoming too personal for my liking and I would ask that we can just pretend this is 3rd comment in this issue and go back to our happy selves?

[lehni]

Yes I agree, this spiraled out of control. The whole event handling code is a complex beast and I've spent far too much time getting to where it is right now, which is a full rewrite of the previous code. I'm very reluctant to making changes on it, also because it is harder to unit-test than the rest of the library... Happy to go back to this.

[georeith]

@lehni Totally understand, I really value that. Thanks dude.

[lehni]

@georeith I finally managed to find a nice fix for this issue, that should make us both happy : )

[georeith]

@lehni Awesome, just reading the code am I right in thinking it returns the item the event triggered on or in the case that it was a listener on the view runs a hitTest and gets the highest item under the mouse?

[lehni]

Yes that's what it's doing... I'm just now realizing that this is not how Event#target is defined in HTML land. argh. I'm giving up : ) https://developer.mozilla.org/en-US/docs/Web/API/Event/target

[lehni]

I thought it was weird that when you install a listener on view, target would be different based on whether you also installed a listener on an item that the user clicks on or not.

[georeith]

You mean if I have a listener on the view and on the item that I click? I think HTML has the target be the same throughout... whichever first caught the event, then currentTarget is the current item that's receiving the event.

So in that case in HTML I believe it would be:

trigger event on item -> target = item, currentTarget = item propagate to view -> target = item, currentTarget = view

[lehni]

Oh cool, so we're consistent.

[georeith]

But as you said I don't think we have to match the HTML spec, although I guess it's nice for interoperability. The main thing is just being able to tell if event was propagated and what it was propagated from.

[georeith]

I believe that's what this means:

event . target Returns the object to which event is dispatched.

event . currentTarget Returns the object whose event listener's callback is currently being invoked.

Dispatched being the item which initiated the event. And currentTarget being a bit more self explanatory. Find the spec hard to read as you often have to follow their other definitions, but I would assume thats what they mean by dispatched here.

[lehni]

How do you feel about the current implementation in paper.js? I think it's consistent.

[georeith]

@lehni I believe we differ from the spec slightly. Take this example:

https://jsfiddle.net/gv0hzrtf/1/

If you click on the red they both receive it as the target and then themselves as the currentTarget even though it has no event listener.

I like both ways the current kind because you can tell when something is propagated (target is not itself), but thinking about the HTML way it allows you to put a listener on the parent (in our case the view) and use it as a listener for all elements (delegated events). With the implementation above you would need to this manually with a hitTest to be sure. Not sure which is more useful now I think about.

EDIT: To do the above would probably be quite intensive for all events. So maybe we have a good trade-off not sacrificing performance for stuff like mousemove if the user doesn't need it. Could be useful to expose currentTarget though which can always be the element that owns the listener for the current callback.

[lehni]

Isn't currentTarget the same as this inside the callback?