removeLayer may break feature selection

[elemoine]

Removing a layer may prevent feature selection from working.

Example scenario

• add a base layer
• add a vector layer
• add overlay 1
• add overlay 2
• add a select feature control (here the vector layer's z-index is set to a high value so that feature selection work even though some overlays were added to the map after the vector layer)
• remove overlay 2

After the removal of overlay 2, every layer's z-index is reset (based on the layer idx). Thus, feature selection no longer works because the vector layer has been placed under overlay 1.

[elemoine]

Except MultiMap which doesn't pass on current trunk, all unit tests pass on FF2, FF3, IE6 and IE7. Please review.

[euzuro]

maybe see #1147

[ahocevar]

Eric, although your patch solves the issue in the example, it does not solve the entire problem. If the Feature handler is deactivated after a layer was removed, the layer will be at a wrong z-index because of the z-index stored at handler activation time, which does not reflect the changes caused by removing a layer.

Instead of storing the layerIndex as member variable, the Feature handler should at least check for the layer's map in its deactivate method, and do a map.resetLayersZIndex() if found.

[ahocevar]

After thinking about it again, a completely different approach might be better: store the layer's zIndexBase on the layer, and use that in Map::setLayerZIndex. The layer's zIndexBase would have to be changed when a layer gets or loses baseLayer status, and when a Feature handler starts/stops using the layer. Map.Z_INDEX_BASE should perhaps get another entry, e.g. "FeatureHandler".

The advantage of this method would be that the order of layers relative to each other always gets respected within the scope of a zIndexBase, even if multiple layers have active Handler.Feature controls. This would, however, break a common workaround that people are using to change the selection layer. They seem to deactivate and activate the SelectFeature control, whereas with this new method they would have to move the layer up or down in the map's layer stack using Map::raiseLayer.

[ahocevar]

Wait. The aim is to allow selection for multiple layers, right? (See #434). So why should we put properties on the layer that we only need on the current implementation of the feature handler? The whole thing could also be solved by listening for the map events "removelayer" and "changelayer". Those listeners should be added in activate(), and removed in deactivate().

Patch to come.

[ahocevar]

Added patch as described above. Tests pass in FF2 and IE7.

[elemoine]

Thanks a lot for picking this up Andreas. Question: what if there are multiple feature handlers activated and one them gets deactivated? It seems to me that the layer z-index will be set back to index-based value, preventing the handlers that are still activated from working. Unless I'm missing something...

[ahocevar]

Replying to elemoine:

Question: what if there are multiple feature handlers activated and one them gets deactivated? It seems to me that the layer z-index will be set back to index-based value, preventing the handlers that are still activated from working. Unless I'm missing something...

You mean if there are many Feature handlers on one layer? That's a good point.

[ahocevar]

1628-r7874.B1.patch makes one minor change to Map.js (adding a new Z_INDEX_BASE called "Feature", between "Overlay" and "Popup"). The Feature handler itself now uses the zIndex of the layer to determine if it is the first/last Feature handler on the layer or not, and sets the layer zIndex accordingly.

This solves the problem pointed out by elemoine. Tests still pass in FF2 and IE7, and the manual test has been modified to test the correct behaviour.

[elemoine]

Replying to ahocevar:

Replying to elemoine:

Question: what if there are multiple feature handlers activated and one them gets deactivated? It seems to me that the layer z-index will be set back to index-based value, preventing the handlers that are still activated from working. Unless I'm missing something...

You mean if there are many Feature handlers on one layer? That's a good point.

Yes, that's what I meant. This can easily occur if one uses, for example, a select or modify feature control and a home-made, feature handler-based control.

[elemoine]

Replying to ahocevar:

1628-r7874.B1.patch makes one minor change to Map.js (adding a new Z_INDEX_BASE called "Feature", between "Overlay" and "Popup"). The Feature handler itself now uses the zIndex of the layer to determine if it is the first/last Feature handler on the layer or not, and sets the layer zIndex accordingly. This solves the problem pointed out by elemoine. Tests still pass in FF2 and IE7, and the manual test has been modified to test the correct behaviour.

I don't get it.

Why do you attempt to move the layer's z-index back to its index-based value on change-layer-order events? If one activates a select feature control, then raises some non-vector layer's z-index using map.raiseLayer, then the select feature control will stop functioning because the vector layer's z-index is back to its index-based value. That doesn't make sense to me.

To me the removelayer and changelayer events correspond to same thing from the feature handler's perspective: the vector layer's z-index may have been set back to its index-based value. So the feature handler should behave the same on both event types and place the vector layer on top.

I'm pretty sure I'm missing the point of your patch, I just don't see what... Please enlighten me...

A couple of other comments:

• the ND comment of the moveLayerBack makes it believe that method unconditionally sets the layer z-index to its index-based value
• the ND comments of the moveLayerBack include "@param"
• OpenLayers.Util.indexOf(this.map.layers, this.layer) could be replaced by this.map.getLayerIndex(this.layer)
• in moveLayerToTop getting the layer z-index using layer.div.style.zIndex and setting it using layer.setZIndex is a bit confusing, I would either add a getZIndex method to Layer or use layer.div.style.zIndex for getting the value as well for setting it in moveLayerToTop

[ahocevar]

new patch with a separate handleMapEvents listener method

[elemoine]

I've done some testing with the patch. It works great. Please commit if the unit tests pass. Thanks a lot for the collaboration on this ticket Andreas.

[ahocevar]

(In [7879]) Made the Feature handler more robust to things that are related to changing layer order on the map, by registering an event handler that will bring the handler's layer back to the top of the layer stack in the DOM. Contains a manual test. r=elemoine (closes #1628)