Opened 8 years ago

Closed 8 years ago

#22950 closed Cleanup/optimization (fixed)

Subclassing choice select widgets should be easier

Reported by: Patrick Robertson <django@…> Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

So say I want to change the HTML of a RadioSelect widget from using <ul> to something like:

<div>
<label>label</label>
<input>
</div>

It sounds pretty simple right? But it looks like I need all of this code, which still doesn't work:

class BSChoiceFieldRenderer(widgets.ChoiceFieldRenderer):
    def render(self):
        """
        Outputs a <ul> for this set of choice fields.
        If an id was given to the field, it is applied to the <ul> (each
        item in the list will get an id of `$id_$i`).
        """
        id_ = self.attrs.get('id', None)
        start_tag = format_html('<div id="{0}">', id_) if id_ else '<div>'
        output = [start_tag]
        for i, choice in enumerate(self.choices):
            choice_value, choice_label = choice
            if isinstance(choice_label, (tuple, list)):
                attrs_plus = self.attrs.copy()
                if id_:
                    attrs_plus['id'] += '_{0}'.format(i)
                sub_ul_renderer = ChoiceFieldRenderer(name=self.name,
                                                      value=self.value,
                                                      attrs=attrs_plus,
                                                      choices=choice_label)
                sub_ul_renderer.choice_input_class = self.choice_input_class
                output.append(format_html('<div>{0}{1}</div>', choice_value,
                                          sub_ul_renderer.render()))
            else:
                w = self.choice_input_class(self.name, self.value,
                                            self.attrs.copy(), choice, i)
                output.append(format_html('<div>{0}</div>', force_text(w)))
        output.append('</div>')
        return mark_safe('\n'.join(output))
        
class BSRadioFieldRenderer(BSChoiceFieldRenderer):
    choice_input_class = widgets.RadioChoiceInput
    
# Subclass of the RadioSelect widget
class BSRadioSelect(widgets.RadioSelect):
    renderer = BSRadioFieldRenderer

...
forms.py

something = forms.ChoiceField(choices=choices, widget=BSRadioSelect())

If I try and put the render() method in the BSRadioFieldRenderer subclass (so as to avoid having to subclass ChoiceFieldRenderer) I get an error about NoneType not being callable.
With the code as it stands here, the widget isn't actually rendered, just escaped HTML is printed

Attachments (1)

22950-1.diff (2.1 KB) - added by Claude Paroz 8 years ago.
Proof-of-concept patch

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by Tim Graham

Although I have not looked into the ticket in detail, I guess a solution might be #15667 - Implement template-based widget rendering.

If not, do you have other ideas? I guess you probably didn't mean it this way, but your post comes off as a complaint without suggesting any ways to improve the situation which can be discouraging for people who spend a lot of time working on Django. Consider avoiding words like "nightmare", thanks.

Changed 8 years ago by Claude Paroz

Attachment: 22950-1.diff added

Proof-of-concept patch

comment:2 Changed 8 years ago by Claude Paroz

I tried to rewrite the rendering code to factor out the HTML tags. Could you please see if that patch would allow you to more easily customize the produced HTML (overwriting only ChoiceFieldRenderer.outer_html and ChoiceFieldRenderer.inner_html in your subclass)?

comment:3 Changed 8 years ago by Patrick Robertson <django@…>

Hi claudep,

Yes, I apologise - my initial submission probably wasn't the most helpful for you guys. It's amazing to see you've already created a patch, so quickly!
Your patch is most certainly easier (there is now no need to subclass ChoiceFieldRenderer, I can put the two instance vars in my RadioFieldRenderer subclass.

Perhaps what I wasn't more clear about is that, for some reason - when I subclass either ChoiceFieldRenderer or RadioFieldRenderer, then the outputted HTML ( with {{ form.field }} in my template is always escaped.

So even if I just do:

#renderer subclass
class BSRadioFieldRenderer(widgets.RadioFieldRenderer):
    outer_html = '<div{id_attr}>{content}</div>'
    inner_html = '<div>{choice_value}{sub_widgets}</div>'

# Subclass of the RadioSelect widget
class BSRadioSelect(widgets.RadioSelect):
    renderer = BSRadioFieldRenderer

the outputted HTML is escaped. If, I now simply replace the outer_html and inner_html vars with pass, everything works as expected (except uls are used of course)

So perhaps my gripe is that using any non-escaped HTML anywhere in the subclasses causes rendering to break (although I don't know much about rendering, so I could be wrong as to the reason why)

comment:4 Changed 8 years ago by Patrick Robertson <django@…>

P.S. using {{ form.field|safe }} in my template works, but that just doesn't seem right.

comment:5 Changed 8 years ago by Claude Paroz

Component: UncategorizedForms
Has patch: set
Needs tests: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 1.7-rc-1master

comment:6 Changed 8 years ago by Claude Paroz

Needs tests: unset

I've created a pull request including a test: https://github.com/django/django/pull/2878
However I wasn't able to reproduce your escaping issue. Indeed, you shouldn't have to append the |safe filter. Maybe a sample project might help debugging your issue.

comment:7 Changed 8 years ago by Patrick Robertson <django@…>

OK - I've narrowed down the issue with rendering escaped HTML. It only occurs when actually rendering a single form field (with {{ form.field_name }] in the template)

See my sample project at:

https://github.com/pjrobertson/django_22950

I think that once this issue is pinpointed, some tests should be created to ensure that the output rendered from {{ form }} and {% for field in form %} {{ field }} {% endfor %} (or whatever) are the same

comment:8 Changed 8 years ago by Claude Paroz

Thanks for the sample project, it was useful to debug this weird issue, being that rendering returned a bytestring which was then converted to Unicode and therefore lost its safe escaping status.
I've updated the pull request.

comment:9 Changed 8 years ago by django@…

Great, it looks good!

I guess I'm probably asking too much at this point... but is there any chance the rendering bug can be in Django 1.7rc2? I think it was the main cause of my 'nightmare' issues, and isn't specific to this pull request (I originally came across it when subclassing the render() method)

Thanks for all your hard work

comment:10 Changed 8 years ago by Claude Paroz <claude@…>

In 9209049211acbe1a53c3dc409dd5fe26edf21634:

Ensured bound field renders as unicode safe data

Refs #22950.

comment:11 Changed 8 years ago by Claude Paroz

I just pushed the rendering issue to master: [9209049211acbe1]
I'm rather confident that it is a harmless change for the 1.7 branch. I'd just like a second opinion from a core dev.

comment:12 Changed 8 years ago by django@…

Now you have done more than you should have - thanks claudep, the Django dev team is awesome!

comment:13 Changed 8 years ago by Aymeric Augustin

Summary: Subclassing choice select widgets is a nightmareSubclassing choice select widgets should be easier

I've changed the title to something less link-baity, I hope you don't mind ;-)

comment:14 Changed 8 years ago by Aymeric Augustin

claudep: this looks safe for backporting even at this stage.

comment:15 Changed 8 years ago by Claude Paroz <claude@…>

In 83a185a3f7d07af295880ed02efba4af4c86a204:

[1.7.x] Ensured bound field renders as unicode safe data

Refs #22950.
Backport of 920904921 from master.

comment:16 Changed 8 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In dd9a23d5cfb44cb6f150610c267d0de1d963b849:

Fixed #22950 -- Eased markup customization for choice field rendering

Thanks Patrick Robertson for the report.

Note: See TracTickets for help on using tickets.
Back to Top