Code

Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#20765 closed Bug (fixed)

HTML5 number input type not working for DecimalField with big decimal_places

Reported by: aleksey.kladov@… Owned by: charettes
Component: Forms Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: charettes 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

DecimalFiled's widget is a NumberInput with step attribute equal to 10-decimal_placecese. https://github.com/django/django/blob/stable/1.6.x/django/forms/fields.py#L374).

If decimal_places is too big(the exact value for big should be found here http://www.w3.org/TR/2012/WD-html5-20121025/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values) the step value parses to 0, which is not valid (http://www.w3.org/TR/2012/WD-html5-20121025/common-input-element-attributes.html#attr-input-step). It is not good in itself, but it also causes surprising and unwanted behaviour.

For example in Google Chrome input with type="number" and step="0.0000000000000000000000000000001" accepts only integer values!

A possible fix is to add optional step argument to DecimalField and, and if it is not provided just use "any".

Attachments (2)

Change History (18)

comment:1 Changed 9 months ago by charettes

  • Cc charettes added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

We shouldn't add a step argument to DecimalField since it only concern the widget.

Do you know the exact value for big?

We could attempt defaulting step to the current behavior when decimal_place < big and fallback to any just how `FloatField` does.

comment:2 Changed 9 months ago by anonymous

Do you know the exact value for big?

No, I'm not an IEEE 754 expert =(

We could attempt defaulting step to the current behavior when decimal_place < big and fallback to any just how ​FloatField does.

I think that step > 0.001 is anyway useless in general. May be just .setdefault("step", "any") exactly as in FloatField? In the face of ambiguity, refuse the temptation to guess.

comment:3 Changed 9 months ago by charettes

  • Severity changed from Release blocker to Normal

It seems to work until 1-18.

IMHO the float parsing algorithm is quite unambiguous and the default step attribute is still overridable. I think we should be safe assuming our step generation mechanism works for decimal_places <= 18.

comment:4 Changed 9 months ago by matklad

the default step attribute is still overridable

Not really, look again at https://github.com/django/django/blob/stable/1.6.x/django/forms/fields.py#L374

Even if I do

answer = fields.DecimalField(max_digits=50,
                             decimal_places=25,
                             widget=NumberInput(attrs={"step": "any"}))

the step gets overriden.

comment:5 Changed 9 months ago by charettes

I guess we should use setdefault here instead of direct assignment.

comment:6 Changed 9 months ago by matklad

setdefult defenetelly should be there.

However I think that it's still worth to check that decimal_places <= 18 and use any as a default otherwise(or just always use any).

comment:7 Changed 9 months ago by claudep

What's definitely a bug is that step is not overridable.

As of the number of decimal places, it's browser-dependant, and I'm less enthousiast to do something about it.

comment:8 Changed 9 months ago by charettes

  • Owner changed from nobody to charettes
  • Status changed from new to assigned

I'll write a patch and fix tests for the setdefault behavior.

comment:9 Changed 9 months ago by matklad

I would be completely happy if I could override step. But it's because I know that I have to override it. Let me try to convince you for the last time that guessing step is not a good idea. There will be three points.

  1. Suppose I know nothing about this issue and just wrote in one of my forms number = fields.DecimalField(max_digits=50, decimal_places=25). I use firefox and the form is working OK in it. Moreover, tests are working too. However, very soon I get a bug report that it's impossible to use non-integer numbers in Chrome! So I have to look into the source of the page, notice suspicious step="0.0000000000000000000000000001", and find out that it is the reason of strange behaviour. Next, I have to look inside django's source, to find out, from where this step emerges. And only then I realize that I need to override it.
  1. Current default can produce invalid html.

The step attribute, if specified, must either have a value that is a valid floating-point number that parses to a number that is greater than zero, or must have a value that is an ASCII case-insensitive match for the string "any".

  1. If I close my eyes, forget about everything in the universe and ask myself, "what is the most reasonable default for step?" I hear a voice whispering "any".

Also, this issue can show more often than it seems. 10-18 is quit a little number, and it looks like most users won't use such a high precision. However, it is the DecimalField. And if someone is using a Decimal instead of Float, it means that he cares about high precision. So it's rather likely that he uses many decimal places. Also, python's default prec for decimals is 28 places!

comment:10 Changed 9 months ago by charettes

  • Has patch set

Added a patch that still sets step as default for max_digits <= 18 and 'any' in the other case.

comment:11 Changed 9 months ago by matklad

This looks good. I can see only one very minor caveat. If decimal_places=0 then step is equal to any, while 1 seems more reasonable.

Last edited 9 months ago by matklad (previous) (diff)

comment:12 Changed 9 months ago by charettes

I think I've found a middleground. Parsing works flawlessly in exponential form. Submitting an updated patch.

comment:13 Changed 9 months ago by matklad

Looks perfect!

As a bonus, exponential form improves html readability.

comment:14 Changed 9 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

RFC, but please either add a comment about the rationale or refer to this ticket. Thanks!

comment:15 Changed 9 months ago by Simon Charette <charette.s@…>

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

In 415a36947cb4b8b61230698e13cca2df9400e245:

Fixed #20765 -- Set small values of step using exponential notation.

Browsers parse small factors of 10 as 0 under decimal notation.

Thanks to Trac alias matklad for the report and Claude Paroz for the review.

comment:16 Changed 9 months ago by Simon Charette <charette.s@…>

In 9d3f7a21a337aa82d5c9ac5ef9a6a3888bff62a8:

[1.6.x] Fixed #20765 -- Set small values of step using exponential notation.

Browsers parse small factors of 10 as 0 under decimal notation.

Thanks to Trac alias matklad for the report and Claude Paroz for the review.

Backport of 415a36947c from master.

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.