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

Consider stopPropagation on Marker click when draggable is true #9624

Open
andrewharvey opened this issue Apr 24, 2020 · 8 comments
Open

Consider stopPropagation on Marker click when draggable is true #9624

andrewharvey opened this issue Apr 24, 2020 · 8 comments
Labels

Comments

@andrewharvey
Copy link
Collaborator

mapbox-gl-js version: 1.9.1

browser: any

playground at https://jsfiddle.net/g39b5n2c/

When a Marker is draggable, dragging the Marker by a few pixels or by zero pixels causes a map click event to fire (map click doesn't fire when dragging the marker by a larger distance).

While for static markers without any interaction, I think it's a reasonable default to let the click flow down to the map and let the developer choose to stop this when they like with

    marker.getElement().addEventListener('click', function (e) {
      e.stopPropagation();
    })

The question is when draggable is set, should we automatically stopPropagation on click to avoid a small drag also triggering a map click event, or should we leave as is and leave this to the developer?

@ryanhamley
Copy link
Contributor

Hmm, it shouldn't be possible to have a drag of zero pixels that triggers a click. The marker's _onMove handler sets pointerEvents to none to suppress click events and the internal state system requires at least one drag event to happen before dragend happens. If I add listeners for drag and dragstart to your example, I do see them firing before the dragend and click events fire. It looks like sometimes what feels like a click to us is triggering a single drag event. That makes it seem like we're treating very tiny drag events differently by firing the click event afterwards. I'd probably consider this a bug and say that it should be fixed.

The _onMove method is set as an event handler on mousemove and touchmove so the fix might need to be some sort of click tolerance.

@ChristopherChudzicki
Copy link
Contributor

ChristopherChudzicki commented Apr 27, 2020

The _onMove method is set as an event handler on mousemove and touchmove so the fix might need to be some sort of click tolerance.

Yes!

The map itself has a clickTolerance (i.e., drag threshold, default 3px) that controls whether a pointer movement on the map is a click or a drag. Markers seem to have 0px as their drag threshold though. (But do require at least 1 mouse/touch move event to fire.)

It would be great to add similar tolerances to the markers... do not drag and do not cancel events if movement is less than click tolerance.

Questions:

  • should markers have a separate click tolerance, or use the same tolerance as the map?
  • if markers have separate tolerance and marker.tolerance < map.tolerance, then small marker drag would still fire map clicks. And these map clicks are really hard to cancel b/c dragged markers have pointer-events: none, so they do not bubble up from the marker.

PS: I also made a fiddle to illustrate this before I realized this issue existed already https://jsfiddle.net/cchudzicki/q0bg7r18/2/

@andrewharvey
Copy link
Collaborator Author

@ChristopherChudzicki is that the same issue as this ticket? I'm not asking for a Marker drag tolerance, I still want 1px drags to work, just asking that a map click is not triggered for small drags or for 0 pixel drags (ie. marker clicks) where draggable is true. I found it was easy to workaround this as noted above.

The reason why this was a problem for me is the UI allows the marker to be set by clicking on the map, or dragging the marker, but due to this issue small drags triggered the map click and caused the marker to jump a few pixels off where the user expects it to be.

@ChristopherChudzicki
Copy link
Contributor

ChristopherChudzicki commented Apr 27, 2020

is that the same issue as this ticket?

@andrewharvey Fair question, maybe I will post it as a separate ticket.

I do think it is related because as @ryanhamley mentioned:

The marker's _onMove handler sets pointerEvents to none [on the marker] to suppress click

Consequently, tiny marker drags (below map drag threshold) fire click events that originate from the map, not from the marker. Which means they cannot be cancelled by marker event listens (stopPropagation, as you mention).

Edit: Although related, I do think these issues could be addressed

  • [this issue] always suppress map clicks when marker drag occurs (needs to be done on the map, not on the marker, if I understand the situation correctly).
  • [marker drag threshold] only fire marker drag events if pointer moves above a threshold

@andrewharvey
Copy link
Collaborator Author

Consequently, tiny marker drags (below map drag threshold) fire click events that originate from the map, not from the marker. Which means they cannot be cancelled by marker event listens (stopPropagation, as you mention).

Strange I was able to to stop the map click event for tiny drags on the marker with

    marker.getElement().addEventListener('click', function (e) {
      e.stopPropagation();
    })

@ChristopherChudzicki
Copy link
Contributor

@andrewharvey In both firefox and chrome, I see consecutive "marker dragend" and "map click" logs in the console of your fiddle. Make sure you drag events are really small, less than 3 px, otherwise the click will be suppressed by the map's default clickTolerance.

@andrewharvey
Copy link
Collaborator Author

@andrewharvey In both firefox and chrome, I see consecutive "marker dragend" and "map click" logs in the console of your fiddle. Make sure you drag events are really small, less than 3 px, otherwise the click will be suppressed by the map's default clickTolerance.

Oh sorry you're right, I was testing this by just clicking on the marker (0px drag) which without the stopPropagation was triggering a map click, so seems this workaround is only a partial workaround.

@ryanhamley
Copy link
Contributor

ryanhamley commented Apr 27, 2020

@ChristopherChudzicki I think you're correct in diagnosing these are two separate but related issues. If you're interested in opening a ticket for the click tolerance issue, that would be great. We'd also welcome a PR for either or both issues if you're interested.

should markers have a separate click tolerance, or use the same tolerance as the map?

I would vote for them having the same tolerance as the map by default and perhaps allow that to be configurable on the markers.

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

3 participants