Opened 11 months ago

Closed 10 months ago

Last modified 10 months ago

#23075 closed Bug (fixed)

Type-specific input fields cause cross-browser issues and wrong error messages

Reported by: sehmaschine Owned by: nobody
Component: Forms Version: 1.7-rc-1
Severity: Normal Keywords:
Cc: sehmaschine@… 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

Django recently introduced a NumberInput which is rendered with type="number". While this seems like a good idea at first view, it leads to a couple of issues with different browsers:

a) Safari removes (!) any invalid values from this input field. This leads to unpredictable situations when trying to save with the admin.
b) Other browsers render non stylable error messages before (!) saving the form: This contradicts the standard Django form behaviour with the admin, when error messages are shown after (!) the form is being submitted.

I suggest using type="text" for any NumberInput or remove the field altogether. The current behaviour with this field is cross-browser madness and a conflict with how Django handles errors with forms.

Change History (12)

comment:1 Changed 11 months ago by erikr

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from NumberInput cross-browser issues and wrong error messages to Type-specific input fields cause cross-browser issues and wrong error messages
  • Triage Stage changed from Unreviewed to Accepted

I see the issue, but I'm not sure what the appropriate resolution is.

This is not limited to numbers: I've had this issue myself with URLField: that is now type="url", which enforces browser-specific validation, but this is inconsistent across browsers. For example, google.com is valid in django's URL validator, also accepted by Safari, but not by Chrome. So while using type-specific fields is in general a good practice, we have created browser-dependant inconsistent validation which is hard to influence. And as you note, the behaviour between different fields may now also be inconsistent: some are validated immediately, others only later.

These issues may also affect EmailInput. As a workaround for now, you can still set the widget of any field you define to forms.TextInput().

comment:2 Changed 11 months ago by sehmaschine

From my point of view, the appropriate solution is to use type="text" until all issues are resolved with these types.
Especially with the admin interface, form validation should not be moved to the frontend (unless otherwise clearly stated and consistently integrated).

Besides, I think the "workaround" should be reversed:
Using type="text" should be the standard way of handling form fields. The new types could be implemented with a widget – if you exactly know what you're doing and if you are keen to accept the inconsitencies for your users (or administrators).

If this solution is not accepted, I suggest adding a bold warning with the release of Django 1.7: The new behaviour could lead to severe errors with your application.

comment:3 Changed 11 months ago by sehmaschine

Here is an example in order to better understand the possible consequences:

Let's say you have a Model with is edited using TabularInlines with the admin interface. The Model consists of a couple of fields with one DecimalField which is not required (blank=True). So far, an editor could not save an inline row if the value of that DecimalField was wrong (e.g. due to a simple typo). With the new behaviour, the whole inline-row is being saved without any error message (at least with Safari). In my opinion, this is a major shift in how the admin works with the new type-specific fields.

comment:4 Changed 11 months ago by claudep

Note that these specific input types have been introduced in Django 1.6, not 1.7.

comment:5 Changed 11 months ago by erikr

Thanks for the additional info. Personally, I'm inclined to make TextInput() the default widget, but I've brought it up on the django-developers mailing list for wider discussion: https://groups.google.com/forum/#!topic/django-developers/TGwABHUPrw8

comment:6 Changed 11 months ago by timo

While the input types were introduced in Django 1.6 as Claude said, they weren't added to the admin until 1.7 (2a979d2a7bec485e4b90b7ae99ace0dd16faa948), so backwards compatibility isn't an issue if we want to revert that.

comment:7 Changed 11 months ago by sehmaschine

I just tested with Django 1.6 and DecimalFields are being rendered as type="number" with the admin.

comment:8 Changed 11 months ago by timo

Yes, just EmailInput and URLInput were added to the admin in 1.7.

comment:9 Changed 10 months ago by erikr

  • Has patch set

I created https://github.com/django/django/pull/3105 in which I've implemented the last solution I suggested in the mailing list thread:

I've had another look at this. The novalidate attribute on the form for URL and email fields indeed disables the validation in both Chrome and Safari. For number fields, I can reproduce Patrick's test: Safari will still silently drop the value.

So, for URL and email fields, this issue is resolved by setting novalidate on the form. I think we should document a recommendation for users to add this attribute to their forms, and change the forms in the admin to always include the novalidate attribute, as Bruno suggested.

This does not resolve the issue of Safari silently discarding invalid numbers. I don't see a way to resolve that, other than to revert to the text field. Perhaps this is a Safari bug. I haven't managed to find a standard on what browsers should be doing in this case, but this behaviour seems awful. But even if it is a Safari bug, that wouldn't fix the issue for a while anyways. Overall, I'd rather go for the partial novalidate solution for now, than do nothing at all.

comment:10 Changed 10 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 10 months ago by Erik Romijn <eromijn@…>

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

In cbdda28208c9c2aea479d5a482ff27bf37869d34:

Fixed #23075 -- Added documentation on novalidate attribute and made it default for admin

Thanks to sehmaschine for the report and to Tim Graham for the review.

comment:12 Changed 10 months ago by Erik Romijn <eromijn@…>

In 307eef20e35e78b1e812dc347c6c959e380267cf:

[1.7.x] Fixed #23075 -- Added documentation on novalidate attribute and made it default for admin

Backport of cbdda28208c9c2aea479d5a482ff27bf37869d34 from master.

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