Skip to content
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

Leaflet Map Marker Tap prevented by preventGhostClick() #1137

Closed
jboothe opened this issue Apr 15, 2014 · 25 comments
Closed

Leaflet Map Marker Tap prevented by preventGhostClick() #1137

jboothe opened this issue Apr 15, 2014 · 25 comments
Assignees
Labels
needs: reply the issue needs a response from the user

Comments

@jboothe
Copy link

jboothe commented Apr 15, 2014

Leaflet map markers do not respond to a tap which prevents opening/closing marker popups. preventGhostClick(e) method was sending back a false positive. Specifically, isScrolledSinceStart(e) was returning true on a tap on the map marker. Hijacking this method when the map was active was the only short term solution.

We are using side menu in our app which may helpful info.

@rvanbaalen
Copy link

+1

I'm experiencing the same issue with the nightly. It was all good in beta1 but there have been some significant changes / improvements ever since.

I didn't mention it as a bug yet since I wasn't fully able to create a reproducable example. Are you, @jboothe ?

I too have a sidemenu setup. Even worse: A tabs view inside the side menu content view and one of the tabs has a (full screen) map.

@jboothe
Copy link
Author

jboothe commented Apr 15, 2014

@rvanbaalen, due to a 3 week deadline, we had to roll with 0.9.27. Beta1.0 was released a few days before our launch so we have not fully tested the app with beta1.0. We did, however, do a super quick smoke test by sliding ionic.js beta1.0 into the app and we experienced the same results.

Now that we've recovered from 3 weeks of sleep deprivation, our next step is to complete a full upgrade of our app to beta1.0 and test again. I'll report back with the results. I have not had smidgin of time to put together a plunker, but thought I'd bring this issue to the boys' attention sooner than later.

@rvanbaalen
Copy link

You already did a great job filtering the issue out to a couple of methods. That gives me confidence to upgrade to the nightly and implement a short term workaround myself.

@rvanbaalen
Copy link

@jboothe Could you perhaps share the short term fix you've implemented to temporarily fix this issue in your project?

@jboothe
Copy link
Author

jboothe commented Apr 15, 2014

WARNING: This is a very ugly hack - like lipstick on a pig, kind-of-ugly. Preferably, I would prototype the preventGhostClick() method or better yet, the isScrolledSinceStart() method so that future Ionic updates would not overwrite my fix (baring method renaming or other refactorings).

First, in the stateProvider we dirtied up the global scope by setting a viewingSnfMap flag in the onEnter and onExit handlers like so:

.state('eventmenu.map', {
    url: '/map',
    views: {
        'menuContent': {
            templateUrl: 'templates/map.html',
            controller: 'MapController'
        }
    },
    onEnter: function(){
        window.viewingSnfMap = true;
    },
    onExit: function(){
        window.viewingSnfMap = false;
    }
})

Second, we throw a return into a conditional block around line 2415'ish in ionic.js

function preventGhostClick(e) {
    void 0;

    // BEGIN ugly Hack -------- //
    if(window.viewingSnfMap){
        return;
    }
    // END ugly Hack -------- //

    if(e.target.control || isRecentTap(e) || isScrolledSinceStart(e)) {

      return stopEvent(e);
    }
    recordCoordinates(e);
  }

Interesting how lack of time affects tolerance thresholds - any solution that worked at 3am the day before the launch was damn good looking code. :)

@rvanbaalen
Copy link

Much appreciated. As it turns out, I seem to like lipstick on pigs ;-)

@ajoslin Is this issue addressed already in the nightly build or beta.2?

@ajoslin
Copy link
Contributor

ajoslin commented Apr 16, 2014

Not yet! It's in progress, we are on the last tiny bit of the tap detection refactor.

Just sooo many edge cases to cover.

Right now we're all intensively working on tap, keyboard fixes, and virtual scroll - the first two are nearing done.

@rvanbaalen
Copy link

Great news! As you might have noticed, I can't wait to start using it. High five ✋

