Leaflet 1.7.1 causes 2 click events to be emitted by Leaflet Controls in Safari

Steps to reproduce
Steps to reproduce the behavior:

set up a basic map, with a control that has a button in it, which listens for click events and posts the events to the console
click the button in the control using Safari on a Mac
see two click events in the console, one with isTrusted:true and another with isTrusted:false attributes

(Using 'force touch' to click the button on a trackpad results in only a single click event being emitted, but it also triggers the Safari preview popup.)

Expected behavior

There should only be one click event emitted, with the isTrusted:true attribute.

Would Leaflet be able to safely filter out the click events with isTrusted:false?

Current behavior

In version 1.6 the expected behaviour is observed. In version 1.7.1 Chrome exhibits the expected behaviour, but Safari doesn't.

The differences between the two events are included below.

MouseEvent 1:

MouseEvent

_simulated: true
composed: false
isTrusted: false
layerX: 15
layerY: 28
offsetX: 15
offsetY: 28
pageX: 410
pageY: 46
screenX: 1390
screenY: 189
timeStamp: 499337
webkitForce: 0
x: 410
y: 38

MouseEvent 2:

MouseEvent

composed: true
isTrusted: true
layerX: 15
layerY: 20
offsetX: 15
offsetY: 20
pageX: 410
pageY: 38
screenX: 1390
screenY: 189
timeStamp: 538265
webkitForce: 1
x: 410
y: 38

_simulated, composed, isTrusted and perhaps webkitForce seem to be the key differences, but it also seems odd that there are different timestamps and Y dimensions between the two events.

Environment

Leaflet version: 1.7.1
Browser (with version): Safari 13.1.2
OS/Platform (with version): OS X 10.13.6 and 10.15.6

Additional context

I noticed this after OpenStreetMap updated to the latest version of Leaflet, where the issue manifests itself by preventing some of the map tools to be used as the first event toggles a tool on and the second toggles it off. Related issue posted at openstreetmap/openstreetmap-website#2811

Minimal example reproducing the issue

Please see https://codepen.io/dankarran/pen/JjXMXzd

Minimal example reproducing the issue

