-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix boxZoom bug #4528
fix boxZoom bug #4528
Conversation
src/ui/handler/drag_pan.js
Outdated
// if boxZoom is disabled and then re-enabled, the drag_pan event listener for mousedown will fire before | ||
// the boxZoom event listener, so map.boxZoom.isActive() will return false resulting in an erroneous | ||
// drag on mousemove once the boxZoom is released, so we need this additional check. | ||
if (map.boxZoom && e.shiftKey && e.button === 0) return true; |
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.
Should this check also include max.boxZoom.isActive()
?
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.
the reason for this check is because map.boxZoom.isActive()
returns false
if BoxZoomHandler is disabled / re-enabled, so, no 🙃
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.
I think I need to re-word this comment to make that more clear. its due to the firing order of event listeners attached to the keydown
event.
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.
Oh dear 🤦♂️. I swear I really did read that comment , but then got turned upside-down thinking about event logic...
What made me ask was thinking about something like:
- mousdown: start dragpan
- press (and hold) shift
- mouseup: i think the expectation would be to stop panning, but this check would prevent that from happening (if I'm reading it right?)
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're right this is no bueno! thanks for catching. I'll rethink how to handle this 💭
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 ready now, right? @mollymerp
Yep! Thanks @mourner |
fix #2237 where disabling then re-enabling the boxZoomHandler would result in a bug where the drag handler would be erroneously active after a boxZoom triggered
mouseup
eventLaunch Checklist