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

[dankarran]

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

[IvanSanchez]

Minimal example reproducing the issue

For the sake of clarity: that example is using

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

[IvanSanchez]

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?).

[IvanSanchez]

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?

[dankarran]

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]

@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.

[johnd0e]

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

[IvanSanchez]

* 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 :-)

[johnd0e]

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).

[systemed]

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.

[IvanSanchez]

@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.

[systemed]

Spot on - that works fine. 👍

[johnd0e]

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.

[dankarran]

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.

[dankarran]

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.

[johnd0e]

@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.

[dankarran]

@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)?

[dankarran]

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?

[dankarran]

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);
	}

[alex-kowalczyk]

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

[AduchiMergen]

please release 1.7.2 with this fix

[Falke-Design]

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

[piotr-cz]

@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

[NicoHood]

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

[mourner]

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.

[SpencerKaiser]

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 😅

[mourner]

@SpencerKaiser can you try leaflet@beta?

[mourner]

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.

[SpencerKaiser]

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 😉

[Malvoz]

👮