343905 - Input should not alter invalid/out-of-range data

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

The fix for bug 283278 has regressed at some point, so opening this bug to fix that regression.

The bug is basically that if a xf:input is bound to a boolean type that isn't a proper boolean value, it will show as an invalid or out-of-range checkbox that isn't checked.  Right now tabbing to the checkbox and tabbing off of it (without interacting with it) will cause the value to change to 'false' and all the sudden become valid.  This should not happen.

Comment 1

[aaronr]

Attachment: testcase

Comment 2

[aaronr]

Attachment: corrected testcase

fixed submission in the testcase to point to xformstest.org for echo'ing

Comment 3

[aaronr]

Attachment: testcase with incremental false

will need to pass this testcase too in order to be a good fix.

Comment 4

[aaronr]

Attachment: patch

I reintroduced the concept of a 'change' flag for the boolean inputs.

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Why do you need ._changed and .changed?
Why not just field called "changed" 

Comment 6

[aaronr]

(In reply to comment #5)
> Why do you need ._changed and .changed?
> Why not just field called "changed" 
> 

can't say that I gave it much thought.  Just copied the style of the other XBL properties that we have.  If you want it to be just a field, no problem.

Comment 7

[aaronr]

Attachment: patch2

fixed Olli's comment

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 229165 [details] [diff] [review]
patch2


>         </body>
>       </method>
>+

No need to add extra new lines.

Comment 9

[aaronr]

Attachment: patch with olli's nit fixed

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 229186 [details] [diff] [review]
patch with olli's nit fixed

 
>+      // We need to remember if the user has interacted with this control.
>+      // We don't want to change the value of the node that this control is
>+      // bound to unless the user caused the change.
>+      <field name="changed">false</field>

It looks like updateInstanceData() is called from point other than event handlers. Aaron, where from?

Comment 11

[aaronr]

(In reply to comment #10)
> (From update of attachment 229186 [details] [diff] [review] [edit])
> 
> >+      // We need to remember if the user has interacted with this control.
> >+      // We don't want to change the value of the node that this control is
> >+      // bound to unless the user caused the change.
> >+      <field name="changed">false</field>
> 
> It looks like updateInstanceData() is called from point other than event
> handlers. Aaron, where from?
> 


The call that was causing us problems here was the blur handler for boolean input.  It would call updateInstanceData which would call get value for the control.  get value returned 'true' if the checkbox was currently checked and 'false' if the checkbox wasn't currently checked.  However, the checkbox could be unchecked without the bound value actually being 'false' so we'd end up setting the bound value to false even though the user never interacted with the checkbox.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> 
> The call that was causing us problems here was the blur handler for boolean
> input.  It would call updateInstanceData which would call get value for the
> control.  get value returned 'true' if the checkbox was currently checked and
> 'false' if the checkbox wasn't currently checked.  However, the checkbox could
> be unchecked without the bound value actually being 'false' so we'd end up
> setting the bound value to false even though the user never interacted with the
> checkbox.
> 

Ok, clear. I'm not sure I like a lot this approach since it is not common. I guess we can get the same problem for rest of controls. Probably should we listen xforms-out(in)-of-range/xforms-(in)valid events or something else?

Comment 13

[aaronr]

(In reply to comment #12)
> (In reply to comment #11)
> 
> > 
> > The call that was causing us problems here was the blur handler for boolean
> > input.  It would call updateInstanceData which would call get value for the
> > control.  get value returned 'true' if the checkbox was currently checked and
> > 'false' if the checkbox wasn't currently checked.  However, the checkbox could
> > be unchecked without the bound value actually being 'false' so we'd end up
> > setting the bound value to false even though the user never interacted with the
> > checkbox.
> > 
> 
> Ok, clear. I'm not sure I like a lot this approach since it is not common. I
> guess we can get the same problem for rest of controls. Probably should we
> listen xforms-out(in)-of-range/xforms-(in)valid events or something else?
> 

If we listen for xforms-out-of-range and not update the instance data when the control is out of range, then the control will never go 'in range' unless another control is bound to the same instance data node.

Comment 14

[alexander :surkov (:asurkov)]

Ok. The problem is I guess we shouldn't try to update instance data if value of control wasn't been changed in fact. To have more flexibility I think we should move 'changed' field in 'xformswidget-base' binding and field change name of 'changed' property to reflect for what it is needed :). Probably we should have property instead field if we move it to base widget.

Comment 15

[aaronr]

(In reply to comment #14)
> Ok. The problem is I guess we shouldn't try to update instance data if value of
> control wasn't been changed in fact. To have more flexibility I think we should
> move 'changed' field in 'xformswidget-base' binding and field change name of
> 'changed' property to reflect for what it is needed :). Probably we should have
> property instead field if we move it to base widget.
> 

I don't want to move a property to xformswidget-base that is only being used for boolean inputs.  Can you think of some examples where other controls will want to use this property?  I'm thinking for now we put it in just for boolean inputs and if sometime later someone needs the change property, they can move it to where they need it (as long as it includes boolean input, also).  What do you think of that approach, Alex?

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> (In reply to comment #14)
> > Ok. The problem is I guess we shouldn't try to update instance data if value of
> > control wasn't been changed in fact. To have more flexibility I think we should
> > move 'changed' field in 'xformswidget-base' binding and field change name of
> > 'changed' property to reflect for what it is needed :). Probably we should have
> > property instead field if we move it to base widget.
> > 
> 
> I don't want to move a property to xformswidget-base that is only being used
> for boolean inputs.  Can you think of some examples where other controls will
> want to use this property?  I'm thinking for now we put it in just for boolean
> inputs and if sometime later someone needs the change property, they can move
> it to where they need it (as long as it includes boolean input, also).  What do
> you think of that approach, Alex?
> 

The first issue why I prefer to have that property inside xformswidget-base is we shouldn't update instance data until data isn't changed. The second I guess is xforms author can use constraint MIP and it will lead we'll get out-of-range for control. Will refresh() method be called in that case? If it will then again we can update instance data when if it's not needed.

Comment 17

[aaronr]

> The first issue why I prefer to have that property inside xformswidget-base is
> we shouldn't update instance data until data isn't changed.

We already prevent recalc, revalidate, refresh if the instance data hasn't changed, so I guess what you are looking for is that we don't even call this.accessors.setvalue if 'changed' hasn't been set.  I'd say that if we want to do this for every control then we need to do that in a different bug and that is when we'd set up a 'changed' property for every control.  However, I don't know if that is an efficient idea for every control.  For controls where user keystrokes alter the value (like most xf:inputs, xf:secret, xf:textarea, etc.) you'd have to put in some kind of before and after value compare and we'll still need to do the test during setvalue anyway because we can't ensure that a skinned control will set the changed flag.  So we'd end up doing the test twice in many cases.

> The second I guess is xforms author can use constraint MIP 
> and it will lead we'll get out-of-range
> for control. 

when @constraint evaluates to false, it makes the node invalid, not out-of-range

> Will refresh() method be called in that case? If it will then
> again we can update instance data when if it's not needed.
> 

Hmmmm, maybe I don't understand the question that you are asking.  If the value of a node changes that a control is bound to, we'll test the constaints on it.  If it is invalid, we'll set the NS_EVENT_STATE_INVALID intrinsic state.  If it is valid, we'll set the NS_EVENT_STATE_VALID intrinsic state.  Refresh() will get called either way.  Refresh gets called because the node value changed, not because of a state change.

Comment 18

[alexander :surkov (:asurkov)]

Sorry, constaints using don't leads to fail. But problem "we shouldn't update instance data if constrol value wasn't changed" is still actual I guess.

Also we can reduce the patch by using heuristic rule "incremental update cause is user action", "non incremental update couse is blur event". It means:

// <method name="updateInstanceData">
if aIncremental == true
  this.changed = true;
  if (this.incremental)
    // update
else
  if this.changed == true
    // update

// <method name="refresh">
  this.changed = false;

Comment 19

[aaronr]

(In reply to comment #18)
> Sorry, constaints using don't leads to fail. But problem "we shouldn't update
> instance data if constrol value wasn't changed" is still actual I guess.

We don't update the instance data unless the value changed.  That is how SetValue works (actual test is made in nsXFormsMDGEngine::SetNodeValueInternal...if old value == new value, then return).  But you are right in that we jump from the widget to the accessor's .cpp code and do all the work to get to SetNodeValueInternal in many cases without verifying that the user actually changed something.  So we might be able to put in earlier checks in .js, but we can't do away with the checks in SetNodeValueInternal in any case.  If we want to do this for every widget in the system, then I'd say that we should do that under a different bug.

> 
> Also we can reduce the patch by using heuristic rule "incremental update cause
> is user action", "non incremental update couse is blur event". It means:
> 
> // <method name="updateInstanceData">
> if aIncremental == true
>   this.changed = true;
>   if (this.incremental)
>     // update
> else
>   if this.changed == true
>     // update
> 
> // <method name="refresh">
>   this.changed = false;
> 

Nice thinking!  Expanding on that a little more, no sense duplicating the same 'update' in both sections.  I'll attach a new patch that uses your idea but that doesn't duplicate the update section.

Comment 20

[aaronr]

Attachment: patch with surkov's approach

Alex, how's this?

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 230478 [details] [diff] [review]
patch with surkov's approach


> 
>+      // We need to remember if the user has interacted with this control.
>+      // We don't want to change the value of the node that this control is
>+      // bound to unless the user caused the change.
>+      <field name="changed">false</field>

Js like comments looks good in XML file :)

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 230478 [details] [diff] [review]
patch with surkov's approach


>+          if (aIncremental) {
>+            // user interacted with the control.  Unless the form author
>+            // specifically said to wait on the updating, then update the
>+            // bound node's value now
>+            this.changed = true;
>+            if (this.getAttribute("incremental") == "false")
>+              return;
>+          } else {
>+            // blur happened.  Update the bound node value if the user
>+            // interacted with the control AND the form author told us to
>+            // defer the update.
>+            if (!this.changed) {
>+              return;
>             }
>           }

Probably instead big comments before two lines of code we should combine comments and move them to comment for method. Let it will be as you wish though.

The patch looks good as for the rest.

Comment 23

[aaronr]

Attachment: patch with surkov's comments

patch I'm going to checkin.

Comment 24

[aaronr]

checked into trunk

Comment 25

[aaronr]

checked into 1.8.0 branch on 2006/09/21

Comment 26

[aaronr]

checked into 1.8 branch on 2006/11/21