Layer order in `L.Control.Layers` is not preserved with GeoJson/FeatureGroup #2086

Closed
jieter opened this Issue Oct 10, 2013 · 25 comments

Comments

Projects
None yet
@jieter
Contributor

jieter commented Oct 10, 2013

When I do

L.control.layers({
    'OSM': L.tileLayer('http://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png').addTo(map)
}, {
    'Circle': L.circle([53, 4], 111111),
    'Polygon': L.polygon([[48, -3], [50, -4], [52, 4]]),
    'GeoJSON': L.geoJson(geojson)
}, {
    collapsed: false
}).addTo(map);

I expect the order of the overlays to be as I supplied them, however, GeoJSON and FeatureGroup layers are moved up top. This is because the FeatureGroup gets its _leaflet_id while it's created, while the other layers get it while they're added to Control.Layers.
FeatureGroup is stamped because it's passed as the context to layer.on in FeatureGroup.js:19.

Not sure how to fix this:

  • stamping each layer while initializing? Seems dirty.
  • Different sorting mechanism in Control.Layers?

http://jsfiddle.net/km5H9/1/

@tmcw

This comment has been minimized.

Show comment
Hide comment
@tmcw

tmcw Oct 15, 2013

Contributor

Hm, if Control.Layers implies sort, it really should take a list rather than an object. Sure, the 'not necessarily sorted' behavior of objects in browsers ends up being 'mostly sorted', but it's also tricky/impossible to reorder properties in an object without creating a new one.

Contributor

tmcw commented Oct 15, 2013

Hm, if Control.Layers implies sort, it really should take a list rather than an object. Sure, the 'not necessarily sorted' behavior of objects in browsers ends up being 'mostly sorted', but it's also tricky/impossible to reorder properties in an object without creating a new one.

@Zverik

This comment has been minimized.

Show comment
Hide comment
@Zverik

Zverik Oct 28, 2013

Contributor

I too don't like that L.Control.Layers relies on _leaflet_id for sorting. It too often messes up the order of layers, even tile layers (e.g. L.BingLayer also uses L.Util.stamp(this) in the constructor). Can the order assumed by _addLayer calls be preserved without stamping?

Contributor

Zverik commented Oct 28, 2013

I too don't like that L.Control.Layers relies on _leaflet_id for sorting. It too often messes up the order of layers, even tile layers (e.g. L.BingLayer also uses L.Util.stamp(this) in the constructor). Can the order assumed by _addLayer calls be preserved without stamping?

@ghost ghost assigned mourner Oct 28, 2013

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Oct 28, 2013

Member

Here's the issue: https://github.com/Leaflet/Leaflet/blob/master/src/control/Control.Layers.js#L140
The problem is not in using objects for configuration of the control, but in using an internal id-keyed object for referencing layers, and then iterating over it when rendering. I did it mostly with performance considerations (e.g. not having to use indexOf on an array on each map layeradd/remove event to check if we should update the control). If we can come up with a more efficient way of tracking control changes, we can get rid of id-keyed object for layers.

Member

mourner commented Oct 28, 2013

Here's the issue: https://github.com/Leaflet/Leaflet/blob/master/src/control/Control.Layers.js#L140
The problem is not in using objects for configuration of the control, but in using an internal id-keyed object for referencing layers, and then iterating over it when rendering. I did it mostly with performance considerations (e.g. not having to use indexOf on an array on each map layeradd/remove event to check if we should update the control). If we can come up with a more efficient way of tracking control changes, we can get rid of id-keyed object for layers.

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Oct 28, 2013

Member

I think the fix will fit well into a bigger refactoring of layers code: all layers will fire add/remove event, and the control will subscribe to those events of its layers instead of listening to all layers added/removed.

Member

mourner commented Oct 28, 2013

I think the fix will fit well into a bigger refactoring of layers code: all layers will fire add/remove event, and the control will subscribe to those events of its layers instead of listening to all layers added/removed.

