Raruto / leaflet-rotate

Leaflet plugin that allows to add rotation functionality to map tiles
GNU General Public License v3.0
81 stars 23 forks source link

Panning map issue when click-drag is initiated on marker #28

Closed darklomba closed 8 months ago

darklomba commented 1 year ago

I'm having problems when I try to drag a marker using Leaflet 1.9.3

Youtube video

Any clues? Thank you!!

Raruto commented 1 year ago

Hi @darklomba

I'm having problems when I try to drag a marker using Leaflet 1.9.3

Youtube video

Any clues? Thank you!!

It should be related to a new accessibility feature introduced in the latest versions of leaflet, here it is explained in quite detail: https://github.com/Raruto/leaflet-rotate/issues/18

Thanks for the report 👋 Raruto

siimaoo commented 1 year ago

Hi @Raruto! I'm with the same problem again :( I'm using leaflet 1.9.3 and leaflet-rotate 0.2.0 (the version that it should fix the problem, I also tried to use 0.2.5 but same problem)

Raruto commented 1 year ago

Hi @siimaoo please also provide a minimal example which triggers this bug programmatically.

👋 Raruto

siimaoo commented 1 year ago

Hi @Raruto!

GIF: CleanShot 2023-03-01 at 12 35 48 Sandbox: https://codesandbox.io/s/react-leaflet-forked-opowuo?file=/src/index.js

Raruto commented 1 year ago

I'm using leaflet 1.9.3 and leaflet-rotate 0.2.0 (the version that it should fix the problem, I also tried to use 0.2.5 but same problem)

Sandbox: https://codesandbox.io/s/react-leaflet-forked-opowuo?file=/src/index.js

@siimaoo ok, but that example is still depends on version 0.2.0.. 😞

Please report here a few "minimal examples" (ie. leaflet-rotate + leaflet only) that can reproduce this bug in a simple and deterministic way, so that everyone can replicate it by directly copying-pasting within the test/index.html file.

Here's an example you can start with:

// set an initial map status
map.setBearing(-150);
map.setView([51.505, -0.091], 13);

// instantiate various marker (with bugged options)
// const marker1 = L.marker([51.505, -0.091], {...}).addTo(map);
// const marker2 = L.marker([51.505, -0.091], {...}).addTo(map);

// show everyone how to programmatically trigger the bug(s)
// map.panInside(marker1.getLatLng()) // 
// ...

Have a nice day, Raruto

siimaoo commented 1 year ago

@Raruto In the gif below I used the test/index.html of master branch. It a little bit hard to reproduce.

CleanShot 2023-03-01 at 15 26 05

Raruto commented 1 year ago

Hi @siimaoo

here's a little advice on how to try to write some code that can trigger that bug https://github.com/Raruto/leaflet-rotate/issues/28#issuecomment-1450650384:

// ref: https://github.com/Raruto/leaflet-rotate/blob/580362a2e308d955d9eb082697dc042f82e3da0f/test/index.html#L122-L135
// markers[4] is 'Trondheim': [63.41, 10.41]

map.setBearing(50);

map.setView(markers[4].getLatLng(), 30, {animate: false});

// I advise you to play a bit with various positions (un-comment, re-center...)
map.panBy([0, map._container.clientHeight/2], {animate: false});
// map.panBy([0, -markers[4].getElement().clientHeight], {animate: false});

// run this multiple times to check an unexpected behavior
map.panInside(markers[4].getLatLng());
// map.panInside(markers[4].getLatLng());

// find out why when called programmatically it doesn't seem to trigger any bug
// markers[4].click() 

