Opened 4 years ago

Closed 4 years ago

Last modified 4 months ago

#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: master
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 alexpirine)

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

Description: modified (diff)

comment:2 Changed 4 years ago by alexpirine

Description: modified (diff)

comment:3 Changed 4 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 1.10master

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_attribute method.

comment:4 Changed 4 years ago by Andrew Nester

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 Changed 4 years ago by Claude Paroz

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

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 Changed 4 years ago by Jon Dufresne

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 Changed 4 years ago by Josef Rousek

Owner: changed from nobody to Josef Rousek
Status: newassigned

comment:9 Changed 4 years ago by Josef Rousek

Has patch: set

comment:10 Changed 4 years ago by 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 the renderer attribute (I still can not find it in documentation).

comment:11 in reply to:  10 Changed 4 years ago by Josef Rousek

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 the renderer attribute (I still can not find it in documentation).

Thanks for review! I'll fix those issues.

comment:12 Changed 4 years ago by Simon Charette

Triage Stage: AcceptedReady for checkin

Patch LGTM pending some cosmetic comments.

comment:13 Changed 4 years ago by Josef Rousek

I applied those cosmetic changes.

comment:14 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In aaecf038:

Fixed #27370 -- Prevented Select widget from using 'required' with a non-empty first value.

comment:15 Changed 4 months ago by Aurélien Pardon

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 Changed 4 months ago by Aurélien Pardon

Cc: Aurélien Pardon added
Patch needs improvement: set

comment:17 Changed 4 months ago by Carlton Gibson

Patch needs improvement: unset

Follow-up in #31295.

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