Raruto / leaflet-rotate

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

Glitched map autopan when marker icon get focused (click or tab navigation) #18

Closed siimaoo closed 1 year ago

siimaoo commented 1 year ago

I found a bug when rotate the map (maybe related to map bounds).

If you have a map with a cluster in the border of the map or near, rotate the map, in some positions the cluster doesn't spiderfy anymore.

codesandbox example: https://codesandbox.io/s/react-leaflet-forked-opowuo video example: https://user-images.githubusercontent.com/34813589/213180778-10d06c34-bdef-449a-86d6-444b71a56cf8.mp4

Raruto commented 1 year ago

Hi @siimaoo,

first take a look at these two previous issues:

In particular, inspect the html code generated by leaflet to know exactly which layers are taken into account during rotation (ie. you can search within the .leaflet-norotate-pane and .leaflet-norotate-pane elements), because in general it could simply be a problem related of how / where your layer is added (most of the time it really depends on how a plugin was developed..).

Have a nice day, Raruto

siimaoo commented 1 year ago

I try add markers to a custom pane in norotate-pane but same problem :/ https://codesandbox.io/s/react-leaflet-forked-opowuo

Raruto commented 1 year ago

I try add markers to a custom pane in norotate-pane but same problem :/

@siimaoo you can also try using an older version of leaflet (e.g. v1.7)

ref: https://github.com/Raruto/leaflet-rotate/issues/15

siimaoo commented 1 year ago

In codesandbox example works fine, but in my project same problem. I tried fit map bounds but the problem intensified.

Raruto commented 1 year ago

Try to create an example of clustering without the use of react (just to isolate the problem around the markerlusterer library), then if it helps you for debugging, also take a look to the library used for this purpose in the demo

In codesandbox example works fine, but in my project same problem.

Perhaps it has nothing to do with it, but I would also try provide smaller marker icons.. 😄

I tried fit map bounds but the problem intensified.

Unfortunately I don't have such a thorough knowledge of this part of the code, most of the development of this library is the result of the re-work of what have done Iván Sánchez Ortega before (ref: Leaflet/rotate), but since he is a core developer I think he has other things to do than dedicate himself to this complex functionality....

Anyway, keep me posted if you have any news about it

Raruto

siimaoo commented 1 year ago

if i dont use clusters and disable marker autopan, everything works fine. unfortunately, clusters dont have an autopan option, so, the problem continues.

I tried remove react wrappers for leaflet and use pure js to create the map, but nothing change, same behavior.

Raruto commented 1 year ago

In codesandbox example works fine, but in my project same problem. I tried fit map bounds but the problem intensified.

@siimaoo maybe I didn't understand, but locally it gives you problems even using leaflet v1.7? (I saw that you also updated the demo and it seems to work fine with that)

if i dont use clusters and disable marker autopan, everything works fine. unfortunately, clusters dont have an autopan option, so, the problem continues.

I tried remove react wrappers for leaflet and use pure js to create the map, but nothing change, same behavior.

I have no experience with these libraries (react-leaflet and markerclusterer), however if you upload even a simple demo made without react it will surely speed up the debug work of those who will come after you 😉

Raruto commented 1 year ago