in any case, I also suggest you try experimenting with larger icons (as in your other example https://github.com/Raruto/leaflet-rotate/issues/28#issuecomment-1450355010) and also trying perfoming some tests on a Marker that doesn't have any Popup or Tooltip attached.

Thunderseb commented 1 year ago

Hello, it seems I had the same issue with some markers. I finally found that setting autoPanOnFocus: false on the L.marker solve the problem

siimaoo commented 1 year ago

for react-leaflet users remove tabindex from all markers seems solve the problem

const markers = document.querySelectorAll(".leaflet-marker-icon");

markers.forEach((marker) => {
    marker.removeAttribute("tabindex");
});

ps: keyboard={false} prop in <MapContainer /> component not solve the problem

Raruto commented 1 year ago

Hello, it seems I had the same issue with some markers. I finally found that setting autoPanOnFocus: false on the L.marker solve the problem

@Thunderseb thanks for the clarification, although as you can see I already talked about it here 👉 https://github.com/Raruto/leaflet-rotate/issues/18#issuecomment-1399349255

Anyway, I wouldn't consider disabling an accessibility feature to be an acceptable solution.. 😞

darklomba commented 1 year ago

Hi @darklomba

I'm having problems when I try to drag a marker using Leaflet 1.9.3

  • which version of leaflet-rotate are you using?
  • do some other tests with older versions of leaflet as well, so we can know when this bug was first introduced
  • provide a sample public demo for others to start debugging

Youtube video Any clues? Thank you!!

It should be related to a new accessibility feature introduced in the latest versions of leaflet, here it is explained in quite detail: #18

Thanks for the report 👋 Raruto

I'm sorry. I tried but I couldn't create a code sandbox to check the issue. When I tried everything worked as expected. Thanks to @siimaoo for the example!

Do you know wich combination of Leaflet and Leaflet-rotate that works as expected?

siimaoo commented 1 year ago

to me leaflet 1.9.3 + leaflet-rotate 0.2.5 works fine!

darklomba commented 1 year ago

to me leaflet 1.9.3 + leaflet-rotate 0.2.5 works fine!

Mmmmm... still not working for me with this combination of versions @siimaoo

siimaoo commented 1 year ago

@darklomba

do you need to keep the keyboard control to accessibility? if you dont need, set keyboard to false to any item of your map. (map, markers, cluster, popup, etc). if it dont solve your problem, use patch-package to rewrite some lines of code of leaflet, you need to remove tabindex from every place that have tabIndex = 0 and also remove tabIndex in create$1 function (as this step is very particular, I dont put any piece of code here, but if need help send a dm in twitter)

Raruto commented 1 year ago

Tryed to start to automate: https://github.com/Raruto/leaflet-rotate/issues/28#issuecomment-1453789833 in v0.2.7

https://github.com/Raruto/leaflet-rotate/blob/e082bf38b1fbb5be6cf0ec07af9e57552e70aa67/examples/leaflet-rotate.spec.js#L56-L104

👋 Raruto

darklomba commented 1 year ago

Hi @Raruto!! Is this update supposed to fix the panning error? I can try if it helps! Thanks!

Raruto commented 1 year ago

Hi @darklomba,

in https://github.com/Raruto/leaflet-rotate/commit/fb536995c1a30f601c43b2693ee3e71ecc894f98 i just added some sort of automated testing.

If you feel like doing some tests, you could try to reproduce locally the bug based on this map https://leafext.de/wordpress/popuprotate/ (I saw that with leaflet-rotate@v0.2.6 it occurs with 100% probability, even without setting any bearing..). <-- NB that map was created with a wordpress plugin, so the first step would be to convert it into a plain html example..

👋 Raruto

Raruto commented 1 year ago

I think it's a matter of miscalculating the actual size of the map's container rectangle (eg. getBoundingClientRect()).

image

Right now, you can see this quite easily by running these commands:

// Navigate to our public example (leaflet-rotate@v0.2.6)
location.href = "https://raruto.github.io/leaflet-rotate/examples/leaflet-rotate.html";

// First, put the marker at the very top edge of the map
map.setBearing(50);
map.setView(markers[4].getLatLng(), 30, { animate: false });
map.panBy([0, map._container.clientHeight/2], { animate: false });

// Trigger the bug
map.panInside(markers[4].getLatLng());

// Now you can run that command again to continue to see the bug..
// map.panInside(markers[4].getLatLng());
// map.panInside(markers[4].getLatLng());

Then, try it again by zero-ing all the additional "margin" rules:

html, body, #map {
  margin: 0;
  padding: 0;
  border: 0;
}

image

image

Offending lines:

https://github.com/Raruto/leaflet-rotate/blob/e082bf38b1fbb5be6cf0ec07af9e57552e70aa67/src/map/Map.js#L319-L333

👋 Raruto

Raruto commented 1 year ago

@darklomba @siimaoo @Thunderseb please do some testing with the latest release: v0.2.8 👉 https://github.com/Raruto/leaflet-rotate/pull/34#issuecomment-1634489970

@yuri-karelics a few more "automated" tests in this regard it will never hurt.. 😌

👋 Raruto