Skip to main content

Search and Top Navigation

#4695 closed bug (fixed)

Opened July 18, 2009 05:07PM UTC

Closed July 12, 2010 02:51PM UTC

Accordion issue (and fix) with fillSpace:true

Reported by: jzaefferer Owned by:
Priority: major Milestone: 1.8.2
Component: ui.accordion Version: 1.7.2
Keywords: Cc:
Blocked by: Blocking:
Description
There is a logical bug in accordion's .resize() method, when fillSpace
is true. It calculates the maximum padding of all content panels, and
then subtracts it from maxHeight to obtain the new panel size. This is
wrong. I, for example, need to use different paddings in different
content divs, and this approach produces wrong results in this case.
The correct thing to do is to subtract each panel's padding
individually.

I was able to fix this (in 1.7.2) by replacing this:

var maxPadding = 0;
this.headers.next().each(function() {
       maxPadding = Math.max(maxPadding, $(this).innerHeight() - $
(this).height());
}).height(Math.max(0, maxHeight - maxPadding))

with this:

this.headers.next().each(function() {
       var padding = $(this).innerHeight() - $(this).height();
       $(this).height(Math.max(0, maxHeight - padding));
})

Please integrate this fix in the dev version, and in 1.7.3.
Attachments (0)
Change History (5)

Changed July 18, 2009 05:08PM UTC by jzaefferer comment:1

description: {{{There is a logical bug in accordion's .resize() method, when fillSpace \ is true. It calculates the maximum padding of all content panels, and \ then subtracts it from maxHeight to obtain the new panel size. This is \ wrong. I, for example, need to use different paddings in different \ content divs, and this approach produces wrong results in this case. \ The correct thing to do is to subtract each panel's padding \ individually. \ \ I was able to fix this (in 1.7.2) by replacing this: \ \ var maxPadding = 0; \ this.headers.next().each(function() { \ maxPadding = Math.max(maxPadding, $(this).innerHeight() - $ \ (this).height()); \ }).height(Math.max(0, maxHeight - maxPadding)) \ \ with this: \ \ this.headers.next().each(function() { \ var padding = $(this).innerHeight() - $(this).height(); \ $(this).height(Math.max(0, maxHeight - padding)); \ }) \ \ Please integrate this fix in the dev version, and in 1.7.3.}}}{{{ \ There is a logical bug in accordion's .resize() method, when fillSpace \ is true. It calculates the maximum padding of all content panels, and \ then subtracts it from maxHeight to obtain the new panel size. This is \ wrong. I, for example, need to use different paddings in different \ content divs, and this approach produces wrong results in this case. \ The correct thing to do is to subtract each panel's padding \ individually. \ \ I was able to fix this (in 1.7.2) by replacing this: \ \ var maxPadding = 0; \ this.headers.next().each(function() { \ maxPadding = Math.max(maxPadding, $(this).innerHeight() - $ \ (this).height()); \ }).height(Math.max(0, maxHeight - maxPadding)) \ \ with this: \ \ this.headers.next().each(function() { \ var padding = $(this).innerHeight() - $(this).height(); \ $(this).height(Math.max(0, maxHeight - padding)); \ }) \ \ Please integrate this fix in the dev version, and in 1.7.3. \ }}}

Changed August 11, 2009 06:58PM UTC by jzaefferer comment:2

resolution: → fixed
status: newclosed

Fixed in r3047.

Changed August 11, 2009 07:14PM UTC by jzaefferer comment:3

resolution: fixed
status: closedreopened

Animations still jumpy for fillSpace.

Changed August 11, 2009 07:26PM UTC by jzaefferer comment:4

The visual testcase passes when replacing the em-padding with px. Animating an element to 15% of 3em doesn't work that well.

Changed July 12, 2010 02:51PM UTC by scottgonzalez comment:5

milestone: 1.91.8.2
resolution: → fixed
status: reopenedclosed

Closing as this is working with pixels. Handling ems is a separate issue that we already know about.