#7801 closed defect (fixed)
dojox.layout.ContentPane.setContent() not working
Reported by: | s_i_z_o | Owned by: | bill |
---|---|---|---|
Priority: | high | Milestone: | 1.2.2 |
Component: | DojoX Layout | Version: | 1.2.0 |
Keywords: | ContentPane setHref setContent | Cc: | Josh Trutwin |
Blocked By: | Blocking: |
Description
After one calling setHref() method on dojox.layout.ContentPane? or setting href=".." attribute on <div> tag, following calls to setContent() or setHref() method return error:
already called!
This can be easily reproduced with attached example.html
It is actualy for version 1.2
Attachments (5)
Change History (28)
Changed 12 years ago by
Attachment: | example.html added |
---|
comment:1 Changed 12 years ago by
Owner: | changed from Adam Peller to Sam Foster |
---|
comment:2 follow-up: 3 Changed 12 years ago by
Changed 12 years ago by
Attachment: | not_impl.html added |
---|
comment:3 Changed 12 years ago by
i have added file not_impl.html - just plain file.
I can reproduce error in FF2.x, FF3.x, IE7 and Google Chrome. All show same error.
Maybe i should write steps to reproduce this:
- Click on "1st" button (displays "1st content" in ContentPane? - works fine)
- Click on "2nd" button (display alert box with "1", displays "2nd content" in ContentPane? - works fine)
- Click on "Href" button (display content "Not instaled" in ContentPane? - works fine)
- Click on "1st" button - ERROR "already called!"
basicaly, after first call to setHref - setContent returns error
comment:4 Changed 12 years ago by
I can also reproduce this in FF3 and IE7.
File test_dojox_cp.html is attached.
Changed 12 years ago by
Attachment: | test_dojox_cp.html added |
---|
comment:6 Changed 12 years ago by
Stevep and I traced this problem down last night to [15339] where Bill deleted references to a bunch of unload handlers it seems in dijit.layout.ContentPane?....
Wasn't able to dive any further into it though after this.
comment:8 Changed 12 years ago by
Cc: | Josh Trutwin added |
---|
comment:9 Changed 12 years ago by
Problem also manifests itself when running dijit.byId('myPane').refresh();
comment:10 Changed 12 years ago by
comment:11 Changed 12 years ago by
Version: | 1.2beta → 1.2.0 |
---|
comment:12 Changed 12 years ago by
Some findings and a workaround.
I traced the problem to this line in dojox.layout.ContentPane? (in the _onUnloadHandler function):
if(this.onUnloadDeferred){
this.onUnloadDeferred.callback(true);
}
When this happens, the "already called" error is thrown by _check() in Deferred.js because this Deferred object has this.fired = 0 (not -1 for some reason) and this.silentlyCancelled = false, which causes the error. I know little of the inner workins of the innards of Deferred.js, but indeed it seems like this deferred was already "called", hence the error - from where though I cannot say.
The workaround is a simple one - wrap the callback above in a try/catch. Then the error is unfortunately silently ignored, but it appears the setHref works.
I tried adding some items back into dojox.layout.ContentPane? to see if I could figure out a way to make it happy, no love though. I also tried to move the dijit.layout.ContentPane?.prototype._unLoadHandler.apply() to the top of the dojox version, but same results.
I'm not sure if a solution would be to re-create these Deferred somehow? Anyway, that's what I have....
Josh
comment:13 Changed 12 years ago by
Another possible fix. I tried changing this in dojox.layout.ContentPane?'s cancel() method:
cancel: function(){ // summary: cancels a inflight download if(this._xhrDfd && this._xhrDfd.fired == -1){ // we are still in flight, which means we should reset our DeferredHandle // otherwise we will trigger onUnLoad chain of the canceled content, // the canceled content have never gotten onLoad so it shouldn't get onUnload this.onUnloadDeferred = null; } dijit.layout.ContentPane.prototype.cancel.apply(this, arguments); },
to this:
cancel: function(){ // summary: cancels a inflight download if(this._xhrDfd && this._xhrDfd.fired == -1 || this.onUnloadDeferred.fired == 0){ // we are still in flight, which means we should reset our DeferredHandle // otherwise we will trigger onUnLoad chain of the canceled content, // the canceled content have never gotten onLoad so it shouldn't get onUnload this.onUnloadDeferred = new dojo.Deferred(); } dijit.layout.ContentPane.prototype.cancel.apply(this, arguments); },
and it works, (note the change to the if condition and creating a new Deferred instead of setting to null.
With this, the this.onUnloadDeferred.callback(true); in the _onUnloadHandler works without try/catch on setHref / refresh.
Who knows if this breaks something else though?
Josh
comment:14 Changed 12 years ago by
oops: condition with proper precedence:
if((this._xhrDfd && this._xhrDfd.fired == -1) || this.onUnloadDeferred.fired == 0){
comment:15 Changed 12 years ago by
I think the issue here, is that dijit.layout.ContentPane? is calling _onUnloadHandler() twice, when loading an href. The first time, to actually unload the old content and then second time, to remove the "Loading..." text.
In the dojox version (dojox.layout.ContentPane?), it calls a dojo.Deferred object in _onUnloadHandler() which is where the error is coming from. The main thing there, is that it will sometimes replace its onUnloadDeferred object with this._nextUnloadDeferred, but not always.
So, I see two possible fixes here:
- Make dijit.layout.ContentPane? not call _onUnloadHandler twice.
- Make dojox.layout.ContentPane? atleast clear the onUnloadDeferred when its called, so:
this.onUnloadDeferred = (this._nextUnloadDeferred) ? this._nextUnloadDeferred : null;
Which is more sane is up to the hyper-intelligent dijit developers. ;-)
comment:16 Changed 12 years ago by
Ah, I just tried what I said in my last comment for my 2nd possible fix and it doesn't straight up work.
What is actually happening, is that onUnloadDeferred *is* getting replaced by _nextUnloadDeferred and that is what is getting called twice. So, the real 2nd fix (which I tested to work) is like this:
this.onUnloadDeferred = this._nextUnloadDeferred; delete this._nextUnloadDeferred;
That way, onUnloadDeferred is only getting called once even though _onUnloadHandler is getting called multiple times.
comment:17 Changed 12 years ago by
I've decided to back up both my possible fixes with patches, because it can't hurt!
Both patches *aren't* needed, only one. The difference is that one fixes the problem in dijit's ContentPane? (by making _onUnloadHandler() called only once) and the other fixes the problem in dojox's ContentPane? (by making the onUnloadDeferred called only once).
Patches coming in a moment!
Changed 12 years ago by
Attachment: | dojox-ContentPane.patch added |
---|
Fix for the problem by making dojox's ContentPane? not call the onUnloadDeferred twice.
comment:18 Changed 12 years ago by
I've been thinking a bit about my dijit-ContentPane?.patch and I'm not sure its doing the right thing..
The way its setup, it'll be the 2nd _setContent() that gets the _onUnloadHandler() called (ie. when the actual content is set). But its probably the 1st one (ie. when the loading message is set) that the user expects, because they may want to mess with the previous contents before they go away. Not sure how to implement that though, I'll have to think about it.
Changed 12 years ago by
Attachment: | dijit-ContentPane.patch added |
---|
Fix for the problem by making dijit's ContentPane?? not call _onUnloadHandler twice.
comment:19 Changed 12 years ago by
I updated my patch, based on my previous comment and a suggestion from bill on the mailing list. It now will call _onUnloadHandler() when destroying the "real" content by tracking that the "Loading.." and error messages are not real with a _isRealContent flag.
comment:21 Changed 12 years ago by
Owner: | changed from Sam Foster to bill |
---|
comment:22 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:23 Changed 11 years ago by
Component: | Dojox → DojoX Layout |
---|
I cant reproduce the error here. "Already called sounds like an error resulting from re-calling a dojo.Deferred, which is a plausible error in theory, but I need more detail apparently.