#14190 closed defect (fixed)
_DataBindingMixin doesn't properly handle being bound to a widget with NaN value
Reported by: | ben hockey | Owned by: | rahul |
---|---|---|---|
Priority: | high | Milestone: | 1.7.1 |
Component: | DojoX MVC | Version: | 1.7.0 |
Keywords: | Cc: | cjolif | |
Blocked By: | Blocking: |
Description
in _updateBinding
of _DataBindingMixin
, the value
watch handler gets into an infinite loop if the current value of a widget is NaN
. the code is currently:
binding.watch("value", function (name, old, current){ if(old === current){return;} if(pThis.get('value') === current){return;} pThis.set("value", current); }),
the infinite loop occurs because when checking if old === current
or if pThis.get('value') === current
, both conditions will return false if the values are both NaN
because NaN === NaN
is false
.
the fix should be something like:
binding.watch("value", function (name, old, current){ if(old === current || (isNaN(old) && isNaN(current))){return;} var pValue = pThis.get('value'); if(pValue === current || (isNaN(pValue) && isNaN(current))){return;} pThis.set("value", current); }),
i'll try to work on a test case.
Attachments (2)
Change History (10)
Changed 8 years ago by
Attachment: | 14190.html added |
---|
comment:1 Changed 8 years ago by
Ben the fix breaks other testcases where we are setting strings into the model. We will need to find another way to avoid the loop. You can see the fix breaks this testcase in a few places: dojox/mvc/tests/doh_mvc_form-kitchensink.html One failure is getting this: dojox/mvc/_DataBindingMixin:257 old = #000000 dojox/mvc/_DataBindingMixin:258 current = #ffffff dojox/mvc/_DataBindingMixin:259 isNaN(old) = true dojox/mvc/_DataBindingMixin:260 isNaN(current) = true Another got this: dojox/mvc/_DataBindingMixin:258 old = three dojox/mvc/_DataBindingMixin:258 current = ten dojox/mvc/_DataBindingMixin:259 isNaN(old) = true dojox/mvc/_DataBindingMixin:260 isNaN(current) = true
comment:2 Changed 8 years ago by
i created an _isEqual function that tests for strict equality and will catch the case when both values are NaN
. by adding the function, i've added some overhead for extra function calls but it's also made it so that it could be overridden. i don't mind if you prefer to have it inlined rather than as a function. with this change, the tests seem to pass for me (apart from the usual random robot failures).
-
dojox/mvc/_DataBindingMixin.js
224 224 } 225 225 }, 226 226 227 _isEqual: function(one, other){ 228 // test for equality 229 return one === other || 230 // test for NaN === NaN 231 isNaN(one) && typeof one === 'number' && 232 isNaN(other) && typeof other === 'number'; 233 }, 234 227 235 _updateBinding: function(name, old, current){ 228 236 // summary: 229 237 // Set the data binding to the supplied value, which must be a … … 252 260 this._modelWatchHandles = [ 253 261 // 1. value - no default 254 262 binding.watch("value", function (name, old, current){ 255 if( old === current){return;}256 if(pThis. get('value') === current){return;}263 if(pThis._isEqual(old, current)){return;} 264 if(pThis._isEqual(pThis.get('value'), current)){return;} 257 265 pThis.set("value", current); 258 266 }), 259 267 // 2. valid - default "true"
comment:3 Changed 8 years ago by
Ok Ben, that worked well for me too, I like the _isEqual function. I will update a test for this, and create a patch.
comment:4 Changed 8 years ago by
Cc: | cjolif added |
---|
Changed 8 years ago by
Attachment: | mvc-14190.patch added |
---|
Another updated patch, thanks to Christophe for the review of the previous one.
comment:6 Changed 8 years ago by
Milestone: | tbd → 1.8 |
---|
comment:8 Changed 8 years ago by
Milestone: | 1.8 → 1.7.1 |
---|
focus the NumberTextBox? and tab out