Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#13263 closed defect (fixed)

dojox.mvc - ref doesn't work in a templated widget

Reported by: ben hockey Owned by: Ed Chatelain
Priority: blocker Milestone: 1.8
Component: DojoX MVC Version: 1.7.0b1
Keywords: Cc: rahul, cjolif
Blocked By: Blocking:

Description

there is a condition that causes a widget in a template to not find it's parent as the binding reference. _WidgetsInTemplateMixin::startup calls startup on its children and then continues to call this.inherited to start itself. this is the opposite order to when the parser runs over the DOM - parents are started before children. (see logging in attached file)

when a widget in a template is started with a ref it starts walking up the DOM to find a parent with a binding. since the parent only sets its binding during startup, the problem arises that when the child is started before the parent, the child doesn't find a parent with a binding.

in addition, when the parent sets its binding during startup, this._beingBound prevents it from setting up the bindings for its children.

Attachments (1)

13263.html (2.0 KB) - added by ben hockey 8 years ago.

Download all attachments as: .zip

Change History (23)

Changed 8 years ago by ben hockey

Attachment: 13263.html added

comment:1 Changed 8 years ago by ben hockey

severity: blockermajor

i reduced the severity of this ticket because i have a workaround which is to call _updateChildBindings in the startup of my templated widget.

startup: function () {
  this.inherited(arguments);
  this._updateChildBindings();
}

comment:2 Changed 8 years ago by rahul

Manually updating the child bindings could be one work around, but requires knowledge of too many implementation details, which you have!

Separately, its unclear to me why the parser and templated widget behavior is different in terms of parent child startup ordering so I'll have to look into that first (it'd have been good if it were consistent :-).

comment:3 Changed 8 years ago by ben hockey

Cc: bill added

yeah the workaround is just my short term solution to keep moving ahead.

cc'd bill to see if he has any input regarding the difference in behavior for parsed vs templated widgets.

i think given the different behavior with templated vs parsed, the final solution may be along the lines of refactoring the code to have _setRefAttr and _setBingindAttr in place of large parts of _dbstartup and _setupBinding (outlined below).

parsed

children are created after the parent (but before the parent is started) and the child startup is called after the parent startup

in this case, the initial binding needs to be triggered during the child's startup. the child walks up the DOM to find a parent with a binding and binds to that. this is the current implementation.

templated

children are created during buildRendering of the parent and the child startup is called before the parent startup

in this case, the initial binding needs to be triggered by the parent's _setBindingAttr (automatically called as needed following buildRendering). when the parent's binding is set/updated, it should search for any children and update them. this search for children is safe to do in the parsed case as well but will return no children during initialization.

solution

an outline for a solution is as follows:

_setRefAttr: function (ref) {
    // this is in place of adding a watch to ref in _dbstartup.
    // rather than watching, we'll do the work as part of the setting.
    // this largely replaces `_setupBinding` but doesn't search for a parent binding
    // via the DOM so then we can safely do this work any time this is called.
    // walking the DOM for the parent binding only ever needs to happen during startup
    // if a widget has a ref but no binding.

    // if we have a ref that includes the binding object 
    // (ie expr, programmatic, widget... anything that is not a relative ref)
    // then translate and set this.ref to be a value relative to that object and then
    // call this.set('binding', binding); with the object referred to by the ref.
    // this will turn around and call this.set('ref', this.ref) which will 
    // be understood as a relative binding which we handle as the atomic case.

    // when we have this.ref as a relative ref and this.binding as an object then we
    // bind to the object as defined by the ref.
},
_setBindingAttr: function (binding) {
    // detach from any previous binding and attach to our new binding.
    // this can be done by setting this.binding = binding; first 
    // and then calling this.set('ref', this.ref);
    // and have _setRefAttr contain all the code needed to attach/detach
    // to a binding.

    // search for all child widgets and update their bindings
    forEach(dijit.findWidgets(this.domNode), function (w) {
        w.set('binding', binding);
    });
},
_dbstartup: function () {
    // for the parsed case, a child may have a ref with no binding by the time startup is called.
    // in this case we need to walk the DOM to find the parent.
    if (this.ref && !this.binding) {
        // walk the DOM to find a parent for binding
        var binding = this._getParentBindingFromDOM();
        // then trigger setting the binding property which in turn
        // will set the ref property
        this.set('binding', binding);
    }

    // for the templated case, nothing should need to happen during startup
    // a child should already have the binding due to _setBindingAttr having
    // set the binding for all its children
}

the outline above is a really rough sketch and i haven't followed the idea far enough to see if there's anything preventing this approach so take it for what it is. it would restructure the code to look quite a bit different to how it looks right now but i think it should be possible to do it in a way that wouldn't break anything.

i think that this kind of approach follows more along the lines of how existing dijit code works and i expect it should therefore have less problems with working inside the bounds of the dijit "framework".

let me know if you need me to follow this idea further to see if i can produce a patch. it would take a considerable amount of time and so i'd rather only do it if you don't have resources available to you.

comment:4 Changed 8 years ago by Ed Chatelain

