Programmatic spiderfy fails if cluster already spiderfied

matburnham commented 5 years ago

[x] I'm reporting a bug, not asking for help
[x] I'm sure this is a Leaflet.MarkerCluster code issue, not an issue with my own code nor with the framework I'm using (Cordova, Ionic, Angular, React…)
[x] I've searched through the issues to make sure it's not yet reported

How to reproduce

Leaflet version I'm using: 1.5.1
Leaflet.MarkerCluster version I'm using: 1.4.1
Browser (with version) I'm using: Google Chrome 74.0.3729.157 (Official Build) (64-bit)
OS/Platform (with version) I'm using: Windows 10 Version 1803 (OS Build 17134.766)

Expand cluster by clicking with mouse
Programatically expand same cluster.
Note remaining spider lines
Click again to get error in error console.

What behaviour I'm expecting and which behaviour I'm seeing

I would expect the library to handle this without client code having to check cluster status beforehand. This line of suggests similar, but does not appear to work as expected.

https://github.com/Leaflet/Leaflet.markercluster/blob/93bf5408ad84b74ea42f55ccb6ca0450daa41427/src/MarkerCluster.Spiderfier.js#L18

I understand insufficient internals to debug much further, despite trying.

Minimal example reproducing the issue

[x] this example is as simple as possible
[x] this example does not rely on any third party code

See https://jsfiddle.net/j49gurdv/5/ for a minimal example.

Reference

For search purposes, this is the error raised by the minimal example if you continue to step 4.

DomUtil.js:230 Uncaught TypeError: Cannot set property '_leaflet_pos' of null
    at setPosition (DomUtil.js:230)
    at NewClass._setPos (Marker.js:290)
    at NewClass._animationUnspiderfy (MarkerCluster.Spiderfier.js:320)
    at NewClass.unspiderfy (MarkerCluster.Spiderfier.js:48)
    at NewClass._unspiderfy (MarkerCluster.Spiderfier.js:448)
    at NewClass.spiderfy (MarkerCluster.Spiderfier.js:28)
    at NewClass._zoomOrSpiderfy (MarkerClusterGroup.js:854)
    at NewClass.fire (Events.js:190)
    at NewClass.fire (MarkerClusterGroup.js:798)
    at NewClass._propagateEvent (Events.js:262)
setPosition @ DomUtil.js:230
_setPos @ Marker.js:290
_animationUnspiderfy @ MarkerCluster.Spiderfier.js:320
unspiderfy @ MarkerCluster.Spiderfier.js:48
_unspiderfy @ MarkerCluster.Spiderfier.js:448
spiderfy @ MarkerCluster.Spiderfier.js:28
_zoomOrSpiderfy @ MarkerClusterGroup.js:854
fire @ Events.js:190
fire @ MarkerClusterGroup.js:798
_propagateEvent @ Events.js:262
fire @ Events.js:199
_propagateEvent @ Events.js:262
fire @ Events.js:199
_fireDOMEvent @ Map.js:1439
_handleDOMEvent @ Map.js:1396
handler @ DomEvent.js:79

matburnham commented 5 years ago

Looks like #525 may relate, and this may be an embodiment of #892.

ghybs commented 5 years ago

Hi,

What happens in your specific example is that you progrmmatically try to spiderfy a Cluster which is at a different zoom level (18) than the one that is currently spiderfied (by user click) (13).

var addressPoints = [
  // Markers at exact same position => cluster spiderfies at current zoom
  [52.002, 0.02, "b1"],
  [52.002, 0.02, "b2"],
  [52.002, 0.02, "b3"],
];
var map = L.map('map', {center: latlng, zoom: 13, layers: [tiles]}); // <= user click siperfies cluster at zoom 13
addr['b2'].__parent.spiderfy(); // <= retrieve closest parent cluster, i.e. for zoom 18, and try to spiderfy it

By making a small utility function to retrieve the parent cluster at current zoom, you properly get rid of this issue:

// Utility to retrieve the parent cluster corresponding to current zoom
function getParentAtCurrentZoom(marker) {
  var currentZoom = map.getZoom();
  while (marker.__parent && marker.__parent._zoom >= currentZoom) {
    marker = marker.__parent;
  }
  return marker;
}

Updated JSFiddle: https://jsfiddle.net/0sfz2uc9/

Then what happens when you try to spiderfy the cluster for zoom 18, is that the parent cluster for zoom 13 tries to unspiderfy; since it is animated, the unspiderfication uses a timeout, which removes the Markers, whereas they were supposed to remain for the cluster 18 spiderfication... Then the error message you report is due to these missing Markers.

That being said, a potential improvement would be ensure the cluster to be spiderfied is visible on map.

Leaflet / Leaflet.markercluster