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.
mapbox · May 12, 2020 · 9b2b36a · 9b2b36a
1 parent fb6d18d
commit 9b2b36a
Show file tree

Hide file tree

Showing 5 changed files with 67 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,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
@@ -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
@@ -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();
+});