@patrickarlt

This comment has been minimized.

Show comment
Hide comment
@patrickarlt

patrickarlt Oct 29, 2013

Member

Layer order is also not preserved when using ImageOverlay with Control.Layers @danzel and I have been discussing it in #2132. Because ImageOverlay uses appendChild when it gets added to the map it ways gets added on top of other ImageOverlays.

So doing something like...

L.control.layers({
  image1: L.imageOverlay(url, bounds);
  image2: L.imageOverlay(url, bounds);
}).addTo(map);

and then enabling image2 and then image1, image1 appear on top of image2.

Member

patrickarlt commented Oct 29, 2013

Layer order is also not preserved when using ImageOverlay with Control.Layers @danzel and I have been discussing it in #2132. Because ImageOverlay uses appendChild when it gets added to the map it ways gets added on top of other ImageOverlays.

So doing something like...

L.control.layers({
  image1: L.imageOverlay(url, bounds);
  image2: L.imageOverlay(url, bounds);
}).addTo(map);

and then enabling image2 and then image1, image1 appear on top of image2.

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Nov 4, 2013

Member

Pushed to 0.8

Member

mourner commented Nov 4, 2013

Pushed to 0.8

@luca76

This comment has been minimized.

Show comment
Hide comment
@luca76

luca76 Apr 16, 2014

This bug is critical, since it is a feature that is broken in leaflet. It's impossible to use ImageOverlay layers along TileLayers, because the ImageOverlay are ALWAYS on bottom and so "invisible".

luca76 commented Apr 16, 2014

This bug is critical, since it is a feature that is broken in leaflet. It's impossible to use ImageOverlay layers along TileLayers, because the ImageOverlay are ALWAYS on bottom and so "invisible".

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Apr 16, 2014

Member

@luca76 this is weird since ImageOverlay should be always on top of TileLayer, as the first are placed in overlay pane while the last are in tile pane. Can you make a JSFiddle that reproduces this?

Member

mourner commented Apr 16, 2014

@luca76 this is weird since ImageOverlay should be always on top of TileLayer, as the first are placed in overlay pane while the last are in tile pane. Can you make a JSFiddle that reproduces this?

@meengla

This comment has been minimized.

Show comment
Hide comment
@meengla

meengla Apr 18, 2014

I guess the problem I am having, as I posted to Stackoverflow is because of this bug?

http://stackoverflow.com/questions/23141602/leaflet-js-wms-layer-zoom-not-working-on-double-click

meengla commented Apr 18, 2014

I guess the problem I am having, as I posted to Stackoverflow is because of this bug?

http://stackoverflow.com/questions/23141602/leaflet-js-wms-layer-zoom-not-working-on-double-click

@tomchadwin

This comment has been minimized.

Show comment
Hide comment
@tomchadwin

tomchadwin Jun 24, 2014

Contributor

I presume I am experiencing this. Race conditions seem to apply when loading multiple remote layers. More complex geometry loads last and therefore covers simpler (smaller filesize) data.

What I don't understand is that if I simply create the GeoJSON layer without its data and add it to the map, and then addData() via AJAX this still seems to destroy layer order. Or am I getting something wrong?

Contributor

tomchadwin commented Jun 24, 2014

I presume I am experiencing this. Race conditions seem to apply when loading multiple remote layers. More complex geometry loads last and therefore covers simpler (smaller filesize) data.

What I don't understand is that if I simply create the GeoJSON layer without its data and add it to the map, and then addData() via AJAX this still seems to destroy layer order. Or am I getting something wrong?

@prebm

This comment has been minimized.

Show comment
Hide comment
@prebm

prebm Oct 16, 2014

With Leaflet 0.8 the ImageOverlay is on top of the TileLayers, but on top of all TileLayers:

var baseMaps = {
    "OSM": layerOSM
};

var overlayMaps = {
    "Image": layerIMG,
    "Labels": layerTonerLabels
};

