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

Click event isn't fired on the original map #569

Closed
vkammerer opened this issue Aug 8, 2018 · 30 comments
Closed

Click event isn't fired on the original map #569

vkammerer opened this issue Aug 8, 2018 · 30 comments
Assignees

Comments

@vkammerer
Copy link

Since #565 landed in 3.3.4, the original map layer doesn't capture clicks anymore

const map = this.mapRef.getMap();
map.on('click', e => {
  console.log(e);
  // nothing gets logged
});

This makes it impossible to use certain features, such as handling clicks on clusters for example.

Is there a way to get the clicks to be passed to the original map layer?

@Pessimistress
Copy link
Collaborator

Why do you need to register your own event listener? Does the onClick prop not work?

@vkammerer
Copy link
Author

My use case is that I want to be able to handle clicks on clusters, but I guess there could be many others..

@Pessimistress
Copy link
Collaborator

This module is a React wrapper for mapbox-gl so you are encouraged to use the React API wherever possible. There is no guarantee for any mapbox-gl native examples to work.

In your use case, please use the onClick prop to access the features under the pointer.

@vkammerer
Copy link
Author

There is no guarantee for any mapbox-gl native examples to work

Ok wow, this is a very strong statement. Thank you for the clarification. I believe this will be a major blocker for many teams, as it means that once you opt in for this library, you cannot benefit from the original API.

This should definitely be stated at the top of the README to warn users.

@Pessimistress
Copy link
Collaborator

To be clear, here is how you can capture a click event and recreate that example:

_onClick(event) {
  // either
  const feature = event.features.find(f => f.layer.id === 'clusters');
  // or
  const point = [event.center.x, event.center.y];
  const feature = this.mapRef.queryRenderedFeatures(point, { layers: ['clusters'] })[0];

  if (feature) {
    // look up cluster expansion zoom
  }
}

render() {
  return <MapGL onClick={this._onClick} ... />;
}

this is a very strong statement.

Not really. Front and foremost, the goal of this library is to provide an interface that conforms with the React programming conventions. Similarly, you cannot go find a vanilla HTML/JavaScript code snippet and expect it to work as-is with React.

In this specific case, mapbox-gl's native event API does not support itself being used as a stateless component, forcing us to hijack its event handling system. It is indeed not an idea situation. We are in conversation with Mapbox but their ETA is unclear. I agree with you that the documentation can be more clear about breaking the native event handling, and I will make sure it is reflected in the docs.

I hope you can give the above solution a try.

@vkammerer
Copy link
Author

Thank you very much for your reply and the code sample. Your method will work well with my application and I'm happy to remove the quick hack I had implemented:

.overlays {
  pointer-events: none !important;
}

I certainly don't understand the whole rationale behind the decision to hijack Mapbox's events, but I am surprised by your argument on the lack of importance of ensuring that the Mapbox API is still fully operational..

From my experience working with another react layer on top of webgl (react-three-renderer), a complex application will use both APIs:

  1. the React layer for:
  • the many benefits of the declarative approach
  • the consistency with the rest of the application
  1. and also sometimes the original underlying imperative API for:
  • specific effects that the react lib has not implemented yet
  • performance micro optimizations
  • or simply to copy paste an example among the many provided by the original API's documentation (and Mapbox's documentation is great in that regard)

So in my opinion, while a user of this library should of course be encouraged to use the react layer as much as possible, it is also very important to ensure that the original mapbox gl API is fully functional.

But appart from this: thanks a lot for the example :) this will help me and was the solution I was looking for.

@Pessimistress
Copy link
Collaborator

I certainly don't understand the whole rationale behind the decision to hijack Mapbox's events

The primary reason is that Mapbox's move event is fired after it updates, while in the React context, any event-triggered rerender is propagated to all components in the next animation frame. This causes controls/overlays to be always one frame behind the base map during user interaction or viewport animation.

This would not be an issue if you use Mapbox's native controls. However, because of how Mapbox's controls API is designed (particularly the usage of innerHTML) we have to re-implement all control component in React instead of wrapping their native counterparts.

In short: you can't use any React controls or overlays with the base map, while using the native event handling. You are not the first or last person who wishes to leverage the native events more; and trust me it's not fun to maintain our own event handling system to parity. I have a pending request on the mapbox-gl-js repo.

@vkammerer
Copy link
Author

Thank you very much for the detailed reply. I am trying to stay within the library's boundaries so far and I haven't encountered any real blocker.

@vkammerer
Copy link
Author

Hi @Pessimistress, thank you again for the previous response.
I have followed your advice and used the onClick prop on InteractiveMap to handle events.

However, I have noticed that there is a 300ms delay between the moment a user clicks and the moment the onClick handler is actually called. I digged a bit in the library and understand that https://github.com/uber-web/mjolnir.js is used, and that the 300ms is necessary to handle a doubleClick event separately from the click event.

But is there still a way to get rid of the 300ms delay, and to cancel the upcoming doubleClick on certain conditions?

This would make my current map much more responsive than what it is today.

@vkammerer vkammerer reopened this Aug 21, 2018
@raja
Copy link

raja commented Aug 24, 2018

Also looking to remove the 300ms delay.

@Pessimistress
Copy link
Collaborator

@vkammerer @raja Do you wish to completely remove the double click to zoom functionality, or have the click event fired even when double clicking?

If the former, I could make a change so that if doubleClickZoom is set to false, the 300ms delay is removed.

@vkammerer
Copy link
Author

vkammerer commented Aug 24, 2018

@Pessimistress thank you for the reply.

