Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30459 closed Bug (fixed)

If a StackedInline has fieldsets with the "collapsed" class, the "Show" link doesn't work on inline forms added with the "Add another [inline object]" link

Reported by: Antonis Christofides Owned by: Carlton Gibson
Component: contrib.admin Version: 2.2
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

I attach a screenshot with explanations, which is the easiest way to get a grip of this problem.

Steps to reproduce the problem illustrated in the screenshot:

  1. Create a myblog django project and a blog app.
  1. Specify this models.py in the blog app:
    from django.db import models
    
    class BlogPost(models.Model):
        content = models.TextField()
    
    
    class Author(models.Model):
        blog_post = models.ForeignKey(BlogPost, on_delete=models.CASCADE)
        name = models.CharField(max_length=100)
        birthday = models.DateField(blank=True)
    
  1. Specify this admin.py:
    from django.contrib import admin
    
    from . import models
    
    
    class AuthorInline(admin.StackedInline):
        model = models.Author
        extra = 1
        fieldsets = [
            ("Essential", {"fields": ("name",), "classes": ("collapse",)}),
            ("Advanced", {"fields": ("birthday",), "classes": ("collapse",)}),
        ]
    
    
    @admin.register(models.BlogPost)
    class BlogPostAdmin(admin.ModelAdmin):
        inlines = [AuthorInline]
    
  1. Add the blog app to INSTALLED_APPS, makemigrations, migrate, createsuperuser, runserver.
  1. Visit /admin/, login, go to "Blog posts", click on "Add new blog post", then click on "Add new Author".

Result: The "Show" links on the new Author form don't work. See the screenshot for more information.

Attachments (2)

Selection_011.png (62.3 KB ) - added by Antonis Christofides 5 years ago.
Screenshot that illustrates the problem
Screenshot 2019-05-09 at 12.47.34 AM.png (33.3 KB ) - added by Ruchit Vithani 5 years ago.

Download all attachments as: .zip

Change History (15)

by Antonis Christofides, 5 years ago

Attachment: Selection_011.png added

Screenshot that illustrates the problem

comment:1 by Ruchit Vithani, 5 years ago

I could not reproduce this. See this screencast, both the links in my case works properly. Pardon me if did something wrong, or misunderstood something

by Ruchit Vithani, 5 years ago

comment:2 by Carlton Gibson, 5 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

OK, yes. Thanks for the report

This a regression in ba83378a7762c51be235b521aa5b48233d6c6c82, which was part of Django v2.2.
(Reproduced at af5ec222ccd24e81f9fec6c34836a4e503e7ccf7)

The change from using jQuery's `on()` did not correctly account for the _delegation_ of event handlers.
Instead of attaching directly to the a.collapse-toggle elements (which in this case do not exist in the DOM at when the listener is added) we should attach to a parent element and filter events by the event target class (as jQuery does/did for us).

comment:3 by Claude Paroz, 5 years ago

Are you sure that's the regression? Looking at the code, jQuery was also directly attaching the handler to the a link ($("fieldset.collapse a.collapse-toggle").on('click', function(ev) {).

comment:4 by Carlton Gibson, 5 years ago

Hey Claude.

I've tested on both 2.1 and 2.2. It works on 2.1, but not 2.2.

I (manually) bisected to the referenced commit, using this history view:

https://github.com/django/django/commits/master/django/contrib/admin/static/admin/js

It works at a5f139236f930df06ae0642507530ca98081e2a9 but fails at the next commit ba83378a7762c51be235b521aa5b48233d6c6c82 — so I pinned it to that. (It's the only change that looks relevant.)

You are right about the on() call though... 🤔

I'll manually run git bisect between those two commits later.

I was going to write a js_tests case to reproduce this.

(Either way, I think the need to _delegate_ the event listener is correct.)

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:5 by Claude Paroz, 5 years ago

Carlton, did you progress with the test or was it just an intention?

comment:6 by Claude Paroz, 5 years ago

I think this has something to do with the jQuery clone(true) method (called from inlines.js) which is supposed to also copy events from the original DOM element. It may be that it doesn't work with vanilla JS events? To be investigated.

comment:7 by Carlton Gibson, 5 years ago

Hey Claude.

No, I haven’t got there yet. (Worked out where to put it but not actually written it.)

My plan was to look at it next week (2019 WK20). If you beat me to it, that is fine. 🙂

comment:8 by Claude Paroz, 5 years ago

Has patch: set
Needs tests: set

A suggested fix, without tests though.

comment:9 by Carlton Gibson, 5 years ago

Awesome. Thanks Claude! I'm happy to add the test in the week. Super.

comment:10 by Carlton Gibson, 5 years ago

Owner: changed from nobody to Carlton Gibson
Status: newassigned

comment:11 by Carlton Gibson, 5 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In e286987a:

Fixed #30459 -- Delegated hide/show JS toggle to parent div.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 34a357d5:

[2.2.x] Fixed #30459 -- Delegated hide/show JS toggle to parent div.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

Backport of e286987a27271c8ee7eb6e4d4332b563c4e6094b from master

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