Opened 11 years ago

Closed 11 years ago

#20684 closed New feature (fixed)

Support form element attributes with no value

Reported by: sneethling@… 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 Claude Paroz)

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 Claude Paroz, 11 years ago

Description: modified (diff)

Description reformatted, please use preview when posting.

comment:2 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

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 Shai Berger, 11 years ago

Actually, I'd rather use some special marker other 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']: ...

Last edited 11 years ago by Shai Berger (previous) (diff)

comment:4 by loic84, 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()

Last edited 11 years ago by loic84 (previous) (diff)

comment:5 by loic84, 11 years ago

Cc: loic@… added
Easy pickings: set
Has patch: set
Needs documentation: set
Patch needs improvement: set
Version: 1.5master

POC: https://github.com/loic/django/compare/ticket20684

I'll write the docs for it if that's validated.

comment:6 by Simon Charette, 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 loic84, 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 Shai Berger, 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 Shai Berger, 11 years ago

Cc: shai@… added

comment:10 by loic84, 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.

in reply to:  10 ; comment:11 by Shai Berger, 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.

in reply to:  11 ; comment:12 by loic84, 11 years ago

Replying to shai:

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?

Because explicit is better than implicit, people who really want "True" should cast the boolean to a string.

in reply to:  12 comment:13 by Simon Charette, 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 Shai Berger, 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 Claude Paroz, 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 anonymous, 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} becomes attr="True", with DeprecationWarning('The meaning of Boolean values for widget attributes will change in Django 1.8')

Django 1.8:

{'attr':True} becomes attr

What do you think?

comment:17 by Shai Berger, 11 years ago

(comment:16 is me, of course).

comment:18 by Claude Paroz, 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 loic84, 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.

in reply to:  18 comment:20 by Shai Berger, 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:21 by loic84, 11 years ago

There wouldn't be any new feature, only a DeprecationWarning.

comment:22 by Carl Meyer, 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.

comment:23 by loic84, 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 Shai Berger, 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.

in reply to:  23 comment:25 by Simon Charette, 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 Claude Paroz, 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:27 by Claude Paroz <claude@…>, 11 years ago

In 1116df0751cc0d5862590b08adfffe7bacd6bf43:

Deprecate usage of boolean value for widget attributes

Django 1.7 will loudly warn when widget attributes are assigned
boolean values. In Django 1.8, False will mean attribute is not
present while True will mean attribute present without value.
Refs #20684.

comment:28 by Jacob, 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 Cliff Dyer, 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.

Last edited 11 years ago by Cliff Dyer (previous) (diff)

comment:30 by Cliff Dyer, 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.

Last edited 11 years ago by Cliff Dyer (previous) (diff)

comment:31 by Cliff Dyer, 11 years ago

Cc: jcd@… added

comment:32 by Shai Berger, 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:33 by Tim Graham <timograham@…>, 11 years ago

In 8165c2cfd1922bb9d56a9e68e6456736acacf445:

Improved deprecation warning for change in form boolean values.

refs #20684

Thanks jacob, jcd, and shai for the suggestions.

comment:34 by Tim Graham, 11 years ago

Easy pickings: unset

comment:35 by Tim Graham, 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 loic84, 11 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:37 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In e61d99d96daedfea2f6ba071945ef0d2f86883c7:

Fixed #20684 -- Added support for HTML5 boolean attributes to form widgets.

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