Opened 15 years ago

Closed 13 years ago

Last modified 12 years ago

#10573 closed (fixed)

Bug when auto-focusing on field with MultiWidget in admin site

Reported by: rduffield Owned by: Colin Copeland
Component: contrib.admin Version: dev
Severity: Keywords:
Cc: ales.zoulek@… 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

The admin site JavaScript, when creating a new instance of a model with a DateTimeField, attempts to focus on an element that does not exist. Take this basic example:

class Reservation(models.Model):
    start_date = models.DateTimeField(blank=False, null=False)

This results in the use of the AdminSplitDateTime widget. The two respective HTML fields have IDs of id_start_date_0 and id_start_date_1. However, the JavaScript that is generated is:

document.getElementById("id_start_date").focus();

This fails, due to there not being an element on the page with an ID of id_start_date.

I tracked this down to the _auto_id method within the BoundField class. Creating the auto_id depends on the field's html_name; in the case outlined above, both fields have html_names of start_date. However, the rendering of a MultiWidget is such that the IDs are generated differently than normal widgets.

My naive patch introduces a new property on a BoundField called focus_id. The focus_id returns auto_id as before for all widgets that are not MultiWidgets. Otherwise, it returns the first element within a MultiWidget set. The admin change_form.html template now also takes advantage of this field. I did not want to change any logic within auto_id itself; this seemed a little more dangerous. :-)

Attachments (8)

admin.diff (1.5 KB ) - added by rduffield 15 years ago.
tests.diff (1.7 KB ) - added by rduffield 15 years ago.
focus_first.patch (1.1 KB ) - added by ales.zoulek@… 15 years ago.
focusing first active form element
focus_first.2.patch (1.3 KB ) - added by Ales Zoulek 15 years ago.
10573_r12827.diff (728 bytes ) - added by Colin Copeland 14 years ago.
update using jquery
10573_r12827_with_fields.diff (784 bytes ) - added by Colin Copeland 14 years ago.
alternative patch if #12627 is closed
10573.1.diff (3.6 KB ) - added by Ramiro Morales 13 years ago.
Alternate Python-based approach
10573.2.diff (4.4 KB ) - added by Julien Phalip 13 years ago.

Download all attachments as: .zip

Change History (26)

by rduffield, 15 years ago

Attachment: admin.diff added

by rduffield, 15 years ago

Attachment: tests.diff added

comment:1 by rduffield, 15 years ago

I have attached a test that reproduces this bug.

Note that I should have been more clear; this bug is only evident if the DateTime field is of course the first field rendered in the admin form.

comment:2 by Collin Anderson, 15 years ago

Can we instead have the javascript find the first form element automatically and focus that?

in reply to:  2 comment:3 by rduffield, 15 years ago

That certainly seems like a great alternative to me; I was simply trying to maintain what I figured was the convention.

comment:4 by Jacob, 15 years ago

milestone: 1.1
Triage Stage: UnreviewedAccepted

comment:5 by anonymous, 15 years ago

Cc: ales.zoulek@… added

by ales.zoulek@…, 15 years ago

Attachment: focus_first.patch added

focusing first active form element

comment:6 by ales.zoulek@…, 15 years ago

I added a patch. Focusing first form field could not be as trivial at it seems. But it should work fine.

comment:7 by Ales Zoulek, 15 years ago

Owner: changed from nobody to Ales Zoulek

by Ales Zoulek, 15 years ago

Attachment: focus_first.2.patch added

comment:8 by Ales Zoulek, 15 years ago

updated the patch to first try it the old way and if it fails to fall back to for-loop.

comment:9 by Jacob, 15 years ago

milestone: 1.11.2

Pushing to 1.2: this is a UI issue which is annoying but doesn't impact usability.

comment:10 by Colin Copeland, 14 years ago

Owner: changed from Ales Zoulek to Colin Copeland
Status: newassigned

by Colin Copeland, 14 years ago

Attachment: 10573_r12827.diff added

update using jquery

by Colin Copeland, 14 years ago

alternative patch if #12627 is closed

comment:11 by Colin Copeland, 14 years ago

Tested in:

  • Mac: Safari 4.0.5, Firefox 3.6, Chrome 5.0.307.11 beta
  • Windows: Internet Explorer 6.0.2800.1106

comment:12 by James Bennett, 14 years ago

milestone: 1.21.3

This is another ticket that's not really release-critical; pushing off the 1.2 milestone, and we'll likely hit it in 1.3 (which is being scheduled to focus on these sorts of minor-annoyance issues).

comment:13 by Ramiro Morales, 13 years ago

#14364 and #14858 were also a duplicates, both proposed JS-based solutions.

by Ramiro Morales, 13 years ago

Attachment: 10573.1.diff added

Alternate Python-based approach

comment:14 by Ramiro Morales, 13 years ago

I've just attached a patch that, just like rduffield's patch, proposes solving this on the Python side by adding a property to django.forms.BoundField.

Difference is that it uses a strategy similar to the label_tag() method to make sure it returns a form ID usable for e.e. auto-focusing the field. The new property name is stable_field, for lack of a better name, can be changed if native English speakers deem necessary.

by Julien Phalip, 13 years ago

Attachment: 10573.2.diff added

comment:15 by Julien Phalip, 13 years ago

Triage Stage: AcceptedReady for checkin

Ramiro, I like the approach in your patch. I've just updated it with a test for when the first field only has a single widget. I've also renamed the stable_id property to id_for_label, which may be a bit more versatile. Thanks!

comment:16 by Russell Keith-Magee, 13 years ago

Resolution: fixed
Status: assignedclosed

In [15452]:

Fixed #10573 -- Corrected autofocus problem in admin when the first widget displayed is a multiwidget. Thanks to rduffield for the report, and to Ramiro and Julien Phalip for the patch.

comment:17 by Russell Keith-Magee, 13 years ago

In [15456]:

[1.2.X] Fixed #10573 -- Corrected autofocus problem in admin when the first widget displayed is a multiwidget. Thanks to rduffield for the report, and to Ramiro and Julien Phalip for the patch.

Backport of r15452 from trunk.

comment:18 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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