351901 - select1 should allow values with spaces

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

Now we have common code for select/select1 implementation that leads we split the instance value by spaces. The behaviour is valid for select, but it seems it's wrong for select1.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 237433 [details] [diff] [review]
patch

I think this is right. Or this is the way I understood the spec too.

Comment 4

[aaronr]

Comment on attachment 237433 [details] [diff] [review]
patch

>Index: extensions/xforms/resources/content/select.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select.xml,v
>retrieving revision 1.24
>diff -u -8 -p -r1.24 select.xml
>--- extensions/xforms/resources/content/select.xml	21 Aug 2006 21:24:08 -0000	1.24
>+++ extensions/xforms/resources/content/select.xml	9 Sep 2006 01:41:19 -0000
>@@ -305,31 +305,17 @@
>           // we encountered them.  This allows us to figure out later if any
>           // were not hit, which requires us to send an event.
>           this._defaultHash = new Object();
> 
>           // holds an array of DOMElements that exist under bound node,
>           this._selectedElementArray = new Array();
> 
>           if (!aContainsNonText) {
>-            // replace new line (\n), tabs (\t) and carriage returns (\r) with
>-            // "".
>-            var value = "";
>-            var accessValue = this.accessors.getValue();
>-            if (accessValue)
>-              value = accessValue.replace(/\n|\t|\r/g, " ");
>-
>-            // get an array of values selected in the bound node
>-            var selectedArray = value.split(/\s/);
>-
>-            // when the string is empty, split returns an array containing
>-            // one empty string, rather than an empty array.  We'll allow that
>-            // into the defaultHash so that xforms-out-of-range will be
>-            // correctly generated if none of the items in the select have
>-            // an empty string as a value
>+            var selectedArray = this._getValuesArray();
> 
>             for (var run = 0; run < selectedArray.length; run++) {
>               this._defaultHash[selectedArray[run]] = {hits: 0}
>             }
>             return;
>           }
> 
>           var boundNode = this.accessors.getBoundNode();
>@@ -344,22 +330,17 @@
>               var string = child.nodeValue;
>               var nonWhitespace = false;
>               if (string) {
>                 // this regexp tests whether only whitespace is contained
>                 // between the beginning and ending of the string.
>                 nonWhitespace = !(/^\s*$/.test(string));
>               }
>               if (nonWhitespace) {
>-                // replace new line (\n), tabs (\t) and carriage returns (\r)
>-                // with " ".
>-                var value = string.replace(/\n|\t|\r/g, " ");
>-
>-                // get an array of values selected in the bound node
>-                var selectedArray = value.split(" ");
>+                var selectedArray = this._getValuesArray();
> 

Your change here won't work right.  This part of the code should be going through the child nodes under the bound node one by one since there could be text nodes and element nodes intermingled.  With your change, we'll only ever be adding the result from accessors.getValue to the array.  You'll miss any other text node under the bound node.

If this is a select1, then there should only ever be a text node (containing no whitespace characters) or an element node under the bound node.  If more than one of either of these, then the control should be out of range.  More info inside select1's refresh method: http://lxr.mozilla.org/mozilla/source/extensions/xforms/resources/content/select1.xml#564

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> Your change here won't work right.  This part of the code should be going
> through the child nodes under the bound node one by one since there could be
> text nodes and element nodes intermingled.  With your change, we'll only ever
> be adding the result from accessors.getValue to the array.  You'll miss any
> other text node under the bound node.
> 
> If this is a select1, then there should only ever be a text node (containing no
> whitespace characters) or an element node under the bound node.  If more than
> one of either of these, then the control should be out of range.  More info
> inside select1's refresh method:
> http://lxr.mozilla.org/mozilla/source/extensions/xforms/resources/content/select1.xml#564
> 

I guess the pointed issue is rather in-range/out-of-range issue than whitespaces for select1 keeping issue. I'd like to have separate bug for this since in-range/out-of-range is not only in code you pointed and the patch doesn't broke current behaviour I guess. So, what do you think?

Comment 6

[aaronr]

(In reply to comment #5)
> (In reply to comment #4)
> 
> > Your change here won't work right.  This part of the code should be going
> > through the child nodes under the bound node one by one since there could be
> > text nodes and element nodes intermingled.  With your change, we'll only ever
> > be adding the result from accessors.getValue to the array.  You'll miss any
> > other text node under the bound node.
> > 
> > If this is a select1, then there should only ever be a text node (containing no
> > whitespace characters) or an element node under the bound node.  If more than
> > one of either of these, then the control should be out of range.  More info
> > inside select1's refresh method:
> > http://lxr.mozilla.org/mozilla/source/extensions/xforms/resources/content/select1.xml#564
> > 
> 
> I guess the pointed issue is rather in-range/out-of-range issue than
> whitespaces for select1 keeping issue. I'd like to have separate bug for this
> since in-range/out-of-range is not only in code you pointed and the patch
> doesn't broke current behaviour I guess. So, what do you think?
> 


We can do the out-of-range bit via bug 352398 if you want.  But you'd still need to back out the changes you made under the "if (nonWhitespace)" test.  The way you coded that part won't work, I don't believe.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> We can do the out-of-range bit via bug 352398 if you want. 

I'd like.

> But you'd still
> need to back out the changes you made under the "if (nonWhitespace)" test.  The
> way you coded that part won't work, I don't believe.
> 

Sorry, I got you. Probably I'll add argument to _getArrayValues() method to force it to work not only with instance data. It will allow to keep split/replace calls in one place. Does it work for you?

Comment 8

[aaronr]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > We can do the out-of-range bit via bug 352398 if you want. 
> 
> I'd like.
> 
> > But you'd still
> > need to back out the changes you made under the "if (nonWhitespace)" test.  The
> > way you coded that part won't work, I don't believe.
> > 
> 
> Sorry, I got you. Probably I'll add argument to _getArrayValues() method to
> force it to work not only with instance data. It will allow to keep
> split/replace calls in one place. Does it work for you?
> 


This is kinda where the xforms-out-of-range for select1 comes in.  Since at that point in the code we've already determined that there are non-text nodes in the instance data, then we shouldn't have to worry about text nodes any more.  Because if there are any, they make us out of range for select1's.  And if there are more element nodes, then we are also out of range for a select1.  So I'd think that in this one section of the code you could go back to what was there originally and either do the xforms-out-of-range processing now in this patch, or add a XXX and point it to bug 352398

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 11

[alexander :surkov (:asurkov)]

needs to check in

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Checked in to trunk

Comment 13

[aaronr]

checked into 1.8.0 on 2006/09/27

Comment 14

[aaronr]

checked into 1.8 branch on 2006/11/21