Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#23075 closed Bug (fixed)

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

Reported by: Patrick K 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 by Sasha Romijn, 10 years ago

Summary: NumberInput cross-browser issues and wrong error messagesType-specific input fields cause cross-browser issues and wrong error messages
Triage Stage: UnreviewedAccepted

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 by Patrick K, 10 years ago

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 by Patrick K, 10 years ago

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

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

comment:5 by Sasha Romijn, 10 years ago

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 by Tim Graham, 10 years ago

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 by Patrick K, 10 years ago

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

comment:8 by Tim Graham, 10 years ago

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

comment:9 by Sasha Romijn, 10 years ago

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 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Erik Romijn <eromijn@…>, 10 years ago

Resolution: fixed
Status: newclosed

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 by Erik Romijn <eromijn@…>, 10 years ago

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