L.control.layers(baseMaps, overlayMaps).addTo(map);

In this example the image is above the labels, where it should be vice verca. Is it not possible to put the image also in the tile pane?

prebm commented Oct 16, 2014

With Leaflet 0.8 the ImageOverlay is on top of the TileLayers, but on top of all TileLayers:

var baseMaps = {
    "OSM": layerOSM
};

var overlayMaps = {
    "Image": layerIMG,
    "Labels": layerTonerLabels
};

L.control.layers(baseMaps, overlayMaps).addTo(map);

In this example the image is above the labels, where it should be vice verca. Is it not possible to put the image also in the tile pane?

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Oct 16, 2014

Member

@prebm you can specify pane for your ImageOverlay through pane: 'tilePane' option.

Member

mourner commented Oct 16, 2014

@prebm you can specify pane for your ImageOverlay through pane: 'tilePane' option.

@prebm

This comment has been minimized.

Show comment
Hide comment
@prebm

prebm Oct 17, 2014

@mourner as I can see adding the ImageOverlay to the tilePane does not assign a z-index to the ImageOverlay, hence the ImageOverlay is not visible at all. After inspecting with Firebug i saw, that the would be in the correct order and also the other TileLayers had the correct z-indexes to allow the ImageOverlay to be "between" them. Adding the z-value manually then produced the right outcome.
As the ImageOverlay does not offer a setZIndex there is no way to add that programmatically, am I right? Would it be possible over CSS?

prebm commented Oct 17, 2014

@mourner as I can see adding the ImageOverlay to the tilePane does not assign a z-index to the ImageOverlay, hence the ImageOverlay is not visible at all. After inspecting with Firebug i saw, that the would be in the correct order and also the other TileLayers had the correct z-indexes to allow the ImageOverlay to be "between" them. Adding the z-value manually then produced the right outcome.
As the ImageOverlay does not offer a setZIndex there is no way to add that programmatically, am I right? Would it be possible over CSS?

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Oct 17, 2014

Member

Yeah, possible via CSS or by adding ImageOverlay after all tile layers. But I should probably add zIndex option to ImageOverlay.

Member

mourner commented Oct 17, 2014

Yeah, possible via CSS or by adding ImageOverlay after all tile layers. But I should probably add zIndex option to ImageOverlay.

@prebm

This comment has been minimized.

Show comment
Hide comment
@prebm

prebm Oct 21, 2014

Ok, thanks

prebm commented Oct 21, 2014

Ok, thanks

@tomchadwin tomchadwin referenced this issue in Geolicious/qgis2leaf Apr 3, 2015

Closed

Layers in wrong order in layer control #249

@yohanboniface yohanboniface modified the milestones: 1.0-beta3, 1.0 Oct 12, 2015

michaelkirk added a commit to michaelkirk/project-sandbox that referenced this issue Nov 13, 2015

@talltom talltom referenced this issue in smart-facility/petajakarta-web Dec 4, 2015

Open

Ordering of layers in map layersControl is arbitrary #69

@Rikaelus

This comment has been minimized.

Show comment
Hide comment
@Rikaelus

Rikaelus Jan 24, 2016

I think this is still valid?
I have a geoJSON layer among several tileLayers. Initial placement works fine but on certain page events that may change layer availability I trigger a "refresh" by removing and re-adding all layers in the same order as before, but my geoJSON layer now appears at the top of the control.

I was able to work around this by removing the old geoJSON layer and replacing it with a new one (ostensibly with a new internal ID in a proper order) and it appears in the correct place.

I think this is still valid?
I have a geoJSON layer among several tileLayers. Initial placement works fine but on certain page events that may change layer availability I trigger a "refresh" by removing and re-adding all layers in the same order as before, but my geoJSON layer now appears at the top of the control.

I was able to work around this by removing the old geoJSON layer and replacing it with a new one (ostensibly with a new internal ID in a proper order) and it appears in the correct place.

