Opened 13 years ago

Closed 9 years ago

Last modified 9 years ago

#2056 closed defect (fixed)

ContentPane: Menu inside ContentPane not cleaned up on ContentPane destroy or reload.

Reported by: gbettencourt@… Owned by: bill
Priority: high Milestone: 1.6
Component: Dijit Version: 0.4
Keywords: Cc: ben hockey
Blocked By: Blocking:

Description (last modified by bill)

Only occurs in IE ( I'm using IE7 ), reproduced with 0.4.1rc2 also.

The attached page has a two content panes, one which has a button to load a new page into the second pane. The page initially loaded by the pane contains a menu widget. After loading another page which does not contain a menu widget, clicking anywhere on the page will result in a "offsetWidth is null or not an object" js error. This only occurs if the new page does not have a menu, loading a new page with a menu works fine.

I also ran into another bug while creating this repro case, in dom.js dojo.dom.destroyNode does node.outerHTML= to prevent a memory leak. In my repro case, node can be undefined. We should insure that node is not undefined before trying to clear the outerHTML.

Attachments (5)

test_Menu2_contentPane.html (579 bytes) - added by gbettencourt@… 13 years ago.
repro case, place in tests/widget
test_Menu2_contentPane.2.html (614 bytes) - added by gbettencourt@… 13 years ago.
a more simple test
test_Menu2_contentPane_pane1.html (297 bytes) - added by gbettencourt@… 13 years ago.
simple test
test_Menu2_contentPane_pane2.html (50 bytes) - added by gbettencourt@… 13 years ago.
simple test
r22835.html (1.2 KB) - added by ben hockey 9 years ago.
updated test case

Download all attachments as: .zip

Change History (43)

Changed 13 years ago by gbettencourt@…

Attachment: test_Menu2_contentPane.html added

repro case, place in tests/widget

comment:1 Changed 13 years ago by bill

Milestone: 0.4.10.5

It doesn't seem valid for a ContentPane? to contain a Menu2 w/contextMenuForWindow="true". Can you try a simpler test, where the content pane just has a menu bar, or maybe a menu attached to a certain node?

Changed 13 years ago by gbettencourt@…

a more simple test

Changed 13 years ago by gbettencourt@…

simple test

Changed 13 years ago by gbettencourt@…

simple test

comment:2 Changed 13 years ago by gbettencourt@…

test_Menu2_contentPane.2.html uses two simple pages (attached) as content for the pane, and still reproduces the problem.

comment:3 Changed 13 years ago by gbettencourt@…

not sure if this is the correct fix, but overriding the destroy method in dojo.widget.Menu2 solves the problem:

destroy: function() {

dojo.widget.PopupManager?.closed(this);

},

comment:4 Changed 13 years ago by bill

Component: WidgetsDijit
Milestone: 0.91.0

The PopupManager? code has been completely redone for 0.9 so this probably isn't an issue anymore, but need to test.

comment:5 Changed 12 years ago by bill

Milestone: 1.01.1

comment:6 Changed 12 years ago by bill

Summary: js error when using a menu2 inside a content paneMenu inside ContentPane not cleaned up on ContentPane destroy or reload.

See related bugs #3300 and #4980. This is currently broken because menus are moved to document.body and thus the cleanup doesn't work.

comment:7 Changed 12 years ago by bill

Milestone: 1.11.3

comment:8 Changed 11 years ago by bill

Description: modified (diff)
Milestone: 1.31.2
Owner: changed from bill to Sam Foster

Looks like this will be fixed by sfoster's conversion of ContentPane to use dojo.html.

comment:9 Changed 11 years ago by Sam Foster

(In [14987]) Refs #5647 Refs #2056 Refs #4980

Refactored _setContent to use dojo.html._ContentSetter

  • a dojo.html._ContentSetter instance is created and stored on first use (at this._contentSetter)
  • each subsequent call to _setContent remixes in the config properties
  • _ContentSetter.empty() is called implicitly when you call this._contentSetter.set() - this will destroy any widgets from a previous set operation - including Menus, Dialogs etc, as the contentSetter.parseResults array keep widget references. This is partly redundant with the destroyDescendents() call in _setContent(), but catches this corner case. It does not fix the case where widgets have been created by the parser from inlined, initial content
  • added a few unit tests to the test page to verify this behavior

!strict

comment:10 Changed 11 years ago by Sam Foster

The _setContent refactoring addresses half of this issue. When you call attr("content", ..) (or indirectly via attr("href"..) the content will be parsed for widgets, and a reference to those widgets is kept so they can be destroyed when the content is unloaded, or the widget itself is destroyed. However, widgets inlined inside the ContentPane? when the page is initially parsed are not provided to the ContentPane? to track, and the issue will still be present in that case.

comment:11 Changed 11 years ago by bill

Milestone: 1.21.3
Owner: changed from Sam Foster to bill

Ok, only half fixed. Given that a Menu/Dialog? might exist outside of a ContentPane altogether, can't depend on ContentPane to fix this for us.

I think the final solution will be to leave a placeholder node where the Menu/Dialog? was originally declared, or perhaps don't move the Menu as a child of <body> until you need to show it (and then move the Menu domNode back where it came from when the menu disappears)

In any case, the good news is that you can solve your problem in 1.2 by using href to load the ContentPane content.

comment:12 Changed 11 years ago by bill

Summary: Menu inside ContentPane not cleaned up on ContentPane destroy or reload.ContentPane: Menu inside ContentPane not cleaned up on ContentPane destroy or reload.

See umbrella ticket #6954.

comment:13 Changed 11 years ago by bill

Milestone: 1.32.0

comment:14 Changed 9 years ago by bill

Milestone: 2.01.6
Status: newassigned

After the parser changes in 1.5 this actually seems trivial to implement...

comment:15 Changed 9 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [22835]) Make ContentPane process initial srcNodeRef.innerHTML as though it was specified as an initialization parameter, like new dijit.layout.ContentPane({content: ...}). The content is now parsed inside of ContentPane rather than by dojo.parser run on <body> at page load. Thus, ContentPane.destroyRecursive() will remove initially specified popup widgets like Menu and Dialog.

Fixes #2056 !strict.

comment:16 Changed 9 years ago by ben hockey

Resolution: fixed
Status: closedreopened

something in [22835] breaks attach points nested in content panes. see attached test case.

comment:17 Changed 9 years ago by bill

Cc: ben hockey added

Hmm, yes, it's true that a widget template like below won't work:

dojo.declare("Missing", [dijit._Widget, dijit._Templated], {
	templateString:
	'<div>' +
		'<div dojoType="dijit.layout.ContentPane">' +
			'<div dojoType="dijit.form.Button" ' +
				' dojoAttachPoint="missingButton">Missing...</div>' +
		'</div>' +
	'</div>',

I didn't know that worked even before the patch, or that anyone was doing it.

I think I can special case the parser and ContentPane? to revert to the old behavior for widgetsInTemplate parsing.

But I'm wondering, what are you using this type of design for? If you call ContentPane.set("content", ...) or ContentPane.set("href", ...) for example, the parent widget ("Missing") will get confused because missingButton is no longer valid. MIssing._supportingWidgets will also be left with a stale reference. I'm not even sure that Missing.destroyRecursive() will work correctly, it seems like it might try to destroy the Button twice.

comment:18 Changed 9 years ago by ben hockey

my example above is of course contrived but in one of my "real life" use cases, i've extended a BorderContainer? with dijit._Templated and the ContentPane? is used in the template simply as a wrapper for content in a region where the content contains some widgets but is not itself a widget. i don't use any of the other functionality provided by ContentPane?. if there was another widget that was just a container that parsed it's content i'd use it instead :)

i could probably make something to meet my needs. however, using ContentPane? in this way was something which was already being done in this project when i came into the project so it's quite possible that more people may be doing things this way but we just don't know about it.

comment:19 Changed 9 years ago by bill

OK, thanks for the explanation, I see.

I'll hack something in for 1.x, although for 2.0 maybe we should have a separate widget or something.

comment:20 Changed 9 years ago by bill

Resolution: fixed
Status: reopenedclosed

(In [22853]) When parsing a template (for widgetsInTemplate), don't stop at ContentPane nodes because then the dojoAttachPoint's won't work.

Fixes #2056 again, !strict.

comment:21 Changed 9 years ago by ben hockey

this seems to have caused problems with nested ContentPanes? now... i won't be able to build a test case until next week but just giving you warning that this hasn't fixed everything.

comment:22 Changed 9 years ago by bill

Resolution: fixed
Status: closedreopened

Doh, OK, thanks for the heads up, I'll wait for the test case. I didn't expect people using nested ContentPanes? in templates either, guess that's another automated test we need to add.

comment:23 Changed 9 years ago by ben hockey

ok... fortunately, the problem is simple to reproduce - i was afraid it would be difficult to make a minimal test case that failed. just add an id to the button widget in the templateString

<div dojoType="dijit.form.Button" dojoAttachPoint="missingButton" id="${id}_duplicate">Missing...</div>

you'll get the error that a widget with that id has already been registered. i suspect that this is because the parser has instantiated the widget due to dijit._Templated and then the ContentPane? tries to instantiate the widget again when it parses it's content.

Changed 9 years ago by ben hockey

Attachment: r22835.html added

updated test case

comment:24 Changed 9 years ago by bill

(In [22868]) Call _checkIfSingleChild() every time resize() is called, in case app has manually mucked w/the content of the ContentPane?, like Editor demo does, rather than changing it through the set("content", ...) API. Refs #2056 !strict.

comment:25 Changed 9 years ago by bill

Resolution: fixed
Status: reopenedclosed

(In [22869]) When a ContentPane is in a template, it's contents are pre-parsed, so it shouldn't try to parse again. Fixes #2056 again, !strict.

comment:26 Changed 9 years ago by ben hockey

Resolution: fixed
Status: closedreopened

sorry... still not fixed - try the r22835.html which includes an id for the button. it still fails. i'm not sure why your tests pass but this still fails...

comment:27 Changed 9 years ago by ben hockey

fwiw, i copied the template from _Templated-widgetsInTemplate.html into r22835.html and it worked. the difference being that r22835.html isn't using data-dojo-xxx and _Templated-widgetsInTemplate.html is.

comment:28 Changed 9 years ago by ben hockey

Resolution: fixed
Status: reopenedclosed

thanks - it works. for some reason, the ticket didn't close when you committed r22871

comment:29 Changed 9 years ago by bill

(In [23119]) Fix args to parse() so that dir and lang get correctly passed down to children, refs #2056, fixes #11913 !strict.

comment:30 Changed 9 years ago by bill

(In [23120]) Avoid modifying original params object since that breaks NodeList instantiation. Fixes #11906, refs #2056 !strict.

comment:31 Changed 9 years ago by bill

(In [23126]) Add "startup" parameter to dojo.html._ContentSetter to control whether startup() is called or not. Currently only works when the parser is enabled.

Fix ContentPane so it doesn't call startup() on children until ContentPane.startup() is called.

The FeedPortlet tests are still failing but at least this fixes one problem.

Refs #2056 !strict.

comment:32 Changed 9 years ago by bill

(In [23129]) Resolve issues destroying unstarted AccordionContainer (causing failures in AccordionContainer.html destroy test), refs #2056 !strict.

comment:33 Changed 9 years ago by bill

(In [23244]) Due to [23126] and the dojo.html.test.DeclarativeContentSetter in the html/test_set.html test, need to allow for widgets (or rather, objects handled by the parser) to have a startup attribute that isn't a function. Refs #2056 !strict.

comment:34 Changed 9 years ago by Nick Fenwick

I'm not sure how to check SVN status. The fix for this was said to fix #11905, a pretty important Dialog positioning bug. If not already done, can this patch be committed onto the 1.5 branch, so it gets into 1.5.1 please? This seems quite important for general 1.5.x.

#11257 was recently updated onto that branch in the same way.

comment:35 Changed 9 years ago by bill

Sorry, this change is much too big to even consider for a 1.5.x. 1.6 is already beta though, you can use that.

comment:36 Changed 9 years ago by bill

(In [23871]) ContentPane needs to call startup() on non-widget children too, like dojo.dnd.Source. Fixes regression from [23126], refs #2056 !strict.

comment:37 Changed 9 years ago by bill

(In [23882]) Avoid repeat startup() calls even when child forgets to set this._started, such as happens with the test widgets in ContentPaneLayout.html. Refs #2056 !strict.

comment:38 Changed 9 years ago by bill

(In [23901]) Account for user manually destroying widgets after parse, like happens in "destroy" test in AccordionContainer.html. Refs #2056 !strict.

Note: See TracTickets for help on using tickets.