#27370 closed Cleanup/optimization (fixed)
Django's Select widget adds a required="required" attribute, even if created with empty_label=True
| Reported by: | alexpirine | Owned by: | Josef Rousek |
|---|---|---|---|
| Component: | Forms | Version: | dev |
| Severity: | Normal | Keywords: | HTML, Select, wdiget, ModelChoiceField |
| Cc: | Aurélien Pardon | 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 (last modified by )
I didn't think it was wrong in HTML, but the W3C validator says so, and after reading this discussion, it seems to be right:
https://github.com/validator/validator/issues/47
This is how I could explain it:
If a form field is required and has the empty_label=True argument, the generated <select> will have a required="required" attribute. This doesn't make sense since the first item will be selected anyways, and there is no way for the user to unselect a value.
So, the required="required" attribute is useless.
Example:
class TestForm(forms.Form): some_field = forms.ModelChoiceField(queryset=SomeModel.objects.all(), empty_label=None)
results in:
<select id="id_some_field" name="some_field" required> <option value="1">Value 1</option> <option value="2">Value 2</option> <option value="3">Value 3</option> <!-- ... --> </select>
You can check the following code using the W3C validation service:
<!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8"> <title>Validation test</title> </head> <body> <select id="id_some_field" name="some_field" required> <option value="1">Value 1</option> <option value="2">Value 2</option> <option value="3">Value 3</option> <!-- ... --> </select> </body> </html>
In order to fix this, the generated <select> widget should not have a required attribute.
Change History (17)
comment:1 by , 8 years ago
| Description: | modified (diff) |
|---|
comment:2 by , 8 years ago
| Description: | modified (diff) |
|---|
comment:3 by , 8 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|---|
| Type: | Bug → Cleanup/optimization |
| Version: | 1.10 → master |
comment:4 by , 8 years ago
Agree with Claude Paroz - all we need to do is to set use_required_attribute = False in form where such field used.
Something like this:
class TestForm(forms.Form):
use_required_attribute = False
some_field = forms.ModelChoiceField(queryset=SomeModel.objects.all(), empty_label=None)
I guess we don't need some additional changes for this, what do you think @alexpirine?
comment:5 by , 8 years ago
If Django has all needed elements to determine the value of use_required_attribute in such a situation, it should do it automatically and not require the user to add it explicitly.
comment:6 by , 8 years ago
I agree with Claude.
Also, use_required_attribute applies to all fields, which is not desired here. We only want to remove the required attribute from the Select widget.
comment:7 by , 8 years ago
alexpirine, I believe Claude is referring to Widget.use_required_attribute, not Form.use_required_attribute. For reference, this method will be documented soon.
comment:8 by , 8 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
follow-up: 11 comment:10 by , 8 years ago
I reviewed the code:
The overall logic and tests seem to be correct, except in case of an empty <select>.
But I can not make a full judgment about the code, since I lack the understanding of some parts of widgets.py. For instance I didn't know anything about the renderer attribute (I still can not find it in documentation).
comment:11 by , 8 years ago
Replying to alexpirine:
I reviewed the code:
The overall logic and tests seem to be correct, except in case of an empty <select>.
But I can not make a full judgment about the code, since I lack the understanding of some parts of
widgets.py. For instance I didn't know anything about therendererattribute (I still can not find it in documentation).
Thanks for review! I'll fix those issues.
comment:12 by , 8 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Patch LGTM pending some cosmetic comments.
comment:15 by , 5 years ago
There is a huge performance regression as a side-effect of aaecf038. For ModelChoiceField with empty_label=None, it doubles the queries to the database.
With this simple_app/models.py:
from django.db import models class MyModel(models.Model): my_field = models.CharField(max_length=1)
the following code:
from django import forms from simple_app.models import MyModel class MyForm(forms.Form): my_form_field = forms.ModelChoiceField(MyModel.objects.all(), empty_label=None) MyForm().as_ul()
makes two times the same request to the database:
>>> from django.db import connection >>> from django.db import reset_queries >>> reset_queries() >>> _ = MyForm().as_ul() >>> connection.queries [{'sql': 'SELECT "simple_app_mymodel"."id", "simple_app_mymodel"."my_field" FROM "simple_app_mymodel"', 'time': '0.000'}, {'sql': 'SELECT "simple_app_mymodel"."id", "simple_app_mymodel"."my_field" FROM "simple_app_mymodel"', 'time': '0.000'}]
Before this commit, it only makes one request (as intended IMO):
>>> from django.db import connection >>> from django.db import reset_queries >>> reset_queries() >>> _ = MyForm().as_ul() >>> connection.queries [{'sql': 'SELECT "simple_app_mymodel"."id", "simple_app_mymodel"."my_field" FROM "simple_app_mymodel"', 'time': '0.000'}]
comment:16 by , 5 years ago
| Cc: | added |
|---|---|
| Patch needs improvement: | set |
Here's the validator error message:
The first child option element of a select element with a required attribute, and without a multiple attribute, and without a size attribute whose value is greater than 1, must have either an empty value attribute, or must have no text content. Consider either adding a placeholder option label, or adding a size attribute with a value equal to the number of option elements.At first sight, this should be doable without difficulty in the widget's
use_required_attributemethod.