Opened 11 years ago
Closed 11 years ago
#20684 closed New feature (fixed)
Support form element attributes with no value
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | loic@…, shai@…, jcd@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
There are a couple of form attributes such as required that was introduced in HTML5 that should not contain a value. For exaple required="required" is valid but for example required="true" will result in failed validation with true being marked as a bad value.
The correct use would be:
<input type="email" required />
Currently in forms.py attributes cannot be specified without a value which rely on the author knowing that a value of 'true' not being valid but 'required' being ok. it would be great if Django supports valueless attributes.
Perhaps doing:
widget=forms.TextInput( attrs={ 'required': '' }))
will result in an input of:
<input type="text" required />
Change History (37)
comment:1 by , 11 years ago
Description: | modified (diff) |
---|
comment:2 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Agreed. However I'd rather use None
instead of the empty string to mean no attribute value, because there might be cases where an empty value is wanted.
At first sight, this seems rather trivial to implement in flatatt
(https://github.com/django/django/blob/master/django/forms/util.py#L15).
comment:3 by , 11 years ago
Actually, I'd rather use some special marker rather than None -- most such attributes are essentially boolean, and None -- which is False when evaluated as a boolean -- would come to mark True, and this can be confusing.
I'd introduce a new constant, and call it NoValue -- probably along the lines of class NoValue(object): pass
. Then you could write
widget=forms.TextInput( attrs={ 'required': NoValue }))
And also have correct behavior on things like if widget.attrs['required']: ...
comment:4 by , 11 years ago
As Shai pointed out, these *are* booleans (http://www.w3.org/html/wg/drafts/html/master/infrastructure.html#boolean-attributes), why not treat them as such?
Value in python should be either True or False, False being noop in flatatt()
comment:5 by , 11 years ago
Cc: | added |
---|---|
Easy pickings: | set |
Has patch: | set |
Needs documentation: | set |
Patch needs improvement: | set |
Version: | 1.5 → master |
POC: https://github.com/loic/django/compare/ticket20684
I'll write the docs for it if that's validated.
comment:6 by , 11 years ago
As @claudep initially suggested I would have chosen None
for that but in the light of @shai's comment I think @loic84's approach is the correct one.
comment:7 by , 11 years ago
Interestingly comments on a feature branch get lost upon forced pushes.
@charettes wasn't convinced that the False
value was necessary.
I brought up the use case of setting the value programmatically with an expression like 'required': is_required()
and we agreed that it could be useful.
comment:8 by , 11 years ago
Yes, this makes a lot of sense; the reason I suggested a special marker is backwards-compatibility -- if code exists which relies on the value being converted to string, and so expects that attr=True will manifest in HTML as attr="True"
, this may very well break it.
I still find it preferable to use a special marker -- but I guess NoValue
is a bad name.
comment:9 by , 11 years ago
Cc: | added |
---|
follow-up: 11 comment:10 by , 11 years ago
A special marker is a bit overkill IMO and it's not exactly convenient since you have to import it before using it.
Also relying on the implicit conversion of True
to "True"
is not a pattern we should encourage, we could list the change as backward incompatible in the release notes.
follow-up: 12 comment:11 by , 11 years ago
Replying to loic84:
Also relying on the implicit conversion of
True
to"True"
is not a pattern we should encourage,
Why not? Especially in data-
attributes?
we could list the change as backward incompatible in the release notes.
Yes, but exactly because of the "required": is_required()
pattern you brought up, uses may be hard to detect.
follow-up: 13 comment:12 by , 11 years ago
comment:13 by , 11 years ago
Replying to loic84:
A special marker is a bit overkill IMO and it's not exactly convenient since you have to import it before using it.
It is also overkill IMO. W3 defined the semantic of boolean attributes and I think we should try to stick to it. As pointed out by @loic84 a release note concerning backward compatibility should be enough.
Replying to loic84:
Because explicit is better than implicit, people who really want
"True"
should cast the boolean to a string.
Agreed that True
-> "True"
implicit conversion is not a pattern we should encourage.
comment:14 by , 11 years ago
I don't feel very strongly about this, but I'm also not convinced. In my opinion, we shouldn't make code stop working unless it's required to fix a bug, or the code that gets broken can be seen as a bug. IMO, this feature is not important enough, and the implicit conversion is not buggy enough, to justify a breaking change.
But maybe that's just me.
comment:15 by , 11 years ago
I can see the point of Shai, but I'd tend to agree with the boolean version also.
We are not in a hurry though, so let's see if others have strong opinions about this.
comment:16 by , 11 years ago
Having slept about it,
(a) I concede that the implicit conversion should not be encouraged, and
(b) I agree that a special marker would be, essentially, interface cruft that we would keep just for the benefit of something we don't really want.
But backwards compatibility still is a problem; I suggest to solve it with a deprecation cycle. Given (a) above, I think even an expedited cycle may be enough.
So,
Django 1.7:
{'attr':True}
becomesattr="True"
, withDeprecationWarning('The meaning of Boolean values for widget attributes will change in Django 1.8')
Django 1.8:
{'attr':True}
becomesattr
What do you think?
follow-up: 20 comment:18 by , 11 years ago
@shai: good idea, however I don't see anything preventing us from advancing the planning: Deprecation on 1.6 and change in 1.7.
comment:19 by , 11 years ago
+1 I'll write the doc for the original patch and I'll write another one that simply raises a DeprecationWarning against the 1.6 branch.
comment:20 by , 11 years ago
Replying to claudep:
I don't see anything preventing us from advancing the planning: Deprecation on 1.6 and change in 1.7.
I was under the impression that 1.6 was feature-frozen at this stage.
comment:22 by , 11 years ago
I'd always been under the impression that new deprecations should not be introduced after the beta, either (unless absolutely necessary, e.g. a security fix). The point of the feature freeze is to avoid regressions after the beta, so that people can test their codebase on 1.6 beta and trust (as much as possible) that it will still work without further changes on 1.6 final. Adding a deprecation breaks this (for some people it's a blocker or even breaks CI if their codebase begins to emit a warning). I would hesitate to even add a PendingDeprecationWarning
post-beta, let alone an accelerated DeprecationWarning
.
follow-up: 25 comment:23 by , 11 years ago
@carljm: That feature will completely stall until 1.8 otherwise, 1.9 even if it doesn't go through an accelerated cycle.
I think we are being extra cautious already by offering a deprecation path, I don't think we should delay it by a couple of years.
comment:24 by , 11 years ago
@loic84: Django is now moving to a shorter version cycle; if all goes well, 1.6 will come only ~8 months after 1.5.
More generally, I don't see why this feature is so urgent. Yes, it's nice, but it doesn't even really enable new functionality.
comment:25 by , 11 years ago
@loic84: I agree with @shai that this feature is not that urgent, users can still workaround it by specifying 'required': 'required'
and output valid markup.
We've lived with 'checked': 'checked'
and 'selected': 'selected'
for a while now.
comment:26 by , 11 years ago
OK, then do we agree on the fast Deprecation process as proposed in comment:16 ? Let's consider that we are still talking about corner cases.
comment:28 by , 11 years ago
This is a pretty bad error message - 'The meaning of boolean values for widget attributes will change in Django 1.8'
doesn't give anyone any clues about *what* will change, what it'll change *to*, and what they should do about it.
Can we think a bit and change this error message to something more user-friendly?
comment:29 by , 11 years ago
Here are some thoughts on the deprecation warning text.
One option is to name the feature that we are implementing (HTML5 boolean attributes).
(1) "Starting in Django 1.x, widget attributes with boolean values will render according to the rules for HTML5 boolean attributes."
If they don't know what that means, they have the search term for HTML5 boolean attributes. I admit this is still a little obtuse for users who aren't familiar with all the rules for HTML5, but it's also terse. Explaining the new feature in the DeprecationWarning will be a bit verbose for a one-line warning, but could be done:
(2) "Starting in Django 1.x, the rendering of attributes with boolean values will change; Attributes marked True will render as the name of the attribute, and attributes marked False will not render at all [in accordance with the rules for HTML5 boolean attributes."
Alternatively, we could separate out true and false values into separate DeprecationWarnings, so we can be explicit about exactly what will happen and why, while still being terse..
(3) True: "Starting in Django 1.x, widget attributes set to 'True' will render as the name of the attribute with no value, in accordance with the rules for HTML5 boolean attributes."
False: "Starting in Django 1.x, widget attributes set to 'False' will not be rendered, in accordance with the rules for HTML5 boolean attributes."
I think my preference would be for separate True and False DeprecationWarnings.
Edit: Added numberings to the various proposals, for ease of reference.
comment:30 by , 11 years ago
For more verbosity (to answer @jacob's question of what they should do about it):
(4) True: "Starting in Django 1.x, widget attributes set to True will render as the name of the attribute with no value, in accordance with the rules for HTML5 boolean attributes. If you want your HTML attribute set to the value "True", use the string 'True' instead of the boolean True.
False: "Starting in Django 1.x, widget attributes set to False will not be rendered, in accordance with the rules for HTML5 boolean attributes. If you want your HTML attribute set to the value "False" use the string 'False' instead of the boolean False.
I think my preference is still for option 3.
comment:31 by , 11 years ago
Cc: | added |
---|
comment:32 by , 11 years ago
Constant string: Starting from Django 1.8, boolean values to widget attributes will determine if the attribute appears (with no value). To preserve current behavior, use string values explicitly
.
Or, taking the approach of @jcd -- why not go all the way?
When value is True: Starting from Django 1.8, widget attribute %(attr_name)s=True will be rendered as '%(attr_name)s'. To preserve current behavior, use the string 'True' instead of the boolean True.
When value is False: Starting from Django 1.8, widget attribute %(attr_name)s=False will be not be rendered. To preserve current behavior, use the string 'False' instead of the boolean False.
comment:34 by , 11 years ago
Easy pickings: | unset |
---|
comment:35 by , 11 years ago
This ticket can now move forward on master. Loic, if you'd like to update your branch and write the documentation, I'll merge it.
comment:36 by , 11 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:37 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Description reformatted, please use preview when posting.