_DataBindingMixin doesn't properly handle being bound to a widget with NaN value

[ben hockey]

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.

[ben hockey]

focus the NumberTextBox? and tab out

[Ed Chatelain]

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

[ben hockey]

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 @@
                        }
                },
 
+               _isEqual: function(one, other){
+                       // test for equality
+                       return one === other ||
+                               // test for NaN === NaN
+                               isNaN(one) && typeof one === 'number' &&
+                               isNaN(other) && typeof other === 'number';
+               },
+
                _updateBinding: function(name, old, current){
                        // summary:
                        //              Set the data binding to the supplied value, which must be a
@@ -252 +260 @@
                                this._modelWatchHandles = [
                                        // 1. value - no default
                                        binding.watch("value", function (name, old, current){
-                                               if(old === current){return;}
-                                               if(pThis.get('value') === current){return;}
+                                               if(pThis._isEqual(old, current)){return;}
+                                               if(pThis._isEqual(pThis.get('value'), current)){return;}
                                                pThis.set("value", current);
                                        }),
                                        // 2. valid - default "true"

[Ed Chatelain]

Ok Ben, that worked well for me too, I like the _isEqual function. I will update a test for this, and create a patch.

[Ed Chatelain]

Another updated patch, thanks to Christophe for the review of the previous one.

[cjolif]

In [27070]:

fixes MVC for NaN values and adding to empty arrays from edchat (IBM, CCLA). fixes #14190.

[cjolif]

In [27170]:

Fixes #14190 in 1.7.1 for edchat.