@siimaoo in the debug library I just added (here: https://github.com/Raruto/leaflet-rotate/pull/21/commits/2b60fbec6885c196a25f0cdfba6e6b852d9d8d09) a visual reference to "current" map Pixel Bounds to allow you to better understand on which markers you should expect to run into that bug on click.

pixelbounds

The panning to an "unexpected" direction (after clicking on it, or by using tab navigation, ref: https://github.com/Leaflet/Leaflet/pull/8042 for more info about it) is nothing more than an attempt by the leflalet library to center marker not fully visible on current view, ref: https://github.com/Leaflet/Leaflet/commit/4f639a85efffa49c3e64a07dc0b6f5aa73f13449 and marker-autopanonfocus option added in leaflet v1.8:

// Leaflet/src/layer/marker/Marker.js (v1.9.3)

...

// @option autoPanOnFocus: Boolean = true
// When `true`, the map will pan whenever the marker is focused (via
// e.g. pressing `tab` on the keyboard) to ensure the marker is
// visible within the map's bounds
autoPanOnFocus: true,

...

/**
 * HERE is where the bug starts (press tab or click on marker to replicate it)
 *
 * @since https://github.com/Leaflet/Leaflet/commit/4f639a85efffa49c3e64a07dc0b6f5aa73f13449
 * @see https://github.com/Leaflet/Leaflet/pull/8042
 */
if (this.options.autoPanOnFocus) {
  DomEvent.on(icon, 'focus', this._panOnFocus, this);
}

...

_panOnFocus() {
  const map = this._map;
  if (!map) { return; }

  const iconOpts = this.options.icon.options;
  const size = iconOpts.iconSize ? point(iconOpts.iconSize) : point(0, 0);
  const anchor = iconOpts.iconAnchor ? point(iconOpts.iconAnchor) : point(0, 0);

  map.panInside(this._latlng, {
    paddingTopLeft: anchor,
    paddingBottomRight: size.subtract(anchor)
  });
},

Most likely the problem behind this is due to using the L.Map::getPixelBounds() function inside the leaflet core, and specifically here:

// Leaflet/src/map/Map.js (v1.9.3)

...

// @method panInside(latlng: LatLng, options?: padding options): this
// Pans the map the minimum amount to make the `latlng` visible. Use
// padding options to fit the display to more restricted bounds.
// If `latlng` is already within the (optionally padded) display bounds,
// the map will not be panned.
panInside(latlng, options) {
  options = options || {};

  const paddingTL = toPoint(options.paddingTopLeft || options.padding || [0, 0]),
    paddingBR = toPoint(options.paddingBottomRight || options.padding || [0, 0]),
    pixelCenter = this.project(this.getCenter()),
    pixelPoint = this.project(latlng),

  /**
   * A "rectangular" boundary is used to determine if the marker falls
   * within the current view (and thus pan the map), but on a rotated
   * map the current view is a "trapezoid" (see image above to get an idea
   * of how `getPixelBounds` could generally look on a rotated map), or 
   * somehow those rectangular bounds should be calculated starting
   *  from the unrotated div (eg. `norotatePane`)
   */
    pixelBounds = this.getPixelBounds(),

    paddedBounds = toBounds([pixelBounds.min.add(paddingTL), pixelBounds.max.subtract(paddingBR)]),
    paddedSize = paddedBounds.getSize();

  if (!paddedBounds.contains(pixelPoint)) {
    this._enforcingBounds = true;
    const centerOffset = pixelPoint.subtract(paddedBounds.getCenter());
    const offset = paddedBounds.extend(pixelPoint).getSize().subtract(paddedSize);
    pixelCenter.x += centerOffset.x < 0 ? -offset.x : offset.x;
    pixelCenter.y += centerOffset.y < 0 ? -offset.y : offset.y;
    this.panTo(this.unproject(pixelCenter), options);
    this._enforcingBounds = false;
  }
  return this;
},

...

// @method getPixelBounds(): Bounds
// Returns the bounds of the current map view in projected pixel
// coordinates (sometimes useful in layer and overlay implementations).
getPixelBounds(center, zoom) {
  const topLeftPoint = this._getTopLeftPoint(center, zoom);
  return new Bounds(topLeftPoint, topLeftPoint.add(this.getSize()));
},

_I hope I was clear enough about the reasons for this bug, but above all I hope someone can find a smarter solution than simply disabling the marker-autopanonfocus option (unlike: https://github.com/Raruto/leaflet-rotate/pull/21#discussion_r1083128400)_

Hoping for good debug sessions, Raruto

Raruto commented 1 year ago

@siimaoo @heyajohnny in relation to https://github.com/Raruto/leaflet-rotate/issues/18#issuecomment-1399349255, I think you can get a better idea of the reasons for the bug by working at 90° / 180°

immagine

That is, when "top" becomes "bottom", some assumptions made in leaflet core calculations become incorrect, for example:

• src/layer/marker/Marker::_panOnFocus()

_panOnFocus: function () {
  var map = this._map;
  if (!map) { return; }

  var iconOpts = this.options.icon.options;
  var size = point(iconOpts.iconSize);
  var anchor = point(iconOpts.iconAnchor);

  map.panInside(this._latlng, {
    paddingTopLeft: anchor,                    // <-- Top Left becomes Bottom Right (at least at 180°)
    paddingBottomRight: size.subtract(anchor)  // <-- Bottom Right becomes Top Left (at least at 180°)
  });
},

In addition to that, the toBounds() function (aka. L.bounds()) returns two values {min, max} which may not take into account the current map rotation:

// Leaflet/src/map/Map.js (v1.9.3)

panInside(latlng, options) {
  ...
  paddedBounds = toBounds([pixelBounds.min.add(paddingTL), pixelBounds.max.subtract(paddingBR)]),
  ...
}

In this regard I point out that a solution similar to this type of issue has already been somehow taken into consideration by Sören Schwert to internally adjust the calculation of the analogous L.Map::getBounds() function (in that latter case the returned value was a L.latLngBounds, and not a pair of screen pixel coordinates, but even then there was this kind of problem where NE (north-east) points could become SW (south-west) in a rotated map).

// Leaflet/src/map/Map.js (ref: https://github.com/fnicollet/Leaflet/pull/22)

getBounds: function () {
  // var bounds = this.getPixelBounds(),
  //     sw = this.unproject(bounds.getBottomLeft()),
  //     ne = this.unproject(bounds.getTopRight());

  var size = this.getSize();
  var sw = this.layerPointToLatLng(this.containerPointToLayerPoint([0, size.y]));
  var ne = this.layerPointToLatLng(this.containerPointToLayerPoint([size.x, 0]));

  return new L.LatLngBounds(sw, ne);
},

I don't know what could be a solution for this kind of problem (perhaps the calculations should all be done starting from the fixed point of reference used internally, aka. pivot point), but in any case I think that all this should be completely independent of the current "rotation angle" (i.e. effectively abandoning the concept of "top" and "bottom" points from calculations).

siimaoo commented 1 year ago

21 will fix map autopan.

Raruto commented 1 year ago

https://github.com/Raruto/leaflet-rotate/pull/21 will fix map autopan.

@siimaoo that pull doesn't really fix the bug, it just disables a new option added by default in leaflet v1.8.0

_I hope I was clear enough about the reasons for this bug, but above all I hope someone can find a smarter solution than simply disabling the marker-autopanonfocus option (unlike: https://github.com/Raruto/leaflet-rotate/pull/21#discussion_r1083128400)_

I'm reopening this just to make it clear to everyone.

Raruto commented 1 year ago

Patch released in v0.2.0