-
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
Fix "Marker#togglePopup", refactor "Marker" #3104
Conversation
* set on this `Marker` instance is unset | ||
* @returns {Marker} `this` | ||
*/ | ||
|
||
setPopup: function (popup) { | ||
if (popup == null) { | ||
this._closePopup(); | ||
delete this._popupHandlersAdded; |
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.
Instead of tracking state in
_popupHandlersAdded
, callremoveEventListener
when the popup is removed. This will prevent memory leaks and unexpected behavior if theMarker
continues to be used without a popup. You'll need to save thethis
-bound version of_openPopup
, say by usingutil.bindAll(['_openPopup'], this)
, so that it can be passed toremoveEventListener
.
This prevents all click events on a marker from being propagated as map |
@jfirebaugh Makes sense. Done in the latest commit. |
Thank you @lucaswoj 😳 |
@mollymerp my 😳 for merging #3056 prematurely! Ready to 🚢 @jfirebaugh @mollymerp? |
I really think it will be surprising for clicks on markers not to produce map |
@jfirebaugh I understand now. I misread #3104 (comment). The latest commit ensures that this event is forwarded to |
// the map's container. If we attached the listener syncronously | ||
// the popup would close when the click event propogated to `map`. | ||
var that = this; | ||
setTimeout(function() { |
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.
Is there another way? Ad hoc setTimeout
s in library code are really dangerous. For example, this will throw if for some reason the popup is removed before the timeout triggers, because that._map
will be undefined
after remove
deletes it.
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.
Good point. There are some other approaches we can take. I'll push another commit soon.
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 latest commit fixes this problem by attaching the click listener to the map and checking the event's target to determine whether or not the popup should toggle. I did not see any performance degradation.
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.
Good solution! It will probably be a performance improvement for lots of markers because adding native event listeners tends to be slower than event delegation.
This PR fixes #3096 and addresses @jfirebaugh's feedback in #3056
cc @jfirebaugh @mollymerp @apkoponen
Checklist