DivOverlay/Popup/Tooltip refactoring and fixes (#7540)

spec/suites/layer/PopupSpec.js

@@ -499,4 +499,31 @@ describe('L.Layer#_popup', function () {
 		});
 		expect(popup.isOpen()).to.be.ok();
 	});
+
+	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,45 @@ export var DivOverlay = Layer.extend({
 		return this;
 	},
 
-	_prepareOpen: function (parent, layer, latlng) {
-		if (!(layer instanceof Layer)) {
-			latlng = layer;
-			layer = parent;
-		}
-
-		if (layer instanceof FeatureGroup) {
-			for (var id in parent._layers) {
-				layer = parent._layers[id];
-				break;
+	// prepare bound overlay to open: update latlng pos / content source (for FeatureGroup)
+	_prepareOpen: function (latlng) {
+		var source = this._source;
+		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 (latlng) {
+		if (this._popup && this._popup._prepareOpen(latlng)) {
 			// open the popup on the map
 			this._map.openPopup(this._popup, latlng);
 		}
@@ -438,12 +436,12 @@ Layer.include({
 
 	// @method togglePopup(): this
 	// Opens or closes the popup bound to this layer depending on its current state.
-	togglePopup: function (target) {
+	togglePopup: function () {
 		if (this._popup) {
 			if (this._popup._map) {
 				this.closePopup();
 			} else {
-				this.openPopup(target);
+				this.openPopup();
 			}
 		}
 		return this;
@@ -471,33 +469,25 @@ Layer.include({
 	},
 
 	_openPopup: function (e) {
-		var layer = e.layer || e.target;
-
-		if (!this._popup) {
+		if (!this._popup || !this._map) {
 			return;
 		}
-
-		if (!this._map) {
-			return;
-		}
-
 		// prevent map click
 		DomEvent.stop(e);
 
-		// 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);
+		var target = e.layer || e.target;
+		if (this._popup._source === target && !(target instanceof Path)) {
+			// treat it like a marker and figure out
+			// if we should toggle it open/closed
+			if (this._map.hasLayer(this._popup)) {
+				this.closePopup();
+			} else {
+				this.openPopup(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) {
-			this.closePopup();
-		} else {
-			this.openPopup(layer, e.latlng);
-		}
+		this._popup._source = target;
+		this.openPopup(e.latlng);
 	},
 
 	_movePopup: function (e) {

src/layer/Tooltip.js

@@ -331,10 +331,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 (latlng) {
+		if (this._tooltip && this._tooltip._prepareOpen(latlng)) {
 			// open the tooltip on the map
 			this._map.openTooltip(this._tooltip, latlng);
 		}
@@ -353,12 +351,12 @@ Layer.include({
 
 	// @method toggleTooltip(): this
 	// Opens or closes the tooltip bound to this layer depending on its current state.
-	toggleTooltip: function (target) {
+	toggleTooltip: function () {
 		if (this._tooltip) {
 			if (this._tooltip._map) {
 				this.closeTooltip();
 			} else {
-				this.openTooltip(target);
+				this.openTooltip();
 			}
 		}
 		return this;
@@ -386,12 +384,12 @@ Layer.include({
 	},
 
 	_openTooltip: function (e) {
-		var layer = e.layer || e.target;
-
 		if (!this._tooltip || !this._map) {
 			return;
 		}
-		this.openTooltip(layer, this._tooltip.options.sticky ? e.latlng : undefined);
+		this._tooltip._source = e.layer || e.target;
+
+		this.openTooltip(this._tooltip.options.sticky ? e.latlng : undefined);
 	},
 
 	_moveTooltip: function (e) {