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]

We could probably add it in a really simple way, through Emitter#emit(). If there's an event object, it could set #currentTarget to this before emitting the event...

[lehni]

If that's too extreme (probably), then maybe the Event classes that should behave this way could define a "private" property called #_updateTarget, and if that's set to true, it would do the above.

[georeith]

Sounds good.

One question now I've had time to think about it. Are we being consistent with the hitTest as sometimes its the this value of the first triggered listener and other times its the highest item under the mouse no? I might just be misunderstanding the possible values of Event#target though.

Perhaps we should just set Event#target to whatever the first currentTarget is and ensure it is always the same throughout. If its the view its the view, if it fires on a object before propagating to view then it is that, assuming we do child -> parent propagation and not just view (haven't actually tested that).

E.g.,
http://sketch.paperjs.org/#S/nZKxbsMgEEB/BbE4kRCyGV11ytC9GWMPFF9sZHpYmCRKIv97wbFrR0qXMqDjne7ugbhTlN9Ac7pvwauGMqpsFc9n6YgD5SXWBjLyThAuZN/IDvjnjDf3AklYndXoc3JIGUlL9mC9vkFAWRpg2GZ81MbsrLEuJ4m6SkwmHi0C+p0oYmLYvhVY4JOK+LeKiCritYqD6i+TLHkSqZ09dZPDR4znyVPdmJ97qUabygGG6csF2Opdy6W3hgu3uDNataH92GZ1XkpeQbGCxxMqry1u4Azot2TSUxZ7a4AbWz8y3EtXg+fRmxHf6H4Mo80QvsGXA9mOr9nT/FAOPw==

click on rectangle1:

1. target: rectangle1, currentTarget: rectangle1
2. target: rectangle1, currentTarget: group
3. target: rectangle1, currentTarget: view

click on rectangle2:

1. target: rectangle2, currentTarget: rectangle2
2. target: rectangle2, currentTarget: group
3. target: rectangle2, currentTarget: view

click on group:

1. target: group, currentTarget: group
2. target: group, currentTarget: view

click on view:

1. target: view, currentTarget: view

Unless I'm misunderstanding Event#target there and it is always the highest visible item like the DOM (if so ignore this I'm being a numpty). This lets you do everything, determine that the event is propagated and from where, and if you need to delegate events just hitTest the item with the event.point to get the child it hit.

[lehni]

Yeah that's what I was wondering about too. So this whole hitTest() / getTarget() magic wasn't even necessary. Oh well. : )

[georeith]

Ha sorry I can't really remember what we were going over before it might have already been suggested, was just running it over my head last night and seems like the most elegant without degrading performance. Also I messed up the naming a bit in that sketch this is a better one:

http://sketch.paperjs.org/#S/nZKxbsMgEIZfBbE4kZDlMLrqlKF7M8YeKL7YyOTOwiRRG/ndC44tO1K6lAFx34n/PiHuHNUZeM4PLXjdcME1VbG+KsccaK+wtrBj7wzhxg6N6iD9nPHmXiALqyODPmfHTLCsFA/Wmx8IaJcFGLYZn4y1e7Lkcpbob4XJxKNFQMvE2Bi2bwUW+KQi/60io4p8reKg+stEJk8itaNLNzl8xPM8ebo39ucs3RhbOcAwfckTq3ctl2wDt5Rwb41uQ/wYs6qXK6+gXMHTBbU3hBu4Avotm/Q0YU8WUkv1o5N65WrwafQWzDemH4/RZgjf4MuBasfX7Hl+LIdf

[lehni]

Yeah I think I had a misconception about what #target should return, hence our endless disagreements and looping earlier in the year, probably. Apologies for that. I will revert the hitTest() magic.

[georeith]

@lehni Not at all. I think we both just had a stressful day and felt we were right I was making assumptions on snippets of code, I know you were fed up with the events just having refactored them and rightly so, I know the feeling. I think we are on to a winner here though.

[lehni]

@georeith yeah I do remember some preexisting levels of stress that day, unrelated : )