@minskmaz

This comment has been minimized.

Show comment
Hide comment
@minskmaz

minskmaz Jan 24, 2016

Hi folks. I'm also experiencing this with geojson featuregroups. Lots of overlapping polygons and the zindex changes confuse the user. Can anyone suggest a canonical/somewhat-future-proof method for maintaining group display order ?

Hi folks. I'm also experiencing this with geojson featuregroups. Lots of overlapping polygons and the zindex changes confuse the user. Can anyone suggest a canonical/somewhat-future-proof method for maintaining group display order ?

@tomchadwin

This comment has been minimized.

Show comment
Hide comment
@tomchadwin

tomchadwin Jan 24, 2016

Contributor

Afraid not. I build an array of layers in their correct order, and cycle through the whole array each time layers are added, bringing every layer in turn to the front. Horrible, but it works. I have no alternative at this stage.

Contributor

tomchadwin commented Jan 24, 2016

Afraid not. I build an array of layers in their correct order, and cycle through the whole array each time layers are added, bringing every layer in turn to the front. Horrible, but it works. I have no alternative at this stage.

@Rikaelus

This comment has been minimized.

Show comment
Hide comment
@Rikaelus

Rikaelus Jan 25, 2016

I'm basically doing what tomchadwin has except more explicit, since I've just kind of grown into the code. If I get around to refactoring it I'll either use a similar array or probably make my own little Javascript snippet to manage my own control interface and stick it within the map.

I'm basically doing what tomchadwin has except more explicit, since I've just kind of grown into the code. If I get around to refactoring it I'll either use a similar array or probably make my own little Javascript snippet to manage my own control interface and stick it within the map.

@patrickarlt

This comment has been minimized.

Show comment
Hide comment
@patrickarlt

patrickarlt Jan 25, 2016

Member

If anyone here is using Leaflet 1.0.0 you should be able to use the new pane option to control this:

map.createPane('bottom');
map.createPane('middle');
map.createPane('top');

// now pane order controls layer ordering not the control!
L.control.layers({
    'OSM': L.tileLayer('http://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png').addTo(map)
}, {
    'bottom': L.geoJson(geojson, {pane: 'bottom'}),
    'middle': L.geoJson(geojson, {pane: 'middle'}),
    'top':  L.geoJson(geojson, {pane: 'top'}),,
}, {
    collapsed: false
}).addTo(map);

You can see this inaction with lots of overlapping polygons here.

Member

patrickarlt commented Jan 25, 2016

If anyone here is using Leaflet 1.0.0 you should be able to use the new pane option to control this:

map.createPane('bottom');
map.createPane('middle');
map.createPane('top');

// now pane order controls layer ordering not the control!
L.control.layers({
    'OSM': L.tileLayer('http://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png').addTo(map)
}, {
    'bottom': L.geoJson(geojson, {pane: 'bottom'}),
    'middle': L.geoJson(geojson, {pane: 'middle'}),
    'top':  L.geoJson(geojson, {pane: 'top'}),,
}, {
    collapsed: false
}).addTo(map);

You can see this inaction with lots of overlapping polygons here.

@minskmaz

This comment has been minimized.

Show comment
Hide comment
@minskmaz

minskmaz Jan 28, 2016

patrickarlt thought I'd let you know I implemented the pattern you outlined above. This does lay out the order of the panes properly -- panes and their associated layer group are added to the map such that the pane appears beneath successively added panes. However, when using l.control.layers.addOverlay to then associate L.control.layers with panes on the map --> zero order to the layer names as they appear in the control seems possible. Half way there! Any suggestions ?

patrickarlt thought I'd let you know I implemented the pattern you outlined above. This does lay out the order of the panes properly -- panes and their associated layer group are added to the map such that the pane appears beneath successively added panes. However, when using l.control.layers.addOverlay to then associate L.control.layers with panes on the map --> zero order to the layer names as they appear in the control seems possible. Half way there! Any suggestions ?

