Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#12550 closed defect (fixed)

ContentPane: destroys contents on refresh even if href is ""

Reported by: David M Owned by: bill
Priority: high Milestone: 1.6.1
Component: Dijit Version: 1.6.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

Browser type: FF, IE, Chrome Browser version: FF4, IE7, Chrome 10 OS and OS Version: Windows XP, all All steps necessary to reproduce the issue:

Put anything in a dijit ContentPane, cause the pane to be refreshed. If you have a dockable FloatingPane, a refresh will be triggered when it's minimized/restored. Also see attached test.html for example to reproduce.

An email address of the person filing the bug or a way to get a hold of him/her: dmiddlecamp@…

Notes:

Previous functionality allowed the creation of a dijit ContentPane with any nodes / etc inside it. The Refresh function should be smart enough not to attempt an XHR request if HREF is "". This causes errors in Firefox 4, and IE 7, because href is "", which is an "invalid argument" in IE7, and causes the content to be replaced with "Loading..." in all browsers. Could we add a simple

if (this._href !== "") { return; }

or something in the Refresh function in ContentPane? to avoid this?

Attachments (2)

contentPane_12550.patch (480 bytes) - added by David M 9 years ago.
suggested patch
test.html (2.6 KB) - added by David M 9 years ago.
example to reproduce 12550

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by David M

Attachment: contentPane_12550.patch added

suggested patch

comment:1 Changed 9 years ago by bill

Component: GeneralDijit
Description: modified (diff)
Summary: dijit.layout.ContentPane - destroys contents on refresh even if href is ""[regression] ContentPane: destroys contents on refresh even if href is ""

Thanks, BTW you can and should register you email in trac, to get notifications.

Sounds like you are saying this is a regression from 1.5?

comment:2 Changed 9 years ago by David M

sorry about that, tried to add my email after the fact, but I can't seem to modify these. Yes, this is a dijit regression from 1.5

comment:3 Changed 9 years ago by bill

Milestone: tbd1.6.1

@dmiddlecamp - I meant that you should click "preferences" in the upper right, and put your email there.

comment:4 Changed 9 years ago by bill

Milestone: 1.6.1tbd
Summary: [regression] ContentPane: destroys contents on refresh even if href is ""ContentPane: destroys contents on refresh even if href is ""

@dmiddlecamp - I tried your test case. It doesn't actually work, since the parser isn't getting called, but after fixing that, I see the failure. However, you shouldn't be calling refresh() on a ContentPane without an href. () So I tried dojox/layout/tests/test_FloatingPane.html. You said that the refresh() is triggered when a dockable pane is minimized. But I tried minimizing/restoring the "un-closable, dockable" pane and the _load() method is never called.

Also, I tried this against 1.5/ and 1.4/, and those also fail when calling refresh() on a pane w/no href. So this doesn't seem like a regression, although you said that "previous functionality allowed...". Which version were you talking about?

So, can you show a test case where refresh() is getting called when it shouldn't be?

comment:5 Changed 9 years ago by David M

Basically the call stack is:

*User clicks DockNode? -->dojox.layout._DockNode.restore

-->if(!this.paneRef.isLoaded){ this.paneRef.refresh(); }

-->dijit.layout.ContentPane?.refresh

I'll revise my example so this is more obvious. This is a major regression from dojo 1.5 to dojo 1.6 for my app. This only started after we upgraded to 1.6, maybe something just isn't setting 'isLoaded' like it used to. Regardless, ContentPane? is supposed to take in dom nodes when built, and probably shouldn't destroy its content if href isn't set.

comment:6 Changed 9 years ago by David M

still working on example, but this is from ContentPane?.js:

line 237, in _setContentAttr:

clear href so we can't run refresh and clear content refresh should only work if we downloaded the content this._set("href", "");

seems to imply to me that the ContentPane? should have a guard in the refresh function preventing it from destroying content if href is not set or ""...

Changed 9 years ago by David M

Attachment: test.html added

example to reproduce 12550

comment:7 Changed 9 years ago by David M

I added a newer version of test.html that reproduces this bug, I had trouble getting it to work properly in cross domain mode (all in one file), but it works normally if you have a build locally. Just minimize and restore the floatingpane, and you'll see the issue.

comment:8 Changed 9 years ago by bill

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

OK thanks, I see it, thanks for the test case.

Turns out ContentPane does have guard code, here:

if(this.href){
	if(!this._xhrDfd && // if there's an href that isn't already being loaded
		(!this.isLoaded || this._hrefChanged || this.refreshOnShow)
	){
		return this.refresh();	// If child has an href, promise that fires when the load is complete
	}
}

It's just outside of the refresh() method. The FloatingPane code should be calling _onShow(), not calling refresh() directly. I'll make that change.

If we run into other code that's calling refresh() even though there's no href, I'll add the guard code into refresh() as you suggested.

comment:9 Changed 9 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [24214]) Fix problem when refresh() is called even when there's no href. Fixes #12550 on 1.6/ branch, !strict.

comment:10 Changed 9 years ago by bill

(In [24215]) Fix problem when refresh() is called even when there's no href. Fixes #12550 on trunk, !strict.

Note: See TracTickets for help on using tickets.