From 34dc53812274638ceaa731c932ab5fd2f5139e76 Mon Sep 17 00:00:00 2001 From: Matt Walker Date: Fri, 10 Jan 2020 10:48:15 +0000 Subject: [PATCH 1/5] Stop events that originate with a removed target As discussed in https://github.com/openlayers/openlayers/issues/6948#issuecomment-565375694 The check to see if the target is within the "page" uses the viewport as the MapBrowserEventHandler instance adds it's listeners to the viewport. Using Node.contains appears to have a slight performance benefit over manually walking the DOM. --- src/ol/PluggableMap.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/ol/PluggableMap.js b/src/ol/PluggableMap.js index fac2b185e2..f5252ed81b 100644 --- a/src/ol/PluggableMap.js +++ b/src/ol/PluggableMap.js @@ -934,11 +934,13 @@ class PluggableMap extends BaseObject { } let target = /** @type {Node} */ (mapBrowserEvent.originalEvent.target); if (!mapBrowserEvent.dragging) { - while (target && target !== this.viewport_) { - if (target.parentElement === this.overlayContainerStopEvent_) { - return; - } - target = target.parentElement; + if (this.overlayContainerStopEvent_.contains(target) || !this.viewport_.contains(target)) { + // Abort if the event target is a child of the container that doesn't allow + // event propagation or is no longer in the page. It's possible for the target to no longer + // be in the page if it has been removed in an event listener, this might happen in a Control + // that recreates it's content based on user interaction either manually or via a render + // in something like https://reactjs.org/ + return; } } mapBrowserEvent.frameState = this.frameState_; From f3d94b31324266f98f1e42602d2aba49abcb9547 Mon Sep 17 00:00:00 2001 From: Matt Walker Date: Fri, 10 Jan 2020 11:16:54 +0000 Subject: [PATCH 2/5] Fix lint error --- src/ol/PluggableMap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ol/PluggableMap.js b/src/ol/PluggableMap.js index f5252ed81b..1966a43035 100644 --- a/src/ol/PluggableMap.js +++ b/src/ol/PluggableMap.js @@ -932,7 +932,7 @@ class PluggableMap extends BaseObject { // coordinates so interactions cannot be used. return; } - let target = /** @type {Node} */ (mapBrowserEvent.originalEvent.target); + const target = /** @type {Node} */ (mapBrowserEvent.originalEvent.target); if (!mapBrowserEvent.dragging) { if (this.overlayContainerStopEvent_.contains(target) || !this.viewport_.contains(target)) { // Abort if the event target is a child of the container that doesn't allow From 5ce532e3e44cd329505c42c92f7905ed24d95b39 Mon Sep 17 00:00:00 2001 From: Matt Walker Date: Mon, 13 Jan 2020 10:46:40 +0000 Subject: [PATCH 3/5] Mock PointerEvent in tests to include target The `target` Event property is readonly as it is set internally when an event is dispatched. This change uses a plain object with the essential properties that a PointerEvent has which is sufficent for map event handling --- test/spec/ol/interaction/draw.test.js | 3 ++- test/spec/ol/interaction/extent.test.js | 3 ++- test/spec/ol/interaction/modify.test.js | 1 + test/spec/ol/interaction/select.test.js | 6 ++++-- test/spec/ol/interaction/translate.test.js | 16 +++++++++------- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/test/spec/ol/interaction/draw.test.js b/test/spec/ol/interaction/draw.test.js index 7a12b66887..835d765b55 100644 --- a/test/spec/ol/interaction/draw.test.js +++ b/test/spec/ol/interaction/draw.test.js @@ -73,8 +73,9 @@ describe('ol.interaction.Draw', function() { // calculated in case body has top < 0 (test runner with small window) const position = viewport.getBoundingClientRect(); const shiftKey = opt_shiftKey !== undefined ? opt_shiftKey : false; - const event = new Event(); + const event = {}; event.type = type; + event.target = viewport.firstChild; event.clientX = position.left + x + width / 2; event.clientY = position.top + y + height / 2; event.shiftKey = shiftKey; diff --git a/test/spec/ol/interaction/extent.test.js b/test/spec/ol/interaction/extent.test.js index 56fd15765e..7bd5f9e86c 100644 --- a/test/spec/ol/interaction/extent.test.js +++ b/test/spec/ol/interaction/extent.test.js @@ -50,8 +50,9 @@ describe('ol.interaction.Extent', function() { // calculated in case body has top < 0 (test runner with small window) const position = viewport.getBoundingClientRect(); const shiftKey = opt_shiftKey !== undefined ? opt_shiftKey : false; - const pointerEvent = new Event(); + const pointerEvent = {}; pointerEvent.type = type; + pointerEvent.target = viewport.firstChild; pointerEvent.button = button; pointerEvent.clientX = position.left + x + width / 2; pointerEvent.clientY = position.top - y + height / 2; diff --git a/test/spec/ol/interaction/modify.test.js b/test/spec/ol/interaction/modify.test.js index 7deae8d5d7..18b2c28063 100644 --- a/test/spec/ol/interaction/modify.test.js +++ b/test/spec/ol/interaction/modify.test.js @@ -84,6 +84,7 @@ describe('ol.interaction.Modify', function() { const position = viewport.getBoundingClientRect(); const pointerEvent = new Event(); pointerEvent.type = type; + pointerEvent.target = viewport.firstChild; pointerEvent.clientX = position.left + x + width / 2; pointerEvent.clientY = position.top + y + height / 2; pointerEvent.shiftKey = modifiers.shift || false; diff --git a/test/spec/ol/interaction/select.test.js b/test/spec/ol/interaction/select.test.js index 50f7bb2c70..e64eb2dc28 100644 --- a/test/spec/ol/interaction/select.test.js +++ b/test/spec/ol/interaction/select.test.js @@ -91,11 +91,13 @@ describe('ol.interaction.Select', function() { // calculated in case body has top < 0 (test runner with small window) const position = viewport.getBoundingClientRect(); const shiftKey = opt_shiftKey !== undefined ? opt_shiftKey : false; - const event = new PointerEvent(type, { + const event = { + type: type, + target: viewport.firstChild, clientX: position.left + x + width / 2, clientY: position.top + y + height / 2, shiftKey: shiftKey - }); + }; map.handleMapBrowserEvent(new MapBrowserPointerEvent(type, map, event)); } diff --git a/test/spec/ol/interaction/translate.test.js b/test/spec/ol/interaction/translate.test.js index a3df2ccebe..79efde34bb 100644 --- a/test/spec/ol/interaction/translate.test.js +++ b/test/spec/ol/interaction/translate.test.js @@ -65,13 +65,15 @@ describe('ol.interaction.Translate', function() { // calculated in case body has top < 0 (test runner with small window) const position = viewport.getBoundingClientRect(); const shiftKey = opt_shiftKey !== undefined ? opt_shiftKey : false; - const event = new MapBrowserPointerEvent(type, map, - new PointerEvent(type, { - clientX: position.left + x + width / 2, - clientY: position.top + y + height / 2, - shiftKey: shiftKey, - preventDefault: function() {} - })); + const event = new MapBrowserPointerEvent(type, map, { + type: type, + target: viewport.firstChild, + pointerId: 0, + clientX: position.left + x + width / 2, + clientY: position.top + y + height / 2, + shiftKey: shiftKey, + preventDefault: function() {} + }); map.handleMapBrowserEvent(event); } From 4e599a370ba95f31d8b27c12bed551ce5b68afaa Mon Sep 17 00:00:00 2001 From: Matt Walker Date: Mon, 13 Jan 2020 10:56:41 +0000 Subject: [PATCH 4/5] Use document.body to check if an event target is within the page Some events will originate outside the map viewport such as keyboard events which originate with the element specified by keyboardEventTarget which could be document.body --- src/ol/PluggableMap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ol/PluggableMap.js b/src/ol/PluggableMap.js index 1966a43035..75c1f4a7be 100644 --- a/src/ol/PluggableMap.js +++ b/src/ol/PluggableMap.js @@ -934,7 +934,7 @@ class PluggableMap extends BaseObject { } const target = /** @type {Node} */ (mapBrowserEvent.originalEvent.target); if (!mapBrowserEvent.dragging) { - if (this.overlayContainerStopEvent_.contains(target) || !this.viewport_.contains(target)) { + if (this.overlayContainerStopEvent_.contains(target) || !document.body.contains(target)) { // Abort if the event target is a child of the container that doesn't allow // event propagation or is no longer in the page. It's possible for the target to no longer // be in the page if it has been removed in an event listener, this might happen in a Control From eeec2b9e7db4a586234607877f340116906df105 Mon Sep 17 00:00:00 2001 From: Matt Walker Date: Mon, 13 Jan 2020 11:32:40 +0000 Subject: [PATCH 5/5] Lint: remove unused imports --- test/spec/ol/interaction/draw.test.js | 1 - test/spec/ol/interaction/extent.test.js | 1 - 2 files changed, 2 deletions(-) diff --git a/test/spec/ol/interaction/draw.test.js b/test/spec/ol/interaction/draw.test.js index 835d765b55..fc571891d8 100644 --- a/test/spec/ol/interaction/draw.test.js +++ b/test/spec/ol/interaction/draw.test.js @@ -15,7 +15,6 @@ import Polygon from '../../../../src/ol/geom/Polygon.js'; import Draw, {createRegularPolygon, createBox} from '../../../../src/ol/interaction/Draw.js'; import Interaction from '../../../../src/ol/interaction/Interaction.js'; import VectorLayer from '../../../../src/ol/layer/Vector.js'; -import Event from '../../../../src/ol/events/Event.js'; import VectorSource from '../../../../src/ol/source/Vector.js'; import {clearUserProjection, setUserProjection, transform} from '../../../../src/ol/proj.js'; import {register} from '../../../../src/ol/proj/proj4.js'; diff --git a/test/spec/ol/interaction/extent.test.js b/test/spec/ol/interaction/extent.test.js index 7bd5f9e86c..63fc0e59c9 100644 --- a/test/spec/ol/interaction/extent.test.js +++ b/test/spec/ol/interaction/extent.test.js @@ -2,7 +2,6 @@ import Map from '../../../../src/ol/Map.js'; import MapBrowserPointerEvent from '../../../../src/ol/MapBrowserPointerEvent.js'; import View from '../../../../src/ol/View.js'; import ExtentInteraction from '../../../../src/ol/interaction/Extent.js'; -import Event from '../../../../src/ol/events/Event.js'; describe('ol.interaction.Extent', function() { let map, interaction;