Code

Opened 10 months ago

Closed 4 weeks ago

#20684 closed New feature (fixed)

Support form element attributes with no value

Reported by: sneethling@… Owned by: nobody
Component: Forms Version: master
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 claudep)

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 />

Attachments (0)

Change History (37)

comment:1 Changed 10 months ago by claudep

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Description reformatted, please use preview when posting.

comment:2 Changed 10 months ago by claudep

  • Triage Stage changed from Unreviewed to 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 Changed 10 months ago by shai

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 10 months ago by shai (previous) (diff)

comment:4 Changed 10 months ago by loic84

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 10 months ago by loic84 (previous) (diff)

comment:5 Changed 10 months ago by loic84

  • Cc loic@… added
  • Easy pickings set
  • Has patch set
  • Needs documentation set
  • Patch needs improvement set
  • Version changed from 1.5 to master

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

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

comment:6 Changed 10 months ago by charettes

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 Changed 10 months ago by loic84

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 Changed 10 months ago by shai

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 Changed 10 months ago by shai

  • Cc shai@… added

comment:10 follow-up: Changed 10 months ago by loic84

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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 10 months ago by 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?

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.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 10 months ago by loic84

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.

comment:13 in reply to: ↑ 12 Changed 10 months ago by charettes

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 Changed 10 months ago by shai

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 Changed 10 months ago by claudep

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 Changed 10 months ago by anonymous

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 Changed 10 months ago by shai

(comment:16 is me, of course).

comment:18 follow-up: Changed 10 months ago by claudep

@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 Changed 10 months ago by loic84

+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 in reply to: ↑ 18 Changed 10 months ago by shai

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 Changed 10 months ago by loic84

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

comment:22 Changed 10 months ago by carljm

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 follow-up: Changed 10 months ago by loic84

@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 Changed 10 months ago by shai

@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 in reply to: ↑ 23 Changed 10 months ago by charettes

@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 Changed 10 months ago by claudep

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 Changed 10 months ago by Claude Paroz <claude@…>

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 Changed 7 months ago by jacob

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 Changed 7 months ago by jcd

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 7 months ago by jcd (previous) (diff)

comment:30 Changed 7 months ago by jcd

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.

Version 0, edited 7 months ago by jcd (next)

comment:31 Changed 7 months ago by jcd

  • Cc jcd@… added

comment:32 Changed 7 months ago by shai

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 Changed 7 months ago by Tim Graham <timograham@…>

In 8165c2cfd1922bb9d56a9e68e6456736acacf445:

Improved deprecation warning for change in form boolean values.

refs #20684

Thanks jacob, jcd, and shai for the suggestions.

comment:34 Changed 7 months ago by timo

  • Easy pickings unset

comment:35 Changed 4 weeks ago by timo

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 Changed 4 weeks ago by loic84

  • Needs documentation unset
  • Patch needs improvement unset

comment:37 Changed 4 weeks ago by Tim Graham <timograham@…>

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

In e61d99d96daedfea2f6ba071945ef0d2f86883c7:

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

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.