From 9b2b36aae048fd04a639c7d0db89e92d8487e821 Mon Sep 17 00:00:00 2001 From: Ansis Brammanis <ansis@mapbox.com> Date: Tue, 12 May 2020 13:24:41 -0400 Subject: [PATCH 1/3] map.isMoving() match move events fix #9647 Previously isMoving() would return true if any interaction handler was active. Handlers are sometimes active even if they haven't changed the map yet. This resulted in the isMoving() returning true when the map hasn't moved. This makes isMoving aligned with movestart/move/moveend events. Since move events may be fired after several mouse events have been batched, the camera changes a mouseevent will *later* cause won't be reflected in isMoving() when that mouseevent is fired. --- src/ui/handler_manager.js | 32 ++++++++++++++++++------ src/ui/map.js | 2 +- test/unit/ui/handler/drag_rotate.test.js | 1 + test/unit/ui/map/isMoving.test.js | 12 +++++++++ test/unit/ui/map_events.test.js | 29 +++++++++++++++++++++ 5 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/ui/handler_manager.js b/src/ui/handler_manager.js index 1589e1d0f59..63160fc020b 100644 --- a/src/ui/handler_manager.js +++ b/src/ui/handler_manager.js @@ -259,6 +259,10 @@ class HandlerManager { return !!this._eventsInProgress.rotate; } + isMoving() { + return Boolean(isMoving(this._eventsInProgress)) || this.isZooming(); + } + _blockedByActive(activeHandlers: { [string]: Handler }, allowed: Array<string>, myName: string) { for (const name in activeHandlers) { if (name === myName) continue; @@ -430,17 +434,23 @@ class HandlerManager { const wasMoving = isMoving(this._eventsInProgress); const nowMoving = isMoving(newEventsInProgress); - if (!wasMoving && nowMoving) { - this._fireEvent('movestart', nowMoving.originalEvent); - } + const startEvents = {}; for (const eventName in newEventsInProgress) { const {originalEvent} = newEventsInProgress[eventName]; - const isStart = !this._eventsInProgress[eventName]; - this._eventsInProgress[eventName] = newEventsInProgress[eventName]; - if (isStart) { - this._fireEvent(`${eventName}start`, originalEvent); + if (!this._eventsInProgress[eventName]) { + startEvents[`${eventName}start`] = originalEvent; } + this._eventsInProgress[eventName] = newEventsInProgress[eventName]; + } + + // fire start events only after this._eventsInProgress has been updated + if (!wasMoving && nowMoving) { + this._fireEvent('movestart', nowMoving.originalEvent); + } + + for (const name in startEvents) { + this._fireEvent(name, startEvents[name]); } if (newEventsInProgress.rotate) this._bearingChanged = true; @@ -454,16 +464,22 @@ class HandlerManager { this._fireEvent(eventName, originalEvent); } + const endEvents = {}; + let originalEndEvent; for (const eventName in this._eventsInProgress) { const {handlerName, originalEvent} = this._eventsInProgress[eventName]; if (!this._handlersById[handlerName].isActive()) { delete this._eventsInProgress[eventName]; originalEndEvent = deactivatedHandlers[handlerName] || originalEvent; - this._fireEvent(`${eventName}end`, originalEndEvent); + endEvents[`${eventName}end`] = originalEndEvent; } } + for (const name in endEvents) { + this._fireEvent(name, endEvents[name]); + } + const stillMoving = isMoving(this._eventsInProgress); if ((wasMoving || nowMoving) && !stillMoving) { this._updatingCamera = true; diff --git a/src/ui/map.js b/src/ui/map.js index 29cd55d40c1..8d726c49e2a 100755 --- a/src/ui/map.js +++ b/src/ui/map.js @@ -853,7 +853,7 @@ class Map extends Camera { * var isMoving = map.isMoving(); */ isMoving(): boolean { - return this._moving || this.handlers.isActive(); + return this._moving || this.handlers.isMoving(); } /** diff --git a/test/unit/ui/handler/drag_rotate.test.js b/test/unit/ui/handler/drag_rotate.test.js index d1107828606..b051ab7a4db 100644 --- a/test/unit/ui/handler/drag_rotate.test.js +++ b/test/unit/ui/handler/drag_rotate.test.js @@ -273,6 +273,7 @@ test('DragRotateHandler ensures that map.isMoving() returns true during drag', ( simulate.mousedown(map.getCanvas(), {buttons: 2, button: 2}); simulate.mousemove(map.getCanvas(), {buttons: 2, clientX: 10, clientY: 10}); + map._renderTaskQueue.run(); t.ok(map.isMoving()); simulate.mouseup(map.getCanvas(), {buttons: 0, button: 2}); diff --git a/test/unit/ui/map/isMoving.test.js b/test/unit/ui/map/isMoving.test.js index 0cf88158a63..f215bb2ddc3 100644 --- a/test/unit/ui/map/isMoving.test.js +++ b/test/unit/ui/map/isMoving.test.js @@ -39,12 +39,18 @@ test('Map#isMoving returns true during a camera zoom animation', (t) => { test('Map#isMoving returns true when drag panning', (t) => { const map = createMap(t); + map.on('movestart', () => { + t.equal(map.isMoving(), true); + }); map.on('dragstart', () => { t.equal(map.isMoving(), true); }); map.on('dragend', () => { t.equal(map.isMoving(), false); + }); + map.on('moveend', () => { + t.equal(map.isMoving(), false); map.remove(); t.end(); }); @@ -65,12 +71,18 @@ test('Map#isMoving returns true when drag rotating', (t) => { // Prevent inertial rotation. t.stub(browser, 'now').returns(0); + map.on('movestart', () => { + t.equal(map.isMoving(), true); + }); map.on('rotatestart', () => { t.equal(map.isMoving(), true); }); map.on('rotateend', () => { t.equal(map.isMoving(), false); + }); + map.on('moveend', () => { + t.equal(map.isMoving(), false); map.remove(); t.end(); }); diff --git a/test/unit/ui/map_events.test.js b/test/unit/ui/map_events.test.js index 21e8041fe88..718cbc678bd 100644 --- a/test/unit/ui/map_events.test.js +++ b/test/unit/ui/map_events.test.js @@ -601,3 +601,32 @@ test(`Map#on click fires subsequent click event if there is no corresponding mou map.remove(); t.end(); }); + +test("Map#isMoving() returns false in mousedown/mouseup/click with no movement", (t) => { + const map = createMap(t, {interactive: true, clickTolerance: 4}); + let mousedown, mouseup, click; + map.on('mousedown', () => mousedown = map.isMoving()); + map.on('mouseup', () => mouseup = map.isMoving()); + map.on('click', () => click = map.isMoving()); + + const canvas = map.getCanvas(); + const MouseEvent = window(canvas).MouseEvent; + + canvas.dispatchEvent(new MouseEvent('mousedown', {bubbles: true, clientX: 100, clientY: 100})); + t.equal(mousedown, false); + map._renderTaskQueue.run(); + t.equal(mousedown, false); + + canvas.dispatchEvent(new MouseEvent('mouseup', {bubbles: true, clientX: 100, clientY: 100})); + t.equal(mouseup, false); + map._renderTaskQueue.run(); + t.equal(mouseup, false); + + canvas.dispatchEvent(new MouseEvent('click', {bubbles: true, clientX: 100, clientY: 100})); + t.equal(click, false); + map._renderTaskQueue.run(); + t.equal(click, false); + + map.remove(); + t.end(); +}); From 73bf826ae90ea839dda2d673e1e77ac6519d968a Mon Sep 17 00:00:00 2001 From: Ansis Brammanis <ansis@mapbox.com> Date: Tue, 12 May 2020 13:39:50 -0400 Subject: [PATCH 2/3] lint --- test/unit/ui/map_events.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/ui/map_events.test.js b/test/unit/ui/map_events.test.js index 718cbc678bd..a0cd8d99d78 100644 --- a/test/unit/ui/map_events.test.js +++ b/test/unit/ui/map_events.test.js @@ -605,9 +605,9 @@ test(`Map#on click fires subsequent click event if there is no corresponding mou test("Map#isMoving() returns false in mousedown/mouseup/click with no movement", (t) => { const map = createMap(t, {interactive: true, clickTolerance: 4}); let mousedown, mouseup, click; - map.on('mousedown', () => mousedown = map.isMoving()); - map.on('mouseup', () => mouseup = map.isMoving()); - map.on('click', () => click = map.isMoving()); + map.on('mousedown', () => { mousedown = map.isMoving(); }); + map.on('mouseup', () => { mouseup = map.isMoving(); }); + map.on('click', () => { click = map.isMoving(); }); const canvas = map.getCanvas(); const MouseEvent = window(canvas).MouseEvent; From 914dd7ef34c10f0ee9ac27d1de54c1c2137d74dc Mon Sep 17 00:00:00 2001 From: Ansis Brammanis <ansis@mapbox.com> Date: Wed, 13 May 2020 12:14:24 -0400 Subject: [PATCH 3/3] fix test --- test/unit/ui/handler/drag_rotate.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/ui/handler/drag_rotate.test.js b/test/unit/ui/handler/drag_rotate.test.js index b051ab7a4db..25da019b54b 100644 --- a/test/unit/ui/handler/drag_rotate.test.js +++ b/test/unit/ui/handler/drag_rotate.test.js @@ -277,6 +277,7 @@ test('DragRotateHandler ensures that map.isMoving() returns true during drag', ( t.ok(map.isMoving()); simulate.mouseup(map.getCanvas(), {buttons: 0, button: 2}); + map._renderTaskQueue.run(); t.ok(!map.isMoving()); map.remove();