#11105 closed defect (fixed)
[regression] _Widget.get breaks dtl.Context when using _DomTemplated
Reported by: | ben hockey | Owned by: | Kris Zyp |
---|---|---|---|
Priority: | high | Milestone: | 1.5 |
Component: | Dijit | Version: | 1.5.0b2 |
Keywords: | regression | Cc: | Neil Roberts |
Blocked By: | Blocking: |
Description
in dtl a Context
has a function called get
and so now if you pass a widget as a context (as _DomTemplated does), then the widget's get
overrides the dtl Context
's get
.
this causes a problem if you have something like the following in your template:
<input type='text' dojoAttachPoint='valueNode' value='{{ value }}'></input>
and then in your js you have something like:
_getValueAttr: function(){ return this.valueNode.value;}
as the template renders, it calls the get
function of the context (which is the widget's get
) and it ends up calling _getValueAttr
and causes a problem because valueNode
does not yet exist. classic chicken and egg problem :)
for now, i've worked around it by adding _setValueAttr
which updates this._value
and use _value
in the template.
<input type='text' dojoAttachPoint='valueNode' value='{{ _value }}'></input>
_setValueAttr: function(value){ this._value = value; if(this._started){ this.render(); } }
i've marked the component as dijit since this is what has caused the error. dtl hasn't changed in about 6 months. also, note that the code i've used here is not my actual code so it might not work but it shows the problem
Attachments (1)
Change History (12)
comment:1 Changed 11 years ago by
Milestone: | tbd → 1.5 |
---|---|
Owner: | set to kriszyp |
Summary: | _Widget.get breaks dtl.Context when using _DomTemplated → [regression] _Widget.get breaks dtl.Context when using _DomTemplated |
comment:2 Changed 11 years ago by
Owner: | changed from kriszyp to Kris Zyp |
---|
comment:3 follow-up: 4 Changed 11 years ago by
This seems to be a problem with DTL's Context though. It mixes in the original context dictionary when the context is created, allowing it to override context methods. Instead it should wrap the dictonaries.
comment:4 Changed 11 years ago by
Replying to markwubben:
This seems to be a problem with DTL's Context though. It mixes in the original context dictionary when the context is created, allowing it to override context methods. Instead it should wrap the dictonaries.
maybe so. but it can also be argued that mixing in the dictionary is a feature that allows you to provide a getter as part of your dictionary to override the default getter of the context. how many people, knowing that this is how it works, have intentionally exploited this "feature"?
personally i don't have a strong opinion either way - i can see both sides. i just happened to experience this regression and raised it as an issue. i'm less concerned about "how" it's resolved and more interested in seeing it resolved somehow soon. wrapping the dictionary would work for me since i've never intentionally exploited this "feature".
in #11220 neil mentions that he's updating the architecture of dtl and if that's the case then it might be possible that changing dtl will be the fix for this issue but i just hope it comes sooner rather than later.
comment:5 Changed 11 years ago by
Priority: | normal → high |
---|---|
severity: | normal → major |
comment:6 follow-up: 7 Changed 11 years ago by
I am not sure I understand, I would have thought that being able to correctly access attributes of a widget through a template (by going through get()) would be a desirable feature. Does markwubben have some patch for whatever he is suggesting? (I don't understand that one either)
comment:7 Changed 11 years ago by
Cc: | Neil Roberts added |
---|
Replying to kzyp:
I am not sure I understand, I would have thought that being able to correctly access attributes of a widget through a template (by going through get()) would be a desirable feature. Does markwubben have some patch for whatever he is suggesting? (I don't understand that one either)
the problem is that dtl has a get
for it's Context objects. the constructor for a Context then mixes in a dictionary which it uses for rendering the properties of that dictionary. in the case of dojox.dtl._DomTemplated
the widget is the context and so the mixin in the constructor of the Context causes the widget's get
to override the Context's get.
it is actually not desirable to "correctly access attributes of a widget" for rendering the template since these attributes could be lazily evaluated with a dependency on a DOM element in the template - eg the value
of a widget often considers the value of an input element in the template when responding to widget.get('value')
. obviously, you can't render a template with properties that have a dependency on elements in the template - ie you can't render the result of widget.get('value')
since it can't be determined until after the template is rendered. it would be preferable to render widget.value
rather than widget.get('value')
what markwubben is suggesting is that rather than have the Context mixin the dictionary, it should wrap it. for example, currently, the code for the constructor is:
dd._Context = dojo.extend(function(dict){ // summary: Pass one of these when rendering a template to tell the template what values to use. dojo._mixin(this, dict || {}); this._dicts = []; }
and the get for the Context is:
get: function(key, otherwise){ if(typeof this[key] != "undefined"){ return this._normalize(this[key]); } for(var i = 0, dict; dict = this._dicts[i]; i++){ if(typeof dict[key] != "undefined"){ return this._normalize(dict[key]); } } return otherwise; }
as i understand it, the approach that markwubben suggests would be more like this:
dd._Context = dojo.extend(function(dict){ // summary: Pass one of these when rendering a template to tell the template what values to use. this._dict = dict || {}; this._dicts = []; }
get: function(key, otherwise){ if(typeof this._dict[key] != "undefined"){ return this._normalize(this._dict[key]); } for(var i = 0, dict; dict = this._dicts[i]; i++){ if(typeof dict._dict[key] != "undefined"){ return this._normalize(dict._dict[key]); } } return otherwise; }
this would allow the template to use the widget.value
property when rendering rather than making a call to widget.get('value');
- this is the way it used to work. however, it is possible that this could break code which had already intentionally exploited the fact that the Context mixes in the dictionary. knowing that the Context mixes in the dictionary, it is possible that someone may have intentionally defined their own get
function in a dictionary, which would no longer be used if we wrap the dictionary. the question is... did anybody ever do this and does it matter so much if we break it on them? i would guess that it probably isn't done often so wrapping the dictionary might be the way to move forward.
i don't know if the code i've provided above will work but it illustrates what i understand markwubben to be suggesting. other code would probably need to be updated, i can see that at least the update
function of the Context would need to be updated but i don't know what else would be affected by this kind of change.
i don't know if neil has any ideas but i've added him in case he has something to say.
comment:8 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:9 Changed 11 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
this does the exact opposite of what the ticket was opened for. to be clear - the getter was already preserved and the ticket was opened because this was not desirable. i wanted the context to ignore the getter since using the getter may have a dependency on the template and you can't render the template with a value that depends on the template.
comment:10 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Changed 11 years ago by
Attachment: | 11105.html added |
---|
comment:11 Changed 11 years ago by
looks like it works - thanks. i should have made a test case earlier but i'm attaching it now in case anyone wants to refer to it in the future.
Regression from #10839