Opened 11 years ago

Closed 10 years ago

Last modified 6 years ago

#9348 closed enhancement (fixed)

_Templated + widgetsInTemplate startup() called at wrong time.

Reported by: dante Owned by: dante
Priority: high Milestone: 1.4
Component: Dijit Version: 1.3.0
Keywords: Cc: bill, Nathan Toone
Blocked By: Blocking:

Description

When using widgetsInTemplate, the entire lifecycle of the child widget is called before the parent widget's postCreate. It would make more sense if the startup() on the children was called immediately before [or perhaps immediately after] the parent startup() is called.

This may technically break people working around [imo, a currently broken] lifecycles as they appeared in 1.2, but only in regard to widgetsInTemplate, and it seems this behavior would be more expected.

proposing adding a _deferStartup parameter to _Templated so people wanting the old behavior have an out. Happy to do all the due diligence on this and ensure all the documentation is in line. Patch attached touches parser.js and _Templated.js

Attachments (5)

_Templated.patch (4.8 KB) - added by dante 11 years ago.
_Templated.2.patch (6.3 KB) - added by dante 11 years ago.
updated patch works against 17621
_Templated.3.patch (8.0 KB) - added by dante 11 years ago.
final patch. fixes rootNode lookup (cleaner, but longer :( ) and adds unit test
_Templated-widgetsInTemplate.html.patch (3.7 KB) - added by Mike Wilson 10 years ago.
_Templated.4.patch (12.2 KB) - added by Nathan Toone 10 years ago.
Updated patch - fixes issues described above (misspellings, multiple startups), updates test cases (nested layout widgets, programmatic creation)

Download all attachments as: .zip

Change History (34)

Changed 11 years ago by dante

Attachment: _Templated.patch added

comment:1 Changed 11 years ago by bill

Looks great, thanks for working on this.

I'd prefer renaming _deferStartup to refer to the templated widgets; as is, it sounds like you are deferring startup of the the main parent widget. And also "defer" is a little odd since you are fixing startup to happen at the right time. Maybe _earlyTemplatedWidgetStartup is good.

comment:2 Changed 11 years ago by Mike Wilson

I'll be damned, I reported problems about this on dojo-interest ~three hours before this ticket was opened! Were you inspired by that, or was it just a coincidence? :-)

I'd like to try out the patch, but I can't get it to apply to latest SVN. Latest SVN trunk:17620 has _Template.js:17500 and dojo.parser:17499 so should be ok with your patch that was made on trunk:17611 ?
Could you check your patch against latest?

The exact issue we're dealing with here touches on the discussion Bill and I had on dojo-contributor back in January about the widget lifecycle event order (startup was one of the examples). I think there are probably more things that are broken in the current widget model - that extend beyond the scope of this ticket - that I might be able to help you pin-point. Do you prefer to discuss that here, in another ticket, or moving over to dojo-contributor?

Best regards
Mike

comment:3 Changed 11 years ago by dante

ooops. Yes, I made this patch against 1.3.1 explicitly, there were some line number changes in both files due to james adding support for scopeName in the parser [mojoType] etc, so it was rejected trying to merge with trunk. Attaching new patch now, with a unit test addition.

Changed 11 years ago by dante

Attachment: _Templated.2.patch added

updated patch works against 17621

comment:4 Changed 11 years ago by Mike Wilson

Nice. Looking at the patch, I can see that you've done just the thing I thought about too when debugging this - delaying the startup event. The semantics for the startup event should be all children created /and/ elements alive in DOM. Running it through the debugger, checking at certain points (instantiate, layout etc), I think the framework now behaves much more like what one would expect. I'd say you might even make an 1.3.2 just to get this out straight away ;-).

But then, parts of this is a bit of a hack; I guess that not many of us get that warm and fuzzy feeling from an extra pair of boolean flags and delayed methods. I think it would be good to (pretty soon) start discussing how the widget system should really work, using the experience attained so far, and refactor it to become hopefully less complex and more robust.
F ex, it would be good to work on an extremely clear separation between core framework, events, widgets and parser. The core framework should be responsible for everything that regards coordination of events depending on different conditions, so in that world this ticket would have updated the core framework instead of the layout widget base class and the parser.

I would also (from previous experience) recommend to have less things that differ between plain widgets and layout widgets from the core framework's view-point, and to do some work to make layout widgets have a "preferred size", so f ex a TabContainer? without an explicit height will adjust to its content instead of getting height=0.

Best regards
Mike

Changed 11 years ago by dante

Attachment: _Templated.3.patch added

