Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#14491 closed defect (fixed)

[patch] [cla] dojox.mvc.Group bug in databinding

Reported by: Martin Repta Owned by: Ed Chatelain
Priority: high Milestone: 1.7.2
Component: DojoX MVC Version: 1.7.1
Keywords: dojox, mvc, group, binging Cc:
Blocked By: Blocking:

Description

We found that binding does not work when element has ID same as ref attribute. I have attached sample of code and also patch fixing this problem

Attachments (6)

mvc_bug.html (1.3 KB) - added by Martin Repta 8 years ago.
patch.txt (409 bytes) - added by Martin Repta 8 years ago.
mvc_14491-fix1.html (1.6 KB) - added by Ed Chatelain 8 years ago.
Option 1 to fix the test.
mvc_14491-fix2.html (1.7 KB) - added by Ed Chatelain 8 years ago.
Option 2 to fix the test.
_DataBindingMixin-patch.txt (7.8 KB) - added by Ed Chatelain 8 years ago.
Cleanup the previous patch, and add a testcase.
_DataBindingMixin-17-patch.txt (7.7 KB) - added by Ed Chatelain 8 years ago.
Patch for dojo 1.7

Download all attachments as: .zip

Change History (19)

Changed 8 years ago by Martin Repta

Attachment: mvc_bug.html added

Changed 8 years ago by Martin Repta

Attachment: patch.txt added

comment:1 Changed 8 years ago by bill

Component: GeneralDojoX MVC
Owner: set to Ed Chatelain
Summary: dojox.mvc.Group bug in databinding[patch] [cla] dojox.mvc.Group bug in databinding

comment:2 Changed 8 years ago by Ed Chatelain

Thanks for the testcase and the patch, I will be on vacation for a few weeks but I will plan to test this out as soon as I get back.

comment:3 Changed 8 years ago by Martin Repta

Hi edchat, did you check it?

comment:4 Changed 8 years ago by Ed Chatelain

Hi, sorry for the delay. Today is my first day back from vacation, so I should be able to look at this late today or tomorrow.

comment:5 Changed 8 years ago by Ed Chatelain

So the basic problem is that you have a declarative setup for the two ValidationTextBoxes? with a ref which is expected to be found relative to the group, but the group does not have a ref set. So when the page is parsed the ref for the ValidationTextBoxes? are not found. You only see an error if the id and the ref are the same because there is coding in _DataBindingMixin._setupBinding calls: binding = lang.getObject(ref); (around line 207) which will return the html element with the id of the ref if it exists, this is expected to be a StatefulModel?, in your case it is not, and since the binding does not have a toPlainObject function an error is thrown. When the call is made and the lang.getObject(ref) call returns undefined no error is thrown.

So there are a few ways you could fix your code:

  1. You could create the model before the parse and set the model on the group declaratively. I will attach a test called mvc_14491_fix1.html to show that option.
  2. You could remove the declarative ref for the 2 ValidationTextBoxes? and set those programmatically. I will attach a test called mvc_14491_fix2.html to show that option.

I could also change the code in _DataBindingMixin._setupBinding to do a console.warn instead of throwing an error when the binding does not have the toPlainObject function, then you would get a warning because the ref was not found on the parse, but when you set the ref on the group it would work. I think that makes sense given the fact that no error is thrown when a ref is set, but not found anywhere, so the binding is undefined. Perhaps I should also give the warning when a ref is set, but no binding is found.

Changed 8 years ago by Ed Chatelain

Attachment: mvc_14491-fix1.html added

Option 1 to fix the test.

Changed 8 years ago by Ed Chatelain

Attachment: mvc_14491-fix2.html added

Option 2 to fix the test.

comment:6 Changed 8 years ago by Martin Repta

I understand what you mean. I was debugging code carefully and I understand how it works.

I do not want set model to do group declaratively, so your first example will not work form me. Second solution, the same...

Please, check my patch file. I think it solves this problem.

comment:7 Changed 8 years ago by Ed Chatelain

Your patch avoids your error, but it makes the code below it checking for the toPlainObject function basically a no-op. So if I take your update someone trying to setup a ref incorrectly may no longer see the error message that the ref was invalid. We can debate whether that is OK or not, but I think it would be better to change the error to a warning, and perhaps add an additional warning when the ref is set and the binding is not found in order to help the developer see why his bindings are not being setup.

If I make any change to verify that the binding is valid, I would rather make the check around line 207 in this code:

try{

binding = lang.getObject(ref);

}catch(err){

And avoid setting the binding if it is not a valid binding something like this:

try{

var t = lang.getObject(ref); if(t && lang.isFunction(t.toPlainObject)){

binding = t;

}

}catch(err){

comment:8 Changed 8 years ago by cjolif

Milestone: tbd1.8

comment:9 Changed 8 years ago by cjolif

Resolution: fixed
Status: newclosed

In [27641]:

fixes #14491. Thanks edchat.

Changed 8 years ago by Ed Chatelain

Attachment: _DataBindingMixin-patch.txt added

Cleanup the previous patch, and add a testcase.

Changed 8 years ago by Ed Chatelain

Patch for dojo 1.7

comment:10 Changed 8 years ago by cjolif

In [27649]:

refs #14491. Improved patch + test case. Thanks edchat.

comment:11 Changed 8 years ago by cjolif

In [27650]:

refs #14491. Small capitalization fix.

comment:12 Changed 8 years ago by cjolif

In [27651]:

refs #14491. Fixes the issue in 1.7.x branch.

comment:13 Changed 8 years ago by cjolif

Milestone: 1.81.7.2
Note: See TracTickets for help on using tickets.