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

Fairly serious bug in the way marker click events are handled. #2619

Closed
ghost opened this issue Apr 9, 2014 · 13 comments
Closed

Fairly serious bug in the way marker click events are handled. #2619

ghost opened this issue Apr 9, 2014 · 13 comments
Labels
Milestone

Comments

@ghost
Copy link

ghost commented Apr 9, 2014

This is really noticeable on Android but easily replicable on desktop - both 0.7.2 and the latest 0.8.0 are affected. All you need is a map and a marker with a 'click' event.

Tap the marker (click handler will fire)
Move the map
Tap the marker (click handler will NOT fire)
Tap the map
Tap the marker (click handler will fire)

It's more noticeable on Android because tapping the map will often result in a 'drag' or 'move' - on desktop you have to actually drag the map. Android probably needs a threshold below which 'touches' are not read as 'moves' but that's a separate issue I think.

The issue is in line 3724 (0.7.2 src.js)
'''if ((!this.dragging || !this.dragging._enabled) && this._map.dragging && this._map.dragging.moved()) { return; }'''

this._map.dragging.moved() = 'true' because the map was moved!! I've no idea why it's checking that in the marker event - but the result is that it's easy to make markers 'unresponsive' - esp on Android

Note: I suspect this is what #2583 is talking about - but as the bug isn't specific to Android, I thought I'd create this one.

@mourner mourner added this to the 0.7.* milestone Apr 18, 2014
@fungiboletus
Copy link
Contributor

I have a similar issue with draggable markers and desktop browsers (Chrome, Firefox, IE).

  • Click on the marker : click event
  • Drag the map
  • Click on the marker : no click event
  • Click on the map
  • Click on the marker : click event

It works fine with not draggable markers. And in the L.Map._fireMouseEvent function, if I change this condition

if (!e._simulated && ((this.dragging && this.dragging.moved()) ||
                                  (this.boxZoom && this.boxZoom.moved()))) { return; }

to

if (!e._simulated && (this.boxZoom && this.boxZoom.moved())) { return; }

it works fine for the markers, but it must break a lot of other things.

@mourner
Copy link
Member

mourner commented Apr 28, 2014

@shrewdlogarithm does it happen only with draggable markers in your case too? I couldn't reproduce this with normal markers.

@mourner
Copy link
Member

mourner commented Apr 28, 2014

@yellowiscool reproduced your issue in master (works fine in stable though).

@ghost
Copy link
Author

ghost commented Apr 28, 2014

All my markers are draggable so it's not something I've checked yet - I have hacked Leaflet to work for now, when I next try the master/stable I'll double-check tho

@fungiboletus
Copy link
Contributor

Oh sorry. I forgot to mention the leaflet version when I rewrote my comment.

So yes I have the problem since I switched to the Leaflet's development version. I was thinking of a problem related to a work in progress, but I saw this issue which is very similar so I added my comment.

@mourner mourner modified the milestones: 1.0-beta, 0.7.* May 23, 2014
@friedbunny
Copy link

I have also encountered this issue with draggable markers not responding (e.g., firing popups or events) while static markers work as expected. Stable does not have this issue, while 0.8-dev does.

http://jsfiddle.net/friedbunny/6EKEG/

Reproduce by:

  1. Click on markers, see popups
  2. Pan map
  3. Click on right/draggable marker — no popup appears

Incidentally, clicking on the static marker will also re-enable draggable-marker's popup. Marker will still drag after pan, but nothing fires on click.

@AndriiHeonia
Copy link
Contributor

@mourner, please look at commit 93214d7. Do you remember, why you added this checking: (this.dragging && this.dragging.moved()) in Map.js -> _fireMouseEvent?

After first map dragging this.dragging.moved() ever will be true (according to the logic in Draggable.js) and obj.fire('preclick') won't be fired.

Without this checking this bug doesn't reproduce. May be we can leave only this condition:

if (!e._simulated && (this.boxZoom && this.boxZoom.moved())) { return; }

?

@mourner
Copy link
Member

mourner commented Aug 2, 2014

@AndreyGeonya it wasn't added in this commit, it was like this all the time — if you look closely, it's only reorganized a bit. The issue should be elsewhere.

@AndriiHeonia
Copy link
Contributor

@mourner It seems that it has been added here: 4984035a#diff-607b516b5121104eef3007cb5727e69cR299 It was 4 years ago :) I think we need to test drag and click events (and related functionality) without this checking and merge it to the master if it doesn't break any functionality...

@mourner
Copy link
Member

mourner commented Aug 4, 2014

The point is not to register dragging a map or a marker as click, which is required.

@AndriiHeonia
Copy link
Contributor

Understood. Fixed it another way. Look at pull request above.

@jordandh
Copy link

Is this fix in any of the released versions or is it expected to be in 1.0? (I am experiencing it on 0.7.2)

@mourner
Copy link
Member

mourner commented Jan 31, 2015

Not sure. If you experience it in 0.7.3, then it's expected to be fixed in 1.0.

eemeyer pushed a commit to t11e/Leaflet that referenced this issue Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants