Opened 11 years ago

Closed 11 years ago

#7598 closed defect (fixed)

"Janky" resize in border container for IE

Reported by: Dustin Machi Owned by: Dustin Machi
Priority: high Milestone: 1.2
Component: Dijit Version: 1.2beta
Keywords: border container resize Cc:
Blocked By: Blocking:

Description

When the 'janky' branch of bc's _layoutChildren gets called it fails to pass on a correct/complete box to the children's resize method. A test file is attached.

This method, in the janky branch is declared

                       var resizeWidget = function(widget, dim){
                                if(widget){
                                        (widget.resize ? widget.resize(dim) : dojo.marginBox(widget.domNode, dim));
                                }
                        };

followed by several lines:

 if(leftSplitter){ leftSplitter.style.height = sidebarHeight; }
                        if(rightSplitter){ rightSplitter.style.height = sidebarHeight; }
                        resizeWidget(this._leftWidget, {h: sidebarHeight}, dim.left);
                        resizeWidget(this._rightWidget, {h: sidebarHeight}, dim.right);

                        if(topSplitter){ topSplitter.style.width = sidebarWidth; }
                        if(bottomSplitter){ bottomSplitter.style.width = sidebarWidth; }
                        resizeWidget(this._topWidget, {w: sidebarWidth}, dim.top);
                        resizeWidget(this._bottomWidget, {w: sidebarWidth}, dim.bottom);

                        resizeWidget(this._centerWidget, dim.center);

all execpt the final resizeWidget call is incorrect. There are only two params to the resizeWidget call. I believe the middle 'object' can just be removed. In my local tests so far I am getting data through that is now close to correct. They height thus far always appears to be 3px off, but this may be something in the app I am working on not the bc.

Attachments (2)

bcPatch.diff (1022 bytes) - added by Dustin Machi 11 years ago.
Patch file
test_BorderContainer_resize.html (3.3 KB) - added by Dustin Machi 11 years ago.
Test file

Download all attachments as: .zip

Change History (13)

Changed 11 years ago by Dustin Machi

Attachment: bcPatch.diff added

Patch file

Changed 11 years ago by Dustin Machi

Test file

comment:1 Changed 11 years ago by Adam Peller

Owner: set to Adam Peller
Status: newassigned

comment:2 Changed 11 years ago by James Burke

I think I have a related issue, but my test is more complex:

Dojo 1.1.1 example: http://jburke.dojotoolkit.org/bugs/7598/ComplexBorderContainer.html

Dojo 1.2beta1 example: http://jburke.dojotoolkit.org/bugs/7598/ComplexBorderContainer12.html

It seems like the margins set on the parent widget (a my.RoundRect? in my example) that contains the middle BorderContainer? are not taken into account. If I manually set equivalent margins on the center border container, things looked better, but the height sizing was still inconsistent.

I was able (at least in the 1.1.1 code) to change the code so that Firefox used the janky path, and the problem was reproducible in that browser.

comment:3 Changed 11 years ago by bill

The problem isn't the calls to resizeWidget() but rather the definition of resizeWidget(). It should be taking three arguments, to match _LayoutWidget.resize() (which takes two arguments, plus the implicit pointer to the widget itself):

resize: function(newSize, currentSize){

comment:4 Changed 11 years ago by Adam Peller

(In [15182]) pass 3rd arg to resize. Refs #7598 !strict

comment:5 Changed 11 years ago by bill

OK, I take back my comment, because:

  • if you resize the browser window need to pass both width and height, because both have changed
  • in the case where one dimension has changed (for example when you move the splitter for the bottom pane) then the height has changed but we are passing the width for second argument
  • (as dustin pointed out to me) the code is currently passing the new size as the third argument to resizeWindow(), not the current size

Anyway, I think Dustin was right originally, sorry for the confusion.

comment:6 Changed 11 years ago by Dustin Machi

Owner: changed from Adam Peller to Dustin Machi
Status: assignednew

comment:7 Changed 11 years ago by Dustin Machi

(In [15216]) update janky section for resize. refs #7598 !strict

comment:8 Changed 11 years ago by Adam Peller

(In [15218]) Back out [15182]. Refs #7598 !strict

comment:9 Changed 11 years ago by bill

[15216] broke mail demo. Click "new message" on IE7 and see the "To" and "Subject" fields overlapping editor.

comment:10 Changed 11 years ago by bill

OK, I take back all my comments above. I was half asleep when I wrote them. It turns out that (knock on wood) the code was functionally basically correctly originally.

BorderContainer is supposed to query the height of the top and bottom panes rather than setting it. It queries the height of the top and bottom panes and then adjusts the height of the center pane to fill the remaining space. The only time it sets the height of the top/bottom panes is when the user drags the splitter, and that's handled in the Splitter class with a dojo.marginBox() call. (Note that at some point this code should be refactored because it's inefficient and dangerous to set the size and then query it immediately after; there are TODO comments in the code for this.)

The API comments for resize(), however, were very confusing (my fault). I'm going to checkin a fix to those comments and revert most of the changes to the BorderContainer class.

The only caveat is that the first argument to resize() might contain only height or only width (or be null). That's the way I always intended the API to be. Unfortunately it wasn't documented very well.

comment:11 Changed 11 years ago by bill

Resolution: fixed
Status: newclosed

(In [15245]) Fixes #7598 !strict: Update the API documentation for resize() to be less confusing, and add missing argument to widget.resize() in BorderContainer code.

The code in resize() wasn't changed, except for parameter name changes.

Note: See TracTickets for help on using tickets.