Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#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)

example.html (913 bytes) - added by s_i_z_o 11 years ago.
not_impl.html (23 bytes) - added by s_i_z_o 11 years ago.
test_dojox_cp.html (1.0 KB) - added by mog 11 years ago.
dojox-ContentPane.patch (505 bytes) - added by dsnopek 11 years ago.
Fix for the problem by making dojox's ContentPane? not call the onUnloadDeferred twice.
dijit-ContentPane.patch (2.0 KB) - added by dsnopek 11 years ago.
Fix for the problem by making dijit's ContentPane?? not call _onUnloadHandler twice.

Download all attachments as: .zip

Change History (28)

Changed 11 years ago by s_i_z_o

Attachment: example.html added

comment:1 Changed 11 years ago by Adam Peller

Owner: changed from Adam Peller to Sam Foster

comment:2 Changed 11 years ago by Sam Foster

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.

  • What browser did you see this in?
  • Is latency an issue? Perhaps you get the error when the response from the first click is still mid-flight?
  • Whats in 'not_impl.html' is that a plain html response, or does it include script blocks?

Changed 11 years ago by s_i_z_o

Attachment: not_impl.html added

comment:3 in reply to:  2 Changed 11 years ago by s_i_z_o

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:

  1. Click on "1st" button (displays "1st content" in ContentPane? - works fine)
  2. Click on "2nd" button (display alert box with "1", displays "2nd content" in ContentPane? - works fine)
  3. Click on "Href" button (display content "Not instaled" in ContentPane? - works fine)
  4. Click on "1st" button - ERROR "already called!"

basicaly, after first call to setHref - setContent returns error

comment:4 Changed 11 years ago by mog

I can also reproduce this in FF3 and IE7.

File test_dojox_cp.html is attached.

Changed 11 years ago by mog

Attachment: test_dojox_cp.html added

comment:5 Changed 11 years ago by mog

I have reproduced this bug in 1.2.0 release.

comment:6 Changed 11 years ago by Karl Tiedt

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:7 Changed 11 years ago by Sam Foster

Ok I'll try and look at this today

comment:8 Changed 11 years ago by Karl Tiedt

Cc: Josh Trutwin added

comment:9 Changed 11 years ago by Josh Trutwin

Problem also manifests itself when running dijit.byId('myPane').refresh();

comment:10 Changed 11 years ago by s_i_z_o

related to: #7915, #7885, and probatly to #7785 and #7784.

Will this get into 1.2.1 ? Also please change version to 1.2.0 (it was not available at the time of ticket creation)

comment:11 Changed 11 years ago by Adam Peller

Version: 1.2beta1.2.0

comment:12 Changed 11 years ago by Josh Trutwin

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 11 years ago by Josh Trutwin

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 11 years ago by Josh Trutwin

oops: condition with proper precedence:

if((this._xhrDfd && this._xhrDfd.fired == -1) || this.onUnloadDeferred.fired == 0){

comment:15 Changed 11 years ago by dsnopek

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:

  1. Make dijit.layout.ContentPane? not call _onUnloadHandler twice.
  2. 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 11 years ago by dsnopek

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 11 years ago by dsnopek

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 11 years ago by dsnopek

Attachment: dojox-ContentPane.patch added

Fix for the problem by making dojox's ContentPane? not call the onUnloadDeferred twice.

comment:18 Changed 11 years ago by dsnopek

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 11 years ago by dsnopek

Attachment: dijit-ContentPane.patch added

Fix for the problem by making dijit's ContentPane?? not call _onUnloadHandler twice.

comment:19 Changed 11 years ago by dsnopek

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:20 Changed 11 years ago by bill

Milestone: tbd1.2.2

Fixed in [15606] on 1.2 branch and trunk.

comment:21 Changed 11 years ago by bill

Owner: changed from Sam Foster to bill

comment:22 Changed 11 years ago by bill

Resolution: fixed
Status: newclosed

comment:23 Changed 9 years ago by bill

Component: DojoxDojoX Layout
Note: See TracTickets for help on using tickets.