Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canvas circleMarker fires 'click' after being dragged #4458

Closed
balciseri opened this issue Apr 18, 2016 · 8 comments · Fixed by #4493
Closed

Canvas circleMarker fires 'click' after being dragged #4458

balciseri opened this issue Apr 18, 2016 · 8 comments · Fixed by #4493
Assignees
Milestone

Comments

@balciseri
Copy link

balciseri commented Apr 18, 2016

https://playground-leaflet.rhcloud.com/yaq/edit

Scenario:
Map initialized with preferCanvas: true .
Added a circleMarker

var markerC = L.circleMarker(myCenter2,{radius:30}).bindPopup("popUp");
    map.addLayer(markerC);

Bug: if you start the drag of the map while having the cursor ontop of the circlemarker and you stop the drag with the cursor still on the circleMarker the click event will fire ( i'm just assuming it's a click event) and the popup will open.
This is not the default behaviour, if you set preferCanvas: false this will not happen.
I've found an intresting piece of code https://github.com/Leaflet/Leaflet/blob/master/src/map/Map.js#L969 but i don't know how to fix this for canvas markers.

EDit: the bug occurs also if you don't actually start the drag with the cursor ontop of the marker, it has just to be ontop of the marker when you end the drag

@balciseri balciseri changed the title Canvas markers bug: after map dragging a click event will fire and popUp will open. Canvas markers bug: after map drag a click event will fire and popUp will open. Apr 18, 2016
@IvanSanchez IvanSanchez changed the title Canvas markers bug: after map drag a click event will fire and popUp will open. Canvas circleMarker fires 'click' after being dragged Apr 19, 2016
@IvanSanchez IvanSanchez added this to the 1.1 milestone Apr 19, 2016
@yohanboniface yohanboniface modified the milestones: 1.0-rc2, 1.1 Apr 19, 2016
@balciseri
Copy link
Author

balciseri commented Apr 19, 2016

http://playground-leaflet.rhcloud.com/tij/1/edit?html,js,output

Maybe this can fix it:

I've found in an older version ( Leaflet 1.0.0-beta.2 (55fe462))
this piece of code:

if ((e.type === 'click' || e.type === 'preclick') && !e._simulated && this._draggableMoved(target)) {
           console.log("let's exit");
           return;
}

in map._fireDOMEvent

So i added it back in the actual rc1 and the bug seems fixed.
But in the actual rc1 this pice of code has been moved before in map._findEventTargets: https://github.com/Leaflet/Leaflet/blob/master/src/map/Map.js#L968
So i'm not sure what to do.

@IvanSanchez
Copy link
Member

@balciseri That code was changed in 64484d5 by @yohanboniface , as a fix for #3971. It is important to not introduce that bug back.

@balciseri
Copy link
Author

balciseri commented Apr 19, 2016

http://playground-leaflet.rhcloud.com/pav/1/edit?html,js,console,output

I'm trying to reproduce that bug but it's seems to not happen ( i made the marker draggable and with no popup and the map click event is not firing after dragging it)
Be aware that in my version there is still the check added by @yohanboniface in map._findEventTargets
but also in map._fireDOMEvent . I'm sorry but i don't have the expertise to refactor these checks.

Edit: wrong link.

@balciseri
Copy link
Author

balciseri commented Apr 19, 2016

But i see there is this new bug: When you click on the markers the Map click event will trigger..

@IvanSanchez
Copy link
Member

But i see there is this new bug: When you click on the markers the Map click event will trigger.

Event delegation is a feature in 1.0.0, not a bug ;-)

@balciseri
Copy link
Author

balciseri commented Apr 19, 2016

Sweet so now if you guys find an elegant way of implementing this double check fix would be awesome :D

@balciseri
Copy link
Author

balciseri commented Apr 19, 2016

@IvanSanchez after looking better into it i see what you mean.
You mean that if for example there is no PopUp binded to the marker and you click on the marker the Click event of the Map will fire.
Instead if you click on the marker with a PopUp binded the Click event of the map will not fire.
This is ok for the normal marker http://playground-leaflet.rhcloud.com/bif/1/edit?html,js,console,output
If you click on the normal marker with the Popup binded the Map click event will not trigger, but if you click on the circleMarker with the popup binded.. the map click event will trigger.
Am i correct that this is not the correct behaviour ?

@yohanboniface
Copy link
Member

Am i correct that this is not the correct behaviour ?

That needs a decision ;)

By default, in 1.0.0 version, an event that is not stopped should bubble to the map. But we have made an exception for marker, in the hope of making the life of the Leaflet user easier, see #3605 (comment) for context.

Now, CircleMarker is a Path descendant, so it does not inherit this hack. I'm unsure if we should add the same hack here.

BTW, given there is not this behaviour when using SVG instead, this seems to mean that the problem is elsewhere.
And clearly a drag should not issue a click, so this is anyway a bug imho.

I'll work on this asap (but at the office right now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants