Opened 17 years ago

Closed 16 years ago

Last modified 12 years ago

#4860 closed (fixed)

Labels missing for tags in newforms widgets

Reported by: Russell Keith-Magee Owned by: Jacob
Component: Forms Version: dev
Severity: Keywords: newforms html validation
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The CheckboxSelectMultiple and RadioInput widgets in newforms add a <label> tag around their multiple-choice options, but don't specify a for="". This apparently causes problems with IE6/7.

Attachments (5)

checkboxselectmultiple.diff (15.7 KB ) - added by sandro 16 years ago.
ca.diff (15.8 KB ) - added by sandro 16 years ago.
ca.2.diff (15.7 KB ) - added by sandro 16 years ago.
4860.labels-with-fors.diff (10.3 KB ) - added by Ivan Sagalaev <Maniac@…> 16 years ago.
Patch with labels and proper fors
7629.labels-with-fors.diff (9.6 KB ) - added by batiste@… 16 years ago.
Patch for labels in new forms. Add a "for" attribute when possible

Download all attachments as: .zip

Change History (26)

comment:1 by Russell Keith-Magee, 17 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Malcolm Tredinnick, 17 years ago

More information garnered from the mailing list thread: the problem is that without these attributes, clicking on the label text does not activate the control.

comment:3 by sandro, 17 years ago

Owner: changed from nobody to sandro

comment:4 by sandro, 17 years ago

Resolution: worksforme
Status: newclosed

All of the generated labels looked good to me, here's how I tested.

PRIVACY_CHOICES = (
    ('Y', 'YES'),
    ('N', 'NO'),
)
class SurveyForm(forms.Form):
    name = forms.CharField(max_length=20)
    likes_a = forms.BooleanField()
    likes_b = forms.BooleanField(widget=forms.CheckboxSelectMultiple(choices=PRIVACY_CHOICES))
    is_private = forms.BooleanField(widget=forms.RadioSelect(choices=PRIVACY_CHOICES))

comment:5 by sandro, 16 years ago

Keywords: newforms html validation added
Resolution: worksforme
Status: closedreopened

Just ran into some test data to validate the issue:

CHOICES=[(1L, u'Marshall'), (2L, u'Miami'), (3L, u'East Carolina'), (4L, u'Virginia Tech'), (5L, u'Connecticut')]

class TeamSelectForm(forms.Form):
    teams = forms.MultipleChoiceField(choices=CHOICES, widget=forms.CheckboxSelectMultiple)


<p><label for="id_teams_0">Teams:</label> <ul>
<li><label><input type="checkbox" name="teams" value="1" id="id_teams_0" /> Marshall</label></li>
<li><label><input type="checkbox" name="teams" value="2" id="id_teams_1" /> Miami</label></li>
<li><label><input type="checkbox" name="teams" value="3" id="id_teams_2" /> East Carolina</label></li>
...
</ul></p>

Each list item should instead look like:

<li><input type="checkbox" name="teams" value="1" id="id_teams_0" /><label for="id_teams_0"> Marshall</label></li>

comment:6 by sandro, 16 years ago

Has patch: set

Added a quick fix for the data above. It might be a good idea to look for other label typos.

by sandro, 16 years ago

Attachment: checkboxselectmultiple.diff added

by sandro, 16 years ago

Attachment: ca.diff added

by sandro, 16 years ago

Attachment: ca.2.diff added

comment:7 by sandro, 16 years ago

Needs tests: unset
Resolution: fixed
Status: reopenedclosed

Added some tests, I'm sure there are other label inconsistencies but I wanted to post this progress as soon as I could. All of the attachments are duplicates can anyone clean them up?

comment:8 by sandro, 16 years ago

Resolution: fixed
Status: closedreopened
Triage Stage: AcceptedReady for checkin

Whoops, didn't mean to close the ticket...looks like I don't know how to use trac properly.

comment:9 by Ivan Sagalaev <Maniac@…>, 16 years ago

Triage Stage: Ready for checkinDesign decision needed

Looks like this patch removes all label tags from radio and checkbox inputs altogether. I believe it's better to just write proper for="" for them because they are useful there. Patch follows.

P.S. I'm setting 'Design decision needed' because may be I'm missing something and labels are not desirable in RadioInput and CheckboxSelectMultiple. Though I can't imagine why...

by Ivan Sagalaev <Maniac@…>, 16 years ago

Attachment: 4860.labels-with-fors.diff added

Patch with labels and proper fors

comment:10 by sandro, 16 years ago

Glad to see some activity! All of the tests for my patch succeeded so I'm unsure of how it removed label tags for unrelated widgets. On another note though, my patch sucks as it maps the for attribute to the related value attribute instead of the related id attribute...agh!

Maybe we could get a css expert to weigh in on the correct usage of the label tag. webstandards.org talks a bit about them:

The article says we need to turn this:

<li><label for="id_language_0"><input type="radio" id="id_language_0" value="P" name="language" /> Python</label></li>

Into this:

<li><input type="radio" id="id_language_0" value="P" name="language" /> <label for="id_language_0">Python</label></li>

comment:11 by anonymous, 16 years ago

All of the tests for my patch succeeded so I'm unsure of how it removed label tags for unrelated widgets.

Ah... Looks like I've misread your patch then. I didn't see that you've extracted inputs out of labels, I thought your removed labels completely. Sorry :-)

Maybe we could get a css expert to weigh in on the correct usage of the label tag.

Well.. According to HTML spec both usages are correct. When you wrap input inside a label you theoretically can skip for=".." and label will automatically relate to an enclosed input. However IE doesn't understand labels without for=".." which this whole bug is about :-).

From styling point of view it's hard to say which is better: sometimes one variant is easier to layout according to design, sometimes the other. I've left inputs inside labels simply for consistency with other Django widgets.

comment:12 by sandro, 16 years ago

Well done Ivan, your patch fixes the problem and stays consistent with Django widgets. I've started a discussion on the google group to try and gain some insight as to which label style Django should support...input is welcome.

http://groups.google.com/group/django-developers/browse_thread/thread/60ae465f9c453f89

comment:13 by Maniac <Maniac@…>, 16 years ago

Triage Stage: Design decision neededUnreviewed

I'm inclined to move this ticket out of "Design decision needed" because both contributors agree on the latest patch. So it just has to be reviewed now.

comment:14 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

comment:15 by Jacob, 16 years ago

Owner: changed from sandro to Jacob
Status: reopenednew

comment:16 by Batiste Bieler, 16 years ago

It a simple and annoying bug. Please do something about it.

by batiste@…, 16 years ago

Attachment: 7629.labels-with-fors.diff added

Patch for labels in new forms. Add a "for" attribute when possible

comment:17 by batiste@…, 16 years ago

$ ./runtests.py --settings t/settings forms
Ran 28 tests in 47.179s
OK

$ ./runtests.py --settings t/settings
Ran 257 tests in 177.968s
OK

comment:18 by Jacob, 16 years ago

Triage Stage: AcceptedReady for checkin

comment:19 by Jacob, 16 years ago

milestone: 1.0

comment:20 by Jacob, 16 years ago

Resolution: fixed
Status: newclosed

Fixed in [7693].

comment:21 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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