Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12347 closed defect (fixed)

StackContainer: redundant call of _showChild (may lead to bug)

Reported by: xMartin Owned by: bill
Priority: high Milestone: 1.7
Component: Dijit Version: 1.5
Keywords: Cc:
Blocked By: Blocking:

Description

If I call startup before adding any children, then add children and then resize the browser window (or resize the StackContainers? DOMNode in IE), resize calls _showChild passing the currently selected widget (if you're lucky...). This is unnecessary as selectChild already did that.

Attached patch should fix this by making sure that it is only called the very first time of resize being called.

Analysis:

StackContainer?'s startup method should call _showChild if there are any children yet for passing either the selected one or the first one. Unfortunately this is done in the resize method (I guess that's because there's simply not the right spot left in the startup method as it inherits different aspects and has to care a lot about where to put this.inherited(arguments)). resize is called by startup and there's a state variable _hasBeenShown that is supposed to care for _showChild only being called if it's the first time resize is called but the condition also depends on any child being selected so it fails if there hasn't been any child initially, thus leaving _hasBeenShown as false. After selecting a child and then triggering resize the condition is met and _showChild is called not doing anything good.

So what?

Besides just being stupid code that triggers at least a dojo.addClass I came across an incredibly hard to find and understand bug that appeared in IE7 with a chance of about 1 out of 7:

In our application we animate the height of a TabContainer? (probably not a very good idea as I know now). After the animation (dojo.connect "onEnd") a selectChild is called. In IE the onresize event of the TabContainer?'s DOMNode triggers resize during the animation. By adding a lot of console.logs it became visible that resize is executed *while* the resize method of the ContentPane? (child) is executed leaving me wondering if I can rely on the single-threaded nature of JavaScript?.

It seems that the following line (Dojo 1.5.0) triggers a DOM operation that lets the resize event handler execute stuff parallely:

this._contentBox = dijit.layout.marginBox2contentBox(cn, mb);

In the case where the bug is not present, after the animation is done selectChild is called and after that there appears to be a call of resize as an event handler. For IE7 the one and only call to resize usually happens after selectChild. resize then finds selectedChildWidget to be the freshly selected widget and _hasBeenShown is false (I called startup before adding children), So it calls _showChild and passes the selectedChildWidget which is not necessary as selectChild just did that but doesn't hurt too much.

In case of the bug appearing, resize is called after selectChild called _hideChild and _showChild (actually it is _transition) *but before* the selectedChildWidget is set (remember the strange thing happening during this._contentBox = dijit.layout.marginBox2contentBox(cn, mb); in the child's resize method). So I get an inconsistent state where there are two children being selected = true. That's it.

Console example with bug appearing:

LOG: animation of TabContainer onEnd
LOG: TabContainer::selectChild() calls _hideChild("pane1")
LOG: TabContainer::selectChild() calls _showChild("pane2")
LOG: ContentPane::resize() at first line of code ("pane2")
LOG: TabContainer::resize(): selected is "pane1", this._hasBeenShown is false
LOG: TabContainer::resize() calls _showChild("pane1")
LOG: ContentPane::resize() at first line of code ("pane1")
LOG: ContentPane::resize() at last line of code ("pane1")
LOG: ContentPane::resize() at lastline of code ("pane2")
LOG: TabContainer::selectChild(): set this.selectedChildWidget to "pane2"
>>dijit.byId('tab_container').getChildren()[0].selected
true

Why am I telling you? Because it took me a long time to understand and because I want this fixed. Please also backport to 1.5.1. Thank you :)

Attachments (2)

stackcontainer-resize.patch (785 bytes) - added by xMartin 8 years ago.
test_StackContainer_12347.html (4.2 KB) - added by xMartin 8 years ago.

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by xMartin

Attachment: stackcontainer-resize.patch added

comment:1 Changed 8 years ago by Adam Peller

Milestone: 1.5.1tbd

comment:2 Changed 8 years ago by ben hockey

could you attach a minimal test case to demonstrate this?

comment:3 Changed 8 years ago by xMartin

I created a test file. Unfortunately I couldn't get it to work with the latest dojo version so this is for 1.5.0.

Changed 8 years ago by xMartin

comment:4 Changed 8 years ago by bill

Milestone: tbd1.7
Owner: set to bill
Status: newassigned

OK, I'll check in your fix, I can even backport it, but note that addChild(child2) still causes a call to child1.resize(), due to logic in StackContainer.js accounting for possible change in size of content area, due to tab labels overflowing from one row to two rows.

comment:5 Changed 8 years ago by bill

Resolution: fixed
Status: assignedclosed

In [26528]:

Avoid redundant call of StackContainer._showChild() when StackContainer is started with no children, then children are added, and then StackContainer.resize() is called (directly or indirectly). Fixes #12347 on trunk, !strict.

comment:6 Changed 8 years ago by bill

In [26529]:

Avoid redundant call of StackContainer._showChild() when StackContainer started with no children, then children are added, and then StackContainer.resize() is called (directly or indirectly).

Fixes #12347 on 1.6 branch, !strict.

comment:7 Changed 8 years ago by bill

In [26530]:

Avoid redundant call of StackContainer._showChild() when StackContainer started with no children, then children are added, and then StackContainer.resize() is called (directly or indirectly).

Fixes #12347 on 1.5 branch, !strict.

Note: See TracTickets for help on using tickets.