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

Contextmenu is not triggered on iOS13 #6817

Closed ginger-777 closed 4 years ago

ginger-777 commented 4 years ago

After iOS was updated to 13 version long press doesn't work and contextmenu event is not triggered.

sveine commented 4 years ago

double-tap to fire doubleClickZoom won't work either in iOS 13. A fix for contextmenu is much more important to solve.

ilblog commented 4 years ago

I do confirm the same issue. After initial examination is seems that iOS 13 Safari does not fire touch events properly.

Long tap in iOS Safari does not send any event to Leaflet library and is handled by iOS itself.

Double click emits following events: mouseover, mousemove, mousedown, mouseup, click, which results in leaflet's event click.

It seems to me like serious issue in mobile Safari iOS 13 and I guess it has something to do to make mobile Safari very similar to desktop Safari. Unless touch events are sent properly to Leaflet I am afraid there is not much to be fixed.

Is here anyone that will report this to Apple?

ginger-777 commented 4 years ago

Last week I send a report to Apple support. There is still no answer.

ginger-777 commented 4 years ago

I have a little bit more information. If add click handler on map div and zoom control is enabled, then the long-press started work as copy/paste context menu. like that

<div id="mapid" onclick="javascript:void(0)"/>

 -webkit-user-select: none;
 -webkit-touch-callout: none;

also doesn't help, only disable copy/paste menu

Maybe it'd be useful for someone.

ginger-777 commented 4 years ago

As a temporary solution for long-press: add this library https://www.cssscript.com/handle-long-press-tap-event-in-javascript-long-press-js/ and in css -webkit-user-select: none; -webkit-touch-callout: none;

than add listener for long-press event. And here in event handler convert coordinates to leaflet coordinates.

ilblog commented 4 years ago

Does the library works? So far I have checked source code and the library listens to touch events that are not sent down to Safari anyway. Just curious.

ginger-777 commented 4 years ago

Yes, it works. For long-press at least. I have checked it on iOS13 and iPadOS13

ilblog commented 4 years ago

great tip thx

ilblog commented 4 years ago

This is final solution as we use it. It detects long tap and emulate 'contextmenu' MenuEvent. Just place it in your JS codes and you should be good.

Note: It is good idea to limit execution just for iOS 13 users, not to have too much listeners

// Copied and modified from https://github.com/john-doherty/long-press-event/

import rs from 'rootScope'
import $ from '$'

if( rs.platform === 'ios' && / OS 13_/.test( navigator.userAgent ) ) {

    // This is our main map el
    const mapEl = $('#map-container')

    // This class adds  -webkit-user-select: none; -webkit-touch-callout: none;
    // to the BODY el
    document.body.classList.add('ios13')

    let timer = null;

    const fireLongPressEvent = originalEvent => {

        clearLongPressTimer();

        const el = originalEvent.target
        , x = originalEvent.touches[0].clientX
        , y = originalEvent.touches[0].clientY

        // This will emulate contextmenu mouse event
        const event = new MouseEvent('contextmenu', {
                                    bubbles: true,
                                    cancelable: true,
                                    clientX: x,
                                    clientY: y
                                })

        // fire the long-press event
        const suppressClickEvent = el.dispatchEvent.call(el,event);

        if (suppressClickEvent) {

            // temporarily intercept and clear the next click
            mapEl.addEventListener('touchend', function clearMouseUp(e) {
                mapEl.removeEventListener('touchend', clearMouseUp, true);
                cancelEvent(e);
            }, true);
        }
    }

    const startLongPressTimer = e => {
        clearLongPressTimer(e);
        timer = setTimeout( fireLongPressEvent.bind(null,e), 1000 );
    }

    const clearLongPressTimer = () => {
        if(timer) {
            clearTimeout(timer);
            timer = null;
        }
    }

    const cancelEvent = e => {
        e.stopImmediatePropagation();
        e.preventDefault();
        e.stopPropagation();
    }

    // hook events that clear a pending long press event
    mapEl.addEventListener('touchcancel', clearLongPressTimer, true);
    mapEl.addEventListener('touchend', clearLongPressTimer, true);
    mapEl.addEventListener('touchmove', clearLongPressTimer, true);

    // hook events that can trigger a long press event
    mapEl.addEventListener('touchstart', startLongPressTimer, true); // <- start

}
felixhageloh commented 4 years ago

came across this thread while looking into a similar issue. I found that calling preventDefault on touchstart will allow you to receive touch events as usual and thus check for double tap as before.

I believe this comes down to the fact that mobile Safari changed the way it is simulating hover events (to make websites work that solely rely on hover events to open menus and such)

ilblog commented 4 years ago

came across this thread while looking into a similar issue. I found that calling preventDefault on touchstart will allow you to receive touch events as usual and thus check for double tap as before.

