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
mapbox · May 14, 2020 · 3b39c58 · 3b39c58
1 parent 1f1b8a5
commit 3b39c58
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 9 deletions.
diff --git 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
@@ -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
@@ -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
@@ -36,12 +36,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();
     });
@@ -62,12 +68,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
@@ -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();
+});