Ben, I was hoping I would be able to work on this before now, but I have not been able to begin working on it yet. At this point I don't think I will be able to work on this until late next week, so if you have time to finish it and produce a patch please do. If not I will work on it as soon as I get past some of the other things I have on my plate. Thanks, Ed

comment:5 Changed 8 years ago by ben hockey

Resolution: fixed
Status: newclosed

(In [25798]) fix dot-separated references in dojox.mvc fixes #13263

comment:6 Changed 8 years ago by ben hockey

Resolution: fixed
Status: closedreopened

referenced wrong ticket. d'oh!

comment:7 Changed 8 years ago by ben hockey

ed,

if i get started on a patch i'll let you know so that we don't double up on the work. i don't know if i'll be able to get anything done soon because for me its not an urgent matter. i have a workaround so i'm able to move forward until it can be fixed. this makes it lower priority for me right now.

comment:8 Changed 8 years ago by Adam Peller

Cc: rahul added; bill removed
Owner: changed from rahul to Ed Chatelain

comment:9 Changed 8 years ago by Colin Snover

Priority: highblocker

Bulk update of open ticket priorities.

comment:10 Changed 7 years ago by Ed Chatelain

I think this problem is fixed with the new APIs added for 1.8. I have created a test using the new 1.8 APIs which works, you can run it from: dojox/mvc/tests/doh_mvc_ref-template-13263.html

Please take a look at that test and see if this problem is fixed.

comment:11 Changed 7 years ago by ben hockey

wow... there are a lot of new APIs! is there a doc somewhere to help me follow all these changes? it feels like i'm starting over again.

my original test still does not work and i expect i'll probably need a spare couple of hours to understand the differences between my original test and the one you've added since there are so many new APIs to understand. once i find some time to look at it i'll let you know if it seems like its fixed.

is StatefulModel going to be deprecated?

comment:12 Changed 7 years ago by Ed Chatelain

Yes there are a lot of updates, sorry we don't have doc ready yet, but there is information in the MVC (dojox.mvc) 1.8 specification. We are still working to finalize the APIs. Many of the changes are related to things which were requested in the email thread on the contributors list, like better array support, and a lighter weight model. Regarding StatefulModel?, yes I think it will be deprecated, but it has not been marked that way yet.

comment:13 Changed 7 years ago by ben hockey

ed, has there been much discussion in the open about these new APIs? i'm looking at _Controller as the first thing i opened up and its really unusual (possibly even just plain wrong) to automatically call startup like is happening here. that breaks the "contract" that is provided by widgets that startup is only called after a widget is guaranteed to be in the DOM. is someone familiar with dijit reviewing your new APIs? my concern is that if you don't have support from dijit (bill), then it may be hard to convince users to adopt mvc.

comment:14 Changed 7 years ago by Ed Chatelain

Hi Ben, Thanks for the feedback, the discussion has been in the MVC (dojox.mvc) 1.8 specification, which is open, but unfortunately Bill said that he has not had time to review it. Can you add your comment above to the document? Or would you rather I do it? I will also ask Bill if he has time to look it over. Thanks, Ed

comment:15 Changed 7 years ago by Ed Chatelain

Actually Ben we are in the middle of a debate now about whether _Controller should just extend _WidgetBase. If we make that change it may resolve your concern.

comment:16 Changed 7 years ago by ben hockey

i really think you need to get more people looking at this code as early as possible. could you move the discussion from the document to the mailing list? i have no notification of when changes have been made to the doc unless someone responds to one of my comments.

for some people, the first they're seeing of this code is the patches that land in trunk and that's if they are even following trunk - i don't think many people look at the document or even know about the github repos. by the time your patches have landed in trunk i'd imagine that certain changes may have a big impact on the rest of your code and so it would help for you to get feedback early.

btw, is there one definitive repository/branch of the code in github that i can follow so that i can see changes and provide feedback early? even if i don't have time to follow the whole discussion i could at least try to look at the code if i know where the definitive copy is kept.

comment:17 Changed 7 years ago by Ed Chatelain

Hi Ben,

Yes I will reply to the previous mailing list discussion to let people know where things are now, and ask for feedback. I agree having people look at the code as early as possible is best, and notification and comments on the document are not ideal.

The definitive repository in github is https://github.com/edchat/dojox_mvc

Thanks, Ed

comment:18 Changed 7 years ago by ben hockey

i'm confused as to why at is mixed in here:

declare('test.Templated', [testGroup, at, _TemplatedMixin, _WidgetsInTemplateMixin], {

comment:19 Changed 7 years ago by cjolif

Cc: cjolif added

comment:20 Changed 7 years ago by Ed Chatelain

Ben, you are correct, at should not be mixed into test.Templated, I will remove it.

comment:21 Changed 7 years ago by Ed Chatelain

Resolution: fixed
Status: reopenedclosed

I am going to go ahead and close this, I believe it was fixed by ticket #14681, and the fix can be seen with the testcase dojox/mvc/tests/doh_mvc_ref-template-13263.html Thanks, Ed

comment:22 Changed 7 years ago by bill

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