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.
Instead of tracking state in
_popupHandlersAdded, callremoveEventListenerwhen the popup is removed. This will prevent memory leaks and unexpected behavior if theMarkercontinues 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 |
lucaswoj
changed the title
|
@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.
Is there another way? Ad hoc setTimeouts 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.
Good point. There are some other approaches we can take. I'll push another commit soon.
There was a problem hiding this comment.
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.
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