Plots2: Map popups not working

[jywarren]

@sagarpreet-chadha reported this, we need to collect more information on it.

---

(original post)

Regarding popups not showing on markers:

If we change this line popup = new Popup(options).setContent(popup); to popup = new Popup(options).setContent(popup._content ? popup._content : popup); in leaflet repo line:

https://github.com/Leaflet/Leaflet/blob/d1a1e97b8290f642eb677284af53c2db64199a76/src/layer/Popup.js#L320

Everything works fine then!

One way to change this line is to use https://www.npmjs.com/package/patch-package , this changes that line in node modules folder after we do yarn install. The advantage is that we can still update leaflet version if there is any (we will kind of become static user of leaflet if we fork leaflet and do this change).

---

My response:

oh goodness, i totally misunderstood, please forgive me @sagarpreet-chadha ! It's a Leaflet bug? I guess I'd prefer to fork Leaflet and fix it and point to our branch, but is it a known issue, that might be fixed by them soon? Or, is there any way for us to override this line in our code, after including Leaflet?

@crisner @nstjean what do you think? I guess we should not hold back the publication of PublicLab.org if we don't have a quick path to fix this yet. I'm opening an issue where we can discuss this a bit more, would love to hear from you. Thanks!

Possibilities:

• Fork and fix Leaflet
• Find issue at Leaflet repo and see how fast their solution is coming
• File solution PR with Leaflet if it's not there already
• Locally override the function in our code until it's fixed upstream

What do folks think? Let's dig a bit on the Leaflet project to see what's up?

[crisner]

