RendererMixin id_for_label causes HTML id uniqueness violation

[Sven Coenye]

When rendering a ChoiceField RadioSelect widget using form.as_p, the result is

<ul id="id_field-field">
  <li><label for="id_field-field_0">
      <input id="id_field-field_0" type="radio" value="1" name="field-field">
      ...
      </label>
  </li>
  <li><label for="id_field-field_1">...
  </li>
  ...
</ul>

Note that the id attribute of the ul element is id_field-field

When rendering the same RadioSelect field manually using

<ul id="{{field.id_for_label}}">
  {% for choice in field %}
    <li><label for="{{ choice.id_for_label }}">{{ choice.tag }}</li>
  {% endfor %}
</ul>

the result is

<ul id="id_field-field_0">
  <li><label for="id_field-field_0">
        <input id="id_field-field_0" type="radio" value="1" name="field-field">
        </label>
  </li>
  <li>...
  ...
</ul>

The outer ul element id now has the same id as the first radio button. This should not be.

The problem is caused by the implementation of id_for_label in forms.widgets.RendererMixin

def id_for_label(self, id_):
        # Widgets using this RendererMixin are made of a collection of
        # subwidgets, each with their own <label>, and distinct ID.
        # The IDs are made distinct by y "_X" suffix, where X is the zero-based
        # index of the choice field. Thus, the label for the main widget should
        # reference the first subwidget, hence the "_0" suffix.
        if id_:
            id_ += '_0'
        return id_

vs. what widgets.ChoiceFieldRenderer.render() does:

id_ = self.attrs.get('id')
...
return format_html(self.outer_html,
                           id_attr=format_html(' id="{}"', id_) if id_ else '',
                           content=mark_safe('\n'.join(output)))

The docs ​https://docs.djangoproject.com/en/1.9/ref/forms/widgets/#django.forms.RadioSelect state

The outer <ul> container will receive the id attribute defined on the widget.

which makes me believe render() is correct. Either way, I think these should be consistent with each other. If not, then the docs should mention using the "name" attribute to avoid the id collision.

[Fyodor Ananiev]

Hello! Will add new attribute for RadioSelect on master branch.

[Tim Graham]

​PR

[Tim Graham]

Marking as "Patch needs improvement" per Preston's comment on the PR. Please uncheck that after the patch is updated.

[Tim Graham]

The discussion on the pull request led to the conclusion that a new attribute might not be the best course of action. Perhaps this documentation addition would suffice: ​PR?

[Sven Coenye]

IMHO, why not just remove the hardcoded _0 if the primary use of the method at this level is to produce the value for the container element's id attribute? Even if a developer really wants to use it in an outer label element, there is no guarantee they would want it to act on the first option.

[Tim Graham]

id_for_label() is meant to generate an id for a <label> tag, not the id for the container element. If you make the change you propose, you'll see the relevant test failures if you run $ ./tests/runtests.py forms_tests.

[Sven Coenye]

Ah. Now I see. I was looking too close in by concentrating on the rendering of just the field.

[Tim Graham <timograham@…>]

In 53e8ab58:

Fixed #26265 -- Clarified RadioSelect container's HTML id.

[Tim Graham <timograham@…>]

In 379bab35:

[1.9.x] Fixed #26265 -- Clarified RadioSelect container's HTML id.

Backport of 53e8ab580f7c0fcfc589ba0b1b6cc2556080e0b2 from master