I believe this comes down to the fact that mobile Safari changed the way it is simulating hover events (to make websites work that solely rely on hover events to open menus and such)

So far works opposite for me, just suppress all the clicks except for long tap. Can you share piece of code that works for you?

felixhageloh commented 4 years ago

sorry I was assuming you were using raw touch events to detect double taps. If you are relying on click/mouse events then preventDefault on touchstart will prevent those.

Either way, it appears to have been a bug in iOS which was fixed in 13.1.

majid701 commented 4 years ago

@felixhageloh I just tested on iOS 13.1 running this demo: http://aratcliffe.github.io/Leaflet.contextmenu/examples/index.html

But it doesn't seem like the issue has been fixed! Is there something I'm missing out? Really critical issue for us since we heavily rely on the long press event to work, any help will be appreciated.

graouts commented 4 years ago

This is issue is referenced from WebKit bug #202143. The WebKit team (in this case, me) is investigating.

ilblog commented 4 years ago

This is issue is referenced from WebKit bug #202143. The WebKit team (in this case, me) is investigating.

We are very happy you are diving into this issue. As a web developers we would be happy the Safari on iOS 13.x would behave the same way as on iOS 12.x or Chrome on Android in case of touch events.

Happy you are here.

felixhageloh commented 4 years ago

@majid701 sorry if I caused some confusion. The issue I was seeing in 13.0 was: double tap (quickly) -> a single touchstart and touchend was fired.

Since 13.1 I am correctly receiving two touchstart and touchend events again. The app I am maintaining uses raw touch events to detect double taps, so this fixed double tap zoom for us. I was incorrectly assuming this would be the case for you as well.

graouts commented 4 years ago

if( rs.platform === 'ios' && / OS 13_/.test( navigator.userAgent ) )

Note @ilblog that this isn't going to work on iPadOS where the user-agent string is the same as Safari on desktop and won't advertise iOS.

graouts commented 4 years ago

