Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#26265 closed Bug (fixed)

RendererMixin id_for_label causes HTML id uniqueness violation

Reported by: Sven Coenye Owned by: Fyodor Ananiev
Component: Forms Version: 1.9
Severity: Normal Keywords:
Cc: tedmx@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (10)

comment:1 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Fyodor Ananiev, 9 years ago

Cc: tedmx@… added
Owner: changed from nobody to Fyodor Ananiev
Status: newassigned

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

comment:3 by Tim Graham, 9 years ago

Has patch: set

comment:4 by Tim Graham, 9 years ago

Patch needs improvement: set

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

comment:5 by Tim Graham, 9 years ago

Patch needs improvement: unset

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?

comment:6 by Sven Coenye, 9 years ago

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.

comment:7 by Tim Graham, 9 years ago

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.

comment:8 by Sven Coenye, 9 years ago

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

comment:9 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In 53e8ab58:

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

comment:10 by Tim Graham <timograham@…>, 9 years ago

In 379bab35:

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

Backport of 53e8ab580f7c0fcfc589ba0b1b6cc2556080e0b2 from master

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