search

perliedman / leaflet-routing-machine

Control for routing in Leaflet
https://www.liedman.net/leaflet-routing-machine/
Other
1.06k stars 347 forks source link

The mouse click for canceling a waypoint propagates to the map level and places a new waypoint #684

Open jude opened 1 year ago

jude commented 1 year ago

I bumped some node_modules so osrm-frontend would work with vector tiles using https://github.com/maplibre/maplibre-gl-leaflet.

Afterwards, clicking on the waypoint, the closeButton would remove the waypoint, but also propagate down to the map level and end up placing a new waypoint at the location the happened to be under the closeButton.

It may be a while before I have the time to submit a PR, but changing the block at https://github.com/perliedman/leaflet-routing-machine/blob/master/src/geocoder-element.js#L82 to the block below stopped the click propagation for me.

if (closeButton) {
    L.DomEvent.on(closeButton, 'click', function(event) {
            this.fire('delete', { waypoint: this._waypoint });
                        event.stopPropagation();
    }, this);

I didn't do any work to see if it was maplibre-gl-leaflet or one of the other module upgrades that introduced this behavior.

curtisy1 commented 1 year ago

Thanks for the detailed information and suggested fix. I'll also have a look at it and try to find out if anything breaks by not propagating the event.

Do you happen to know what versions of maplibre-gl and leaflet you upgraded to?

jude commented 1 year ago

I bumped the following versions from the dependencies section of https://github.com/Project-OSRM/osrm-frontend/blob/gh-pages/package.json:

leaflet: ~1.9.3 leaflet-control-geocoder: ~2.4.0 leaflet-routing-machine: ~3.2.12 leaflet.locatecontrol: ~0.79 osrm-text-instructions: ~0.14.0 maplibre-gl: ^2.4.0 @maplibre/maplibre-gl-leaflet: 0.0.18

curtisy1 commented 1 year ago

Interesting. I did some further investigation and it seems like this broke with leaflet 1.9.0. I'm unsure if this is actually expected behavior within leaflet since their releases do not indicate it as a breaking change. However, since it is a DomEvent, I believe propagating it is usually the right thing to do, hence LRM does something wrong here by not explicitly stopping it.

If you can, feel free to submit a PR with the proposed change. From a quick glance, it shouldn't affect any other functionality, so stopping the event from propagating is perfectly fine