Stop click on _openPopup #3605

yohanboniface · 2015-07-08T09:58:00Z

Fix #3604.
I had a quick break.
Bubbling from marker is supposed to be the default, now, so I just stopped the event on _openPopup.
I could also just add it on bindPopup, if you prefer this pattern.

mourner · 2015-07-08T10:04:26Z

Bubbling from marker is supposed to be the default, now

Hmm, but why? I think I'd prefer to keep not bubbling up from markers the default.

yohanboniface · 2015-07-08T10:17:00Z

Hmm, but why?

The map is an event target, so it's considered while traversing the DOM, and given that it listens for click the event is fired.

I think I'd prefer to keep not bubbling up from markers the default.

Are we sure it's a problem (out of retrocompat) ? It's closer to DOM behaviour, no?

ktoto · 2015-07-08T13:15:29Z

@yohanboniface problem not in openPopup. problem happen when i have 'click' handler in marker and in map.
openPopup just is special case with this binding.

yohanboniface · 2015-07-08T13:24:40Z

@ktoto (take this with caution, as it's not fully decided): with the current code when you listen to a click event and you don't want the map to also receive it, you are responsible of stopping it.
In the case of bindPopup, given that the event listener has been added by Leaflet internally, it's its responsibility to then stop it. Makes sense?

ktoto · 2015-07-08T13:56:35Z

@yohanboniface but before #3307 all be fine. I just want to understand why logic with map is changed?
Why map want click if the first interaction element was marker? Why just not stop on marker at all?

yohanboniface · 2015-07-08T13:59:03Z

Why map want click if the first interaction element was marker? Why just not stop on marker at all?

Just like in normal DOM, when an event is fired by an element, all its ancestors get it, unless stopPropagation is called at some point.
You may have a use case where you want to catch all click events, both on markers (or polygons) and map.

mourner · 2015-07-08T14:01:53Z

I'd rather not follow DOM conventions and do an exception here, since I can't think of a use case where you'd want to trigger both map and marker on marker click, but people will have a very hard time adjusting to this breaking change.

yohanboniface · 2015-07-08T14:06:13Z

I'd rather not follow DOM conventions and do an exception here

You mean exception for marker or for click ? i.e. never bubble click or never bubble click on marker but still bubble it on polygons?
I can think of many use cases where I'd love to receive click events that where initially fired on a polygon. And by the way, being able to receive click on a map whatever the source was would be a killer simplification for all "drawing" application on the map.
But you decide :)

mourner · 2015-07-08T14:17:23Z

Exception for marker specifically, as opposed to vector layers and image overlays (which can be perceived as "click through" in some interfaces).

yohanboniface · 2015-07-08T14:24:33Z

OK, I'll do that asap :)

yohanboniface · 2015-07-08T14:25:12Z

@mourner only for click or we can think about other events?

mourner · 2015-07-08T14:28:55Z

@yohanboniface let's follow the 0.7.3 behavior here.

mourner · 2015-07-09T09:40:23Z

src/core/Events.js

+ options: {
+ unpropagateEvents: '' // Space separated events that should not be propagated
+ },
+


This should be in Layer.

I hesitated ;)

mourner · 2015-07-09T10:18:58Z

src/map/Map.js

@@ -696,7 +696,8 @@ L.Map = L.Evented.extend({
 for (var i = 0; i < targets.length; i++) {
 if (targets[i].listens(type, true)) {
 targets[i].fire(type, data, true);
- if (data.originalEvent._stopped) { return; }
+ if (data.originalEvent._stopped
+ || (targets[i].options.nonBubblingEvents && targets[i].options.nonBubblingEvents.indexOf(type) !== -1)) { return; }


Unfortunately IE only supports array indexOf starting from 9. We'll probably have to iterate over the array in a loop.

Gasp :/
Should I add a polyfill L.Util.inArray for that? To make the code shorter here and more readable.

Yeah, maybe something like L.Util.indexOf.

L.Util.indexOf seems meaning it also will work on strings, is that what we want?

It's fine to keep only for arrays since string indexOf doesn't need an util method. It's an internal utility after all, we don't have to make it generic.

yohanboniface · 2015-07-09T11:51:31Z

Meeeeehhhh, how the console is playing with me!?

Array.prototype.indexOf?

mourner · 2015-07-09T12:05:07Z

Mine is like this :)

I think we're fine just not using native indexOf. It's not that much of a difference.

yohanboniface · 2015-07-09T12:06:13Z

Yeah, Chrome vs FF :/
OK, I make the loop for everybody :)

mourner · 2015-07-09T12:08:00Z

OK, let's rebase now.

yohanboniface · 2015-07-09T12:08:57Z

Actually I think it's fine at this moment :)

Humm, not sure which moment :p Seems I've commit one second before to remove the use of Array.indexOf

mourner · 2015-07-09T12:09:39Z

Nevermind :)

yohanboniface · 2015-07-09T12:15:46Z

Rebased and green :)

