[Django] #27055: Model form for MultiPolygonField has invalid html
Django
----------------------------+--------------------
Reporter: aptiko | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
== Synopsis ==
I have a model with a `MultiPolygonField`. I have Django automatically
create a form for it (using a `CreateView`). I show the form on a template
with `{{ form.as_p}}`. The result is invalid HTML; it has `<style>` inside
a `<p>`.
The reason for this is that the MultiPolygonField is rendered with a
`django.contrib.gis.forms.widgets.OpenLayersWidget`, which uses
`django/contrib/gis/templates/gis/openlayers.html`. This template contains
a `<style>` element followed by a `<div>` element. Apparently whenever
some other code elsewhere decides to show the widget, it wraps it in a
`<p>` or in a `<td>` (I've also seen it wrapped in a `<div>`).
== How to reproduce ==
1) Create or clone the minimal project.
If you prefer to clone it, it's at https://github.com/aptiko/testgishtml.
It's only three commits, of which the first two commits only contain the
empty django project and app created by `django-admin startproject` and
`./manage.py startapp`, so if you merely `git show` when you are at master
it will show you all the custom code.
If you prefer to do it manually, first `django-admin startproject
testgishtml`, then do the following:
* `./manage.py startapp gishtml`.
* Replace file `gishtml/models.py` with this:
{{{#!python
from django.contrib.gis.db import models
class Polygon(models.Model):
mpoly = models.MultiPolygonField()
}}}
* Replace `gishtml/views.py` with this:
{{{#!python
from django.views.generic import CreateView
from .models import Polygon
class CreatePolygonView(CreateView):
model = Polygon
template_name = 'create_polygon.html'
fields = ('mpoly',)
}}}
* Add file `gishtml/templates/create_polygon.html`:
{{{
<!DOCTYPE html>
<title>Create polygon</title>
{{ form.as_p }}
}}}
* Add `django.contrib.gis` and `gishtml` to `INSTALLED_APPS`.
* Add `url(r'^', gishtml.views.CreatePolygonView.as_view())` to
`urlpatterns`.
2) Run `./manage.py migrate` and `./manage.py runserver`.
3) Visit http://localhost:8000/
== Results ==
{{{
<!DOCTYPE html>
<title>Create polygon</title>
<p><label for="id_mpoly">Mpoly:</label>
<style type="text/css">
#id_mpoly_map { width: 600px; height: 400px; }
#id_mpoly_map .aligned label { float: inherit; }
#id_mpoly_div_map { position: relative; vertical-align: top; float:
left; }
#id_mpoly { display: none; }
.olControlEditingToolbar .olControlModifyFeatureItemActive {
background-image: url("/static/admin/img/gis/move_vertex_on.svg");
background-repeat: no-repeat;
}
.olControlEditingToolbar .olControlModifyFeatureItemInactive {
background-image:
url("/static/admin/img/gis/move_vertex_off.svg");
background-repeat: no-repeat;
}
</style>
<div id="id_mpoly_div_map">
<div id="id_mpoly_map"></div>
<span class="clear_features"><a
href="javascript:geodjango_mpoly.clearFeatures()">Delete all
Features</a></span>
<textarea id="id_mpoly" class="vSerializedField required" cols="150"
rows="10" name="mpoly"></textarea>
<script type="text/javascript">
var map_options = {};
var options = {
geom_name: 'MultiPolygon',
id: 'id_mpoly',
map_id: 'id_mpoly_map',
map_options: map_options,
map_srid: 4326,
name: 'mpoly'
};
var geodjango_mpoly = new MapWidget(options);
</script>
</div>
</p>
}}}
If you feed this to http://validator.w3.org/, it says "Element `style` not
allowed as child of element `p` in this context".
--
Ticket URL: <https://code.djangoproject.com/ticket/27055>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
------------------------+------------------------------------
Reporter: aptiko | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_tests: => 0
* version: 1.10 => master
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
To fix this and remove the <style> part of the template, we might:
- attribute classes (e.g. geodjango_div_map/geodjango_map) to the divs
- create a CSS file in gis/static to hold most of the styles
- add the CSS file in the widget Media class
- move some dynamic styling (width, height, float, etc.) to the style
attribute of the HTML tags
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:1>
Django
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Antonis Christofides):
* status: new => assigned
* owner: nobody => Antonis Christofides
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Note that I began to work on related JS stuff in
[https://github.com/django/django/pull/7205 this PR] for #25706. This work
is a bit stalled as I'm struggling making the JS tests pass.
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Antonis Christofides):
OK, it's more complicated than I thought at first:
1. The <style> element will be invalid whether the widget is enclosed in a
<p>, in a <td>, or in a <div>. If there is no way to move the <style>
elsewhere, it may be possible to move stuff to the HTML "style" attribute
and into the Javascript.
2. If, as in the example above, the widget is enclosed in a <p>, then the
<div> is also invalid; the starting "<div>" tag will imply a closing
"</p>" tag, meaning that the "</p>" tag that follows the closing "</div>"
is invalid. If it is expected that widgets contain divs, then this is an
error of the code that decides to enclose widgets within <p>s.
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Antonis Christofides):
I guess that in theory, widgets should not contain <divs>, given that you
can tell it {{ form.as_p }}, in which case it will wrap the widget in a
<p>. Can it work without the divs? Perhaps if the divs are changed to
spans?
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
I don't think it's feasible to display a map inside a non-block tag. It's
probably not advisable to use `as_p` with geometry widgets.
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Antonis Christofides):
Another problem is that the contents of the <style> element are enclosed
in {% block map_css %}. Apparently this block is not redefined anywhere
else in django, but it might be redefined in third-party apps. I guess we
don't want to break such apps.
Here's a solution I've thought (completely untested): We change the
<style> element and make it an undisplayed <div> instead:
{{{#!django
<div id="map_css" style="display: none;">
{% block map_css %}
[The part inside the block we leave unchanged.]
{% endblock %}
</div>
}}}
And then we add this JavaScript:
{{{
#!javascript
var map_css = document.getElementById('map_css').innerHTML;
var style_element = document.createElement('style');
style_element.type = 'text/css';
if(style_element.styleSheet) {
style_element.styleSheet.cssText = map_css;
} else {
style_element.appendChild(document.createTextNode(map_css));
}
document.getElementsByTagName('head')[0].appendChild(style_element);
}}}
It's a bit hacky, particularly the fake <div> which only serves for
JavaScript to grab its contents with .innerHTML; but I can't find any
other way to do it since there's no cross-browser way to have a JavaScript
multiline string.
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Antonis Christofides):
* has_patch: 0 => 1
Comment:
Patch at https://github.com/django/django/pull/7313
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Antonis Christofides):
* has_patch: 1 => 0
Comment:
Summarizing yesterday's discussion with claudep on IRC, the fake div hack
is, ehm, a hack, and we prefer to find a better way to do it. We will
break backwards compatibility with {% block map_css %}. The
".olControlEditingToolbar .olControlModifyFeatureItemActive" section of
the css can be moved into a static file, using a relative URL for
background-image. The rest of the css can be moved to the "style"
attribute of the HTML elements.
Question (which I asked on IRC but missed any response): If we want to
follow CSP and get rid of inline css (and inline JS) altogether, how are
we going to specify this dynamic CSS?
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Antonis Christofides):
If we add a static file with custom css, ```{{ form.media }}``` will need
to be added in the template, at the HTML header. We can document that in
the documentation, but it would also break compatibility with existing
templates (which can also be documented in the release notes).
Is this a proper way to go forward?
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: Antonis
Christofides | Christofides
Type: Bug | Status: assigned
Component: GIS | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
As for me, I think it would be fine to require `form.media` for forms
containing a map widget (if documented, of course).
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:11>
Django
Reporter: Antonis Christofides | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Antonis Christofides => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:12>
Django
--------------------------------------+------------------------------------
Reporter: Antonis Christofides | Owner: (none)
Component: GIS | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* status: new => closed
* resolution: => fixed
Comment:
The `<style>` part was removed in
https://github.com/django/django/commit/44c24bf028353. It's still not
entirely resolving the initial issue, however the forms documentation
already recommends `as_div` over `as_p`
(https://docs.djangoproject.com/en/stable/ref/forms/api/#output-styles),
so I think we can close the ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/27055#comment:13>