Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

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

asyncParser.patch (8.8 KB) - added by bill 7 years ago.
allow async widget creation and make ContentPane? async if the parser returns a promise

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by liucougar

a workaround is to add the following before "function(dojo, parser){":

dijit.TitlePane.prototype.stopParser=false;

comment:2 Changed 7 years ago by bill

Owner: changed from bill to liucougar
Status: newpending

The original test case works for me, see http://bill.dojotoolkit.org/cougarOrig.html. Clicking "No" makes the textbox show up.

comment:3 Changed 7 years ago by bill

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 Changed 7 years ago by Kitson Kelly

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 in reply to:  4 Changed 7 years ago by liucougar

Status: pendingnew

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 7 years ago by bill

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 7 years ago by liucougar

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 7 years ago by bill

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 7 years ago by Kitson Kelly

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 7 years ago by ben hockey

Cc: ben hockey added

comment:11 Changed 7 years ago by bill

Summary: children of title panes are not properly initialized with parserproblems with auto-require of widgets inside ContentPane

I tried my idea from comment:6. As usual, it's made more difficult by needing to maintain backwards compatibility in the parser But it seems to be working and the code changes are fairly minor. Plus it moves us towards fixing #13782. See the attached patch.

Changed 7 years ago by bill

Attachment: asyncParser.patch added

allow async widget creation and make ContentPane? async if the parser returns a promise

comment:12 Changed 6 years ago by bill

Resolution: fixed
Status: newclosed

In [30451]:

Allow for a widget's markupFactory() to return a Promise. The Promise returned by dojo/parser::parse() won't resolve until all widget's promises have resolved. Also, make ContentPanemarkupFactory() return a promise base on the nested parse() call. Together, these changes make sure a parse() call waits for ContentPane's nested parse() calls to complete before calling startup() on any of the widgets.

The change to parser makes markupFactory() a general purpose factory function for widgets, which can even be useful when used programatically. The naming of the function is unfortunate, since it suggests that it's only useful for markup, but I'm reluctant to use a common name like create() since that may already be in use for another purpose.

Also, had to do acrobatics to keep back-compat of the signature of dojo/parser::instantiate().

Fixes #16503, refs #13782 !strict.

comment:13 Changed 6 years ago by bill

Milestone: tbd1.9
Note: See TracTickets for help on using tickets.