-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Event handler / interaction cleanup #1598
Conversation
return _setOffset(el); | ||
}; | ||
exports.mousePos = function (el, e) { | ||
var offset = _cachedOffset || _setOffset(el); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can call exports.setOffset
directly instead of defining an additional function.
Aside from these two minor remarks, the offset caching code looks good to me. |
@bhousel proper event testing can be a big lift so I'd prefer having it as a separate PR. |
ok, sounds good |
* now support `rotatestart`, `rotateend` events * document all events * inertial rotation! * don't rely on contextmenu event
I'm pretty happy with this now. There are some more things I could add to it, but want to avoid any additional scope creep on this PR. @jfirebaugh @mourner want to take a look? |
This looks great! One minor thing: I see some console warnings when I rotate the map. These warnings are not present on (the message traces to |
@lucaswoj Yeah I did notice those too - it's from 73a15d8 where I'm making a copy of the |
@@ -51,10 +51,19 @@ exports.suppressClick = function() { | |||
}, 0); | |||
}; | |||
|
|||
exports.mousePos = function (el, e) { | |||
var cachedOffset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is effectively a global variable, and it makes me nervous about bugs on pages with multiple maps. Are you sure setOffset
always gets called when necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call setOffset
on mouseenter
of the canvas container:
So it should be ok for mouse, but might be a problem for touch?
Rotating with control + drag is not working on this branch. |
Oh interesting - I did not try that. This worked before because control-click is a Mac operating system shortcut for right click, and the rotation handler was relying on |
Can you clarify? It always worked in Leaflet at least. |
Sometimes it fires coincident with But I was using this page for guidance, which is admittedly kind of old, so by now browsers might have standardized on a mousedown contextmenu. |
By the way, this gist is what I use when I'm testing the event handlers. Might be helpful for others: ['mousedown', 'mouseup', 'click', 'dblclick', 'contextmenu',
'dragstart', 'dragend',
'movestart', 'moveend',
'rotatestart', 'rotateend',
'boxzoomstart', 'boxzoomend', 'boxzoomcancel'
].forEach(function (s) {
map.off(s).on(s, function(data) {
var e = data && data.originalEvent;
console.log(s + (e ? ' buttons = ' + e.buttons + ', button = ' + e.button : ''));
});
}); |
Just tested this with IE11 and Edge, and indeed on Windows the A mouseup So that's one other thing that this PR fixes: rotation now works like right-click + drag on Windows too. |
if (map.boxZoom && map.boxZoom.active) return; | ||
if (map.dragRotate && map.dragRotate.active) return; | ||
if (e.touches && e.touches.length > 1) return; | ||
if (!e.touches && e.button !== 0) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is repeated two times so maybe worth separating into a method that returns boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, good idea..
Thanks for explanations about contextmenu — let's try to fix ctrl-drag (will probably require relying on both contextmenu and e.button). The changes generally look good, added some minor comments. |
prevents div/0
(eliminates "webkitMovement" warnings)
From @jfirebaugh:
You are right that caching the Here is what it looks like when you move from one map to another and |
* Split out methods to 1. drain inertia buffer 2. ignore events
I think I addressed all the issues raised - @mourner @jfirebaugh @lucaswoj want to take another look? |
👍 |
🚢 |
Event handler / interaction cleanup
Interaction calls getBoundingClientRect on mousemove events #854Right-clicking on map does not fire 'click' event #1416