Skip to content

Commit 44577c2

Browse files
Give each Draw export button a unique element id (#2251)
The export button was hard-coded as id='export', with the click handler looking it up via document.getElementById('export'). Adding two Draw controls with export=True therefore emitted two elements sharing one id, which is invalid HTML and left the feature broken: every getElementById call resolved to the first button, so the second onclick assignment overwrote the first. The first button exported the second control's FeatureGroup under the second control's filename, and the second button did nothing at all. Derive the id from get_name() instead, which is what the other plugins that inject their own elements already do (see ScrollZoomToggler and FloatImage). Also drop the duplicated `top: 5px` from the button's CSS. `top: 90px` is declared later in the same block and already won, so this is dead code and the rendered position is unchanged. Adds tests/plugins/test_draw.py, which had no coverage before. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent aa22df5 commit 44577c2

2 files changed

Lines changed: 75 additions & 6 deletions

File tree

folium/plugins/draw.py

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,9 +58,8 @@ class Draw(JSCSSMixin, MacroElement):
5858
{% macro html(this, kwargs) %}
5959
{% if this.export %}
6060
<style>
61-
#export {
61+
#export_{{ this.get_name() }} {
6262
position: absolute;
63-
top: 5px;
6463
right: 10px;
6564
z-index: 999;
6665
background: white;
@@ -74,7 +73,7 @@ class Draw(JSCSSMixin, MacroElement):
7473
top: 90px;
7574
}
7675
</style>
77-
<a href='#' id='export'>Export</a>
76+
<a href='#' id='export_{{ this.get_name() }}'>Export</a>
7877
{% endif %}
7978
{% endmacro %}
8079
@@ -123,14 +122,14 @@ class Draw(JSCSSMixin, MacroElement):
123122
});
124123
125124
{% if this.export %}
126-
document.getElementById('export').onclick = function(e) {
125+
document.getElementById('export_{{ this.get_name() }}').onclick = function(e) {
127126
var data = drawnItems_{{ this.get_name() }}.toGeoJSON();
128127
var convertedData = 'text/json;charset=utf-8,'
129128
+ encodeURIComponent(JSON.stringify(data));
130-
document.getElementById('export').setAttribute(
129+
document.getElementById('export_{{ this.get_name() }}').setAttribute(
131130
'href', 'data:' + convertedData
132131
);
133-
document.getElementById('export').setAttribute(
132+
document.getElementById('export_{{ this.get_name() }}').setAttribute(
134133
'download', {{ this.filename|tojson }}
135134
);
136135
}

tests/plugins/test_draw.py

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
"""
2+
Test Draw
3+
---------
4+
"""
5+
6+
import re
7+
8+
import folium
9+
from folium import plugins
10+
from folium.template import Template
11+
from folium.utilities import normalize
12+
13+
14+
def test_draw():
15+
m = folium.Map([45.0, 3.0], zoom_start=4)
16+
draw = plugins.Draw(export=True, filename="my_data.geojson")
17+
m.add_child(draw)
18+
19+
out = normalize(m._parent.render())
20+
21+
# Verify that the export button has been created with a unique id.
22+
tmpl = Template("<a href='#' id='export_{{this.get_name()}}'>Export</a>")
23+
assert normalize(tmpl.render(this=draw)) in out
24+
25+
# Verify that the style targets that same id.
26+
assert normalize(f"#export_{draw.get_name()} {{") in out
27+
28+
# Verify that the click handler is wired to that same id.
29+
assert (
30+
normalize(f"document.getElementById('export_{draw.get_name()}').onclick") in out
31+
)
32+
33+
34+
def test_draw_no_export():
35+
m = folium.Map([45.0, 3.0], zoom_start=4)
36+
draw = plugins.Draw()
37+
m.add_child(draw)
38+
39+
out = normalize(m._parent.render())
40+
41+
assert "Export</a>" not in out
42+
assert f"export_{draw.get_name()}" not in out
43+
44+
45+
def test_two_draw_controls_get_unique_export_ids():
46+
"""Each Draw gets its own export button, wired to its own layers.
47+
48+
A hard-coded ``id='export'`` made the second button dead and pointed the
49+
first at the wrong FeatureGroup.
50+
"""
51+
m = folium.Map([45.0, 3.0], zoom_start=4)
52+
first = plugins.Draw(export=True, filename="first.geojson")
53+
second = plugins.Draw(export=True, filename="second.geojson")
54+
m.add_child(first)
55+
m.add_child(second)
56+
57+
out = m._parent.render()
58+
59+
ids = re.findall(r"id='(export_[^']+)'", out)
60+
assert len(ids) == 2
61+
assert len(set(ids)) == 2, f"export button ids are not unique: {ids}"
62+
63+
# Each handler must reference its own element, layers and filename.
64+
for draw, filename in ((first, "first.geojson"), (second, "second.geojson")):
65+
element_id = f"export_{draw.get_name()}"
66+
assert f"id='{element_id}'" in out
67+
start = out.index(f"document.getElementById('{element_id}').onclick")
68+
handler = out[start : out.index("}", start) + 1]
69+
assert f"drawnItems_{draw.get_name()}.toGeoJSON()" in handler
70+
assert filename in handler

0 commit comments

Comments
 (0)