(reposting from the WebKit bug so people Cc'd to this bug alone can see this.)

The contextmenu Leaflet event is not triggered because the Leaflet code uses pointerdown and touchstart events interchangeably and the availability of the former excludes the other. In Map.Tap.js the _onDown method has this check if (!e.touches) { return; } which will always lead to an early return since a PointerEvent event is not the same as a TouchEvent and does not have a touches property.

So the lack of a Leaflet contextmenu event being triggered is a Leaflet issue specifically.

graouts commented 4 years ago

Actually, this code isn't even run with Pointer Events because of this code:

if (Browser.touch && !Browser.pointer) {
    Map.addInitHook('addHandler', 'tap', Tap);
}

On iOS 13 bothBrowser.touch and Browser.pointer are true so the whole package isn't initialized.

graouts commented 4 years ago

Last week I send a report to Apple support. There is still no answer.

@ginger-777 Do you have a bug number / feedback ID? Generally though the best way to get the attention of WebKit developers is to file a bug directly at bugs.webkit.org.

majid701 commented 4 years ago

@graouts Yes i noticed that as well after reading your last comment. But I tested out disabling Pointer Events from Settings -> General -> Experimental Features and it looked like that it works on this demo website: http://aratcliffe.github.io/Leaflet.contextmenu/examples/index.html

But it doesn't work inside my app. I am loading leaflet inside a Webview.

graouts commented 4 years ago

As for double-tap-to-zoom, that is also an issue with Leaflet. Consider this code in DomEvent.DoubleTap:

function onTouchStart(e) {
    var count;

    if (Browser.pointer) {
        if ((!Browser.edge) || e.pointerType === 'mouse') { return; }
        count = _pointersCount;
    } else {
        count = e.touches.length;
    }

    if (count > 1) { return; }

    var now = Date.now(),
        delta = now - (last || now);

    touch = e.touches ? e.touches[0] : e;
    doubleTap = (delta > 0 && delta <= delay);
    last = now;
}

If a browser supports Pointer Events and it is not Edge then the function exits early. This is the case of Safari or any WebKit-based browser on iOS 13.

As far as I can tell, both issues reported here are Leaflet simply not being ready for a browser that supports both Touch Events and Pointer Events (long press and the contextmenu event) or supports Pointer Events but is not Microsoft Edge (double tap and the dblclick event).

I closed the WebKit bug as there is nothing for us to fix at this stage.

graouts commented 4 years ago

@graouts Yes i noticed that as well after reading your last comment. But I tested out disabling Pointer Events from Settings -> General -> Experimental Features and it looked like that it works on this demo website: http://aratcliffe.github.io/Leaflet.contextmenu/examples/index.html

But it doesn't work inside my app. I am loading leaflet inside a Webview.

Yes @majid701, I think this setting only applies to Safari, not WKWebView or any other way to embed WebKit on iOS. I'm not sure how you can disable Pointer Events outside of the Safari app.

ilblog commented 4 years ago

I think the solution will be to fix Leaflet library which detects iOS 13 Safari as "pinterType" browser and thus does not handle Touch events not correctly.

Thanks a lot @graouts for your effort and a lot of help in this matter.

ilblog commented 4 years ago

There is PR just in motion https://github.com/Leaflet/Leaflet/pull/6815 concerning double tap on mobile devices, patching same lines of code, so I have asked them to cover this issue also.

ilblog commented 4 years ago

if( rs.platform === 'ios' && / OS 13_/.test( navigator.userAgent ) )

Note @ilblog that this isn't going to work on iPadOS where the user-agent string is the same as Safari on desktop and won't advertise iOS.

Well this is is interesting, since it can trigger another series of issues, when developers are unable to distinguish macOS vs iPad. This is not related of this Leaflet bug but I think it should be addressed also by Apple.

https://forums.developer.apple.com/thread/119186

ilblog commented 4 years ago

SOLVED

Thanks to valuable input from Apple I have managed to fix iOS 13 issue at least on iPhone. Since iPad on iOS13 has the same user agent string as macOS, I was unable to fix the issue on iPad.

Just modify src/core/Browser.js like this:

// 'false' for all iOS devices, that (as of version iOS13 support both, touch and pointer events)
// Unfortunately as of iOS13 it is not possible to distinguish iPad from OS X by user agent string
export var pointer = !!(window.PointerEvent || msPointer) && !userAgentContains('iphone');
ilblog commented 4 years ago

Pull request with the fix is here

https://github.com/Leaflet/Leaflet/pull/6827

graouts commented 4 years ago

if( rs.platform === 'ios' && / OS 13_/.test( navigator.userAgent ) )

Note @ilblog that this isn't going to work on iPadOS where the user-agent string is the same as Safari on desktop and won't advertise iOS.

Well this is is interesting, since it can trigger another series of issues, when developers are unable to distinguish macOS vs iPad. This is not related of this Leaflet bug but I think it should be addressed also by Apple.

Generally, making decisions based on the user-agent string is not the optimal approach. Feature detection on a case-by-case basis suits the vast majority of use cases. Additionally, Safari on iPadOS may use a different user-agent string depending on the device class (iPad mini vs. other larger-screen iPad models) or depending on the size of the window in multi-tasking where Safari may use a third of the screen, or even depending on the user preference (request mobile/desktop website).

If there are cases where feature detection is not suitable, please raise a bug on bugs.webkit.org and the WebKit team will be able to determine the best solution.

gonzalolc commented 4 years ago

Any updates on how to solve this issue for iPadOS 13? I'm using mousedown/mouseup events.

filcab commented 4 years ago

Hi all,

I tried a better solution than just sniffing the UA and fudging it, as it should be more stable. https://github.com/Leaflet/Leaflet/pull/6855 It's still sniffing the UA, but just so I don't change code for platforms I can't test. The edge condition seems to be there for bailing out due to possible dblclick events being triggered, or something? It looks to me (I'm not well-versed in JS, though) that the Browser.pointer feature test for Pointer Events should be the only test we need in there, and other tests should be narrow and very specific, instead of "if not Edge, bail out" (missing a "why?"), it looks like it should be "if Edge, do something different" (with a reason in comments).

I'm still having an issue with iOS13 on an iPad, though.

@graouts: Re: https://github.com/Leaflet/Leaflet/issues/6817#issuecomment-536581361 and https://github.com/Leaflet/Leaflet/issues/6817#issuecomment-536587962 : I'm trying to make this work on iPad (fixed it on iPhone with the PR above), but there is no triggering of a second pointerdown if I do a double-tap. I've sent breakpoints, added log() calls, and I can't see any being triggered. It looks like the iPad is omitting the second tap in some situations. I can make it zoom with a "triple/quadruple click", but not with a double-click. logs on the global handlers (which are a compatibility layer for things requesting touchstart/touchend) don't trigger on double-taps. Any idea on what the iPad (with iOS13) could be doing to omit those second taps? Threshold is 250ms, but I can't get the second tap to register if it's faster than ~320ms. I do have <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no" /> in my header, as mentioned in the "Leaflet for mobile" page.

filcab commented 4 years ago

@graouts (sorry, not picking on you, just asking you because you mentioned being from the WebKit team): If you try the following event listener test page: https://patrickhlauke.github.io/touch/tests/event-listener.html

You can see the difference between iOS13 on iPhone and iPad. on the iPhone, I can double-tap the button and see two sequences of pointerdown+pointerup. On the iPad, I can't trigger this sequence with a double tap. It hold events for a while.

Thank you, Filipe

graouts commented 4 years ago

@filcab Please file a bug on bugs.webkit.org and I will take a look! That said, I believe this issue has been fixed in iOS 13.2 for which developer seed builds are already available on developer.apple.com. But always file a webkit.org bug for the team to take a look.

filcab commented 4 years ago

@filcab Please file a bug on bugs.webkit.org and I will take a look! That said, I believe this issue has been fixed in iOS 13.2 for which developer seed builds are already available on developer.apple.com. But always file a webkit.org bug for the team to take a look.

I've filed https://bugs.webkit.org/show_bug.cgi?id=203031

mourner commented 4 years ago

Hey everyone, thanks a lot for all the valuable input β€” we're looking into this ASAP. I'm on vacation so won't be able to code, but pinged some other maintainers to prioritize.

Quickly skimming through the discussion, it looks like the source of the issue is iOS13 supports the pointer events spec, so Leaflet partially takes code paths that we optimized specifically for legacy IE mobile browsers where the usual touch events weren't available.

Agent string sniffing like in #6827 isn't very reliable (can break at any moment); fixing pointer events like in #6855 might be better but still very risky considering we never intended for pointer code paths to work in modern browsers when we wrote that code; maybe we should try disabling pointer events completely if we detected earlier that regular touch events work properly (so, something like export var pointer = !webkit && !!(window.PointerEvent || msPointer))?

filcab commented 4 years ago

Hey everyone, thanks a lot for all the valuable input β€” we're looking into this ASAP. I'm on vacation so won't be able to code, but pinged some other maintainers to prioritize.

Quickly skimming through the discussion, it looks like the source of the issue is iOS13 supports the pointer events spec, so Leaflet partially takes code paths that we optimized specifically for legacy IE mobile browsers where the usual touch events weren't available.

Agent string sniffing like in #6827 isn't very reliable (can break at any moment); fixing pointer events like in #6855 might be better but still very risky considering we never intended for pointer code paths to work in modern browsers when we wrote that code; maybe we should try disabling pointer events completely if we detected earlier that regular touch events work properly (so, something like export var pointer = !webkit && !!(window.PointerEvent || msPointer))?

That sounds good, especially if the pointer events' code isn't as recent as I expected. I'll update my PR with your proposed patch (just tested on iPad and iPhone with iOS 13 and it works on both).

Thank you, Filipe

graouts commented 4 years ago

From a standards point of view, using Pointer Events (where available) rather than touch events is preferable as it is a W3C standard and not a vendor-specific technology.

From a technical point of view, at least for the iOS implementation, there are performance implications. Touch events are always dispatched synchronously, so any JS work performed as a result of handling them will be blocking and may yield inferior responsiveness. Pointer Events were designed in such a way that they can be implemented asynchronously, and on iOS they are.

Two things worth considering, maybe not for this particular issue, but worth keeping in mind for the future.

eliboni commented 4 years ago

could it be that fix #6855 does not work on markers? I'm getting reports that a long tap on a marker does not trigger contextmenu but the share/copy OS menu appears...

filcab commented 4 years ago

Hi @eliboni, It seems you need to use -webkit-touch-callout: none; (or similar... I'm bad at this πŸ˜…) on the elements. Unsure on which, though. Adding * { -webkit-touch-callout: none; } worked on my test (contextmenu event triggered on a marker, using an iPhone). Adjust as you need.

Bitxenio commented 4 years ago

Hi, I don't be able to work the double tap zoom with Leaflet.js. I just read this post, but I not found the solution. Can any one help me please?

Best Regards Kandy.

Bitxenio commented 4 years ago

Finally I found the solution:

We need to update to leaflet 1.5 and enable de Tap on the:

new L.map("mapid",{ attributionControl: false, zoomControl: false, tap: true });

and works fine.

Best Regards

rwillett commented 4 years ago

@bitxenio,

I'm afraid it doesn't work for me using 1.5.1

I'm back to writing my own handler for now as IOS 13.2 also doesn't seem to work either :(

filcab commented 4 years ago

@rwillett, @Bitxenio : What code are you using? Both of you mention specific versions of leaflet. The fix for PR#6855, committed in October, isn't in the latest release, which was back in May.

Are you using master?

Thank you, Filipe

rwillett commented 4 years ago

Ah! We used the official release. We didn't realise that PR#6855 wasn't included. Our fault.

We downloaded the dev branch and we'll try that,

Thanks

Rob

rwillett commented 4 years ago

We downloaded 1.6-dev and it works fine. Both the double tap and the context-menu on ios 13.2.

Thanks for the help, appreciate it.

Rob

cherniavskii commented 4 years ago

Released in 1.6.0

johnd0e commented 4 years ago

maybe we should try disabling pointer events completely if we detected earlier that regular touch events work properly (so, something like export var pointer = !webkit && !!(window.PointerEvent || msPointer))?

@mourner That was risky indeed, and caused issues like #6896. Please join discussion at https://github.com/Leaflet/Leaflet/issues/6977#issuecomment-577638632