backslashes in popup break page.

[jrjdavidson]

Describe the bug
It seems if a backslash followed by a number is included in the text of a popup, the map will fail to generate. Once the map is generated, if an attempt to load the map is map, no map is displayed and the following error is present in the browser console.

Uncaught SyntaxError: octal escape sequences can't be used in untagged template literals or in strict mode code

In my case, it caught me off guard as I only recently had a path come up that caused the issue. Hope the report helps others..

To Reproduce
( i tried to simplify my code, I didn't actual test this)

html="<div>\\file\Shared\SEESPhotoDatabase\Archive\JD\Cave Rock photos\20190221\101MEDIA</div>"
 m = folium.Map()
 popup = folium.Popup(html)

folium.GeoJson(feature['geometry'],
                           popup=popup).add_to(m)

Expected behavior
Normal map with a pop up.

Environment (please complete the following information):

• checked in chrome and firefox

Possible solutions
Check all strings and escape any backslashes(\) before adding as a popup

[Conengmo]

Minimal example to reproduce:

m = Map()
Marker((0, 0), popup=r"<div>\\file\2019</div>",).add_to(m)

Users can fix this on their side by escaping their backslashes. For example:

Marker((0, 0), popup=r"<div>\\\\file\\2019</div>",).add_to(m)

I'm not sure if we should change the behavior on the Folium side. On the one hand, that would make it easier for users. On the other hand it would be a breaking change for users who already escape their backslashes.

In the end, I'd opt for making the change, making Folium better going forward. I'm thinking we should unify the parsing between Popup, Tooltip and perhaps other classes that render user text.

[achieveordie]

I would go about it, @Conengmo, by introducing this unified string-like Element-subclass, sitting in branca called SmartText (of course, this is only a placeholder name).

All classes which require rendering text will optionally start accepting instances of this class. Nothing changes for users who are already escaping backslashes at their end. Then, over the next couple of minor release cycles, we deprecate anything apart from str and SmartText with clear UserWarning for those who use backslashes with string to instead switch to SmartText.

Finally, only str and SmartText will be valid types for texts, where any str objects will directly be instantiated (and after that used) as a SmartText. SmartText will be responsible for sanitizing strings and act as a single entrypoint for all text-like entities.

I'd be happy to work with you on this if you find the idea plausible. I'm yet to take a deep dive in branca and folium so I could be missing something.

[hansthen]

@achieveordie That looks like a promising approach. Two questions/thoughts:

1. Is it necessary to make SmartText a subclass of Element? There are some items in the interface of Element (like the ability to have children) that do not seem to apply to SmartText.
2. I am not super happy about the class name SmartText. Could this be changed to something more descriptive?

[achieveordie]

Hey @hansthen, thanks for getting back.

Is it necessary to make SmartText a subclass of Element? There are some items in the interface of Element (like the ability to have children) that do not seem to apply to SmartText.

While SmartText might not need the functionalities provided by Element in its current state, it will help in being covariant with the rest of the folium type. For example, consider an imaginary use case where I want to clone one of the user-facing classes (like Map or something). For that purpose I will recursively visit all the children node and have a single clone() functionality for all Element subtypes. If SmartText was not a subclass, we will now have to collect all folium instance attributes by using Elements and _BaseText (assuming this was a base interface).

I am not super happy about the class name SmartText. Could this be changed to something more descriptive?

I agree with your opinion on the name since it is only a placeholder. I suppose a more generic class Text(Element) would suffice.

[hansthen]

I'd prefer it if the Text object would not inherit from Element. In my view Element corresponds to html elements, which are only items defined by a start and end tag. Otherwise I think you should just go ahead. Abstracting Text objects would solve the problem of (improperly) escaping strings.

[achieveordie]

Sorry, I missed your reply. I'll start working on it and let you know when it is done.

[Conengmo]

@achieveordie I took a look at python-visualization/branca#170 and I have to say it's quite impressive. I do mean that as a compliment, but I'll also say that I think it's a bit overkill for what we need here. One of my main concerns is keeping Folium maintainable, so the less code the better. In this case I don't think we need to escape more than backticks and backslashes, so a full framework for text escaping is not necessary I think.

What I have in mind is adding a function like the existing escape_backticks but for backslashes. Have it only escape single backslashes, so already escaped backslashes are not touched. Then call that in Popup, and maybe make sure Tooltip works the same. That's little extra code, so it'll be easier to test and maintain.

Hope this doesn't deter you from helping out with Folium, I do appreciate your input.

[achieveordie]

Thanks for taking a look at it, @Conengmo. I can understand your "Maintainability over Comprehensivity" approach, especially in cases where a feature might not be extensively used.

Coming back to the implementation, are you suggesting we should limit ourselves to creating a function similar to escape_backticks() in the utilities module? If a single backslash is found in the string, shouldn't we escape every instance of the backslash found in the string?

Consider the initial example where the original string is: "<div>\\file\2019</div>". We'd escape every instance of the backslash since we found an odd number of backslashes. The correctly escaped string will therefore be "<div>\\\\file\\2019</div>" and not "<div>\\file\\2019</div>".

I probably misunderstood what you meant, so I just wanted to clarify before reworking on it.

[Conengmo]

Yes you are right, we can't really infer whether the string needs to be escaped or not from whether there are only single or also double backslashes in it. Let's keep it simple and just escape, no matter the content of the string.

The correctly escaped string will therefore be "

\\file\2019

"

Yes, that seems correct to me.

I probably misunderstood what you meant, so I just wanted to clarify before reworking on it.

Thanks!