Leaflet / Leaflet

πŸƒ JavaScript library for mobile-friendly interactive maps πŸ‡ΊπŸ‡¦
https://leafletjs.com
BSD 2-Clause "Simplified" License
40.28k stars 5.76k forks source link

Map 'dblclick' event not firing on Chrome #4127

Closed delner closed 8 years ago

delner commented 8 years ago

Using v1.0.0-beta.2 from the CDN, I'm having trouble getting dblclick events from the map to fire in Chrome.

Setup a map as follows:

this.activeLayer = L.tileLayer(tileHost + '/tiles/' + mapName + '/{z}/{x}/{y}.png');
this.map = L.map('map', {
  maxZoom: 7,
  doubleClickZoom: false,
  bounceAtZoomLimits: false,
  layers: [this.activeLayer]
});

Then binding 3 events to test with:

this.map.on('click', function (eventData) { console.log('map click!'); });
this.map.on('dblclick', function (eventData) { console.log('map double click!'); });
$("body").on('dblclick', function (eventData) { console.log('body double click!'); });

Double clicking on the map in:

Switching back to 0.7.7 circumvents the issue.

1.0.0beta2 seemed to work ok in Chrome before, not sure why it isn't working now. Is anyone else seeing similar behavior?

yohanboniface commented 8 years ago

Works for me on Chromium Version 47.0.2526.73 Ubuntu 15.10 (64-bit) and Chrome Version 47.0.2526.106 (64-bit) (Ubuntu).

Can you check here to be sure (latest master): http://playground-leaflet.rhcloud.com/coh/edit?html,console,output

IvanSanchez commented 8 years ago

Also works as expected in my Chromium 47.0.2526.80 on Debian.

delner commented 8 years ago

@yohanboniface Still no luck in Chrome 47.0.2526.106 64-bit on Ubuntu using the playground you linked: here are the results from one double click. Works fine in Firefox 39.0 on Ubuntu.

yohanboniface commented 8 years ago

Very weird. Any Chrome plugin that could explain?

delner commented 8 years ago

Not that I can think of: I run vanilla web browsers (no ad blockers, JS stuff, etc.) The only extensions I have installed are for Google Docs, and those come default, I think.

yohanboniface commented 8 years ago

Just for investigation, can you try with Chromium?

delner commented 8 years ago

I would love to, but I'm running 14.10, and they shut down all updates for it (preventing me from installing Chromium.) I can try on another device at some point. Can anyone else try the 106 version of Chrome I listed above, see if they see the same problem?

yohanboniface commented 8 years ago

Can anyone else try the 106 version of Chrome I listed above, see if they see the same problem?

Just to be sure that's clear: I've tested on 106 before and does not had the issue. But the more tests, the better :)

delner commented 8 years ago

Weeeeirrddd.....

hyperknot commented 8 years ago

Is it not possible that the problem is not the Chrome version, but some other config in the OS, like touchscreen or pointing device or some weird option, which is detected by Chrome + Leaflet 1.0?

delner commented 8 years ago

Potentially. I am running it on a MS Surface Pro 3 that has been dual booted into Ubuntu 14.10. (And I didn't see this on my desktop Chrome Ubuntu 14.04 before, although I'm not sure which version of Chrome I was running.)

However, I'm not sure if/how Ubuntu would care or know about that specific hardware. My user agent reflects as Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36.

delner commented 8 years ago

@hyperknot I think you're onto something here... certain websites are delivering me a mobile experience, so maybe it is the newest Chrome being smart about the device?

If this is the case, is there an equivalent event to the 'dblclick' for mobile I can listen to? Or a tap + hold event? It'd be worth seeing if it responds to those...

yohanboniface commented 8 years ago

dblclick should be sent on touch devices too. What is L.Browser.touch giving?

delner commented 8 years ago

L.Browser.touch yields true.

yohanboniface commented 8 years ago

Are you actually using a mouse or touble tapping?

kohenkatz commented 8 years ago

I am seeing the same problem with 1.0.0-beta.2 in the following browsers:

However, it works fine on the same computer in Firefox 45.0a2 and in Microsoft Edge.

Chrome, NW.js, and Edge all report L.Browser.touch === true, while only Firefox reports false, so it may not be related to that.

Confirmed this using the playground link by @yohanboniface.

kohenkatz commented 8 years ago

More testing on other machines:

On all of those machines and browsers, L.Browser.touch === false, so maybe the Microsoft Edge sample is a bad one and it really is related to that.


One other important note is that even though the original Windows 10 tests I posted in my last comment say that they are touch-enabled, the device is a Lenovo Thinkpad T530 without touch support.

IvanSanchez commented 8 years ago

The issue is reproducible in a Win10 virtual machine, but only when emulating a multi-touch input device in the VM's preferences.

IvanSanchez commented 8 years ago

Funny.

L.DomEvent.on(document.body, 'dblclick', fn) doesn't work, but document.body.addEventListener('dblclick', fn) does.

