Opened 7 years ago

Last modified 2 years ago

#27055 new Bug

Model form with geometry widgets has invalid html

Reported by: Antonis Christofides Owned by:
Component: GIS Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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: 
from django.contrib.gis.db import models


class Polygon(models.Model):
    mpoly = models.MultiPolygonField()
  • Replace gishtml/views.py with this: 
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".

Change History (12)

comment:1 Changed 7 years ago by Claude Paroz

Summary: Model form for MultiPolygonField has invalid html → Model form with geometry widgets has invalid html
Triage Stage: Unreviewed → Accepted
Version: 1.10 → master

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

comment:2 Changed 7 years ago by Antonis Christofides

Owner: changed from nobody to Antonis Christofides
Status: new → assigned

comment:3 Changed 7 years ago by Claude Paroz

Note that I began to work on related JS stuff in ​this PR for #25706. This work is a bit stalled as I'm struggling making the JS tests pass.

comment:4 in reply to:  1 Changed 7 years ago 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.

comment:5 Changed 7 years ago 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?

Last edited 7 years ago by Antonis Christofides (previous) (diff)

comment:6 Changed 7 years ago 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.

comment:7 Changed 7 years ago 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:

<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:

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.

comment:8 Changed 7 years ago by Antonis Christofides

Has patch: set

Patch at ​https://github.com/django/django/pull/7313

comment:9 Changed 7 years ago by Antonis Christofides

Has patch: unset

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?

comment:10 Changed 7 years ago 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?

comment:11 Changed 7 years ago 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).

comment:12 Changed 2 years ago by Mariusz Felisiak

Owner: Antonis Christofides deleted
Status: assigned → new
Note: See TracTickets for help on using tickets.
Back to Top