Stop click on _openPopup

mourner added the in progress label Jul 8, 2015

ktoto mentioned this pull request Jul 8, 2015

update leaflet version 2gis/mapsapi#262

Merged

mourner reviewed Jul 9, 2015
View reviewed changes

yohanboniface mentioned this pull request Jul 9, 2015

Mouse events on markers only return the marker's single latlng point #3607

Closed

yohanboniface force-pushed the stop-click-on-openpopup branch from 0cd3868 to 4f10f2d Compare July 9, 2015 09:58

mourner reviewed Jul 9, 2015
View reviewed changes

yohanboniface force-pushed the stop-click-on-openpopup branch from 720b02e to b20ffaa Compare July 9, 2015 12:10

Add nonBubblingEvents option (fix #3604)

74018f2

yohanboniface force-pushed the stop-click-on-openpopup branch from b20ffaa to 74018f2 Compare July 9, 2015 12:12

mourner added a commit that referenced this pull request Jul 9, 2015

Merge pull request #3605 from Leaflet/stop-click-on-openpopup

32c9156

Stop click on _openPopup

mourner merged commit 32c9156 into master Jul 9, 2015

mourner removed the in progress label Jul 9, 2015

mourner deleted the stop-click-on-openpopup branch July 9, 2015 12:18

dmolineus mentioned this pull request Jul 22, 2015

Leaflet 1.0 support netzmacht/php-leaflet#1

Closed

29 tasks

yohanboniface mentioned this pull request Aug 24, 2015

Clicking on layer triggers both map.on('click') and layer.on('click') events in 1.0 b1 #3756

Closed

yohanboniface mentioned this pull request Nov 11, 2015

Bug with twice event handling in 1.0.0-beta.2 #4008

Closed

This was referenced Apr 13, 2016

Fix mouseout bubbling while initial target was not listening (fix #3648) #3650

Merged

Canvas circleMarker fires 'click' after being dragged #4458

Closed

yohanboniface mentioned this pull request Sep 5, 2016

Marker events are not propagated to map #4882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop click on _openPopup #3605

Stop click on _openPopup #3605

yohanboniface commented Jul 8, 2015

mourner commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

ktoto commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

ktoto commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

mourner commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

mourner commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

mourner commented Jul 8, 2015

mourner Jul 9, 2015

yohanboniface Jul 9, 2015

mourner Jul 9, 2015

yohanboniface Jul 9, 2015

mourner Jul 9, 2015

yohanboniface Jul 9, 2015

mourner Jul 9, 2015

yohanboniface Jul 9, 2015

yohanboniface commented Jul 9, 2015

mourner commented Jul 9, 2015

yohanboniface commented Jul 9, 2015

mourner commented Jul 9, 2015

yohanboniface commented Jul 9, 2015

mourner commented Jul 9, 2015

yohanboniface commented Jul 9, 2015

Stop click on _openPopup #3605

Stop click on _openPopup #3605

Conversation

yohanboniface commented Jul 8, 2015

mourner commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

ktoto commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

ktoto commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

mourner commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

mourner commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

yohanboniface commented Jul 8, 2015

mourner commented Jul 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yohanboniface commented Jul 9, 2015

mourner commented Jul 9, 2015

yohanboniface commented Jul 9, 2015

mourner commented Jul 9, 2015

yohanboniface commented Jul 9, 2015

mourner commented Jul 9, 2015

yohanboniface commented Jul 9, 2015