Always fire DOM event on the map too

[yohanboniface]

Since the new event delegation API, when we add a listener on the map, for example on "mouseover", this listener is not called when the event is targeted by a layer (Polygon, Marker…).

What I understand, is that with the old API, the event was listened both on each layer and on the map. With the new API, it's listened only on the map (which is nice) and then forwarded to the original target, but only to this target.

So here is a suggestion to fix that.

Thanks for reviewing :)

[yohanboniface]

I'm thinking about something else.

As far as I understand, with the old API, as we were listening DOM events both on the vectors or marker and on the map, we were able to use the stopPropagation when catching the event on the vector or marker, which then would prevent the map to catch the event.
I think this will not work anymore with my current patch proposal, as we are dealing with Leaflet events.

Also, I wonder if a better approach is not to extend the propagate API to always also propagate to the map.

(Breakfast time. After that I'll try to add more unit tests to cover the stopPropagation use cases.)

[yohanboniface]

@mourner I think I need your thoughts before going forward :)

[mourner]

Haven't thought about this closely yet, will do :)

[yohanboniface]

@mourner this last commit makes all tests happy, but I feel like workarounding the issue, so I think a more global (expert) view is still needed :)

[MelnikVasya]

In this branch don't work popup

[yohanboniface]

Yes! Because click event is sent both to the layer and to the map, so it opens the popup and close it just in the same stack. So the layer should stop it for it not to be propagated to the map.

But clearly this means that this branch is work in progress. ;)

[mourner]

I'm kinda lost here, need to revisit and rethink how events and propagation work to have some input in this...

[mourner]

@yohanboniface as an alternative, can you please check #3450 and see if it's a good approach for this case? This way we explicitly bubble up all layer events to map. There should be the same problem with popups though.

[IvanSanchez]

Note to self: create a webpage inside /debug/test with overlapping polygons, draggable markers, and mouseover/mouseout event handlers on everything.

[yohanboniface]

@IvanSanchez added :)
So it works smoothly in the map-domevent branch, but it doesn't in the master branch:

• when the mouse is over the marker itself there is no mousemove event fired so the marker stop, then the mouse goes out the marker, and the map fire again the mousemove so the marker move above the mouse again, and so on
• when the mouse is over the polygon, nothing happens for the same reason

[yohanboniface]

@mourner I'm travelling in Italy right now, so not really connected and not really behind my computer, but I will have a look for sure. As first thoughts, this option sounds really interesting, but I do think we still should support bubbling events from features (polygon, marker...) to map, and being able to stop them when we do not want this bubbling.

[IvanSanchez]

So the problem with popups is that the order of the events is:

1. polygon preclick
2. polygon click (popup opens here)
3. map preclick (popup closes here)
4. map click

I'll spend some time trying to figure out how to fix this order into preclick-preclick-click-click.

[yohanboniface]

Good catch :)
Seems to me that, out of what you found, the popup issue is a generic problem introduced by this PR: events listened both on a feature AND the map will often need to be stopped not to trigger twice the listener.
I feel like we need to spend a bit of time more to clean/rationalize the way Leaflet events can be stopped if we want to dig more on this PR.
I'm at work today, but on IRC if you are on this and need a plastic duck to talk to :)

[mourner]

What I don't like in the code changes is that there's now handleDOMEvent method that used to actually handle DOM events, but now also handles Leaflet events in special cases, and there's Leaflet DomEvent.stopPropagation method that used to only deal with DOM events and now has a special case for Leaflet events. It's confusing, and will probably lead us to more traps later.

We should look for a way to handle the issue more gracefully.

[yohanboniface]

We should look for a way to handle the issue more gracefully.

This needs a sprint in Trondheim for sure! ;) cc @IvanSanchez

(/me at SotM France for three days right now)

[yohanboniface]

@mourner, tested with addEventParent/removeEventParent: seems much more cleaner. Not sure about the perfs, though, as we are adding the map in the hash of every single Layer instance. What do you think?

there's Leaflet DomEvent.stopPropagation method that used to only deal with DOM events and
now has a special case for Leaflet events. It's confusing, and will probably lead us to more traps
later.

I agree that, as it is, it's hacky. However, I think that if we take care of propagating events (which I think we should) we should also have of a way to stop them, nope?
I thought that the lightest way was to rely on the cancelBubble property even for Leaflet events, so I've made a quick POC. But maybe we should instead have an internal property, which would be less confusing (like _stopped or whatever). What do you think?

[mourner]

You're probably right about performance, but I need to recheck this.

But maybe we should instead have an internal property, which would be less confusing (like _stopped or whatever). What do you think?

Sounds good. cancelBubble here is indeed a bit confusing.

[yohanboniface]

@mourner Done :)
Should we document that somehow? Or we wait a bit to see if we are sure we go this direction? :)

[mourner]

@yohanboniface lets wait, I'm still not sure about the approach and have a feeling that we may regret merging this too hastily.

[yohanboniface]

OK :)

[yohanboniface]

I've not reached a nice solution when tried (but maybe it was just too much waves ;) ), after fixing the canvas issue. I'll make a new shot just after nomnom :)

[mourner]

Ah what the hell!