This looks really promising! I didn't have time to carefully review all the widget and test changes, but I reviewed the core API code.

awsome. any plan for jinja2 based rendering?

I pushed some updates to the test suite.

My first goal to get all the widget tests working again is complete. The old widget tests are doc tests masquerading as unit tests. They include a ton of duplicate testing for unicode handling, attrs overriding, etc. I started to rectify this by splitting tests for each widget into separate test cases and files. These now live in forms_tests/widget_tests/test_<widget_name>.py. There's some cleanup to do still, but I think it's a big improvement.

My second goal to make the rest of forms_tests work again, at least mostly, is also complete. 90% of the previous test failures were due to an HTML5 required attribute being added to input.html. This breaks hundreds of assertHTMLEqual checks. It would be nice to use HTML5 attributes in the default templates, but for now I'm going to make the templates mimic existing rendering as much as possible. This will keep the size of this patch from ballooning, as well. :)

My next goals are to write tests for the new API changes and to convert the leftover widgets that still use string rendering.

@auvipy jinja2 will be the default renderer. I find this patch to be a bit easier to start with Django templates first, though.

Hi Preston,

Great work so far as usual! One question I had is if you considered other template location namespace than djangoforms? I'm asking because django/forms/widgets sounds more consistent to me.

This would also allow us to put form rendering (as_table and friends) templates in django/forms.

I think keeping the widget rendering as unchanged as possible in this patch is smart - HTML5 required attributes are a good idea IMO, but there's a separate ticket for that and it can be a separate commit: https://code.djangoproject.com/ticket/22383

The widget tests look much improved, but I'd also argue for keeping those tests as unchanged as possible in this PR (even if they're bad) and fix them in a follow-up commit. It's much easier to review and verify a big refactor if you can trust the tests to be mostly unchanged and not have to review them carefully too at the same time. Are the test changes necessary to get them passing with the HTML-rendered widgets, or could those test changes possibly be pulled out and moved to a separate follow-up PR?

I also slightly prefer django/forms/... over djangoforms/, but I don't feel strongly.

@charettes Great point about converting as_* after this is done. There's a ticket for that too: https://code.djangoproject.com/ticket/16922

Regarding the tests, another good option could be to pull out the test changes and commit those first, rather than second, if that's easier. Either way, the benefit of splitting the bulk of the test changes from the rendering changes is that we get a clean test run in between them, which goes a long way towards avoiding unexpected and undesired changes in the output.

That said, if it's really hard to do, it's not strictly necessary - just means we have to review extra carefully :-)

@charettes I didn't think through the namespace that much. django/forms sounds great to me.

@carljm Since the most recent implementation of Widget can instantiate a default renderer if none is specified, the existing tests should be able to pass without any changes. I'm happy to commit those after the initial patch lands. None of those changes introduce new tests.

what about dtlform for dtl based renderer and jinjaform?

@auvipy The DTL and Jinja templates will be in templates/ and jinja2/ top-level directories, as is the normal requirement for separating DTL and Jinja templates in apps in Django 1.8+. These directories aren't part of the template path, though, they are a property of each template engine. Within those directories the path to each template should be exactly the same. That way the widget classes (which is where the template path for each widget's template is defined) are agnostic to template engine, and your TEMPLATES and FORM_RENDERER settings will determine whether the DTL or Jinja template is used.

It's not pushed yet, but I now have most of the widgets updated to use templates. This includes some of the tricky ones, like SelectDateWidget.

The leftover widgets all use RendererMixin, which is a really intrusive API. Based on 339c01f, at least some people are using it, even though it's not documented.

Do you think it's sufficient to remove RendererMixin if we can maintain at least the documented iterator behavior for RadioSelect and CheckboxSelectMultiple through a deprecation cycle?

https://docs.djangoproject.com/en/dev/ref/forms/widgets/#django.forms.RadioSelect

Additionally, I could move the RendererMixin and it's widgets to django.forms.deprecated. This would give projects the option to opt-in to the old behavior.

@prestontimmons I don't think it's feasible to worry about undocumented internal APIs when making a change like this, even if we have some evidence that some people might be using it. If it's undocumented, we've made no guarantees about its future. Just get rid of it (probably give it a mention in the release notes, but that's all).

The documented iteration-over-BoundField behavior, on the other hand, needs to stay, at least for a deprecation path. If the code to support it isn't too bad, I don't really see why it shouldn't stay permanently; it's a handy shortcut that in many cases will still be simpler than defining your own full-blown radio-select template. I guess the issue with supporting is that it will mean either duplicating the individual option markup, or using an include inside the options loop of the radio-select etc templates, which could hurt performance?

Cool. :)

Iterating over the options is easy. It's the option.tag() method that's not. Since this method isn't part of BoundField, it doesn't receive name, values, or attrs when called in the template using {{ option.tag }}. RendererMixin worked around this in a convoluted way. The cleanest solution may be to have BoundField.__iter__ return BoundField instances, or even another class that can render the widget without arguments.

I'll play around with it some and see what I find.

Now that I take a closer look at it I think it will be easy to do.

There's a lot of work left to do, but I have forms_tests passing with both DTL and Jinja2 now.

The tricky part now is finding consistent behavior for the BoundField iteration API.

Unlike BoundField, Widget isn't renderable without extra arguments. In the minimum case, rendering requires name and value to be passed.

To get around that, the existing BoundField.__iter__() yields ChoiceInput instances. Like BoundField, ChoiceInput stores the name and value from the Field object. This means it can be rendered without arguments.

