map.isMoving() match move events fix #9647 #9679
Conversation
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.
| @@ -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(); | |||
There was a problem hiding this comment.
This particular test is now failing from CI, is this call actually processing the events correctly?
There was a problem hiding this comment.
I was missing another one of these calls after the next event.
In old releases map.isMoving() would return true immediately after a mousemove event that causes a rotation. Now map.isMoving() only returns true on the next frame when the move events have fired and the camera events have changed. Does this seem ok to you?
There was a problem hiding this comment.
I would not think that having a one frame delay difference for this type of API as being critical. As long as we still cover the following requirements:
Returns true if the map is panning, zooming, rotating, or pitching due to a camera animation or user gesture.
| @@ -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', () => { | |||
There was a problem hiding this comment.
Do you know if we have enough coverage around whether those events are triggered under a similar context?
There was a problem hiding this comment.
This test case is focused around isMoving. But what I'm thinking is covering whether we call the lambda function at all, if we fail to call that lambda it would go unnoticed.
Maybe collecting how many times those lambdas are called and testing that at the end of this block could be useful.
There was a problem hiding this comment.
I think in this case there is enough coverage elsewhere but we could change the test to avoid this problem.
* 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
* 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
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 pr makes isMoving() aligned with movestart/move/moveend events. isMoving() returns true only:
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.
For example, if you pan the map (and pause to eliminate inertia) a
mouseupevent will haveisMoving() === trueeven though that event will later cause amoveendwithout further camera changes. This behavior existed in 1.9.1 (https://codesandbox.io/s/lucid-hawking-ruywl).Launch Checklist
mapbox-gl-jschangelog:<changelog>Fix bug where map.isMoving() returned true while map was not moving.</changelog>