@ajoslin
Copy link
Contributor

ajoslin commented Apr 16, 2014

💯

@jboothe
Copy link
Author

jboothe commented Apr 16, 2014

@ajoslin as much as I'd like to get rid of my map-tap-hack, virtual scrolling ranks higher by orders of magnitude IMO. You guys rock! Keep up the great work.

@rvanbaalen
Copy link

@jboothe Your hack works like a charm except for (fast) Android devices. In my case a Galaxy S4. This one triggers double clicks now. When I click a marker popup on my map, a modal opens with more information. Now that modal opens immediately due to the fact that I stopped the preventGhostClick to do it's work.

@rvanbaalen
Copy link

@ajoslin Great job on the #tap-keyboard branch so far! I've just managed to install it in my current app using Bower and it seems that none of the above hacks is necessary anymore!

@rvanbaalen
Copy link

@jboothe I think this issue can be closed in general; it's not reproducible in the latest nightly.

@jboothe
Copy link
Author

jboothe commented Apr 17, 2014

Sounds promising. I'll give it a shot with my specific app this weekend and let et al know.

@adamdbradley
Copy link
Contributor

So the more I dove into this the more I realized that 3rd party libraries are going to do whatever they want to do with events. So as you can imagine with something like Leaflet or Google maps, their code has a plenty of event handlers for touchstart, touchend, mousedown, mouseup, and click. I wasn't able get leaflet to work in all scenarios, for all platforms, but again that's largely due to whatever they're doing with the events, like stopPropagation and preventDefault.

Long story short, the latest tap code also checks to see if the tapped element, or one of its ancestors, has the data-tap-disabled attribute:
https://github.com/driftyco/ionic/blob/master/js/utils/tap.js#L208

So if you put wrapped your map with something like this it should completely ignore the tap system and use native clicking:

<div data-tap-disabled="true" >
  <div id="map"></leaflet>
</div>

Please give it a shot and let me know how its working, thanks.

@rvanbaalen
Copy link

@adamdbradley I'm testing this at the moment I'll keep you posted. Looks promising and a very robust solution for future 3rd party implementations.

@rvanbaalen
Copy link

@adamdbradley Unfortunately I have to retract my last comment. When I'm using data-tap-disabled='true' as suggested, I have the 'double tap' issue again on a Galaxy S4 Android 4.3.

When I click one of my markers, it opens the popup and immediately opens an info balloon (which should open when you click the markers' popup).

It seems as if the issue was re-introduced somehow.

@jboothe
Copy link
Author

jboothe commented Apr 22, 2014

Very nice, @adamdbradley. I'll give it a shot and let you know.

@gordol
Copy link

gordol commented Apr 24, 2014

Huzzah! data-tap-disabled="true" did the trick for me.

Like so:

<leaflet data-tap-disabled="true" center="center" markers="markers" height="100%" width="100%"></leaflet>

This latest nightly build is sooooooo much better, radios and checkboxes are so responsive now, it feels almost native in comparison to how it was before. :D

Great work all!

@adamdbradley
Copy link
Contributor

@rvanbaalen could you provide a codepen replicating the issue?

@jtomaszewski
Copy link

Yeah, wrapping the leaflet element with data-tap-disabled="true" makes it work perfectly =)

There are a lot of small useful attributes, classes in Ionic like this or f.e. disable-pointer-events. Shouldn't we make a small doc page for them or something?

@aaronksaunders
Copy link

+1 yes

@coomsie
Copy link

coomsie commented May 5, 2014

Gidday ......oh yes ........ this saves me ... thanks guys. (I can stick my hair back on :p )

@dam1
Copy link

dam1 commented May 26, 2014

I use ionic for a mobile website ( http://m.gymbirds.com/#/event/map )
data-tap-disabled="true" almost solved the problem, it works on desktop and tablet ( nexus 7) but not on my phone ( xperia t )

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 6, 2018

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: reply the issue needs a response from the user
Projects
None yet
Development

No branches or pull requests

9 participants