The popups seem to be working on the /map page and the map embedded in the sidebar(like here https://publiclab.org/wiki/unearthing-pvd). It doesn't seem to work on inline maps or the people page. I checked this locally after bumping LEL and it seems to work when using inline maps on a post but I am getting the same error for the map in people page.

Uncaught TypeError: Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
    at NewClass._updateContent (VM1530 LeafletEnvironmentalLayers.js:21428)
    at NewClass.update (VM1530 LeafletEnvironmentalLayers.js:21371)
    at NewClass.onAdd (VM1530 LeafletEnvironmentalLayers.js:21308)
    at NewClass.onAdd (VM1530 LeafletEnvironmentalLayers.js:21562)
    at NewClass._layerAdd (VM1530 LeafletEnvironmentalLayers.js:18527)
    at NewClass.whenReady (VM1530 LeafletEnvironmentalLayers.js:16407)
    at NewClass.addLayer (VM1530 LeafletEnvironmentalLayers.js:18589)
    at NewClass.openPopup (VM1530 LeafletEnvironmentalLayers.js:21785)
    at NewClass.openPopup (VM1530 LeafletEnvironmentalLayers.js:21893)
    at NewClass._openPopup (VM1530 LeafletEnvironmentalLayers.js:21968)

We will need to look into why the popup is having these issues when executing maps in people page as it seems to be working on other pages. Perhaps we need to compare the code for setting up in LEL in each of these pages?

[crisner]

So, I managed to see the popup in the people page when I commented out lines 128 to130 and L132 without any errors.
https://github.com/publiclab/plots2/blob/b80f193b60827f90b70bef020648d8b2813468fa/app/views/map/_peopleLeaflet.html.erb#L128-L132

I thought it was working in the inline maps here but it turned out only this marker was working and I got the same error on every other marker.

The dom has the popup elements in the popup pane when we click on the markers but they seem to have no content causing the error. Could this be because the requests from the layers are not being completed? It seems weird as it works on the /map page and the small maps embedded on the side as shown here:

If the popup content is being loaded with @sagarpreet-chadha's fix, then we could try to extend leaflet's popup, make the fix there and use it until we figure it out. We may have to replace all the reference to L.popup to our custom one.

[nstjean]

That seems so strange that it would work in one map but not the other. Thanks for doing all this research!

[jywarren]

Thank you so much for the research.

1. If the popup content is being loaded with @sagarpreet-chadha's fix
2. then we could try to extend leaflet's popup, make the fix there and use it until we figure it out.

So let's try out @sagarpreet-chadha's fix? Agreed that we can fix this way. Has anyone had a chance to look this up on the Leaflet repo? I can if you can describe the issue in once sentence so I can properly relay it over!

[jywarren]

Tracking this:

Popups work:

• https://publiclab.org/map
• https://stable.publiclab.org/map
• https://publiclab.org/wiki/puerto-rico - same as unearthing - "Uncaught TypeError: Cannot convert undefined or null to object"
• unearthing layer popups work
• https://publiclab.org/wiki/sandbox-projects - unearthing layer popups work, but layer won't turn off

Popups not working:

• https://stable.publiclab.org/people
• https://publiclab.org/people
• https://publiclab.org/wiki/inline-maps
• https://publiclab.org/wiki/unearthing-pvd
  (solved issue of layers not on)

[jywarren]

setContent usages:

• https://github.com/publiclab/leaflet-environmental-layers/blob/7cf1aa1b19e9b3728901465255d13ab3f028a786/src/layercode.js#L61

• just HTML

• https://github.com/publiclab/leaflet-environmental-layers/blob/38c1c65b598f8469b2511fc35b110968fac8a31c/src/unearthing.js#L56

• passing in just HTML

appendChild used: https://github.com/publiclab/leaflet-environmental-layers/search?q=appendChild&unscoped_q=appendChild

[jywarren]

Goals:

• tracking exactly where setContent attempts to pass in an object
• how different maps' popups are constructed in different places - consistency of instantiating LEL on for example "plots2/app/views/map/_peopleLeaflet.html.erb"
• used in 3 places - 2 are on the people page: https://github.com/publiclab/plots2/search?q=peopleLeaflet
• 1 is on the inline people maps: https://github.com/publiclab/plots2/blob/b015ee350eeab94d94e3c768598f3c941f4cdcca/app/models/concerns/node_shared.rb#L213-L222
• "map_data_string" function https://github.com/publiclab/plots2/blob/b015ee350eeab94d94e3c768598f3c941f4cdcca/app/models/concerns/node_shared.rb#L356

[jywarren]

Ah, and also, to see if upstream Leaflet is tracking any related issue, it seems not, but here is my search: https://github.com/Leaflet/Leaflet/search?q=setContent+popup&type=Issues

[jywarren]

To my last comment, I searched a bit more, and Leaflet specifies only HTML as the parameter: https://leafletjs.com/reference-1.4.0.html#popup-setcontent

I think we need to find where we're passing in popup._content instead of just HTML, as Sagarpreet had pointed out. Maybe we're doing this in plots2 on the people page?

That would be somewhere in this JS file, i think? https://github.com/publiclab/plots2/blob/23e83cdc51a8496f23db932054cbe25ee329e821/app/assets/javascripts/leaflet_helper.js#L25-L51

[jywarren]

Also, i've tried to put a range of good example maps on this page and ensure that stable/production are both showing the same content:

https://stable.publiclab.org/wiki/inline-maps

https://publiclab.org/wiki/inline-maps

[nstjean]

So it looks like only the last map on the page has working popups.

This is when I click on a marker on the top map:

This is when I click on a marker on the bottom map:

They are exactly the same code, exactly the same template _inlineLeaflet.html.erb. The same thing happens with _peopleLeaflet.html.erb and _plainInlineLeaflet.html.erb

I'm going to do some digging to figure out what isn't working correctly when we have multiple maps being called.

[nstjean]

If I put multiple maps in the same template (with unique variable names) they work.

The exact same code pasted into separate templates is throwing the error. I think it might be because of the separate ruby function calls here?
https://github.com/publiclab/plots2/blob/aa3b8644e365080e264e73590529ff03bb63cb08/app/models/concerns/node_shared.rb#L356-L372
I'm going to keep experimenting.

[jywarren]

Great observation! Thanks @nstjean !!

[crisner]

Great catch @nstjean! :tada: :rocket: :rocket:
Could be related to the unique id?

[nstjean]

@crisner I thought so too but changing them didn't make a difference!

[crisner]

Just posting this link on a similar issue here as a note:

• https://gis.stackexchange.com/questions/66193/multiple-instance-of-leaflet-on-page#comment130672_76494
• https://gis.stackexchange.com/questions/110442/multiple-leaflet-maps-on-web-site-in-different-divs

I'll edit this comment to add other similar issues that I find as a reference here.

[crisner]

@nstjean, when you are looking into this issue I think it would be better to pass in the baselayer as done here:
https://github.com/publiclab/plots2/blob/aa3b8644e365080e264e73590529ff03bb63cb08/app/views/map/map.html.erb#L103-L106

in the functions here:
https://github.com/publiclab/plots2/blob/aa3b8644e365080e264e73590529ff03bb63cb08/app/assets/javascripts/leaflet_helper.js#L94-L107

and here:
https://github.com/publiclab/plots2/blob/aa3b8644e365080e264e73590529ff03bb63cb08/app/assets/javascripts/leaflet_helper.js#L143-L155

The problem didn't go away after I changed this on my machine but giving you a head's up just in case.

[crisner]

Also, here are couple of issues I found that could be related.

• https://stackoverflow.com/questions/19186428/refresh-leaflet-map-map-container-is-already-initialized
• https://github.com/axyjo/leaflet-rails/pull/82

[nstjean]

I figured it out! We can't have the map dependencies included for every single map:
https://github.com/publiclab/plots2/blob/aa3b8644e365080e264e73590529ff03bb63cb08/app/views/map/_inlineLeaflet.html.erb#L1
https://github.com/publiclab/plots2/blob/aa3b8644e365080e264e73590529ff03bb63cb08/app/views/map/_plainInlineLeaflet.html.erb#L1
https://github.com/publiclab/plots2/blob/aa3b8644e365080e264e73590529ff03bb63cb08/app/views/map/_peopleLeaflet.html.erb#L1

I see the same issue happen if there's a map in the sidebar, which also loads the dependencies and breaks the marker popups. I think the reason they were only on map pages was because we didn't want to load everything for non-map pages, but I feel like it's better to put it in the header somewhere? @jywarren what do you think?

[crisner]

This is awesome @nstjean! 🎉 🎉 🎉
I was going in the same direction today morning(my time). Tried removing them using developer tools in chrome and didn't work. Probably because I didn't touch the sidebar's dependencies as it was from a different template.

Amazing work!! 🚀 🚀 🚀

[jywarren]

Awesome!!!!! 🎉 🎉 🎉 🎉

So glad you found this! This really seems correct. Let's try it out! Fantastic! 💥