Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#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)

14190.html (1.1 KB) - added by ben hockey 8 years ago.
focus the NumberTextBox? and tab out
mvc-14190.patch (4.2 KB) - added by Ed Chatelain 8 years ago.
Another updated patch, thanks to Christophe for the review of the previous one.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by ben hockey

Attachment: 14190.html added

focus the NumberTextBox? and tab out

comment:1 Changed 8 years ago by 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

comment:2 Changed 8 years ago by 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

     
    224224                       }
    225225               },
    226226
     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
    227235               _updateBinding: function(name, old, current){
    228236                       // summary:
    229237                       //              Set the data binding to the supplied value, which must be a
     
    252260                               this._modelWatchHandles = [
    253261                                       // 1. value - no default
    254262                                       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;}
    257265                                               pThis.set("value", current);
    258266                                       }),
    259267                                       // 2. valid - default "true"

comment:3 Changed 8 years ago by 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.

comment:4 Changed 8 years ago by cjolif

Cc: cjolif added

Changed 8 years ago by Ed Chatelain

Attachment: mvc-14190.patch added

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

comment:5 Changed 8 years ago by cjolif

Resolution: fixed
Status: newclosed

In [27070]:

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

comment:6 Changed 8 years ago by cjolif

Milestone: tbd1.8

comment:7 Changed 7 years ago by cjolif

In [27170]:

Fixes #14190 in 1.7.1 for edchat.

comment:8 Changed 7 years ago by cjolif

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