DivOverlay: refactor _prepareOpen method (and related changes)

1. Fix a couple of edge cases:
   - layer is empty FeatureGroup
   - FeatureGroup contains layers that are not on map

2. Make code logic more clear:
   - get rid of redundant first argument, as bound overlay always contains _source prop.
   - rename `layer` to `source` for the sake of clarity.
   - openPopup/Tooltip: rename `layer` to `target` for the sake of clarity.

3. Allow to prevent overlay opening gracefully on some conditions (like empty FeatureGroup)

spec/suites/layer/PopupSpec.js

@@ -479,4 +479,31 @@ describe('L.Layer#_popup', function () {
 			done();
 		}).to.not.throwException();
 	});
+
+	it("does not open for empty FeatureGroup", function () {
+		var popup = L.popup();
+		L.featureGroup([])
+		  .addTo(map)
+		  .bindPopup(popup)
+		  .openPopup();
+
+		expect(map.hasLayer(popup)).to.not.be.ok();
+	});
+
+	it("uses only visible layers of FeatureGroup for popup content source", function () {
+		var marker1 = L.marker([1, 1]);
+		var marker2 = L.marker([2, 2]);
+		var marker3 = L.marker([3, 3]);
+		var popup = L.popup();
+		var group = L.featureGroup([marker1, marker2, marker3])
+		  .bindPopup(popup)
+		  .addTo(map);
+
+		marker1.remove();
+		marker3.remove();
+		group.openPopup();
+
+		expect(map.hasLayer(popup)).to.be.ok();
+		expect(popup._source).to.be(marker2);
+	});
 });

src/layer/DivOverlay.js

@@ -159,38 +159,50 @@ export var DivOverlay = Layer.extend({
 		return this;
 	},
 
-	_prepareOpen: function (parent, layer, latlng) {
-		if (!(layer instanceof Layer)) {
-			latlng = layer;
-			layer = parent;
+	// prepare bound overlay to open: update latlng pos / content source (for FeatureGroup)
+	_prepareOpen: function (source, latlng) {
+		if (source instanceof Layer) {
+			this._source = source;
+		} else {
+			latlng = source;
+			source = this._source;
 		}
-
-		if (layer instanceof FeatureGroup) {
-			for (var id in parent._layers) {
-				layer = parent._layers[id];
-				break;
+		if (!source._map) { return false; }
+
+		if (source instanceof FeatureGroup) {
+			source = null;
+			var layers = this._source._layers;
+			for (var id in layers) {
+				if (layers[id]._map) {
+					source = layers[id];
+					break;
+				}
 			}
+			if (!source) { return false; } // Unable to get source layer.
+
+			// set overlay source to this layer
+			this._source = source;
 		}
 
 		if (!latlng) {
-			if (layer.getCenter) {
-				latlng = layer.getCenter();
-			} else if (layer.getLatLng) {
-				latlng = layer.getLatLng();
-			} else if (layer.getBounds) {
-				latlng = layer.getBounds().getCenter();
+			if (source.getCenter) {
+				latlng = source.getCenter();
+			} else if (source.getLatLng) {
+				latlng = source.getLatLng();
+			} else if (source.getBounds) {
+				latlng = source.getBounds().getCenter();
 			} else {
 				throw new Error('Unable to get source layer LatLng.');
 			}
 		}
+		this.setLatLng(latlng);
 
-		// set overlay source to this layer
-		this._source = layer;
-
-		// update the overlay (content, layout, ect...)
-		this.update();
+		if (this._map) {
+			// update the overlay (content, layout, etc...)
+			this.update();
+		}
 
-		return latlng;
+		return true;
 	},
 
 	_updateContent: function () {

src/layer/Popup.js

@@ -416,10 +416,8 @@ Layer.include({
 
 	// @method openPopup(latlng?: LatLng): this
 	// Opens the bound popup at the specified `latlng` or at the default popup anchor if no `latlng` is passed.
-	openPopup: function (layer, latlng) {
-		if (this._popup && this._map) {
-			latlng = this._popup._prepareOpen(this, layer, latlng);
-
+	openPopup: function (target, latlng) {
+		if (this._popup && this._popup._prepareOpen(target, latlng)) {
 			// open the popup on the map
 			this._map.openPopup(this._popup, latlng);
 		}
@@ -471,7 +469,7 @@ Layer.include({
 	},
 
 	_openPopup: function (e) {
-		var layer = e.layer || e.target;
+		var target = e.layer || e.target;
 
 		if (!this._popup) {
 			return;
@@ -486,17 +484,17 @@ Layer.include({
 
 		// if this inherits from Path its a vector and we can just
 		// open the popup at the new location
-		if (layer instanceof Path) {
-			this.openPopup(e.layer || e.target, e.latlng);
+		if (target instanceof Path) {
+			this.openPopup(target, e.latlng);
 			return;
 		}
 
 		// otherwise treat it like a marker and figure out
 		// if we should toggle it open/closed
-		if (this._map.hasLayer(this._popup) && this._popup._source === layer) {
+		if (this._map.hasLayer(this._popup) && this._popup._source === target) {
 			this.closePopup();
 		} else {
-			this.openPopup(layer, e.latlng);
+			this.openPopup(target, e.latlng);
 		}
 	},
 

src/layer/Tooltip.js

@@ -324,10 +324,8 @@ Layer.include({
 
 	// @method openTooltip(latlng?: LatLng): this
 	// Opens the bound tooltip at the specified `latlng` or at the default tooltip anchor if no `latlng` is passed.
-	openTooltip: function (layer, latlng) {
-		if (this._tooltip && this._map) {
-			latlng = this._tooltip._prepareOpen(this, layer, latlng);
-
+	openTooltip: function (target, latlng) {
+		if (this._tooltip && this._tooltip._prepareOpen(target, latlng)) {
 			// open the tooltip on the map
 			this._map.openTooltip(this._tooltip, latlng);
 
@@ -390,12 +388,12 @@ Layer.include({
 	},
 
 	_openTooltip: function (e) {
-		var layer = e.layer || e.target;
+		var target = e.layer || e.target;
 
 		if (!this._tooltip || !this._map) {
 			return;
 		}
-		this.openTooltip(layer, this._tooltip.options.sticky ? e.latlng : undefined);
+		this.openTooltip(target, this._tooltip.options.sticky ? e.latlng : undefined);
 	},
 
 	_moveTooltip: function (e) {