Unlike BoundField, ChoiceInput provides two render methods with different output:

• __str__() Renders the widget input element with a wrapping label element.
• tag() Renders the widget input element without a label element.

The iteration API has further inconsistency in the templates. For instance, Widget.render() can take a choices argument. This is used for the Widget options. This doesn't carry through if the field is iterated since BoundField.__iter__() has no access to the choices argument. Iteration looks only at Field.choices and Widget.choices.

Further, because there are two render methods, supporting this API requires either two templates for each option class, or an extra context argument like wrap_label being passed to a single template.

The approach I took in the current patch is to:

• Updated BoundField.__iter__() to return BoundField instances.
• Added tag() and choice_label methods to BoundField for backwards-compatibility.
• Updated Widget.render to take an extra_context argument.
• Updated BoundField.render to pass in wrap_label as extra_context to Widget.render based on whether __str__() or tag() is called.

Another option is to make BoundField.__iter__() return BoundField-like widgets that define __str__() and tag().

Finally, there's some inconsistency I don't like in the templates. The following templates are mutually exclusive:

• select.html and select_option.html
• radio.html and radio_option.html
• checkbox_select.html and checkbox_option.html

Preferably, the parent widget templates would reuse the iteration API and call {{ option.tag }}, but this isn't possible if we support the choices argument to Widget.render().

Is this a useful feature? It looks like it only exists for use in the test suite. It's not documented as far as I can tell.

Good news. I found a better API I'm stoked about.

Widget.get_context() returns a dict element like this:

{
    'widget': {
        'type': 'text',
        'name': 'name',
        'is_hidden': False,
        'required': True,
        'value': '',
        'attrs': {
            'id': 'some_id'
        },
    },
}

Templates access values from the widget variable:

<input type="{{ widget.type }}" name="{{ widget.name }}">

That means widgets like select.html can easily reuse the same option template used by BoundField.__iter__. For example:

<select name="{{ widget.name }}"{% include "django/forms/widgets/attrs.html" %}>{% for group_name, group_choices, group_index in widget.optgroups %}{% if group_name %}
  <optgroup label="{{ group_name }}">{% endif %}{% for option in group_choices %}
  {% include option.template_name with widget=option %}{% endfor %}{% if group_name %}
  </optgroup>{% endif %}{% endfor %}
</select>

Where select_option.html is:

<option value="{{ widget.value }}"{% include "django/forms/widgets/attrs.html" %}>{{ widget.label }}</option>

BoundField.__iter__ yields wrappers around the widget dictionary that implements __str__, tag, and choice_label.

I think I can focus on writing tests and documentation now.

@timgraham Getting build failures on Python 3.4 in this branch:

http://djangoci.com/job/pull-requests-trusty/3078/database=sqlite3,label=trusty-pr,python=python3.4/console

It seems related to using the unittest.subTest context manager in forms_tests.widget_tests.base.WidgetTest.

Any clue why Jenkins would fail on that? I can remove the subTest usage if necessary.

Looks like xmlrunner/unittest-xml-reporting#85

@timograham Can I get your thoughts on the easiest way to commit the tests in this branch?

You'll see in django/tests/forms_tests/widget_tests that I rewrote the existing tests as individual unit tests. This was primarily as a develpment aid, but it also made it easy to run the tests under both DTL and Jinja2. Further, the old tests are currently intact in this branch, sans ones that covered obsolete private API's.

I see a few options:

• Commit the branch with only the old tests in place. This would ensure nothing that worked before is broken. It would only run under Jinja2, which is the default rendering engine, though. The new tests would be added in a separate commit.
• Use the old tests, but retrofit all the existing test cases to run under both DTL and Jinja2. This could be updating each test case to use a custom subclass. Add the new tests in a separate commit.
• Commit both the old and new tests, and remove the old tests in a separate commit.
• Update the existing forms tests to unit tests prior to committing this branch. This would decrease the size of this patch and make it clearer in the commit which tests were specific to the branch.

I'm leaning toward the last option, but I'm happy to go either way if anyone has better ideas.

The last option is what I would do too.

Added PR #5209 for the test changes.

Hi all,

I rebased this patch onto master. Tests are passing again. The branch is close to feature complete, but the docs for it are still a big job.

For anyone interested, here are the steps I believe are left to be taken:

Templates

• Add the is comparison operator to the DTL if tag -- Ticket 26118. (merged in c00ae7f)

Docs

• Release notes
• Template rendering API:
  • The FORM_RENDERER setting.
  • The built-in TemplateRenderer
  • The public API for custom renderers.
  • Overriding the renderer per form and at render call time.
  • Default behavior: Uses Jinja2 if installed, otherwise uses DTL.
• Default template used for each built-in widget
• Public API for the Widget class with examples of overriding templates and customizing behavior.
• Public API for the new BoundWidget class?
• Deprecation notes
• Backwards-compatible change to choice field rendering implementation -- template-wise it works the same as before, but the following classes, which are never officially documented, are removed:
  • ChoiceFieldRenderer
  • SubWidget
  • RadioChoiceInput
  • CheckboxChoiceInput
  • ChoiceInput

Bugs

• The template renderer needs to import the Jinja2 backend lazily so as not to raise import errors when Jinja2 isn't installed.

Optimizations (optional)

• The Jinja2 templates could probably be more idiomatic. I got them working, but they're mainly copied from the DTL templates.

Rebased PR is #6498. This branch is on django/django so anyone can add commits to it. I'll make sure all the comments here are addressed before that gets merged (which is probably at least a week away depending on who else pitches in).