OpenLayers OpenLayers

Ticket #1628 (closed feature: fixed)

Opened 2 years ago

Last modified 2 years ago

removeLayer may break feature selection

Reported by: elemoine Assigned to: ahocevar
Priority: critical Milestone: 2.7 Release
Component: Handler.Feature Version: SVN
Keywords: Cc:
State: Complete

Description

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.

Attachments

patch-1628-r7554-A0.diff (7.3 kB) - added by elemoine on 07/24/08 06:51:22.
1628-r7864.B0.patch (9.0 kB) - added by ahocevar on 08/26/08 12:30:50.
1628-r7874.B1.patch (11.0 kB) - added by ahocevar on 08/27/08 03:48:02.
1628-r7874.B2.patch (11.8 kB) - added by ahocevar on 08/27/08 07:49:28.
1628-r7874.B3.patch (11.8 kB) - added by ahocevar on 08/27/08 08:02:07.
new patch with a separate handleMapEvents listener method

Change History

07/24/08 06:51:22 changed by elemoine

  • attachment patch-1628-r7554-A0.diff added.

07/24/08 06:53:07 changed by elemoine

  • state set to Review.

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

07/28/08 14:11:47 changed by euzuro

  • owner changed from elemoine to ahocevar.

maybe see #1147

07/28/08 14:15:48 changed by euzuro

  • priority changed from major to critical.

08/26/08 10:37:03 changed by ahocevar

  • state changed from Review to Needs More Work.

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.

08/26/08 11:11:02 changed by 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.

08/26/08 11:28:14 changed by 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.

08/26/08 12:30:50 changed by ahocevar

  • attachment 1628-r7864.B0.patch added.

08/26/08 12:34:21 changed by ahocevar

  • state changed from Needs More Work to Review.

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

(follow-up: ↓ 9 ) 08/27/08 02:30:55 changed by 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...

(in reply to: ↑ 8 ; follow-up: ↓ 11 ) 08/27/08 03:00:40 changed by ahocevar

  • state changed from Review to Needs More Work.

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.

08/27/08 03:48:02 changed by ahocevar

  • attachment 1628-r7874.B1.patch added.

(follow-up: ↓ 12 ) 08/27/08 03:51:02 changed by ahocevar

  • state changed from Needs More Work to Review.

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.

(in reply to: ↑ 9 ) 08/27/08 06:26:35 changed by 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.

(in reply to: ↑ 10 ) 08/27/08 07:06:09 changed by 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

08/27/08 07:49:28 changed by ahocevar

  • attachment 1628-r7874.B2.patch added.

08/27/08 08:02:07 changed by ahocevar

  • attachment 1628-r7874.B3.patch added.

new patch with a separate handleMapEvents listener method

08/27/08 08:41:44 changed by 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.

08/27/08 08:41:48 changed by elemoine

  • state changed from Review to Commit.

08/27/08 09:02:22 changed by ahocevar

  • status changed from new to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.

(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)

09/04/08 20:58:20 changed by euzuro

  • component changed from Layer.Vector to Handler.Feature.