select1 should allow values with spaces

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({fixed1.8.0.8, fixed1.8.1.1})

Trunk
fixed1.8.0.8, fixed1.8.1.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 237427 [details]
testcase
(Assignee)

Comment 2

12 years ago
Created attachment 237433 [details] [diff] [review]
patch
Assignee: xforms → surkov.alexander
Status: NEW → ASSIGNED
Attachment #237433 - Flags: review?(Olli.Pettay)
Comment on attachment 237433 [details] [diff] [review]
patch

I think this is right. Or this is the way I understood the spec too.
Attachment #237433 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

12 years ago
Attachment #237433 - Flags: review?(aaronr)

Comment 4

12 years ago
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
Attachment #237433 - Flags: review?(aaronr) → review-
(Assignee)

Comment 5

12 years ago
(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

12 years ago
(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.
(Assignee)

Comment 7

12 years ago
(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

12 years ago
(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
(Assignee)

Comment 9

12 years ago
Created attachment 238629 [details] [diff] [review]
patch2
Attachment #237433 - Attachment is obsolete: true
Attachment #238629 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Attachment #238629 - Attachment is obsolete: true
Attachment #238629 - Flags: review?(aaronr)
(Assignee)

Comment 10

12 years ago
Created attachment 238630 [details] [diff] [review]
patch3
Attachment #238630 - Flags: review?(aaronr)

Updated

12 years ago
Attachment #238630 - Flags: review?(aaronr) → review+
(Assignee)

Comment 11

12 years ago
needs to check in
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 13

12 years ago
checked into 1.8.0 on 2006/09/27
Keywords: fixed1.8.0.8

Comment 14

12 years ago
checked into 1.8 branch on 2006/11/21
Keywords: fixed1.8.1.1
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.