Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#17218 closed Bug (fixed)

Select Filter (its "to" box) has 0 height if in a collapsed fieldset

Reported by: jimallman <jim@…> Owned by: jimallman
Component: contrib.admin Version: master
Severity: Release blocker Keywords: fieldset
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

The select filter creates a pair of "from" and "to" widgets (in SelectFilter2.js), and gives them matching height by applying the height of the "from" box to the "to" box.

But if a model uses collapsed fieldsets in admin pages, the "from" box has 0 height (says jQuery) which effectively hides the "to" box. The widget data is intact, but obviously this is confusing and makes removing individual values impossible in the UI.

This happens in both Firefox/Mac and IE8/Win (discovered while editing Entries in the Zinnia blog app).

Attachments (2)

collapsed-fieldset-hides-to-box.png (45.0 KB) - added by jimallman <jim@…> 3 years ago.
When you expand the fieldset, the widget is missing (height=0)
select-filter-waits-for-collapsed-fieldset.diff (3.4 KB) - added by jimallman <jim@…> 3 years ago.
Simple event trigger on fieldset, resizes each select filter just once.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by jimallman <jim@…>

When you expand the fieldset, the widget is missing (height=0)

comment:1 Changed 3 years ago by jimallman <jim@…>

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to anonymous
  • Patch needs improvement unset
  • Status changed from new to assigned

I'm going to attempt a fix, either by postponing the matching height (perhaps with an event listener) or by using a more sensitive test for $(from_box).outerHeight()

comment:2 Changed 3 years ago by jimallman <jim@…>

  • Owner changed from anonymous to jimallman
  • Status changed from assigned to new

comment:3 Changed 3 years ago by julien

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

Thanks for the report. This is a regression that needs to get fixed before the next release.

comment:4 Changed 3 years ago by jimallman <jim@…>

  • Has patch set
  • Needs tests set

OK, I've submitted a pull request to django/django on Github:

https://github.com/django/django/pull/83

I've run the modified collapse.js through the Closure Compiler, as requested. I don't have automated tests for this, sorry.

To see this pull request as a patch:

https://github.com/django/django/pull/83.diff

Note that toggling a collapsible fieldset will now fire new custom jQuery events (show.fieldset and hide.fieldset) to all its child elements. This should come in handy for other widgets that need to adjust their placement or dimensions. See the changes to SelectFilter2.js for an example of binding widgets to respond to these events.

comment:5 Changed 3 years ago by julien

I'm not sure that triggering the event with everything inside the fieldset is necessary. Why not trigger it just on the fieldset itself. The select filter can then simply subscribe to its parent fieldset's event being triggered.

As for the tests, I'm hoping to wrap up the work on #2879 so that some tests could be added for things like this.

Thanks for working on this!

comment:6 Changed 3 years ago by jimallman <jim@…>

As suggested, the show.fieldset and hide.fieldset events are now only triggered on the fieldset element itself. Each "subscriber" adds a handler to the fieldset to do its thing. (These events also bubble up the DOM, so the body or other elements on the page can respond.)

In this version, each select filter resizes just once, then unbinds its handler for fieldset events. You can get the latest version as a patch using the same URL as before:

https://github.com/django/django/pull/83.diff

...which I realize could become really confusing later. I'm going to attach this version here as a traditional patch, as well.

As before, I've tested this in Safari/Mac, FF/Mac, IE8/Win, and IE7 (via compatibility mode).

Changed 3 years ago by jimallman <jim@…>

Simple event trigger on fieldset, resizes each select filter just once.

comment:7 Changed 3 years ago by adrian

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

In [17181]:

Fixed #17218 -- Fixed bug with SelectFilter where the 'to' box had a height=0 when it was within a collapsed fieldset. Thanks jimallman

comment:8 Changed 3 years ago by adrian

Thanks for the patch, jimallman. I made a few changes before committing:

  • I used jQuery chaining so that we wouldn't have to look up $(this).closest("fieldset") twice.
  • I made a resize_filters() function so that we wouldn't have to define duplicate logic for the height normalization.
  • I used jQuery's one() method, which does the work of unbind().

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.