From fcc33c1f4133d63ddcf6973dec53f802451da749 Mon Sep 17 00:00:00 2001 From: Ansis Brammanis Date: Thu, 14 May 2020 13:21:10 -0400 Subject: [PATCH] map.isMoving() match move events fix #9647 (#9679) * 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. * lint * fix test --- src/ui/handler_manager.js | 32 ++++++++++++++++++------ src/ui/map.js | 2 +- test/unit/ui/handler/drag_rotate.test.js | 2 ++ test/unit/ui/map/isMoving.test.js | 12 +++++++++ test/unit/ui/map_events.test.js | 29 +++++++++++++++++++++ 5 files changed, 68 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, 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..25da019b54b 100644 --- a/test/unit/ui/handler/drag_rotate.test.js +++ b/test/unit/ui/handler/drag_rotate.test.js @@ -273,9 +273,11 @@ 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}); + map._renderTaskQueue.run(); t.ok(!map.isMoving()); map.remove(); 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..a0cd8d99d78 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(); +});