I do like where we ended up here, code looks good! I wasn't keen on the hitTest() hack.

It would be good to have unit tests in place for all this mouse event stuff... I guess we could just emulate prerecorded mouse sequences by the use of dispatchEvent()?

[elliotbonneville]

That's what we've settled on doing with our unit tests. Here's what we've been using to dispatch events:

const scope = new paper.PaperScope();
const document = scope.document;

export const mouse = {
    move(x, y) {
        document.dispatchEvent(new window.MouseEvent('mousemove', {
            clientX: x,
            clientY: y,
        }));
    },

    down(x, y) {
        scope.project.view.element.dispatchEvent(new window.MouseEvent('mousedown', {
            clientX: x,
            clientY: y,
        }));
    },

    up(x, y) {
        document.dispatchEvent(new window.MouseEvent('mouseup', {
            clientX: x,
            clientY: y,
        }));
    },
};

[lehni]

@elliotbonneville that's great! We should do this too. What are you testing for this way?

[lehni]

@georeith now with event.currentTargetand docs.

[lehni]

@georeith I've been juggling too many things this week, didn't realize we made a wrong assumption, and this was masked by the accidental removal of a code optimization that only executes the hit-test when it's sure to be needed. I've added this optimization back now (ab24f92), and it shows that we do need the hitTest() getter magic. See this example:

https://jsfiddle.net/lehni/ou13xkzs/

The event is installed on the outmost <div> only, yet event.target points at whatever element is on top of the stack where you click, wether it subscribes to the event or not. This is what I wanted to achieve with this additional getTarget() code, and also what I pointed out in your suggestion earlier this year as a short-coming.

I hope we can finally clear this confusion up : ) I think we have two options: Keep the code simple and always run a hit-test, wether we need it or not (slow), or bring back my getTarget() getter that uses the already calculated hit-test result if it exists or performs a hit-test if it's the first time it's requested (depending on the value of hitItems).

[georeith]

@lehni Indeed that is the HTML way. I was thinking we would only do top of stack if it subscribes to the event (which deviates from HTML but saves additional hit-testing) leaving it up to the developer to do it or not.

So what I was thinking was the first emitted callback sets event#target to its owner. And any additional propagation has the same value just setting event#currentTarget to its owner.

Although I'm happy with if we do match the HTML spec too. Just worried it will make mousemove listeners slow for scenes with a large number of items. Not sure how optimised hit-test itself is. Under what circumstances does the hit-test already exist? Originally I imagined you ran a hit-test only on items with listeners for that event.

Edit: It could be possible to run the full hit-test once, cache it and invalidate it whenever there is a geometrical change. Is that what the hit-test optimisation does? That would still be slow for what I imagine are common cases like mousemove based translation or resize which would invalidate it on every event though.

[lehni]

The way I had written is, the hit-test is only run when it's directly required (e.g. because of click, mouseenter, mouseleave events), or when event.target is first accessed. And per handling of event, the hit-test is never run more than once. So if you don't use event.target, there is never any impact on performance. And if you do, then it is necessary to behave the same as HTML, which I think is the only logic way now really...

[georeith]

@lehni Ah ok I like that it is in a getter on event.target. Although is it not possible that could be messed up? As in my event callback I move an item over event#point and then access event#target would it not wrongly report that item as being the event#target?

[lehni]

I guess it would, but I guess we could live with that? : ) But yeah,you've got a point. Damn,...

[georeith]

@lehni Hmmm I'm just worried it would create more issues. Its a bit of a strange side effect to include in code, having to remember to read event.target before modifying anything.

[lehni]

Yeah I agree.

[lehni]

Always run the test?

[georeith]

@lehni I guess we have to to match HTML spec.

[lehni]

@georeith matching the spec means always running the hit-test. We could also activate it by default but offer a flag that turns it off?

[lehni]

What I mean is: There could be a need for no "correct" item-level event support. It could be nice to be able to install events on the view only, and not have paper.js take care of event#target, for performance reasons.

[georeith]

@lehni Sounds good, I would just return null or throw an error for the target if you switch it off instead of returning a possibly incorrect value.