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

Cluster stays if tapped at map borders (Android/iOS) #529

Closed
TomRiedl opened this issue Jul 24, 2015 · 8 comments
Closed

Cluster stays if tapped at map borders (Android/iOS) #529

TomRiedl opened this issue Jul 24, 2015 · 8 comments

Comments

@TomRiedl
Copy link

The correct behavior:
working1
working2
This works fine if the cluster is near the center of the map.

But if I move the map to a position where the cluster is near the border of the map, this happens:
notworking1
notworking2

This is reproducible every time on different mobile devices (tested on two Android and three iOS devices).
This is not reproducible in Firefox and Chrome.

The app is build through PhoneGap.

The code the clusters are created:

this._map = L.map('mapNear', { worldCopyJump: true, });
this._map.options.minZoom = 1;
L.control.scale().addTo(this._map);

L.tileLayer(Application.AppCore.Configuration.MapTiles, {
    attribution: '&copy; <a href="http://osm.org/copyright">OpenStreetMap</a> contributors'
}).addTo(this._map);


this._markerCluster = new L.MarkerClusterGroup({ showCoverageOnHover: false });
for (index in items)
{
    [...]
    marker = L.marker([item.Coordinates.Latitude, item.Coordinates.Longitude],
        {
            icon: L.AwesomeMarkers.icon({ icon: item.Icon ? item.Icon : 'cube', markerColor: 'blue', prefix: 'fa', spin: false }),
            zIndexOffset: 10000
        });
    marker.bindPopup('<some html code>');
    this._markerCluster.addLayer(marker);
}
this._markerCluster.addTo(this._map);
@danzel
Copy link
Member

danzel commented Aug 6, 2015

https://github.com/Leaflet/Leaflet.markercluster/blob/master/src/MarkerClusterGroup.js#L892-L893
If you change this so instead of 0 it is a small number does that fix it?
Or make it always 0 and it may reproduce on desktop.

@TomRiedl
Copy link
Author

The changes work like you expected it to happen:

With
latDiff = 0,
lngDiff = 0;
-> reproducible also on desktop.

With
latDiff = Math.abs(sw.lat - ne.lat),
lngDiff = Math.abs(sw.lng - ne.lng);
-> No bugs on mobile devices

@ftechiesnitin
Copy link

Hi @ThomasRiedl, I am facing the same issue for one of my app. Can you tell me how you fixed this issue?

Thanks.

@TomRiedl
Copy link
Author

TomRiedl commented Mar 3, 2016

Hi @ftechiesnitin, I replaced
latDiff = L.Browser.mobile ? 0 : Math.abs(sw.lat - ne.lat),
lngDiff = L.Browser.mobile ? 0 : Math.abs(sw.lng - ne.lng);

with

latDiff = Math.abs(sw.lat - ne.lat),
lngDiff = Math.abs(sw.lng - ne.lng);

in file MarkerClusterGroup.js

I have to change this everytime I update Leaflet.markercluster.

@ftechiesnitin
Copy link

Thanks @TomRiedl that did the trick.

@ghybs
Copy link
Contributor

ghybs commented Mar 4, 2016

Hi,

It seems sad to have to cancel the effect of mobile detection in _getExpandedVisibleBounds to prevent this bug.

It is reproducible with Leaflet 1.0.0 and MCG master: http://jsfiddle.net/3v7hd2vx/18/

It is the same issue as #344, which was solved by commit 254198a.
Unfortunately, the correction was made only in previous master branch (now leaflet-0.7 branch), but not in leaflet-master branch (now master).

Doing the same in current master branch seems to solve the issue: http://jsfiddle.net/3v7hd2vx/19/

Will do a PR shortly.

ghybs added a commit to ghybs/Leaflet.markercluster that referenced this issue Mar 4, 2016
Same as Leaflet#344, which was corrected by commit 254198a in leaflet-0.7 branch only. Do the same in master branch (for Leaflet 1.0.0) as well.
Also added a dedicated test spec in zoomAnimationSpec test suite.

Took the opportunity to refactor the test suite to avoid PhantomJS memory leak.
danzel added a commit that referenced this issue Mar 6, 2016
Corrected #529 in master (Leaflet 1.0.0)
@danzel
Copy link
Member

danzel commented Jan 26, 2017

This works with the latest code:
http://jsfiddle.net/3v7hd2vx/192/

@danzel danzel closed this as completed Jan 26, 2017
@danzel
Copy link
Member

danzel commented Jan 26, 2017

Ah, I see ghybs fixed it in #644 but it wasn't closed then. Thanks :)

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

No branches or pull requests

4 participants