#16503 closed defect (fixed)
problems with auto-require of widgets inside ContentPane
| Reported by: | liucougar | Owned by: | liucougar |
|---|---|---|---|
| Priority: | undecided | Milestone: | 1.9 |
| Component: | Parser | Version: | 1.8.2 |
| Keywords: | Cc: | Kitson Kelly, ben hockey | |
| Blocked By: | Blocking: |
Description
this is reproducible on current trunk as well as 1.8.2
load the following html page
<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
<link href="//ajax.googleapis.com/ajax/libs/dojo/1.8.2/dijit/themes/claro/claro.css" type="text/css" rel="stylesheet"/>
<script src="//ajax.googleapis.com/ajax/libs/dojo/1.8.2/dojo/dojo.js" djConfig="isDebug:true,async:1"></script>
<script>
require(["dojo", "dojo/parser","dojo/domReady!","dijit/TitlePane"],
function(dojo, parser){
dojo.when(parser.parse(document.body), function(l){
});
});
</script>
</head>
<body class="claro" style="">
<div data-dojo-type="dojox/form/Manager">
<script type="dojo/method" data-dojo-event="legalactor" data-dojo-args="value,name">
this.show({invalidLegalActor:value==='no'});
</script>
<div data-dojo-attach-point="P_LegalActor" data-dojo-type="dijit/TitlePane" data-dojo-props="title: 'Confirm legal actor'">
<div>Signature matches?
<input type="radio" data-dojo-type="dijit/form/RadioButton" value="yes" name="validLegalActor" observer="legalactor"> Yes
<input type="radio" data-dojo-type="dijit/form/RadioButton" value="no" name="validLegalActor" observer="legalactor"> No
</div>
<div data-dojo-attach-point="invalidLegalActor" style="display:none">
Missing Signature: <input type="text" data-dojo-type="dijit/form/TextBox" name="missingSignatures">
</div>
</div>
</div>
</body>
</html>
click on "NO" radio box, nothing happens. however, if I remove 'dijit/TitlePane" from the require, then this works as expected
Attachments (1)
Change History (14)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
| Owner: | changed from bill to liucougar |
|---|---|
| Status: | new → pending |
The original test case works for me, see http://bill.dojotoolkit.org/cougarOrig.html. Clicking "No" makes the textbox show up.
comment:3 Changed 9 years ago by
| Cc: | Kitson Kelly added |
|---|
PS: Probably you are hitting a race condition.
First, you are depending on the parser's auto-require feature to pull in RadioButton and TextBox.
There are two parser calls involved in this test case:
- the top level call
- the call from TitlePane: TitlePane extends ContentPane, and ContentPane calls the parser on it's contents
If the first parser call does the auto-requires, then probably everything is fine.
But IIRC, depending on whether or not TitlePane is already loaded, the original parser call may not go looking inside of the TitlePane. Therefore, the second parser call, from TitlePane, will do the auto-requires. In this case the second parser call is asynchronous, and dojox/form/Manager goes querying it's children too soon.
That's just a theory though, and may be wrong. I looked briefly at dojox/form/Manager and it seems to be processing its children in buildRendering(), which doesn't make any sense.
comment:4 follow-up: 5 Changed 9 years ago by
Yes, it appears to be a race condition, mostly because when the TitlePane is not auto-required, it parses it content asynchronously, but the method it needs to have to attach to is in another parse that is occurring synchronously. (the type="dojo/method") and so it just silently fails when attaching the function to the widget.
Here is a JS Fiddle the replicates it: http://jsfiddle.net/kitsonk/XJKck/
Here is a JS Fiddle that requires in all the modules before the parse which resolves the issue as well: http://jsfiddle.net/kitsonk/XJKck/2/
My opinion is that mixing auto-require with not auto-require and using declarative methods is an edge case that would be rather challenging to fix easily and we should just recommend either using auto-require or not, and that mixing the two can result in unanticipated results.
comment:5 Changed 9 years ago by
| Status: | pending → new |
|---|
Replying to kitsonk:
My opinion is that mixing auto-require with not auto-require and using declarative methods is an edge case that would be rather challenging to fix easily and we should just recommend either using auto-require or not, and that mixing the two can result in unanticipated results.
in our use case, all the forms are dynamically loaded, some of them may require special custom widgets. when a form loads, some modules are already loaded (such as TitlePane?). Requiring all these custom widgets upfront, before loading any of the forms which may require them sounds like over kill to me.
I'd like to find a proper fix for this. where do you guys think this should be fixed? the parser or dojox/form/Manager?
in the mean time, this is another workaround:
require(["dojo", "dojo/parser","dojo/promise/all", "dojo/domReady!", "dijit/TitlePane"],
function(dojo, parser, all){
dojo.when(parser.parse(document.body), function(l){
var w=l[0];
var ds=[], def;
dojo.forEach(l, function(w){
if(w.onLoadDeferred){
ds.push(w.onLoadDeferred);
}
});
if(ds.length > 0){
def = all(ds);
}
if(def){
def.then(function(){
w.registerWidgetDescendants(w);
});
}
});
});
comment:6 Changed 9 years ago by
Cougar's suggestion from comment:5 (above) is dangerous because of a case like:
<div data-dojo-type=dijit/layout/TabContainer>
<div data-dojo-type=dijit/layout/ContentPane>selected tab</div>
<div data-dojo-type=dijit/layout/ContentPane href="..."></div>
</div>
The onLoadDeferred for the second ContentPane won't resolve until that tab is selected.
I do understand though wanting the parser to defer calling startup() on any widget until all the widgets have finished initialization. So perhaps we should:
- allow a widget's markupFactory() to return a promise
- make the parser delay calling startup() on any widget until all such promises have resolved
- add a ContentPane.markupFactory() method that returns a promise, but only for the case of inlined data, not for href's
I should also mention that for 2.0 I think I want hidden ContentPane's to delay calling the parser until they are made visible, even for inlined data. But we can deal with that later.
Another option is to make the top level parser call do the auto-require for all the widgets, ignoring the stopParser flag. I was against this idea though when we implemented auto-require, because it means multiple scans of the DOM tree, which would be a big performance hit. Specifically, the nodes inside of the TitlePane would be scanned twice, once by each call to the parser. (Although fast browsers can optimize dojo.query("[data-dojo-type]"), slow browsers like IE6 cannot.)
comment:7 Changed 9 years ago by
ignore stopParser sounds the easiest option, and i think double parsing won't be too bad: afaict, the second time it parses the dom, it would not create any new widgets
comment:8 Changed 9 years ago by
Well, it's true that the widgets are only created once, but the performance issue is about multiple scans of the DOM for data-dojo-type nodes. On IE6 (which is a slow browser and has no support for querySelectorAll()). We need to be cautious because we've had people in the past complain multiple times about parser performance for the sparse widgets case (lots of DOM nodes, few widgets).
comment:9 Changed 9 years ago by
We might end up chasing our tails on this one. With limited support in the query, the DFS walk with the stopParser of the DOM is the most efficient way. We do have the performance tests for the parser in DOH where we have a pretty good baseline to see what order of magnitude we are talking about.
I do think a lot of people were arguing from a point of principle on the empty nodes (there was one ticket and a pretty heated discussion a few years ago which led to the introduction of stop parser). You have to have a significantly large DOM, a descendent of ContentPane?, with very sparse number of decorated nodes in order to have any significant impact on the parse time.
I wonder if breaking out auto-require (the scan for the data-dojo-types), similar to the way the declarative require is might make sense... a "one shot" up front of trying to autoload modules...
comment:10 Changed 9 years ago by
| Cc: | ben hockey added |
|---|
comment:11 Changed 9 years ago by
| Summary: | children of title panes are not properly initialized with parser → problems with auto-require of widgets inside ContentPane |
|---|
Changed 9 years ago by
| Attachment: | asyncParser.patch added |
|---|
allow async widget creation and make ContentPane? async if the parser returns a promise
comment:13 Changed 9 years ago by
| Milestone: | tbd → 1.9 |
|---|
a workaround is to add the following before "function(dojo, parser){":