Opened 7 years ago

Closed 5 years ago

#16641 closed defect (wontfix)

AccordionContainer: sizing problems if resized while animation in progress

Reported by: idleWombat Owned by: idleWombat
Priority: undecided Milestone: tbd
Component: Dijit Version: 1.7.3
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

The layout function of the dijit.layout.AccordionContainer calculates the new verticalSpace for selected accordion widgets. In my opinion the calculation is wrong, because when summing up the total height of all closed accordion widgets, it uses the height of their domNodes instead of the _buttonWidget height.

A spike solution with a patched version of the AccordionContainer can be watched here: http://jsfiddle.net/idleWombat/cjxpu/40/

To use the patched version see the lines:

var accordion = new AccordionContainer();
// use patched version
//var accordion = new PatchedAccordionContainer();

Attachments (1)

bug_report_accordion_container.html (2.8 KB) - added by idleWombat 7 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by bill

Description: modified (diff)
Owner: changed from bill to idleWombat
Status: newpending

I don't see why it would make a difference which one we measure.

Please attach a test case using the "attach file" button. It should be as small as possible to still reproduce the problem, almost always a single HTML file that we can load in the browser (i.e. not PHP, JSP, etc.)

Then, give exact instructions on how to reproduce the problem using your attached test file.

The test case is necessary to confirm that there's a bug, and to confirm that your change fixes the bug.

Thanks!

comment:2 Changed 7 years ago by idleWombat

Status: pendingnew

I have already spend alot of time with that issue and I have posted a JSFiddle link, so you can see the problem: just switch between the accordion items and you will see the problem. I don't see the effort to extract the code from JSFiddle into a single HTML file.

The verticalSpace for the new selected accordion item should be calculated by v_height = contentbox.h - sum of all button heights - [...] Instead of "sum of all button heights" you use the sum of domNode height. In the demonstrated case the height of the last opened accordion item (not only its button height) is used for this calculation. I think its related to programmatically producing a resize when switching between accordion items. Perhaps this is a special case, but it can happen.

Greetings!

Last edited 7 years ago by idleWombat (previous) (diff)

comment:3 Changed 7 years ago by bill

Status: newpending

I see a problem in your fiddle where selecting a different accordion pane changes the total height of the accordion. However, your test case is monkey patching the accordion code.

What I'd like to see is just a plain test case of accordion, without any monkey patching, that shows the problem.

comment:4 Changed 7 years ago by idleWombat

Status: pendingnew

Okay, perhaps I will find the time to do that - I don't know right now.

By the way: I found some hints in your code which support what I say:

1.

During animation there are two dijtAccordionChildWrapper's shown, so we need to compensate for that.

(this leads to the problem when using domNode for the calculation)

Normally the dijtAccordionChildWrapper is hidden for all but one child (the shown child), so the space for the content pane is all the title bars + the one dijtAccordionChildWrapper, which on claro has a 1px border plus a 2px bottom margin.

(so it really should only be the title bars to calculate with)

comment:5 Changed 7 years ago by bill

Status: newpending

OK, I will wait for your test case.

Changed 7 years ago by idleWombat

comment:6 Changed 7 years ago by idleWombat

Status: pendingnew

Attachment (bug_report_accordion_container.html) added by ticket reporter.

comment:7 Changed 7 years ago by bill

Status: newpending

I tried the test case. It's a strange test case because you have code like:

onShow: function(){
   registry.byId("bottomCP").resize({h:200, w:620});
   registry.byId("mainLayout").resize();
},
onHide: function(){
   registry.byId("bottomCP").resize({h:50, w:620});
   registry.byId("mainLayout").resize();
}

Is that essential to reproducing the bug? I do see sometimes when the accordion's height is less than the height of the ContentPane, but that could be because you are calling registry.byId("mainLayout").resize() during the animation, which isn't supported.

comment:8 Changed 7 years ago by idleWombat

Status: pendingnew

These lines were just to reproduce the things that happen in our project. In the project there is a bottom ContentPane? which shows a grid when one of the accordion items is clicked and so the resizing is necessary.

You say that resizing during animation is not supported. I see this and this is the bug in my opinion. We wont be the only ones resizing during the animation. Further I see no reason to measure the domNodes instead of the title buttons, because there will never be more than one widget showing in the AccordionContainer?.

comment:9 Changed 7 years ago by bill

Status: newpending

Presumably the reason is that the wrapper node contains padding, margin, border not in the button node. If you look at test_AccordionContainer.html (using firebug, etc.), note that the <div id="lazyLoadPane_wrapper"> is 25px tall but the <div id="lazyLoadPane_button"> is only 18px tall. So with your suggested calculation, we'd be losing 7px for every closed pane.

comment:10 Changed 7 years ago by bill

Summary: dijit.layout.AccordionContainer layout function verticalSpace calculation falsyAccordionContainer: sizing problems if resized while animation in progress

comment:11 Changed 7 years ago by idleWombat

Status: pendingnew

Okay, so we see the same limitation with resizing during animation, but I somehow get the feeling that you dont want to support this anyway.

If we want to find a solution: I see your point. My fix is improving it but not 100% correct. Can't we determine the wrappers node padding, margin and border and then add it to the button height?

Last edited 7 years ago by idleWombat (previous) (diff)

comment:12 Changed 7 years ago by bill

Well, let's say the animation is 30% complete, so there are two panes that are partially open, one 300px tall and the other 100px tall, plus a few closed panes (25px tall each). Now the accordion gets a resize command to increase it's total height from 500px to 600px. So it needs to resize the first partially open pane to 367px and then resize the other partially open pane to 133px, and then adjust the currently running animations to account for those size changes. That's much too complicated, isn't it?

It seems like what you really want is to do your other work either right before the animations start, or right after they end. There's no reason to do the resizing while the animations are in progress, is there?

comment:13 Changed 7 years ago by idleWombat

You're right. We can also do it after the animation. Although this is a unnecessary delay in my opinion.

The animation state does not have to be considered when calculating the new verticalSpace for the new opened accordion widget. Also the closing accordion widgets height is not important for that calculation in my opinion. We just need the verticalSpace for the new accordion widget and that is: contentBox (retrieved after resize() is called) - sum of all title button heights + the wrapper margin, padding and border. My (not perfect) bug fix should show that this is the right way to think, doesn't it?

Last edited 7 years ago by idleWombat (previous) (diff)

comment:14 Changed 7 years ago by bill

OK. I'm still skeptical that it's worth supporting this corner case. But probably the easiest way to get what you want (without the 7px calculation error) is in layout() to [temporarily] set the containerNode height of the in-progress-being-closed-pane to 0:

array.forEach(this.getChildren(), function(child){
	if(child != openPane){
		child._wrapperWidget.containerNode.style.height = 0; <== here
		totalCollapsedHeight += domGeometry.getMarginSize(child._wrapperWidget.domNode).h;
	}
});

I didn't try it though.

Also, the fix would probably need to be reworked when (in the future) AccordionContainer is switched to use CSS animations.

comment:15 Changed 5 years ago by bill

Resolution: wontfix
Status: newclosed

Sorry, I'm going to close this; we're winding down on enhancements to Dijit V1, and future accordion widgets will be a different ball of wax. They'll use CSS animations and I'm doubtful we'll still support the same concepts of resizing... or if we do it may be implemented using CSS flexbox.

Note: See TracTickets for help on using tickets.