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
Another blind attemp to work around dblclicks on Edge #5268
Conversation
@@ -17,9 +17,7 @@ L.extend(L.DomEvent, { | |||
var count; | |||
|
|||
if (L.Browser.pointer) { | |||
if ((!L.Browser.edge) || e.pointerType === 'mouse') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code won't run on anything but Edge anyway (Chrome+pointer listens only for native dblclick
s), so the extra check is not needed.
@@ -38,6 +36,8 @@ L.extend(L.DomEvent, { | |||
function onTouchEnd() { | |||
if (doubleTap && !touch.cancelBubble) { | |||
if (L.Browser.pointer) { | |||
if (e.pointerType === 'mouse') { return; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why #5180 was firing dblclick
s on mouse after a touchscreen double-tap: the doubleTap
flag is set after a double-tap.
obj.addEventListener('dblclick', handler, false); | ||
} | ||
// Edge 14 also fires native dblclicks, but only for pointerType mouse, see #5180. | ||
obj.addEventListener('dblclick', handler, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the strategy for Edge: always listen for native dblblick
s, but ignore the pointerType
mouse later.
@@ -77,7 +77,10 @@ L.DomEvent = { | |||
if (L.Browser.pointer && type.indexOf('touch') === 0) { | |||
this.addPointerListener(obj, type, handler, id); | |||
|
|||
} else if (L.Browser.touch && (type === 'dblclick') && this.addDoubleTapListener) { | |||
} else if (L.Browser.touch && (type === 'dblclick') && this.addDoubleTapListener && | |||
!(L.Browser.pointer && L.Browser.chrome) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes chrome>55 ignore the addDoubleTapListener hack and rely on native dblclick
s only.
Looks to be working as expected now 👍 |
Seems to be working as expected on:
|
Leaflet 1.0.2+ad27106 |
9680b33
to
300a442
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I tested it with touch in Win10 on Chrome55, IE11 and Edge 14.
* Prevent Browser.chrome = true for pre-Chromium Edge Sample user agent string: "Mozilla/5.0 (Windows NT 10.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36 Edge/18.17763" * fixup! Another blind attemp to work around dblclicks on Edge (#5268) Fix leftover that was meant to change in 2b5d401
With a bit of luck this will fix #5180.
I've copy-pasted this patch into a playground here: https://playground-leaflet.rhcloud.com/kunur/1/edit?html,output - will appreciate testing. cc @perliedman @hyperknot @davetimmins timlohnes
There is also #5267, with a different approach