@yohanboniface Can you have a look at http://playground-leaflet.rhcloud.com/mup with a Win10 VM? You're the expert on all things event-related :-)

yohanboniface commented 8 years ago

@yohanboniface Can you have a look at http://playground-leaflet.rhcloud.com/mup with a Win10 VM? You're the expert on all things event-related :-)

But you are the IE expert around us, no? ;)

IvanSanchez commented 8 years ago

But you are the IE expert around us, no? ;)

@yohanboniface But this is a chrome issue, not a IE issue :-P

I've isolated the bug to DomEvent.DoubleTap.js, see #4131.

@delner @kohenkatz I do not have a physical computer with Win10 or desktop touchscreens, so I could really really use some help testing if that PR fixes the bug and doesn't break any other browser/pointer device combinations.

kohenkatz commented 8 years ago

I don't have a touchscreen either - it's a false positive - but that's a separate issue. (See Modernizr/Modernizr#880, among others)

kohenkatz commented 8 years ago

I have not tested exhaustively, but a few simple tests show that #4131 seems to work for me.

delner commented 8 years ago

@yohanboniface I was double-clicking with a mouse connected to my touchscreen device (MS Surface Pro 3)

@IvanSanchez I have a touchscreen that I can boot into either Windows or Ubuntu on. Is there something specific you'd like me to test for you?

IvanSanchez commented 8 years ago

@delner Try the code changes from #4131 in as many browsers as possible, see how double clicking with a mouse and double-tap on a touchscreen work.

kohenkatz commented 8 years ago

I just got access to my @yakatz's ThinkPad Twist (running Windows 10) to test with a touchscreen, and here are my results.

First, for the original code.

Now for the fix from #4131: (Tested in this playgroud)

IvanSanchez commented 8 years ago

@kohenkatz Thanks a lot for testing!!!

I don't care a lot about the order of the events, specially if applies to the container and window, as long as dblclicks are fired (we can fix that later if it's a big issue).

Right now I see two ways of fixing this:

@mourner @yohanboniface Any preference?

mourner commented 8 years ago

No preference, either is fine if it fixes all use cases. What's more reliable to detect?

IvanSanchez commented 8 years ago

MS Edge sends a UA string containing webkit, so doing something chrome-only will be unreliable. Gotta find a good way to detect the Edge rendering engine, and implement L.Browser.edge.

mourner commented 8 years ago

At least no other browser has the word "Edge" in the UA string.

IvanSanchez commented 8 years ago

At least no other browser has the word "Edge" in the UA string.

... for now.

IvanSanchez commented 8 years ago

OK, I've updated #4131, should prevent the duplicated dblclick events on Edge.

mourner commented 8 years ago

Can you guys @delner @kohenkatz confirm that the issue is fixed by #4131?

kohenkatz commented 8 years ago

I'm away from the office where I have access to a touchscreen, so will not be able to test until next week.

delner commented 8 years ago

I'll see if I can try this tomorrow.

IvanSanchez commented 8 years ago

@delner @kohenkatz Sorry to bother again - any updates on these dblclicks?

delner commented 8 years ago

@IvanSanchez So a novice question... but how would I get a copy of Leaflet from #4131?

IvanSanchez commented 8 years ago

@delner You git clone the main Leaflet repo, then switch to the branch regarding #4131, in this case git checkout chrome-win10-dblclick.

Do check out the github help, and remember you do not need to fork a repo to clone it. If you don't use command-line git, you'll need to do a bit of extra research.

delner commented 8 years ago

@IvanSanchez Right right... once I clone it down locally though, do I have to build the library? I think I'd need a JS file to embed in a <script> to test with it.

IvanSanchez commented 8 years ago

@delner You can build it (see https://github.com/Leaflet/Leaflet/blob/master/CONTRIBUTING.md#setting-up-the-build-system ) or open any example in the debug directory

yohanboniface commented 8 years ago

@delner can you try with this playground http://playground-leaflet.rhcloud.com/bag/edit?html,console,output it has the leaflet code from #4131 ?

delner commented 8 years ago

@IvanSanchez @yohanboniface Ok I did some testing from a compiled version of #4131 from the chrome-win10-dblclick branch

On a MS Surface Pro 3:

Windows:

Ubuntu:

Let me know if I can be of any more help.

IvanSanchez commented 8 years ago

@delner Thanks for testing!

I'm going to guess the Firefox bugs can be converted into upstream bugs so the Mozilla folks can handle them. Gotta make a few minimal test cases and borrow a Surface.

IvanSanchez commented 8 years ago

I've just personally tested @yohanboniface 's playground with a colleague's Surface Pro 4 using Win10 and Firefox nightly 47.0, and all interactions (touchscreen pinch-zoom, touchpad pinch-zoom, touchscreen double-tap, external mouse scrollwheel) work as expected. Yeeee-haw! :smile:

IvanSanchez commented 8 years ago

Closed via #4131

delner commented 8 years ago

@IvanSanchez Sweet, that's great to hear! I was testing using Firefox 43.0.

Thanks @IvanSanchez and @yohanboniface for fixing this!