[Django] #26265: RendererMixin id_for_label causes HTML id uniqueness violation
Django
----------------------------+--------------------
Reporter: scoenye | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26265>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: scoenye | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:1>
Django
Reporter: scoenye | Owner: tedmx
Type: Bug | Status: assigned
Component: Forms | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tedmx):
* owner: nobody => tedmx
* cc: tedmx@… (added)
* status: new => assigned
Comment:
Hello! Will add new attribute for RadioSelect on master branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:2>
Django
Reporter: scoenye | Owner: tedmx
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6209 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:3>
Django
Reporter: scoenye | Owner: tedmx
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Marking as "Patch needs improvement" per Preston's comment on the PR.
Please uncheck that after the patch is updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:4>
Django
Reporter: scoenye | Owner: tedmx
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
Comment:
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:
[https://github.com/django/django/pull/6306 PR]?
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:5>
Django
Reporter: scoenye | Owner: tedmx
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by scoenye):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:6>
Django
Reporter: scoenye | Owner: tedmx
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
`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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:7>
Django
Reporter: scoenye | Owner: tedmx
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by scoenye):
Ah. Now I see. I was looking too close in by concentrating on the
rendering of just the field.
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:8>
Django
Reporter: scoenye | Owner: tedmx
Component: Forms | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"53e8ab580f7c0fcfc589ba0b1b6cc2556080e0b2" 53e8ab58]:
{{{
#!CommitTicketReference repository=""
revision="53e8ab580f7c0fcfc589ba0b1b6cc2556080e0b2"
Fixed #26265 -- Clarified RadioSelect container's HTML id.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:9>
Django
Reporter: scoenye | Owner: tedmx
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"379bab354472b882fed250d0dceb73644d52e220" 379bab35]:
{{{
#!CommitTicketReference repository=""
revision="379bab354472b882fed250d0dceb73644d52e220"
[1.9.x] Fixed #26265 -- Clarified RadioSelect container's HTML id.
Backport of 53e8ab580f7c0fcfc589ba0b1b6cc2556080e0b2 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26265#comment:10>