Skip to content
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

safeguard for being called on a removed popup #4188

Closed
wants to merge 1 commit into from

Conversation

ssured
Copy link

@ssured ssured commented Feb 3, 2016

it seems all methods protect here for the case this._map does not exists. I hit this one when programmatically removing popups from a map

@IvanSanchez
Copy link
Member

No, unfortunately the problem is deeper than this.

We changed the order of layer.onAdd(map) and layer.getEvents() in 61c07bd to fix something else - and this means that this._map is not initialized when calling getEvents on the layer, which in this case means the closePopupOnClick map option does nothing.

I guess we'll have to refactor the preclick handler somehow, or do a late initialization, or something of the likes of that.

@IvanSanchez
Copy link
Member

@ssured Can you please have a look at the unit tests for popups and #4189? The tests should be failing but right now I cannot understand why they don't.

Sorry your bugfix is not as simple as it seemed at first!

@ssured
Copy link
Author

ssured commented Feb 3, 2016

@IvanSanchez my error does not originate from a map click, but when programatically removing the popup using closePopup and unbindPopup. I got the error when refactoring the https://github.com/miguelcobain/ember-leaflet library to support beta 2. The issue is I do not fully understand the flow of events happening inside that library, which makes it hard to create a proper test case here...

My best guess is this happens:

  • we have a marker with a bound popup, using 'bindPopup'
  • the popup is opened by a click event
  • the marker is removed from the map (using .removeLayer)
  • THEN marker.closePopup() is called
  • THEN marker.unbindPopup() is called

Juggling the order around keeps hitting the error. Maybe @miguelcobain can have a look too, as he is the author of ember-leaflet and probably understands the internals a bit better

@miguelcobain
Copy link
Contributor

What happens in popup.js is:

  1. we create an L.popup instance and call bindPopup() on the marker (for example, can be a polygon as well) with the created popup as argument: https://github.com/miguelcobain/ember-leaflet/blob/master/addon%2Fmixins%2Fpopup.js#L57-L59
  2. when we create the previous popup, we also set its content to a document fragment. This will be the "ember" container to which we will render to when the popup opens
  3. we "hijack" leaflet popup's methods to let us know when we need to render/remove to that fragment: https://github.com/miguelcobain/ember-leaflet/blob/master/addon%2Fmixins%2Fpopup.js#L67-L81
    Basically onAdd sets popupOpen to true and onRemove sets popupOpen to false. This popupOpen boolean controls an {{#if gate that controls the render of the popup content itself: https://github.com/miguelcobain/ember-leaflet/blob/master/addon%2Ftemplates%2Fcurrent%2Fpopup.hbs#L2-L4
  4. when the marker is destroyed, we call closePopup() and unbindPopup(): https://github.com/miguelcobain/ember-leaflet/blob/master/addon%2Fmixins%2Fpopup.js#L86-L87
    This is called before the map itself is removed. leaflet-map destroys its children before itself: https://github.com/miguelcobain/ember-leaflet/blob/master/addon/components/leaflet-map.js#L61-L63

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants