Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#10573 closed (fixed)

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

Reported by: rduffield Owned by: copelco
Component: contrib.admin Version: master
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: UI/UX:

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 6 years ago.
tests.diff (1.7 KB) - added by rduffield 6 years ago.
focus_first.patch (1.1 KB) - added by ales.zoulek@… 6 years ago.
focusing first active form element
focus_first.2.patch (1.3 KB) - added by ales_zoulek 6 years ago.
10573_r12827.diff (728 bytes) - added by copelco 5 years ago.
update using jquery
10573_r12827_with_fields.diff (784 bytes) - added by copelco 5 years ago.
alternative patch if #12627 is closed
10573.1.diff (3.6 KB) - added by ramiro 5 years ago.
Alternate Python-based approach
10573.2.diff (4.4 KB) - added by julien 5 years ago.

Download all attachments as: .zip

Change History (26)

Changed 6 years ago by rduffield

Changed 6 years ago by rduffield

comment:1 Changed 6 years ago by rduffield

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 follow-up: Changed 6 years ago by CollinAnderson

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

comment:3 in reply to: ↑ 2 Changed 6 years ago by rduffield

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

comment:4 Changed 6 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 6 years ago by anonymous

  • Cc ales.zoulek@… added

Changed 6 years ago by ales.zoulek@…

focusing first active form element

comment:6 Changed 6 years ago by ales.zoulek@…

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

comment:7 Changed 6 years ago by ales_zoulek

  • Owner changed from nobody to ales_zoulek

Changed 6 years ago by ales_zoulek

comment:8 Changed 6 years ago by ales_zoulek

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

comment:9 Changed 6 years ago by jacob

  • milestone changed from 1.1 to 1.2

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

comment:10 Changed 5 years ago by copelco

  • Owner changed from ales_zoulek to copelco
  • Status changed from new to assigned

Changed 5 years ago by copelco

update using jquery

Changed 5 years ago by copelco

alternative patch if #12627 is closed

comment:11 Changed 5 years ago by copelco

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 Changed 5 years ago by ubernostrum

  • milestone changed from 1.2 to 1.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 Changed 5 years ago by ramiro

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

Changed 5 years ago by ramiro

Alternate Python-based approach

comment:14 Changed 5 years ago by ramiro

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.

Changed 5 years ago by julien

comment:15 Changed 5 years ago by julien

  • Triage Stage changed from Accepted to Ready 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 Changed 5 years ago by russellm

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

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 Changed 5 years ago by russellm

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

  • milestone 1.3 deleted

Milestone 1.3 deleted

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