Django

Code

Ticket #4860 (closed: fixed)

Opened 1 year ago

Last modified 4 months ago

Labels missing for tags in newforms widgets

Reported by: russellm Assigned to: jacob
Milestone: 1.0 Component: Forms
Version: SVN Keywords: newforms html validation
Cc: Triage Stage: Ready for checkin
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

checkboxselectmultiple.diff (15.7 kB) - added by sandro on 12/03/07 12:21:31.
ca.diff (15.8 kB) - added by sandro on 12/03/07 12:22:19.
ca.2.diff (15.7 kB) - added by sandro on 12/03/07 12:23:30.
4860.labels-with-fors.diff (10.3 kB) - added by Ivan Sagalaev <Maniac@SoftwareManiacs.Org> on 12/04/07 11:57:43.
Patch with labels and proper fors
7629.labels-with-fors.diff (9.6 kB) - added by batiste@dosimple.ch on 06/16/08 08:37:14.
Patch for labels in new forms. Add a "for" attribute when possible

Change History

07/12/07 23:26:39 changed by russellm

  • needs_better_patch changed.
  • stage changed from Unreviewed to Accepted.
  • needs_tests set to 1.
  • needs_docs changed.

07/13/07 01:03:33 changed by mtredinnick

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.

09/14/07 00:22:30 changed by sandro

  • owner changed from nobody to sandro.

09/14/07 01:08:22 changed by sandro

  • status changed from new to closed.
  • resolution set to worksforme.

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))

10/17/07 15:21:23 changed by sandro

  • keywords set to newforms html validation.
  • status changed from closed to reopened.
  • resolution deleted.

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>

11/27/07 10:29:26 changed by sandro

  • has_patch set to 1.

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

12/03/07 12:21:31 changed by sandro

  • attachment checkboxselectmultiple.diff added.

12/03/07 12:22:19 changed by sandro

  • attachment ca.diff added.

12/03/07 12:23:30 changed by sandro

  • attachment ca.2.diff added.

12/03/07 12:27:38 changed by sandro

  • status changed from reopened to closed.
  • resolution set to fixed.
  • needs_tests deleted.

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?

12/03/07 12:43:21 changed by sandro

  • status changed from closed to reopened.
  • resolution deleted.
  • stage changed from Accepted to Ready for checkin.

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

12/04/07 11:56:57 changed by Ivan Sagalaev <Maniac@SoftwareManiacs.Org>

  • stage changed from Ready for checkin to Design 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...

12/04/07 11:57:43 changed by Ivan Sagalaev <Maniac@SoftwareManiacs.Org>

  • attachment 4860.labels-with-fors.diff added.

Patch with labels and proper fors

12/04/07 17:56:02 changed by sandro

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>

12/05/07 01:12:19 changed by anonymous

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.

12/05/07 09:31:59 changed by sandro

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

01/20/08 06:35:26 changed by Maniac <Maniac@SoftwareManiacs.Org>

  • stage changed from Design decision needed to Unreviewed.

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.

02/27/08 19:58:45 changed by jacob

  • stage changed from Unreviewed to Accepted.

03/09/08 16:50:58 changed by jacob

  • owner changed from sandro to jacob.
  • status changed from reopened to new.

06/16/08 06:59:00 changed by Batiste Bieler

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

06/16/08 08:37:14 changed by batiste@dosimple.ch

  • attachment 7629.labels-with-fors.diff added.

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

06/16/08 08:39:12 changed by batiste@dosimple.ch

$ ./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

06/16/08 08:53:00 changed by jacob

  • stage changed from Accepted to Ready for checkin.

06/16/08 14:37:22 changed by jacob

  • milestone set to 1.0.

06/18/08 11:35:05 changed by jacob

  • status changed from new to closed.
  • resolution set to fixed.

Fixed in [7693].


Add/Change #4860 (Labels missing for tags in newforms widgets)




Change Properties
Action