For the sake of clarity: that example is using

    L.DomEvent.on(container, "click", function(e) {

Would Leaflet be able to safely filter out the click events with isTrusted:false?

It would be possible to do so, in addOne() in DomEvent.js, around

Leaflet/src/dom/DomEvent.js

Line 109 in 4f32a5e

} else if (type === 'mouseenter' || type === 'mouseleave') {

With a new branch in that if-then-else chain, something probably like

} else if (Browser.safari && !Browser.mobile && type === 'click') {
  handler = function (ev) {
    if (ev.isTrusted) { originalHandler(ev); }
  }
  obj.addEventListener('click', handler, false);
} // etc

However, I do not own any Apple hardware (at the moment), so I'm unable to test this; and I don't know if this might impact previous versions of desktop Safari.

The simulated event comes from

Leaflet/src/map/handler/Map.Tap.js

Line 100 in 4f32a5e

this._simulateEvent('click', first);

So I suggest changing the condition at

Leaflet/src/map/handler/Map.Tap.js

Line 87 in 4f32a5e

if (this._fireClick && e && e.changedTouches) {

into something like

if (this._fireClick && e && e.changedTouches && !(Browser.safari && !Browser.mobile)) {

However, I do not own any Apple hardware (at the moment), so I'm unable to test this; and I don't know if this might impact previous versions of desktop Safari, or other browsers (Does the bug maybe reproduce on firefox mobile?).

I think this bug has been introduced at

https://github.com/Leaflet/Leaflet/pull/7010/files#diff-c2e3d5ef35bf1c4a7c6b549ddf734725R134

@johnd0e , can you have a look? Maybe you introduced that change to account for iOS but unintentionally affected desktop macOS?

Thanks for looking into it @IvanSanchez. I'm happy to help testing any proposed changes on Safari on Mac or iOS.

Just to add, there is a related problem in Safari on iOS, but it seems to behave slightly differently. I haven't done much testing on that.

@johnd0e , can you have a look? Maybe you introduced that change to account for iOS but unintentionally affected desktop macOS?

That was definitely meant to address iOS issues, see discussion: #7010.
And we use safari property intentionally, in order not to be tooo specific.

Possible quickfixes:

introduce Browser.ios property
or use Browser.safari && Browser.mobile

But what if tomorrow we will get macbook with touchscren? That will break it all again.

So I propose to apply quickfix (Browser.safari && Browser.mobile) ASAP, and then discuss ultimate solution here: #6980

* or use `Browser.safari && Browser.mobile`

I like this approach (as a "quick & dirty" fix).

But what if tomorrow we will get macbook with touchscren? That will break it all again.

You know that my answer, in the end, is gonna be #7200 :-)

You know that my answer, in the end, is gonna be #7200 :-)

I am not so sure)
But anyway, I'd prefer to fix all we can staying in current approach, and then move further (to #7200 or whatever).

Unsure whether this is the same issue (so please forgive tagging onto end of existing ticket), but shift-drag to zoom in is also broken in 1.7.1 on desktop Safari. Happy to open a new issue if #7260 hasn't fixed this too.

@systemed Can you please try shift-dragging using https://s3.amazonaws.com/leafletjs-cdn/content/leaflet/master/leaflet-src.js ? (e.g. https://plnkr.co/edit/v4GzrCYSxpIO1aOo ) That's a build from master that already includes the changes from #7260.

Spot on - that works fine. 👍

As appeared, the same issue affects not only desktop Safari, but also legacy Edge (and who knows what else).
AFAIK atm tap handler is needed for iOS only.

So... May be we should change map.options.tap default to false.

I've updated my codepen to use the code built from master, and also to display the events to make it easier to test on mobile.

It's working well on desktop Safari, but unfortunately, it looks like mobile Safari is still firing off two click events.

If the JS at https://s3.amazonaws.com/leafletjs-cdn/content/leaflet/master/leaflet-src.js is the latest version, it's still firing two click events on iOS Safari.

@dagjomar
Are you able to inspect those events?

It may be different issue.

Because on touchstart (and pointerdown) we prevent click here:

Leaflet/src/map/handler/Map.Tap.js

Line 43 in dda26ba

DomEvent.preventDefault(e);

And I do not see any reason why that might not work.

@johnd0e I'll see if I can get some more details out, and let you know. Would it make a difference what type of element was used (e.g. a or button which would have their own click handlers vs div which wouldn't)?

I've updated the codepen to log some simplified event details to the console on there. The events on iOS seem to be the same as seen on the desktop before this update:

type: click
isTrusted: false
composed: false
_simulated: true
webkitForce: 0

type: click
isTrusted: true
composed: true
webkitForce: 1

Also, a and div elements behave the same, and adding e.preventDefault() in my event handler doesn't solve it.

Let me know if there's anything else you'd like me to try @johnd0e?

Is it possible that the following lines aren't needed, if Safari is firing its own click events?

Leaflet/src/map/handler/Map.Tap.js

Lines 98 to 101 in 4f32a5e

    
           // simulate click if the touch didn't move too much 
        
           if (this._isTapValid()) { 
        
           	this._simulateEvent('click', first); 
        
           }

By the way, I've just checked and Chrome (v85) on iOS is behaving the same, with two events fired.

Ok, see what I've found:

Preventing mousedown/pointerdown does not cancel click. Only touchstart can be prevented.
But currently Leaflet has rather weird behavior (#7077): when pointer events are available - it substitutes touch events with pointer events.

That's why we now have those extra clicks.

So to fix that properly we should adopt one of these: #7029, #7026.
@IvanSanchez

If it helps as a quick fix as a stopgap (and doesn't break other things), commenting out that simulated event does seem to work for me in both Chrome and Safari on iOS...

Leaflet/src/map/handler/Map.Tap.js

Line 100 in 4f32a5e

this._simulateEvent('click', first);

@systemed Can you please try shift-dragging using https://s3.amazonaws.com/leafletjs-cdn/content/leaflet/master/leaflet-src.js ? (e.g. https://plnkr.co/edit/v4GzrCYSxpIO1aOo ) That's a build from master that already includes the changes from #7260.

https://s3.amazonaws.com/leafletjs-cdn/content/leaflet/master/leaflet-src.js
^ this version seems to work way better than 1.7.1 in desktop Safari 14.0.
tap or click fires only once 👍

If it helps as a quick fix as a stopgap (and doesn't break other things), commenting out that simulated event does seem to work for me in both Chrome and Safari on iOS...

Leaflet/src/map/handler/Map.Tap.js

Line 100 in 4f32a5e

this._simulateEvent('click', first);

I had the 2 click event issue on all device (desktop firefox, chrome, ios safari, chrome..) and this resolved the issue

The same problem happens to me and only happens in Safari. Chrome and Firefox are only firing the event once MacOS 10.15.6 and Safari 14.0.

I temporarily downgraded Leaflet to 1.6 and everything works normally.

Is there a chance this bug will be fixed?
Does anyone work on it?

As I've already said: the bug is caused by extremely outdated tap handler (#6980).

To get things off the ground we need to make some decisions: #7255 (comment)
@IvanSanchez @mourner

To get things off the ground we need to make some decisions: #7255 (comment)

@johnd0e I don't have neither the time to test this, nor any iOS hardware. Anyhow, I'm trusting you on this, so I'm greenlighting #7026 (which feels cleaner than #7029)

Does anyone work on it?

You know what'd be cool, @PeterPan73? For you to go ahead with making a couple of builds from #7026 and #7029, and testing those for further compatibility issues, instead of just putting more pressure on the maintainers.

so I'm greenlighting #7026 (which feels cleaner than #7029)

Unfortunately in last commit of #7026 I had to apply ugly workarounds in order to overcome other bugs (which in turn can be addressed by #7029 and #7059).

So in the end we still will have to deal with #7029 and other connected PRs, that's why I've united them all under https://github.com/Leaflet/Leaflet/projects/1

I run into this issue on macOS 10.15.7 using Chrome 85 DevTools with iOS device enabled. The quickest workaround for me has been to use lodash debounce function.

Today I ran into an issue, which seems connected to this one, so I will post my experience here.
This occurs with the Macbook trackpad and I discovered it, when trying to add a custom contextmenu to the map.
What happens is that, once the map has focus, even if you right click for the contextmenu, Leaflet will fire a simulated click event, which should not be fired and has button value 0. This leads to weird interactions with my close handler for the contextmenu.

To produce the bug you can go to https://leafletjs.com and then:

Click on the map (so the map has focus)
"Right click"
Click on the map
"Right click"

What you will see is that the cursor is in dragging mode and once you twice click on the map (once the close contextmenu and once to trigger click event) the map will jump.

Maybe this relates also to some other issues (e.g. #6865).

This twice click event (one trusted, one not) occurs for me on 1.7.1 when simulating mobile on Chrome and Firefox on desktop Linux (didn't happen on 1.6). Works well on real Android touch devices though (Chrome/Firefox tested). I have for now solved it by implementing my own bounce timer in the click event handler, filtering out clicks that happen within 150 ms from the last one.

I have for now solved it by implementing my own bounce timer in the click event handler, filtering out clicks that happen within 150 ms from the last one.

Hi @atorger, how did you implement this? I tried event.originalEvent.preventDefault(); in the onClick event handler, but with no success. Can you share a minimal example? 🙏 😄

I have for now solved it by implementing my own bounce timer in the click event handler, filtering out clicks that happen within 150 ms from the last one.

Hi @atorger, how did you implement this? I tried event.originalEvent.preventDefault(); in the onClick event handler, but with no success. Can you share a minimal example? pray smile

sure:

var lastClick = Date.now();
e.addEventListener('click', function(event) {
    var now = Date.now();
    var diff = now - lastClick;
    lastClick = now;
    event.stopPropagation();
    if (diff > 150) {
        // handle the click event
    }
});

Note that this issue also causes bindPopup to not work with markers:

Version 1.7.1: Nothing happens when clicking the marker https://jsfiddle.net/danmichaelo/gnds7a8o/15/
master: Works as expected again 🎉 https://jsfiddle.net/danmichaelo/gnds7a8o/16/

I ran into this problem in firefox as well (and mobile safari). I was not able to reproduce it reliably but the hint that it comes from the tap option led me to the solution which disables tap entirely as I apparently don't need it to open the popup, even on mobile safari:

var map = L.map('map', { "tap": false });

I ran into this problem in firefox as well (and mobile safari). I was not able to reproduce it reliably but the hint that it comes from the tap option led me to the solution which disables tap entirely as I apparently don't need it to open the popup, even on mobile safari:
var map = L.map('map', { "tap": false });

Thank you for this solution, @eikes! I ran into this issue today and that solved it.

I apparently don't need it to open the popup, even on mobile safari:

On mobile safari tap handler is needed to simulate contextmenu event on taphold.

This bug fires two touches when trying to click on map on a mobile device.

I can confirm. Multiple users have reported popups not working on iOS / Safari on our map using Leaflet 1.7.1

See domoritz/leaflet-locatecontrol#280 and the workaround that fixes the issue.

This issue prevents menus from working on Safari. For example, openstreetmap loses its map tools. As no one is assigned to this bug, it may not get attention.

Today I faced with the same bug on the last version of the iOS. Popup appears and closes instantly due to duplicate click.

disabling tap altogether fixes this for me on safari (using leaflet from react-leaflet) and all my tap events still work.

var map = L.map('map', { "tap": false });

works great! Not sure what "tap" actually does since my markers, panning etc all still work.

source: https://bleepcoder.com/leaflet/694192486/leaflet-1-7-1-causes-2-click-events-to-be-emitted-by-leaflet

edit: says @eikes comment was pinned but I actually had to "unhide" it to see it

Disabling tap fixed the problem for me also. But since I'm using React-Leaflet, I don't use L.map(). Instead, the MapContainer component has a tap prop that can be used for disabling tapping.

<MapContainer tap={false}>

For anyone using Vue-Leaflet, setting tap to false seems to work to resolve the issue, but remember that vue-leaflet only allows some options to be passed as props to the map component directly, for the rest (like 'tap') you need to pass a custom 'options' object, as per: https://vue2-leaflet.netlify.app/faq/#how-can-i-specify-leaflet-options-that-aren-t-part-of-a-component-s-props

@johnd0e @IvanSanchez is there now a fix merged or is this still open?

#7026 is meant to fix this, unmerged yet.

Hi! I've read through this thread and I see several notes about this being fixed from the discussions when the thread first started back in 2020 to more recently.

I am still seeing this happening on Safari Desktop. Is this expected ?

Thanks Justin

@justinjngan see #7255 (comment).

This bug is also reproducible on Leaflet 1.7.1 on Gnome Web (Epiphany).

please release 1.7.2 with this fix

@AduchiMergen we are currently in the final phase before releasing 1.8.0. We think we release it with march

@Falke-Design The 1.8 is quite big update which includes breaking changes
IMHO bugfix should be released in a patch update 1.7.2

If 1.8.0 includes breaking changes, why isnt it a 2.0.0 release then??

It has very minor breaking changes that shouldn't affect most codebases, and NPM doesn't upgrade minor versions anyway. v2.0 will have more substantial changes like dropping support for old IE.

Any updates on ETA? I just spent 30 minutes debugging this only to stumble upon this issue after tracing it back to the Marker onClick 😅

@SpencerKaiser can you try leaflet@beta?

@mourner one of the devs on my team (@joswayski) is still looking into it but he did find this one breaking change... looks like the Ukrainian flag is misaligned 🤣 Want us to open new issue for that?

Also do yall have an eta on the next release? I'm fine using the beta build for now, but I'm worried about future releases breaking us, so I'm nervous about checking that in as permanent fix.

@SpencerKaiser yes, please make an issue or at least a minimal reproducible live test case! Seems like some kind of CSS conflict.

Also do yall have an eta on the next release? I'm fine using the beta build for now, but I'm worried about future releases breaking us, so I'm nervous about checking that in as permanent fix.

ETA a few days. Very likely at the end of the week.

~~The off topic police are going a bit overboard here 🙄 the comment directly above this one is directly related to this issue...~~

Edit: Thanks for making the one above on topic again 😉

👮

	// simulate click if the touch didn't move too much
	if (this._isTapValid()) {
	this._simulateEvent('click', first);
	}