In my opinion, it would be best if:

  • setting doubleClickZoom to false removed the 300ms delay indeed.
  • another click event handler was exposed, even when doubleClickZoom is set to true

The use case I have, and which I guess people willing to use this have, is that I want to be able to:

  • respond to a click on a point that contains one of the features I display on the map
  • still be able to benefit from the zoom when the double click happens outside of my points.

The way I see it, I would:

  • pass another handler to InteractiveMap, like onImmediateClick (sorry for the crappy name)
  • check in this handler if the event contains a feature, like:
onImmediateClick(event) {
  const feature = event.features.find(f => f.layer.id === 'clusters');
  if (!feature) return;
  // Otherwise do the things I want to do with a click on my feature
}

@Pessimistress Pessimistress self-assigned this Aug 30, 2018
@dabrowski-adam
Copy link

dabrowski-adam commented Sep 5, 2018

@Pessimistress

I have a relevant issue so I'm posting here—however reading the discussion has only made me more confused. 🙃

I'm on 3.3.4 and when I pass doubleClickZoom={false} to <ReactMapGL/> the click delay is removed. However, once I insert <NavigationControl/> it returns.

@Pessimistress
Copy link
Collaborator

@dabrowski-adam Good catch. That is a bug.

@Pessimistress
Copy link
Collaborator

@vkammerer In your use case, if the user double clicked on a feature, do they both select it AND zoom? Seems to me that you actually want a way to cancel the double click gesture if the first click is on a feature?

@vkammerer
Copy link
Author

@Pessimistress Yes indeed, that would be ideal.

But from my understanding reading the code, it seems like this would require much more effort than just adding another click handler. My hypothesis is that users who double click to zoom will mostly do it outside of pins / markers, so I could definitely with a partial first implementation.

But if you think it is possible (within your time :) ) to cancel a double click, then that's great. Maybe this handler should then return a boolean indicating if the next click should be a part of a double click?

onImmediateClick(event) {
  const feature = event.features.find(f => f.layer.id === 'clusters');
  if (!feature) return true;
  // Otherwise do the things I want to do with a click on my feature and then:
  return false;
}

@Pessimistress
Copy link
Collaborator

@vkammerer We use hammerjs' requireFailure in the current implementation. I can poke around and see if it's possible to drop it in the middle of a gesture.

@cyrilchapon
Copy link

What is the status of this as of february 2019 ?

We're using

"react-map-gl": "^4.0.10"

And we've been trying doubleClickZoom to false without success to remove the delay.
We don't have any overlay

@vkammerer
Copy link
Author

@Pessimistress it looks like a decision was made on mapbox side regarding these events to handle click and dblclick separately: mapbox/mapbox-gl-js#6524

Should the implementation in this repository follow the same spec? This would remove the 300ms delay, which currently degrades the user experience.

I have unfortunately resorted to reenable my previous hack to remove it for now:

.overlays {
  pointer-events: none !important;
}

@Pessimistress
Copy link
Collaborator

@cyrilchapon @vkammerer this seems to be a regression due to #626. I'll put up a patch with the following behavior:

  • No delay before onClick if doubleClickZoom: false and onDblClick is not specified

Regarding Mapbox's click behavior: changing onClick will likely break existing applications. I can add an additional callback that does this. Any suggestion on the prop name? onNativeClick?

@vkammerer
Copy link
Author

onNativeClick sounds ok to me!

@cyrilchapon
Copy link

Just thinking out loud..
Wouldn't it be interesting to expose this delay roughly as a prop.. ?

doubleClickTriggerDelay

@Pessimistress
Copy link
Collaborator

@cyrilchapon @vkammerer onClick fix and onNativeClick addition is published in 4.0.12.

We can potentially expose the double click delay but that will require more changes to the event manager.

@vkammerer
Copy link
Author

Thanks a lot @Pessimistress !

@cyrilchapon
Copy link

WoW thanks for reactivity ☺️
Test and feed-back incoming

@macobo
Copy link
Contributor

macobo commented May 7, 2019

Thanks for adding the native handler! I wonder if there's a more elegant solution here though.

@Pessimistress
Copy link
Collaborator

@macobo I tried to avoid making a breaking change. When we go to the next major release we'll definitely want to revisit the API design.

@PelumiWeb
Copy link

This is all good for react-native, but there is no onClick props for react-map-gl, please does anyone knows how to pass an event listener on the layers with react-map-gl.
or is it not possible to do so?

@ashnewport
Copy link

@PelumiWeb I just added a working solution to click a marker on a layer and display related information in a popup over at #1059 (comment).

@someError
Copy link

someError commented Dec 21, 2021

Thank you very much for your reply and the code sample. Your method will work well with my application and I'm happy to remove the quick hack I had implemented:

.overlays {
  pointer-events: none !important;
}

I certainly don't understand the whole rationale behind the decision to hijack Mapbox's events, but I am surprised by your argument on the lack of importance of ensuring that the Mapbox API is still fully operational..

From my experience working with another react layer on top of webgl (react-three-renderer), a complex application will use both APIs:

  1. the React layer for:
  • the many benefits of the declarative approach
  • the consistency with the rest of the application
  1. and also sometimes the original underlying imperative API for:
  • specific effects that the react lib has not implemented yet
  • performance micro optimizations
  • or simply to copy paste an example among the many provided by the original API's documentation (and Mapbox's documentation is great in that regard)

So in my opinion, while a user of this library should of course be encouraged to use the react layer as much as possible, it is also very important to ensure that the original mapbox gl API is fully functional.

But appart from this: thanks a lot for the example :) this will help me and was the solution I was looking for.

you saved my night dude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants