Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#13674 closed (fixed)

Javascript for adding inline fields doesn't update "for" attribute in label

Reported by: Jonas Owned by: rctay
Component: contrib.admin Version: 1.2
Severity: Keywords: admin
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

I do some checking for required fields using Javascript in the admin. Doing that, I noticed that adding inline fields doesn't update the "for" attribute for the new <label> fields. If the field is called "image", the newly added field label looks like this:

<label class="required" for="id_images-__prefix__-image">Image:</label>

This is the patch that solved it for me:

diff --git a/django/contrib/admin/media/js/inlines.js b/django/contrib/admin/media/js/inlines.js
index cf79023..90b50b5 100644
--- a/django/contrib/admin/media/js/inlines.js
+++ b/django/contrib/admin/media/js/inlines.js
@@ -75,6 +75,14 @@
                                    }).each(function() {
                                        var el = $(this);
                                        el.attr("name", el.attr("name").replace(/__prefix__/g, nextIndex));
+                                   })
+                                   .end()
+                                   .filter(function() {
+                                       var el = $(this);
+                                       return el.attr("for") && el.attr("for").search(/__prefix__/) >= 0;
+                                   }).each(function() {
+                                       var el = $(this);
+                                       el.attr("for", el.attr("for").replace(/__prefix__/g, nextIndex));
                                    });
                                if (row.is("tr")) {
                                        // If the forms are laid out in table rows, insert

Attachments (2)

0001-fix-updating-of-the-for-attribute-in-new-admin-inlin.patch (3.8 KB) - added by rctay 4 years ago.
13674_inlines_label_ids.diff (6.6 KB) - added by julien 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by russellm

  • Component changed from Uncategorized to django.contrib.admin
  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 in reply to: ↑ description Changed 4 years ago by rctay

Replying to Jonas:

This is the patch that solved it for me:

diff --git a/django/contrib/admin/media/js/inlines.js b/django/contrib/admin/media/js/inlines.js
index cf79023..90b50b5 100644
--- a/django/contrib/admin/media/js/inlines.js
+++ b/django/contrib/admin/media/js/inlines.js
@@ -75,6 +75,14 @@
                                    }).each(function() {
                                        var el = $(this);
                                        el.attr("name", el.attr("name").replace(/__prefix__/g, nextIndex));
+                                   })
+                                   .end()
+                                   .filter(function() {
+                                       var el = $(this);
+                                       return el.attr("for") && el.attr("for").search(/__prefix__/) >= 0;
+                                   }).each(function() {
+                                       var el = $(this);
+                                       el.attr("for", el.attr("for").replace(/__prefix__/g, nextIndex));
                                    });
                                if (row.is("tr")) {
                                        // If the forms are laid out in table rows, insert

You should be modifying updateElementIndex() instead - it will be much cleaner.

diff --git a/django/contrib/admin/media/js/inlines.js b/django/contrib/admin/media/js/inlines.js
index cf79023..21e00ad 100644
--- a/django/contrib/admin/media/js/inlines.js
+++ b/django/contrib/admin/media/js/inlines.js
@@ -18,7 +18,7 @@
        $.fn.formset = function(opts) {
                var options = $.extend({}, $.fn.formset.defaults, opts);
                var updateElementIndex = function(el, prefix, ndx) {
-                       var id_regex = new RegExp("(" + prefix + "-\\d+)");
+                       var id_regex = new RegExp(prefix + "-__prefix__");
                        var replacement = prefix + "-" + ndx;
                        if ($(el).attr("for")) {
                                $(el).attr("for", $(el).attr("for").replace(id_regex, replacement));

comment:3 Changed 4 years ago by rctay

Attached a patch for both the full and minified JS files.

Note that apart from changing the regex to search for __prefix___ instead of a number (\d+), the capturing parentheses () has also been removed, since we don't need the matched result.

comment:4 Changed 4 years ago by rctay

  • Owner changed from nobody to rctay

comment:5 Changed 3 years ago by ramiro

#15106 was a duplicate and contained a pattch similar to the fix suggested by the OP of this ticket.

comment:6 Changed 3 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

The patch applies and fixes this bug. See also #14303 for a related (but different) bug.

comment:7 Changed 3 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

Patch applies and fixes the bug, but introduces a different bug -newly created inlines don't validate.

To reproduce:

  • Use a parent model with a TabularInline child, with extra=3
  • apply patch 0001
  • Add a new parent object
  • Add 1 extra inline row
  • Populate all 4 inline objects (the 3 default provided, plus the 1 added inline).
  • Try to save the object

The object will fail validation, and lose the data for the fourth inline. If you re-enter and save, the data is saved successfully.

comment:8 Changed 3 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Thanks for spotting this issue, Russ - and sorry for having missed it. I've attached a patch which should fix the root problem. Taking the liberty to RFC again so it gets on your radar for the 1.3 home stretch.

Changed 3 years ago by julien

comment:9 Changed 3 years ago by russellm

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

In [15436]:

Fixed #13674 -- Ensure that labels on added inlines fields have the right 'for' attribute. Thanks to Jonas for the report, and Julien Phalip for the patch.

comment:10 Changed 3 years ago by russellm

In [15437]:

(The changeset message doesn't reference this ticket)

comment:11 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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.