@patrickarlt

This comment has been minimized.

Show comment
Hide comment
@patrickarlt

patrickarlt Jan 28, 2016

Member

I look a look at the code. It looks like layers are stored internally in the control by the layers internal Leaflet ID https://github.com/Leaflet/Leaflet/blob/master/src/control/Control.Layers.js#L115-L121. The internal ID generator gets called A LOT. For example in my JS Bin http://jsbin.com/vocigipive/1/edit?html,css,js,output it is already at 177 by the time everything gets done.

Since all browsers sort objects when looping like for (i in this._layers) by the order in which they are defined http://stackoverflow.com/questions/280713/elements-order-in-a-for-in-loop this means that you cannot really rely on any specific ordering when using L.Control.layers.

@mourner I'm wondering if this could be refactored to use an array of layers internally rather then an object.

// convert basemaps and overlays to ordered arrays internally
L.control.layers({
    'OSM': L.tileLayer('http://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png').addTo(map)
}, {
    'bottom': L.geoJson(geojson, {pane: 'bottom'}),
    'middle': L.geoJson(geojson, {pane: 'middle'}),
    'top':  L.geoJson(geojson, {pane: 'top'}),,
}).addTo(map);

 // add a new position property to control where the added layer falls in the order. Default would be at the bottom.
control.addOverlay(layer, name, position);
Member

patrickarlt commented Jan 28, 2016

I look a look at the code. It looks like layers are stored internally in the control by the layers internal Leaflet ID https://github.com/Leaflet/Leaflet/blob/master/src/control/Control.Layers.js#L115-L121. The internal ID generator gets called A LOT. For example in my JS Bin http://jsbin.com/vocigipive/1/edit?html,css,js,output it is already at 177 by the time everything gets done.

Since all browsers sort objects when looping like for (i in this._layers) by the order in which they are defined http://stackoverflow.com/questions/280713/elements-order-in-a-for-in-loop this means that you cannot really rely on any specific ordering when using L.Control.layers.

@mourner I'm wondering if this could be refactored to use an array of layers internally rather then an object.

// convert basemaps and overlays to ordered arrays internally
L.control.layers({
    'OSM': L.tileLayer('http://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png').addTo(map)
}, {
    'bottom': L.geoJson(geojson, {pane: 'bottom'}),
    'middle': L.geoJson(geojson, {pane: 'middle'}),
    'top':  L.geoJson(geojson, {pane: 'top'}),,
}).addTo(map);

 // add a new position property to control where the added layer falls in the order. Default would be at the bottom.
control.addOverlay(layer, name, position);
@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Feb 12, 2016

Member

@patrickarlt I think that's a great approach. I would be up for a PR on this.

Member

mourner commented Feb 12, 2016

@patrickarlt I think that's a great approach. I would be up for a PR on this.

@mourner mourner modified the milestones: 1.0, 1.0-rc1 Feb 12, 2016

jieter added a commit to jieter/Leaflet that referenced this issue Feb 12, 2016

jieter added a commit to jieter/Leaflet that referenced this issue Feb 12, 2016

jieter added a commit to jieter/Leaflet that referenced this issue Feb 12, 2016

jieter added a commit to jieter/Leaflet that referenced this issue Feb 16, 2016

jieter added a commit to jieter/Leaflet that referenced this issue Mar 22, 2016

@yohanboniface

This comment has been minimized.

Show comment
Hide comment
@yohanboniface

yohanboniface Apr 2, 2016

Member

@patrickarlt up for working on this before 1.0 or should I postpone? :)

Member

yohanboniface commented Apr 2, 2016

@patrickarlt up for working on this before 1.0 or should I postpone? :)

@jieter

This comment has been minimized.

Show comment
Hide comment
@jieter

jieter Apr 2, 2016

Contributor

Huh, I already made a PR for this...

Contributor

jieter commented Apr 2, 2016

Huh, I already made a PR for this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment