Code

Opened 7 years ago

Closed 6 years ago

Last modified 3 years ago

#4860 closed (fixed)

Labels missing for tags in newforms widgets

Reported by: russellm Owned by: jacob
Component: Forms Version: master
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: UI/UX:

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

Download all attachments as: .zip

Change History (26)

comment:1 Changed 7 years ago by russellm

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago 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.

comment:3 Changed 7 years ago by sandro

  • Owner changed from nobody to sandro

comment:4 Changed 7 years ago by sandro

  • Resolution set to worksforme
  • Status changed from new to closed

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 Changed 7 years ago by sandro

  • Keywords newforms html validation added
  • Resolution worksforme deleted
  • Status changed from closed to reopened

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 Changed 6 years ago by sandro

  • Has patch set

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

Changed 6 years ago by sandro

Changed 6 years ago by sandro

Changed 6 years ago by sandro

comment:7 Changed 6 years ago by sandro

  • Needs tests unset
  • Resolution set to fixed
  • Status changed from reopened to closed

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 Changed 6 years ago by sandro

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage 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.

comment:9 Changed 6 years ago by Ivan Sagalaev <Maniac@…>

  • Triage 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...

Changed 6 years ago by Ivan Sagalaev <Maniac@…>

Patch with labels and proper fors

comment:10 Changed 6 years ago 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>

comment:11 Changed 6 years ago 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.

comment:12 Changed 6 years ago 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

comment:13 Changed 6 years ago by Maniac <Maniac@…>

  • Triage 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.

comment:14 Changed 6 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:15 Changed 6 years ago by jacob

  • Owner changed from sandro to jacob
  • Status changed from reopened to new

comment:16 Changed 6 years ago by Batiste Bieler

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

Changed 6 years ago by batiste@…

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

comment:17 Changed 6 years ago by batiste@…

$ ./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 Changed 6 years ago by jacob

  • Triage Stage changed from Accepted to Ready for checkin

comment:19 Changed 6 years ago by jacob

  • milestone set to 1.0

comment:20 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [7693].

comment:21 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.