final patch. fixes rootNode lookup (cleaner, but longer :( ) and adds unit test

comment:5 Changed 11 years ago by dante

attached a new patch which fixes the rootNode lookup logic and adds a new unit test to parser to verify. please re-apply against trunk and test.

@mikewise - yes, would love to hash out the _true_ lifecycle for Dijit 2.0, but for now we're stuck trying to retain back-compat and keep it stable until we can fix all the details. Let's focus on defining the behavior for 2.0, and "cope" for now.

comment:6 Changed 11 years ago by bill

I think by "rootNode lookup logic" you are talking about having multiple signatures for the parse() function?

Looks OK. But, don't we want to push people towards using the new syntax parse({rootNode: .., foo: bar})? I'm assuming eventually we want to get rid of the separate rootNode argument.

In that case you could just document it as parse(kwArgs) while maintaining the backwards compatibility in code, something like:

parse: function(kwArgs){
     if(kwArgs.nodeType){
        // support 1.2 syntax
         kwArgs = {rootNode: kwArgs};
     }
     ...

comment:7 in reply to:  5 Changed 11 years ago by Mike Wilson

Replying to dante:

@mikewise - yes, would love to hash out the _true_ lifecycle for Dijit 2.0, but for now we're stuck trying to retain back-compat and keep it stable until we can fix all the details. Let's focus on defining the behavior for 2.0, and "cope" for now.

Certainly. There is high value in doing this back-compat stuff, no question about that. What I mean is that it would _also_ be good to get started on the public discussion of how the next revision of the widget architecture should look (and as I said in my first message "discuss that here, in another ticket, or moving over to dojo-contributor?").

Previously, I worked several years full-time on creating layout widgets in X Windows, and I've seen the problems we've touched on here many times before, when different widget systems have been designed or used incorrectly. This makes me believe there is a fair potential for improvement in Dijit, and considering this I think it would be good if the principles and design of the revised architecture start to take shape as soon as possible (whichever Dojo version will map against it when complete).

I'd be glad to contribute my experience in this area, but my time is limited, so it would be good with a fairly long and not too rushed discussion phase (another reason to get started soon ;-).

comment:8 Changed 11 years ago by bill

Pete - I think there's an issue w/this patch about children of layout widgets in the template, see http://bugs.dojotoolkit.org/attachment/ticket/5252/test.html.

Since the LayoutContainer will call startup() on foo.Button, _Templated shouldn't call foo.Button.startup() directly.

Parser deals w/this issue with this code:

(!instance.getParent || !instance.getParent())

BTW it's probably better in the unit test to use a counter rather than a boolean (for the number of times that startup() was called), to catch issues like this.

comment:9 Changed 11 years ago by Nathan Toone

BTW - there is an error in your patch, you name your variable "_earlyTemplatedStartup" - but later in the code, you reference "_earlyTemplatedStart"

comment:10 Changed 10 years ago by Mike Wilson

It seems the latest patch breaks programmatic creation of the targeted widgets. Ie, if MyWidget? is a templated widget with layout widgets in its template, then I get the following:

                      dojoType=MyWidget     new MyWidget()
dojo 1.3.1              fails                 ok
dojo SVN + patch        ok                    fails

comment:11 Changed 10 years ago by Mike Wilson

It would be interesting to hear if anyone else can reproduce my "programmatic" problem. Would you like me to post an example file?

comment:12 Changed 10 years ago by dante

yes, please do. or better, add a unit test to the _Templated unit test illustrating the regression to avoid.

When you say "layout widgets in the template" I immediately think of pottedmeat's _LayoutTemplated mixin, which may be related (something about startup and sizes and recursion iirc, I don't typically embed widgets _that_ deeply, so have never hit whatever bug the mixin is fixing afaik)

comment:13 Changed 10 years ago by Mike Wilson

I have started to make an updated unit test to reproduce the programmatic problem. Anyone, let me know if you have already done this (but not checked in) so I only put this time in if needed.

Changed 10 years ago by Mike Wilson

comment:14 Changed 10 years ago by Mike Wilson

The attached update of the _Templated-widgetsInTemplate unit test will show the following (and corrects what I said previously a bit) :

                      dojoType=Test4Widget   new Test4Widget()
dojo 1.3.1              fails                  fails
dojo SVN + patch        ok                     fails more

Something along the lines of "fails more" looks like it doesn't fully render the HTML while "fails" does that, but still ends up with the wrong layout size-wise.

comment:15 Changed 10 years ago by Mike Wilson

Did anyone try my unit test, and did it make sense?

comment:16 Changed 10 years ago by dante

Status: newassigned

I have not. I will poke into this today. Would love to get this patch in, but it is in such touchy territory and my patches have been so haphazard so far ... the updated tests should help though.

comment:17 Changed 10 years ago by renee zhu

Hi,

Did this patch still broken the programmatic way of initiate widget ? And Is there any fully working solution will be include in dojo 1.4 ? when 1.4 will release ?

Changed 10 years ago by Nathan Toone

Attachment: _Templated.4.patch added

Updated patch - fixes issues described above (misspellings, multiple startups), updates test cases (nested layout widgets, programmatic creation)

comment:18 Changed 10 years ago by Nathan Toone

This latest patch (_Templated.4.patch) combines the previous patches, and fixes the test cases and misspellings. It also checks for parent widgets that may call startup at a future point (as suggested by Bill).

This one appears to be working quite well for me...would appreciate more feedback from others.

comment:19 Changed 10 years ago by renee zhu

is the latest patch _Templated.4.patch against original dojo 1.3.1 source code or some other version ? because the line number are quite different.

and is there anyway I can directly download the whole patch instead modify line by line ?

Thank you

comment:20 Changed 10 years ago by Nathan Toone

The latest patch is against the most recent SVN trunk.

In order to download the entire patch, you can click on the "Original Format" link at the bottom of the patch page

comment:21 Changed 10 years ago by renee zhu

can you also make a patch against offical release dojo1.3.1 or dojo1.3.2 ?

I can only use the stable release for our project, and I try to update dijit/_Templated.js and dojo/parser.js to latest svn and add the patch but it doesn't work.

comment:22 Changed 10 years ago by Nathan Toone

Resolution: fixed
Status: assignedclosed

(In [18843]) Fixes #9348 - fix the startup order of templated widgets

comment:23 Changed 10 years ago by Nathan Toone

(In [18848]) Refs #9348 - _Contained widgets do not start up their children - just on addChild !strict

comment:24 Changed 10 years ago by Mike Wilson

I'll try to explain my unit test a bit further:

Test widget class in unit test

dojo.declare("Test4Widget", [dijit._Widget, dijit._Templated])
  ...
  templateString:
    <div>
      <div dojoType="dijit.layout.TabContainer" style="height: 5em;">
        ...
      </div>
    </div>

(ie, a non-layout widget that contains a layout widget in its template, the latter presumably laid out in its parent with standard CSS mechanisms)

Behaviour in Dojo 1.3.1

<div dojoType="Test4Widget">
-> TabContainer.startup automatically called (incorrect timing)

new Test4Widget()
-> TabContainer.startup automatically called (incorrect timing)

Behaviour in Dojo SVN + patch

<div dojoType="Test4Widget">
-> TabContainer.startup automatically called (correct timing)

new Test4Widget()
-> *** does not automatically call TabContainer.startup ***

Findings

As you can see, the patch is stopping the startup event from automatically being submitted to layout widget children in the new:ed widget.

As the goal of this patch was to correct the event ordering, I expected the event ordering to be corrected for new:ed widgets as well, and not actually remove the event altogether. I may be wrong here though, but I think automatically dispatching a startup event to these children is the right thing to do, so this behaviour shouldn't be removed.

FWIW, to me, manually calling startup on Test4Widget in your update of my unit test "feels" wrong as this event should only be applied to layout widgets, which Test4Widget is not? But I respect that I may be missing something here.

comment:25 Changed 10 years ago by Mike Wilson

Ignore my previous message. As Nathan kindly pointed out, startup should not fire until the widget's elements are attached to the DOM (my bad, and I actually know this!) so Pete's patch is doing the right thing in squelching it. Holding off startup until DOM attachment is the whole point of this patch, so sorry for the noise :-S...

comment:26 Changed 10 years ago by Nick Fenwick

I just wanted to nudge this ticket with a comment on how this change seems to trip up a basic use of widgets in templates. I must confess I do not yet understand the full discussion on widget lifecycle and when these methods 'should' be called.

I have a Templated dijit with this in the template:

<button dojoType="dijit.form.DropDownButton" label="Simple dropdown test">
<div dojoType="dijit.TooltipDialog"><p>Here is the dropdown!</p></div>
</button>

On dojo 1.4b1 (and presumably 1.3.2 or later) the dropdown button fails to operate - in the _HasDropDown.js click handler, this.dropDown is null. The Button's this.dropDown is never set up, apparently, and setting a breakpoint in Button.startup() shows it is never called.

Setting "_earlyTemplatedStartup: true" for my dijit fixes the button. Its startup() method is then called, this.dropDown is set up, and clicking the button raises the popup as expected.

Am I really expected to set _earlyTemplatedStartup for my dijit to make the button work? It seems hacky, and there should be a better way of making this work - a code change to Button.js, or something.

comment:27 Changed 10 years ago by bill

I'd need to see a full testcase to comment. Can you open another ticket and then use the attach file to attach a test case?

comment:28 Changed 10 years ago by Nick Fenwick

Thanks Bill, I raised #10219.

comment:29 Changed 6 years ago by Bill Keese <bill@…>

In 014c5bfa2364e3441f93f4e1c2462111edc0ed00/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.