From c3226d2eed2db171f2d1666f2147db46337ae855 Mon Sep 17 00:00:00 2001 From: johndoe Date: Fri, 2 Apr 2021 15:17:23 +0300 Subject: [PATCH 1/2] 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 | 27 +++++++++++++++++ src/layer/DivOverlay.js | 54 +++++++++++++++++++++------------- src/layer/Popup.js | 16 +++++----- src/layer/Tooltip.js | 10 +++---- 4 files changed, 71 insertions(+), 36 deletions(-) diff --git a/spec/suites/layer/PopupSpec.js b/spec/suites/layer/PopupSpec.js index 019ef49006a..3d980a3305f 100644 --- a/spec/suites/layer/PopupSpec.js +++ b/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); + }); }); diff --git a/src/layer/DivOverlay.js b/src/layer/DivOverlay.js index 7bbd37b4198..83c9fe12185 100644 --- a/src/layer/DivOverlay.js +++ b/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 () { diff --git a/src/layer/Popup.js b/src/layer/Popup.js index f74fe15f892..12bcf7234e0 100644 --- a/src/layer/Popup.js +++ b/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); } }, diff --git a/src/layer/Tooltip.js b/src/layer/Tooltip.js index 2afc3b55fef..32a14ffaefb 100644 --- a/src/layer/Tooltip.js +++ b/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 (target, latlng) { + if (this._tooltip && this._tooltip._prepareOpen(target, latlng)) { // open the tooltip on the map this._map.openTooltip(this._tooltip, latlng); } @@ -386,12 +384,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) { From b8f25fd2711aa46da479374c3e04c6f1b2afa93d Mon Sep 17 00:00:00 2001 From: johndoe Date: Thu, 1 Apr 2021 17:38:10 +0300 Subject: [PATCH 2/2] DivOverlay/Popup/Tooltip: refactor to avoid of undocumented arguments for public methods --- src/layer/DivOverlay.js | 9 ++------- src/layer/Popup.js | 40 ++++++++++++++++------------------------ src/layer/Tooltip.js | 14 +++++++------- 3 files changed, 25 insertions(+), 38 deletions(-) diff --git a/src/layer/DivOverlay.js b/src/layer/DivOverlay.js index 83c9fe12185..36c0b241fee 100644 --- a/src/layer/DivOverlay.js +++ b/src/layer/DivOverlay.js @@ -160,13 +160,8 @@ export var DivOverlay = Layer.extend({ }, // 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; - } + _prepareOpen: function (latlng) { + var source = this._source; if (!source._map) { return false; } if (source instanceof FeatureGroup) { diff --git a/src/layer/Popup.js b/src/layer/Popup.js index 12bcf7234e0..99bd0ea4fb2 100644 --- a/src/layer/Popup.js +++ b/src/layer/Popup.js @@ -416,8 +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 (target, latlng) { - if (this._popup && this._popup._prepareOpen(target, latlng)) { + openPopup: function (latlng) { + if (this._popup && this._popup._prepareOpen(latlng)) { // open the popup on the map this._map.openPopup(this._popup, latlng); } @@ -436,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; @@ -469,33 +469,25 @@ Layer.include({ }, _openPopup: function (e) { - var target = 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 (target instanceof Path) { - this.openPopup(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 === target) { - this.closePopup(); - } else { - this.openPopup(target, e.latlng); - } + this._popup._source = target; + this.openPopup(e.latlng); }, _movePopup: function (e) { diff --git a/src/layer/Tooltip.js b/src/layer/Tooltip.js index 32a14ffaefb..fd0320c7d54 100644 --- a/src/layer/Tooltip.js +++ b/src/layer/Tooltip.js @@ -331,8 +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 (target, latlng) { - if (this._tooltip && this._tooltip._prepareOpen(target, latlng)) { + openTooltip: function (latlng) { + if (this._tooltip && this._tooltip._prepareOpen(latlng)) { // open the tooltip on the map this._map.openTooltip(this._tooltip, latlng); } @@ -351,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; @@ -384,12 +384,12 @@ Layer.include({ }, _openTooltip: function (e) { - var target = e.layer || e.target; - if (!this._tooltip || !this._map) { return; } - this.openTooltip(target, 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) {