Opened 6 months ago

Closed 5 months ago

Last modified 5 months 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 6 months ago.
Screenshot that illustrates the problem
Screenshot 2019-05-09 at 12.47.34 AM.png (33.3 KB) - added by Ruchit Vithani 6 months ago.

Download all attachments as: .zip

Change History (15)

Changed 6 months ago by Antonis Christofides

Attachment: Selection_011.png added

Screenshot that illustrates the problem

comment:1 Changed 6 months ago by Ruchit Vithani

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

Changed 6 months ago by Ruchit Vithani

comment:2 Changed 6 months ago by Carlton Gibson

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 Changed 6 months ago by Claude Paroz

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 Changed 5 months ago by Carlton Gibson

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 months ago by Carlton Gibson (previous) (diff)

comment:5 Changed 5 months ago by Claude Paroz

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

comment:6 Changed 5 months ago by Claude Paroz

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 Changed 5 months ago by Carlton Gibson

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 Changed 5 months ago by Claude Paroz

Has patch: set
Needs tests: set

A suggested fix, without tests though.

comment:9 Changed 5 months ago by Carlton Gibson

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

comment:10 Changed 5 months ago by Carlton Gibson

Owner: changed from nobody to Carlton Gibson
Status: newassigned

comment:11 Changed 5 months ago by Carlton Gibson

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:12 Changed 5 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 5 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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