358712 - reorganize full selects

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

The approach when we have native widgets inside xforms selects leads to some problems described in bug 344685. Also, if selects will not contain native widgets then we'll get more clean accessible objects for them. Native widgsts for full selects is far-fetched thing since full selects acutuall haven't native widget.

Comment 1

[alexander :surkov (:asurkov)]

I can see one possible implementation.
1) add additional method to xtf part of select, something like refreshSelectItems. This method will do all the magic that happens now in xbl part. Moving some functionality to xtf code doesn't hit us and helps with performance.
2) refreshSelectItems() should select items of xforms select element. Therefore xbl part of item element should define additional attribute like selected.
3) if item element becomes to be selected then select element should check if item is allowed to be selected. Therefore xtf part of select should have additional method to check it.
4) also xtf part of select should have method that will update instance data.

If you fine with that approach in general then I'll start the work.

Comment 2

[alexander :surkov (:asurkov)]

Now we hide item/choices elements and generate native widgets that will represent xforms select/select1 elements. I try to solve two problems that we get with this approach.

1) clean accessibility objects. Only those elements that are represented can have accessibility objects. So I like to expose ally objects for select/choices/item elements. That means item and choices elements should be displayed and be real presentation elements. Otherwise some accessibility information will be unavailable because I am forced to expose other objects for accessibility hierarchy.

2) All user styles for xforms item, xforms label of item is lost. I guess I can't find more things we can lost with current approach.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> 2) All user styles for xforms item, xforms label of item is lost. I guess I
> can't find more things we can lost with current approach.
> 

Also, we will solve the problems with focusing. Since almost every refresh rebuild items then focus is lost. With proposed approach we avoid this problem because we won't add/remove elements from tree.

Comment 4

[aaronr]

(In reply to comment #1)
> I can see one possible implementation.
> 1) add additional method to xtf part of select, something like
> refreshSelectItems. This method will do all the magic that happens now in xbl
> part. Moving some functionality to xtf code doesn't hit us and helps with
> performance.

Sure it hits us.  Calling from JS to C++ (and vice versa) isn't cheap.  Try walking through it with a debugger.  But if it is the correct solution, then no reason we can't do it.

I don't have any problem with the change if it helps with the known problems.  Is this reorg going to affect normal select and select1?

Comment 5

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

Sounds always good if one plan is to speed things up.
Alex, do you see any problems with the approach or does it remove any
features or flexibility?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (In reply to comment #1)
> > I can see one possible implementation.
> > 1) add additional method to xtf part of select, something like
> > refreshSelectItems. This method will do all the magic that happens now in xbl
> > part. Moving some functionality to xtf code doesn't hit us and helps with
> > performance.
> 
> Sure it hits us.  Calling from JS to C++ (and vice versa) isn't cheap.  Try
> walking through it with a debugger. 

Sorry? All our js methods do is work with DOM. Therefore I guess any js code moving to c++ is speed up of us.

> But if it is the correct solution, then no
> reason we can't do it.

I like it because xbl part becomes to be more clear. It will hold only presentation things,  basically it is all why we need xbl.

> I don't have any problem with the change if it helps with the known problems. 
> Is this reorg going to affect normal select and select1?
> 

Yes, only full selects. This reorg makes sense for them because they have not native widgets. The idea of this reorg is do like minimal select1 for xhtml do excepting that I want to move many functionality code to c++.


(In reply to comment #5)
> Sounds always good if one plan is to speed things up.
> Alex, do you see any problems with the approach or does it remove any
> features or flexibility?
> 
I don't see any problem. The approach should not hit other selects, and makes clean xbl parts of full selects.

Comment 7

[alexander :surkov (:asurkov)]

How are copy and value used together? For example, if I have instance node that contains one textnode with value '11 12'. Also select has three items. Two items contain value elements that point to '11' and '12'. 3d item contains copy element that points to text node with value '11 12'. What items should be selected? Who wins? What will instance node contain if user select all those items?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> How are copy and value used together? For example, if I have instance node that
> contains one textnode with value '11 12'. Also select has three items. Two
> items contain value elements that point to '11' and '12'. 3d item contains copy
> element that points to text node with value '11 12'. What items should be
> selected? Who wins? What will instance node contain if user select all those
> items?
> 

Can xpath expression return text node? Olli pointed to xpath spec about text() function:
"text() selects all text node children of the context node".
But what do text() return nodeset or string?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Sounds always good if one plan is to speed things up.
> Alex, do you see any problems with the approach or does it remove any
> features or flexibility?
> 

If someone wants to treat instance data by someother way we do (if he wants to bind select to specific datatype) then it will be difficult to support his datatype if we will do datatype processing in c++. It's not flexible I think.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: simple example

Comment 11

[alexander :surkov (:asurkov)]

Attachment: idea

Comment 12

[aaronr]

(In reply to comment #7)
> How are copy and value used together? For example, if I have instance node that
> contains one textnode with value '11 12'. Also select has three items. Two
> items contain value elements that point to '11' and '12'. 3d item contains copy
> element that points to text node with value '11 12'. What items should be
> selected? Who wins? What will instance node contain if user select all those
> items?
> 

In this scenario, only the first two items 11 and 12 are selected.  A copy element won't work if it isn't bound to an element node (http://www.w3.org/TR/xforms/index-all.html#ui-adv-copy).

Comment 13

[aaronr]

(In reply to comment #8)
> (In reply to comment #7)
> > How are copy and value used together? For example, if I have instance node that
> > contains one textnode with value '11 12'. Also select has three items. Two
> > items contain value elements that point to '11' and '12'. 3d item contains copy
> > element that points to text node with value '11 12'. What items should be
> > selected? Who wins? What will instance node contain if user select all those
> > items?
> > 
> 
> Can xpath expression return text node? Olli pointed to xpath spec about text()
> function:
> "text() selects all text node children of the context node".
> But what do text() return nodeset or string?
> 


nodeset of all of the text nodes that are immediate children of the context node (I asked sicking to make sure).

Comment 14

[aaronr]

(In reply to comment #13)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > How are copy and value used together? For example, if I have instance node that
> > > contains one textnode with value '11 12'. Also select has three items. Two
> > > items contain value elements that point to '11' and '12'. 3d item contains copy
> > > element that points to text node with value '11 12'. What items should be
> > > selected? Who wins? What will instance node contain if user select all those
> > > items?
> > > 
> > 
> > Can xpath expression return text node? Olli pointed to xpath spec about text()
> > function:
> > "text() selects all text node children of the context node".
> > But what do text() return nodeset or string?
> > 
> 
> 
> nodeset of all of the text nodes that are immediate children of the context
> node (I asked sicking to make sure).
> 

To be even more exact the nodeset would also include any cdata nodes that are immediate children of the context node.

Comment 15

[aaronr]

Comment on attachment 244213 [details] [diff] [review]
idea

>Index: extensions/xforms/resources/content/select-xhtml.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select-xhtml.xml,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 select-xhtml.xml
>--- extensions/xforms/resources/content/select-xhtml.xml	27 Oct 2006 10:49:09 -0000	1.7
>+++ extensions/xforms/resources/content/select-xhtml.xml	31 Oct 2006 18:50:33 -0000
>@@ -459,10 +459,86 @@
>           event.originalTarget.firstChild.checked = true;
>           this.updateInstanceData(true);
>         }
>       ]]>
>       </handler>
>     </handlers>
>   </binding>
> 
>+
>+  <binding id="select-full"
>+           extends="chrome://xforms/content/select.xml#select-base">
>+    <content>
>+      <children includes="label"/>
>+      <html:div>
>+        <children/>
>+      </html:div>
>+    </content>
>+  </binding>

I guess that this div should have the xf-value designation?

>+
>+
>+  <binding id="select-full-item"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-general">
>+    <content>
>+      <html:input type="checkbox" anonid="control"/>
>+      <children includes="label"/>
>+      <html:span style="display: none;">
>+        <children/>
>+      </html:span>
>+    </content>

I assume that this un-displayed span is just to make sure that the children of xf:item's don't show up?


>Index: extensions/xforms/resources/content/select.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select.xml,v
>retrieving revision 1.28
>diff -u -8 -p -r1.28 select.xml
>--- extensions/xforms/resources/content/select.xml	27 Oct 2006 10:49:09 -0000	1.28
>+++ extensions/xforms/resources/content/select.xml	31 Oct 2006 18:50:33 -0000
>@@ -34,16 +34,247 @@
>    - use your version of this file under the terms of the MPL, indicate your
>    - decision by deleting the provisions above and replace them with the notice
>    - and other provisions required by the GPL or the LGPL. If you do not delete
>    - the provisions above, a recipient may use your version of this file under
>    - the terms of any one of the MPL, the GPL or the LGPL.
>    -
>    - ***** END LICENSE BLOCK ***** -->
> 
>+<bindings id="xformsSelectBindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:html="http://www.w3.org/1999/xhtml"
>+          xmlns:xbl="http://www.mozilla.org/xbl">
>+
>+  <!-- BASE for select/select1 elements. -->
>+  <binding id="selectcontrols-base"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+
>+    <implementation implements="nsIXFormsUIWidget">
>+      <method name="refresh">
>+        <body>
>+        <![CDATA[
>+          this.valuesArray = this.getValuesArray();
>+          this.valuesHitArray = new Array(this.valuesArray.length);
>+          this.nodesArray = this.getNodesArray();
>+          this.nodesHitArray = new Array(this.nodesArray.length);
>+
>+          for (var child = this.firstChild; child; child = child.nextSibling)
>+            this.selectItemElements(child);
>+
>+          var outOfRange = this.isOutOfRange(this.valuesHitArray) ||
>+            this.isOutOfRange(this.nodesHitArray);
>+          if (this.outOfRange == null || this.outOfRange != outOfRange)
>+            this.accessors.setInRange(outOfRange);
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <method name="selectItemElements">
>+        <parameter name="aContextNode"/>
>+        <body>
>+        <![CDATA[
>+          if (aContextNode.namespaceURI != this.XFORMS_NS)
>+            return;
>+
>+          var context;
>+          switch (aContextNode.localName) {
>+            case "choices":
>+              context = aContextNode;
>+              break;
>+            case "itemset":
>+              context = aContextNode.anonymousItemSetContent;
>+              break;
>+            case "item":
>+              var item = aContextNode.
>+                QueryInterface(Components.interfaces.nsIXFormsItemElement);
>+              if (item.isCopyItem) {
>+                for (var i = 0; i < this.nodesArray.length; i++) {
>+                  if (this.nodesArray[i] == item.copyNode) {

FYI, I'm sure that you know that this test is too simplistic.  Can't compare two element nodes like this.  At some point you'll have to call into c++ code and call nsXFormsUtils::AreNodesEqual.  Currently we do that via: nsXFormsItemElement::SelectItemByNode

>+                    item.selected = true;
>+                    this.nodesHitArray[i] = true;
>+                    return;
>+                  }
>+                }
>+              } else {
>+                for (var i = 0; i < this.valuesArray.length; i++) {
>+                  if (this.valuesArray[i] == item.value) {
>+                    item.selected = true;
>+                    this.valuesHitArray[i] = true;
>+                    return;
>+                  }
>+                }
>+              }
>+          }
>+
>+          if (context) {
>+            for (var child = context.firstChild; child; child = child.nextSibling) {
>+              this.selectItemElements(child);
>+            }
>+          }
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <method name="updateInstanceData">
>+        <parameter name="aIncremental"/>
>+        <body>
>+        <![CDATA[
>+          if (aIncremental && this.getAttribute("incremental") == "false")
>+            return;
>+
>+          var values = "";
>+          for (var child = this.firstChild; child; child = child.nextSibling) {
>+            values = this.getValuesFrom(child, values);
>+          }
>+          this.accessors.setValue(values);
>+        ]]>
>+        </body>
>+      </method>

FYI, once you support xf:copy and are updating the instance data, you'll need to make sure that all selected xf:value's are space delimited in the first textnode under the bound node (like setValue does) and then all selected copyItems will have their nodes cloned underneath.  And keep in mind that you should put them under the bound node all at the same time so that the rebuild, recalculate, revalidate and refresh sequence only has to run once.

>+
>+      <method name="getValuesFrom">
>+        <parameter name="aContextNode"/>
>+        <parameter name="aValues"/>
>+        <body>
>+        <![CDATA[
>+          if (aContextNode.namespaceURI != this.XFORMS_NS)
>+            return aValues;
>+
>+          var context;
>+          switch (aContextNode.localName) {
>+            case "choices":
>+              context = aContextNode;
>+              break;
>+            case "itemset":
>+              context = aContextNode.anonymousItemSetContent;
>+              break;
>+            case "item":
>+              var item = aContextNode.
>+                QueryInterface(Components.interfaces.nsIXFormsItemElement);
>+              if (item.selected && !item.isCopyNode) {
>+                if (aValues)
>+                  aValues += " ";
>+                aValues += item.value;
>+                return aValues;
>+              }
>+          }
>+
>+          if (context) {
>+            for (var child = context.firstChild; child; child = child.nextSibling) {
>+              aValues = this.getValuesFrom(child, aValues);
>+            }
>+          }
>+          return aValues
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <field name="outOfRange">null</field>
>+
>+      <field name="valuesArray">null</field>
>+      <field name="valuesHitArray">null</field>
>+      <field name="nodesArray">null</field>
>+      <field name="nodesHitArray">null</field>


I'd like the 'hit' kept in the same array as its corresponding value or node if at all possible, to keep related things grouped together rather than split apart.

>+    </implementation>
>+  </binding>
>+
>+
>+  <!-- SELECT: BASE -->
>+  <binding id="select-base"
>+           extends="#selectcontrols-base">
>+
>+    <implementation>
>+      <method name="getValuesArray">
>+        <body>
>+          // A limitation of the XML Schema list datatypes is that white space
>+          // characters in the storage values (the value element) are always
>+          // interpreted as separators between individual data values.
>+          // Therefore, authors should avoid using white space characters within
>+          // storage values with list simpleContent.
>+
>+          // Replace new line (\n), tabs (\t) and carriage returns (\r) with
>+          // space (" ").
>+          var value = this.stringValue.replace(/\n|\t|\r/g, " ");
>+
>+          return value.split(/\s/);
>+        </body>
>+      </method>

Its not really that simple.  What about the text nodes that exist in between the element nodes?  This function won't get them.

>+
>+      <method name="getNodesArray">
>+        <body>
>+          var array = [];
>+          if (!this.accessors.hasBoundNode())
>+            return array;
>+
>+          var nsIDOMNode = Components.interfaces.nsIDOMNode;
>+
>+          var boundNode = this.accessors.getBoundNode();
>+          for (var child = boundNode.firstChild; child; child = child.nextSibling) {
>+            if (child.nodeType == nsIDOMNode.ELEMENT_NODE)
>+              array.push(child);
>+          }
>+
>+          return array;
>+        </body>
>+      </method>
>+
>+      <method name="isOutOfRange">
>+        <parameter name="aHitArray"/>
>+        <body>
>+        <![CDATA[
>+          for (var i = 0; i < aHitArray.length && aHitArray[i]; i++);
>+          return i != aHitArray.length;
>+        ]]>
>+        </body>
>+      </method>
>+    </implementation>
>+  </binding>
>+
>+
>+  <!-- SELECT1: BASE -->
>+  <binding id="select1-base"
>+           extends="#selectcontrols-base">
>+
>+    <implementation>
>+      <method name="getValuesArray">
>+        <body>
>+          return [this.stringValue];
>+        </body>
>+      </method>
>+
>+      <method name="getNodesArray">
>+        <body>
>+          var array = [];
>+          if (!this.accessors.hasBoundNode())
>+            return array;
>+
>+          var nsIDOMNode = Components.interfaces.nsIDOMNode;
>+
>+          var boundNode = this.accessors.getBoundNode();
>+          for (var child = boundNode.firstChild; child; child = child.nextSibling) {
>+            if (child.nodeType == nsIDOMNode.ELEMENT_NODE)
>+              array.push(child);
>+          }
>+
>+          return array;
>+        </body>
>+      </method>
>+
>+      <method name="isOutOfRange">
>+        <parameter name="aHitArray"/>
>+        <body>
>+        <![CDATA[
>+          return aHitArray.length > 1;
>+        ]]>
>+        </body>

this won't work for select1.  You are testing both the values array and the node array in selectcontrols-base.  If each of these arrays have one hit in them then isOutOfRange will return false for each so the test in selectcontols-base will show the control as being in range even though it wouldn't be

In short I don't have a problem with how you seem to be splitting things out.  The organizational stuff looks ok.  But there are a LOT of special tests and scenarios in select and select1 that you need to make sure carry over.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: idea2

Comment 17

[aaronr]

Comment on attachment 244471 [details] [diff] [review]
idea2

I don't see any xforms-select or xforms-deselect messages being generated.  And I really don't like the isCopyNode method in the item interface.  But I can't say that I have a better idea.  Otherwise I think your ideas seem ok.

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch

Let's start!

Comment 19

[aaronr]

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

>Index: extensions/xforms/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/jar.mn,v
>retrieving revision 1.22
>diff -u -8 -p -r1.22 jar.mn
>--- extensions/xforms/jar.mn	25 Sep 2006 09:59:13 -0000	1.22
>+++ extensions/xforms/jar.mn	6 Nov 2006 17:19:32 -0000
>@@ -21,16 +21,19 @@ xforms.jar:
>   content/xforms/input-xhtml.xml               (resources/content/input-xhtml.xml)
>   content/xforms/input-xul.xml                 (resources/content/input-xul.xml)
>   content/xforms/select1.xml                   (resources/content/select1.xml)
>   content/xforms/range.xml                     (resources/content/range.xml)
>   content/xforms/range-xhtml.xml               (resources/content/range-xhtml.xml)
>   content/xforms/select.xml                    (resources/content/select.xml)
>   content/xforms/select-xhtml.xml              (resources/content/select-xhtml.xml)
>   content/xforms/select-xul.xml                (resources/content/select-xul.xml)
>+  content/xforms/selects.xml                    (resources/content/selects.xml)
>+  content/xforms/selects-xhtml.xml              (resources/content/selects-xhtml.xml)
>+  content/xforms/selects-xul.xml                (resources/content/selects-xul.xml)

nit: align file names
nit: ugh, I'm not much of a fan of having a select-xhtml.xml, select.xml, selects-xhtml.xml and selects.xml.  The names are too similar.  How about renaming selects*.xml to select-full-*.xml if all they are going to contain is the implementations for the select/select1 full bindings.

>Index: extensions/xforms/resources/content/select.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select.xml,v
>retrieving revision 1.30
>diff -u -8 -p -r1.30 select.xml
>--- extensions/xforms/resources/content/select.xml	6 Nov 2006 02:50:37 -0000	1.30
>+++ extensions/xforms/resources/content/select.xml	6 Nov 2006 17:19:32 -0000

>@@ -866,25 +865,24 @@
>           ]]>
>         </body>
>       </method>
> 
>       <method name="dispatchSelectEvent">
>         <parameter name="aElement"/>
>         <parameter name="aName"/>
>         <body>
>-          var ev = document.createEvent("Events");
>-          ev.initEvent(aName, true, false);
>+        <![CDATA[
>           var elm = aElement;
>           // per http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#evt-select
>           // we send the event to the itemset if it is a parent.
>-          if (elm.parentNode.localName == "itemset")
>-            elm = elm.parentNode;
>-          elm.dispatchEvent(ev);
>-          return true;
>+          var parent = this.ownerDocument.getBindingParent(this);
>+          var elm = parent && parent.localName == "itemset" ? parent : this;
>+          return this.dispatchXFormsNotificationEvent(aName, elm);
>+        ]]>

I don't see why you changed this.  It looks to me like you are getting the binding parent of 'this' which is the xf:select element, right?  And then looking to see if the select's parent is an itemset?  The xforms-select and xforms-deselect need to go to the item or itemset element.  If you want to use dispatchXFormsNotificationEvent, you  still can using the original code.  Just remove the createEvent, initEvent and dispatchEvent calls and add in your dispatchXFormsNotificationEvent call.


>Index: extensions/xforms/resources/content/selects-xhtml.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects-xhtml.xml
>diff -N extensions/xforms/resources/content/selects-xhtml.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects-xhtml.xml	6 Nov 2006 17:19:33 -0000
>@@ -0,0 +1,171 @@

>+  <!-- ITEM of SELECT APPEARANCE FULL -->
>+  <binding id="select-full-item"
>+           extends="#select-full-item-base">
>+    <content>
>+      <html:input type="checkbox" anonid="control"/>
>+      <children/>
>+    </content>
>+
>+    <handlers>
>+      <handler event="click">
>+        if (this.disabled)
>+          return;
>+

nit: comment here about why item would get a click, but the checkbox wouldn't be the target.  Mention how the original target must be the item label which means we need to act the same way as if the checkbox were clicked directly.

>+        if (event.originalTarget != this.control) {
>+          this.selected = !this.selected;
>+          this.focus();
>+        }
>+
>+        this.updateInstanceData(true);
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+
>+  <!-- ITEM of SELECT1 APPEARANCE FULL -->
>+  <binding id="select1-full-item"
>+           extends="#select-full-item-base">
>+    <content>
>+      <html:input type="radio" anonid="control"/>
>+      <children/>
>+    </content>
>+
>+    <handlers>
>+      <handler event="click">
>+      <![CDATA[
>+        if (this.disabled)
>+          return;
>+
>+        if (event.originalTarget != this.control && !this.selected) {
>+          this.selected = true;
>+          this.focus();
>+        }

shouldn't you set focus even if the item was already selected?  The focus could be coming from another control or even another window.

>+
>+        this.updateInstanceData(true);
>+      ]]>
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+</bindings>
>+
>Index: extensions/xforms/resources/content/selects-xul.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects-xul.xml
>diff -N extensions/xforms/resources/content/selects-xul.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects-xul.xml	6 Nov 2006 17:19:33 -0000
>@@ -0,0 +1,198 @@

>+
>+  <!-- ITEM of SELECT1 APPEARANCE FULL -->
>+  <binding id="select1-full-item"
>+           extends="#select-full-item-base">
>+    <content>
>+      <xul:hbox>
>+        <xul:radio anonid="control"/>
>+        <children/>
>+      </xul:hbox>
>+    </content>
>+
>+    <implementation>
>+      <constructor>
>+        var select = this.selectControl;
>+        this.control.__defineGetter__("radioGroup", function radioGroup() {
>+          return select.control;
>+        });
>+      </constructor>

do you still need to define this getter?  I don't see that you use it anywhere.

This is as far as I got today.  I'll try to finish it up tomorrow.

>Index: extensions/xforms/resources/content/selects.xml
>===================================================================

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)

> nit: ugh, I'm not much of a fan of having a select-xhtml.xml, select.xml,
> selects-xhtml.xml and selects.xml.  The names are too similar.  How about
> renaming selects*.xml to select-full-*.xml if all they are going to contain is
> the implementations for the select/select1 full bindings.

Once I have plans to use bindings of selects.xml for xhmlt minimal select1 control and implement this control in selects-xhtml.xml file. What will we do with names?

> >Index: extensions/xforms/resources/content/select.xml
> >===================================================================
> >RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select.xml,v
> >retrieving revision 1.30
> >diff -u -8 -p -r1.30 select.xml
> >--- extensions/xforms/resources/content/select.xml	6 Nov 2006 02:50:37 -0000	1.30
> >+++ extensions/xforms/resources/content/select.xml	6 Nov 2006 17:19:32 -0000
> 
> >@@ -866,25 +865,24 @@
> >           ]]>
> >         </body>
> >       </method>
> > 
> >       <method name="dispatchSelectEvent">
> >         <parameter name="aElement"/>
> >         <parameter name="aName"/>
> >         <body>
> >-          var ev = document.createEvent("Events");
> >-          ev.initEvent(aName, true, false);
> >+        <![CDATA[
> >           var elm = aElement;
> >           // per http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#evt-select
> >           // we send the event to the itemset if it is a parent.
> >-          if (elm.parentNode.localName == "itemset")
> >-            elm = elm.parentNode;
> >-          elm.dispatchEvent(ev);
> >-          return true;
> >+          var parent = this.ownerDocument.getBindingParent(this);
> >+          var elm = parent && parent.localName == "itemset" ? parent : this;
> >+          return this.dispatchXFormsNotificationEvent(aName, elm);
> >+        ]]>
> 
> I don't see why you changed this.  It looks to me like you are getting the
> binding parent of 'this' which is the xf:select element, right?  And then
> looking to see if the select's parent is an itemset?  The xforms-select and
> xforms-deselect need to go to the item or itemset element.  If you want to use
> dispatchXFormsNotificationEvent, you  still can using the original code.  Just
> remove the createEvent, initEvent and dispatchEvent calls and add in your
> dispatchXFormsNotificationEvent call.

This changes is of type "while I'm here" :). item.parentNode returns anonymous node of itemset therefore I changed to use bindingParent.

> >+      <handler event="click">
> >+      <![CDATA[
> >+        if (this.disabled)
> >+          return;
> >+
> >+        if (event.originalTarget != this.control && !this.selected) {
> >+          this.selected = true;
> >+          this.focus();
> >+        }
> 
> shouldn't you set focus even if the item was already selected?  The focus could
> be coming from another control or even another window.

sorry, I didn't get you. I just think I should focus too item element when I click its label.

> >+      <constructor>
> >+        var select = this.selectControl;
> >+        this.control.__defineGetter__("radioGroup", function radioGroup() {
> >+          return select.control;
> >+        });
> >+      </constructor>
> 
> do you still need to define this getter?  I don't see that you use it anywhere.

Sorry, I forgot to put comment here. This is because xul:radio must be placed inside explicit content of xul:radiogroup. Therefore I redefine property of xul:radio and method of xul:radiogroup to use them in anonymous content. I agree it's a trick but I can't see more nice way.

Comment 21

[aaronr]

(In reply to comment #20)
> (In reply to comment #19)
> 
> > nit: ugh, I'm not much of a fan of having a select-xhtml.xml, select.xml,
> > selects-xhtml.xml and selects.xml.  The names are too similar.  How about
> > renaming selects*.xml to select-full-*.xml if all they are going to contain is
> > the implementations for the select/select1 full bindings.
> 
> Once I have plans to use bindings of selects.xml for xhmlt minimal select1
> control and implement this control in selects-xhtml.xml file. What will we do
> with names?

well, if you put minimal select1 into selects.xml and get rid of select1.xml, then we'll be left with select.xml and selects.xml (plus the xhtml and xul ones, of course).  And both select.xml and selects.xml will have both select and select1 implementations in them. 'select' and 'selects' are very similar names and the files will hold similar types of content.  That is very confusing.  We need to distinguish what types of bindings are in each file and have a filename that reflects that.  For example, if selects.xml will hold full and minimal and select.xml holds the compact implementations, then we can call them selects-compact and selects-full-minimal.  Just something descriptive so that at a glance we know what should live in each.

> 
> > >Index: extensions/xforms/resources/content/select.xml
> > >===================================================================
> > >RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select.xml,v
> > >retrieving revision 1.30
> > >diff -u -8 -p -r1.30 select.xml
> > >--- extensions/xforms/resources/content/select.xml	6 Nov 2006 02:50:37 -0000	1.30
> > >+++ extensions/xforms/resources/content/select.xml	6 Nov 2006 17:19:32 -0000
> > 
> > >@@ -866,25 +865,24 @@
> > >           ]]>
> > >         </body>
> > >       </method>
> > > 
> > >       <method name="dispatchSelectEvent">
> > >         <parameter name="aElement"/>
> > >         <parameter name="aName"/>
> > >         <body>
> > >-          var ev = document.createEvent("Events");
> > >-          ev.initEvent(aName, true, false);
> > >+        <![CDATA[
> > >           var elm = aElement;
> > >           // per http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#evt-select
> > >           // we send the event to the itemset if it is a parent.
> > >-          if (elm.parentNode.localName == "itemset")
> > >-            elm = elm.parentNode;
> > >-          elm.dispatchEvent(ev);
> > >-          return true;
> > >+          var parent = this.ownerDocument.getBindingParent(this);
> > >+          var elm = parent && parent.localName == "itemset" ? parent : this;
> > >+          return this.dispatchXFormsNotificationEvent(aName, elm);
> > >+        ]]>
> > 
> > I don't see why you changed this.  It looks to me like you are getting the
> > binding parent of 'this' which is the xf:select element, right?  And then
> > looking to see if the select's parent is an itemset?  The xforms-select and
> > xforms-deselect need to go to the item or itemset element.  If you want to use
> > dispatchXFormsNotificationEvent, you  still can using the original code.  Just
> > remove the createEvent, initEvent and dispatchEvent calls and add in your
> > dispatchXFormsNotificationEvent call.
> 
> This changes is of type "while I'm here" :). item.parentNode returns anonymous
> node of itemset therefore I changed to use bindingParent.
> 

That's fine.  I don't mind the change, I'm just saying that it won't work the way you have it coded.  You need something more like:

var parent = this.ownerDocument.getBindingParent(aElement);
var elm = parent && parent.localName == "itemset" ? parent : aElement;
return this.dispatchXFormsNotificationEvent(aName, elm);

> > >+      <handler event="click">
> > >+      <![CDATA[
> > >+        if (this.disabled)
> > >+          return;
> > >+
> > >+        if (event.originalTarget != this.control && !this.selected) {
> > >+          this.selected = true;
> > >+          this.focus();
> > >+        }
> > 
> > shouldn't you set focus even if the item was already selected?  The focus could
> > be coming from another control or even another window.
> 
> sorry, I didn't get you. I just think I should focus too item element when I
> click its label.

Which is fine.  But you are focusing the item element when the label is clicked ONLY if !this.selected.  Shouldn't you always do it?  Like this:

if (event.originalTarget != this.control) {
  if (!this.selected)
    this.selected = true;
  this.focus();
}

> 
> > >+      <constructor>
> > >+        var select = this.selectControl;
> > >+        this.control.__defineGetter__("radioGroup", function radioGroup() {
> > >+          return select.control;
> > >+        });
> > >+      </constructor>
> > 
> > do you still need to define this getter?  I don't see that you use it anywhere.
> 
> Sorry, I forgot to put comment here. This is because xul:radio must be placed
> inside explicit content of xul:radiogroup. Therefore I redefine property of
> xul:radio and method of xul:radiogroup to use them in anonymous content. I
> agree it's a trick but I can't see more nice way.
> 

<constructor>
  var select = this.selectControl;
  this.control.__defineGetter__("radioGroup", function radioGroup() {
    return select.control;
  });
</constructor>

So what this is doing is that when the select1-full-item binding is applied to a xul xf:item, it will make sure to create a getter called radioGroup on the item control, right?  So that if someone calls item.radioGroup, it will return the anonymous content xul:radiogroup that is contained under the select1 control, right?  Which is fine to me.  But I don't see where item.radioGroup is ever used so I don't see why the function is needed.  I'm probably just missing it in the code.  Can you point out to me where that getter is called?

Comment 22

[aaronr]

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

Before you check this in, please verify that we are still processing xforms-select, xforms-deselect, and xforms-value-changed events correctly.  All xforms-deselects should fire first, then the xforms-select then the xforms-value-changed.  Looking at your code, I see a few places where this might go wrong, but I'm not sure just by looking at the code.


>Index: extensions/xforms/resources/content/selects.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects.xml
>diff -N extensions/xforms/resources/content/selects.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects.xml	6 Nov 2006 17:19:33 -0000

>+
>+  <!-- BASE for select/select1 elements. -->
>+  <binding id="selectcontrols-base"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+
>+    <implementation implements="nsIXFormsUIWidget">
>+
>+    <!-- nsIXFormsUIWidget -->
>+      <method name="refresh">
>+        <body>
>+        <![CDATA[
>+          var boundNode = this.accessors.getBoundNode();
>+          if (!boundNode)
>+            return null;
>+
>+          var valuesArray = this.getValuesArray(boundNode);
>+          var nodesArray = this.getNodesArray(boundNode);
>+
>+          this.traverseItems(this, this.selectItemElements, valuesArray,
>+                             nodesArray);
>+
>+          var outOfRange = this.isOutOfRange(valuesArray) ||
>+            this.isOutOfRange(nodesArray);
>+          if (this.outOfRange == null || this.outOfRange != outOfRange)
>+            this.accessors.setInRange(outOfRange);
>+        ]]>

shouldn't you be passing '!outOfRange' to this.accessors.setInRange?  If outOfRange is true, then shouldn't you be setting InRange to false?

>+
>+    <!-- private -->
>+      <!-- Return true if item can be selected. -->
>+      <method name="isItemAllowed">
>+        <parameter name="aItem"/>
>+        <body>
>+        <![CDATA[
>+          var boundNode = this.accessors.getBoundNode();
>+          if (!boundNode)
>+            return true;
>+
>+          aItem.QueryInterface(Components.interfaces.nsIXFormsItemElement);
>+          return !aItem.isCopyItem ||
>+            boundNode.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE;
>+        ]]>
>+        </body>
>+      </method>

Do you still need this?  I don't see anyone calling isItemAllowed anywhere.

>+      <!-- Run through item elements and build content of instance node for
>+        selected item elements.
>+
>+        @param aContextNode - select element child node that should be either
>+                              choices or itemset or item element
>+        @param aInstanceNode - new instance node
>+      -->
>+      <method name="buildInstanceData">
>+        <parameter name="aItem"/>
>+        <parameter name="aInstanceNode"/>
>+        <body>
>+        <![CDATA[
>+          if (!aItem.selected)
>+            return true;
>+
>+          var nsIDOMNode = Components.interfaces.nsIDOMNode;
>+
>+          if (aItem.isCopyItem) {
>+            aInstanceNode.appendChild(aItem.copyNode.cloneNode(true));
>+            return true;
>+          }
>+
>+          if (aInstanceNode.nodeType != nsIDOMNode.ELEMENT_NODE) {
>+            if (aInstanceNode.textContent)
>+              aInstanceNode.textContent += " ";


In updateInstanceData you set node.textContent = "", so doesn't that mean that node.textContent will exist, but just have a length of 0?  Or am I wrong?  Just wondering if this test shouldn't be aInstanceNode.textContent.length > 0

>+            aInstanceNode.textContent += aItem.value;
>+            return true;
>+          }
>+
>+          var fchild = aInstanceNode.firstChild;
>+          if (fchild && (fchild.nodeType == nsIDOMNode.TEXT_NODE ||
>+              fchild.nodeType == nsIDOMNode.CDATA_SECTION_NODE)) {
>+            fchild.nodeValue += " " + aItem.value;

what if nodeValue is empty to start with?  Won't we now have a value that starts with a space?  So instead of <boundNode>firstValue</boundNode>, we'd have <boundNode> firstValue</boundNode>.

>+      <method name="updateInstanceNodeByItem">
>+        <parameter name="aIncremental"/>
>+        <parameter name="aItem"/>
>+        <body>
>+        <![CDATA[
>+          // per http://www.w3.org/TR/xforms/slice4.html#evt-select
>+          // we send the event to the itemset if it is a parent.
>+          var parent = this.ownerDocument.getBindingParent(this);
>+          var target = parent && parent.localName == "itemset" ? parent : this;
>+
>+          var eventName = aItem.selected ? "xforms-select" : "xforms-deselect";
>+          this.dispatchXFormsNotificationEvent(eventName, target);
>+
>+          this.updateInstanceData(aIncremental);
>+        ]]>
>+        </body>
>+      </method>

Like I mentioned previously, this won't dispatch the xforms-select/xforms-deselect correctly.  Also make sure that if there were like 4 items selected and then the user clicked on just one item, that all of the deselect events fire for the deselected items before the xforms-select fires for the selected item. (http://www.w3.org/TR/xforms/index-all.html#rpm-event-seq-swvc)


>+  <!-- SELECT1: BASE -->
>+  <binding id="select1-base"
>+           extends="#selectcontrols-base">
>+

>+      <method name="updateInstanceNodeByItem">
>+        <parameter name="aIncremental"/>
>+        <parameter name="aItem"/>
>+        <body>
>+        <![CDATA[
>+          if (aItem.selected) {
>+            // per http://www.w3.org/TR/xforms/slice4.html#evt-select
>+            // we send the event to the itemset if it is a parent.
>+            var parent = this.ownerDocument.getBindingParent(this);
>+            var target = parent && parent.localName == "itemset" ? parent : this;
>+
>+            var eventName = aItem.selected ? "xforms-select" : "xforms-deselect";
>+            this.dispatchXFormsNotificationEvent("xforms-select", target);

As I mentioned before, this won't work correctly.

>+
>+            function _unselectItems(aItem, aNewSelectedItem, aTarget) {
>+              if (aItem != aNewSelectedItem) {
>+                if (aItem.selected) {
>+                  aItem.selected = false;
>+                  this.dispatchXFormsNotificationEvent("xforms-deselect",
>+                                                       aTarget);
>+                  return false;
>+                }
>+              }
>+              return true;
>+            }
>+            this.traverseItems(this, _unselectItems, aItem, target);
>+          }

Also, according to the spec -> http://www.w3.org/TR/xforms/index-all.html#rpm-event-seq-swvc, deselect fires before select.

This is as far as I got today.  I'll try to finish this up tonight.

>+  <!-- ITEMSET -->
>+  <binding id="itemset"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">

Comment 23

[aaronr]

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

>+
>+  <!-- ITEM -->
>+  <binding id="select-item-base"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-general">
>+
>+    <implementation>
>+    <!-- interface -->
>+      <property name="selected"
>+                onget="throw Error(Components.results.NS_ERROR_NOT_IMPLEMENTED);"
>+                onset="throw Error(Components.results.NS_ERROR_NOT_IMPLEMENTED);"/>
>+
>+      <property name="disabled"
>+                onget="throw Error(Components.results.NS_ERROR_NOT_IMPLEMENTED);"
>+                onset="throw Error(Components.results.NS_ERROR_NOT_IMPLEMENTED);"/>
>+
>+      <method name="focus">
>+        <body>
>+          this.control.focus();
>+        </body>
>+      </method>
>+
>+    <!-- successor's interface -->
>+      <method name="updateInstanceData">
>+        <parameter name="aIncremental"/>
>+        <body>
>+          if (this.selectControl)
>+            this.selectControl.updateInstanceNodeByItem(aIncremental, this);
>+        </body>
>+      </method>
>+
>+    <!-- private -->
>+      <property name="selectControl" readonly="true">
>+        <getter>
>+        <![CDATA[
>+          if (!this._selectControl) {
>+            var node = this;
>+            
>+            while (node = node.parentNode) {
>+              if (node.namespaceURI == this.XFORMS_NS &&
>+                  (node.localName == "select" || node.localName == "select1"))
>+                break;
>+            }
>+            if (node)
>+              this._selectControl = node;
>+          }
>+          return this._selectControl;
>+        ]]>

nit: why not just put the 'this._selectControl = node;' instruction before the 'break'?  Then you don't need to test for node.

r-'ing for the select/deselect event stuff and to hear back about my latest comments.  Most of the stuff was trivial.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > 
> > > nit: ugh, I'm not much of a fan of having a select-xhtml.xml, select.xml,
> > > selects-xhtml.xml and selects.xml.  The names are too similar.  How about
> > > renaming selects*.xml to select-full-*.xml if all they are going to contain is
> > > the implementations for the select/select1 full bindings.
> > 
> > Once I have plans to use bindings of selects.xml for xhmlt minimal select1
> > control and implement this control in selects-xhtml.xml file. What will we do
> > with names?
> 
> well, if you put minimal select1 into selects.xml and get rid of select1.xml,
> then we'll be left with select.xml and selects.xml (plus the xhtml and xul
> ones, of course).  And both select.xml and selects.xml will have both select
> and select1 implementations in them. 'select' and 'selects' are very similar
> names and the files will hold similar types of content.  That is very
> confusing.  We need to distinguish what types of bindings are in each file and
> have a filename that reflects that.  For example, if selects.xml will hold full
> and minimal and select.xml holds the compact implementations, then we can call
> them selects-compact and selects-full-minimal.  Just something descriptive so
> that at a glance we know what should live in each.

We have the next:

select.xml - base widgets for select/select1 controls that are hold native widgets.
select-xhtml.xml - contains compact select/select1 controls
select-xul.xml - contains compact select/select1 and minimal select1 for xul controls.

selects.xml - defines base widgets for select/select1 controls don't use any native widget and every item/choices element has representation.
selects-xhtml.xml - will contains full select/select1 and minimal select1 for xhtml controls
selects-xul.xml - contains full select/select1 controls.

Implementation of select/select1 controls of selects.xml files is more right (and I believe once we should get rid implementation that is used in select.xml files) and  'selects' name is more proper since this file contains implementation as for select element as well for select1 element. But again I agree that 'select' and 'selects' name are too similiar and will confuse. We can change name for 'selects' files or we can do cvs copy of 'select' files to someother one. Aaron, what way and file names would you prefer?

Comment 25

[alexander :surkov (:asurkov)]

Attachment: patch2

(In reply to comment #21)

> <constructor>
>   var select = this.selectControl;
>   this.control.__defineGetter__("radioGroup", function radioGroup() {
>     return select.control;
>   });
> </constructor>
> 
> So what this is doing is that when the select1-full-item binding is applied to
> a xul xf:item, it will make sure to create a getter called radioGroup on the
> item control, right?  So that if someone calls item.radioGroup, it will return
> the anonymous content xul:radiogroup that is contained under the select1
> control, right?  Which is fine to me.  But I don't see where item.radioGroup is
> ever used so I don't see why the function is needed.  I'm probably just missing
> it in the code.  Can you point out to me where that getter is called?
> 

This property is for xul:radio but not for item control. It is used internaly by radio binding. I removed this though and added new bindings for radio and radiogroup element. It seems to me code looks better.


(In reply to comment #22)
> (From update of attachment 244817 [details] [diff] [review] [edit])
> Before you check this in, please verify that we are still processing
> xforms-select, xforms-deselect, and xforms-value-changed events correctly.  

Some beaufours tests shows xforms-select/xforms-deselect is fired properly. But xforms-value-changed is not fired when control is bound to element node. I guess it's because setNodeContent() doesn't set proper state for nodeState objects. Looks like different bug.

> shouldn't you be passing '!outOfRange' to this.accessors.setInRange?  If
> outOfRange is true, then shouldn't you be setting InRange to false?

Right. Fixed.

> Do you still need this?  I don't see anyone calling isItemAllowed anywhere.

Right. I don't need. Fixed.

> >+          if (aInstanceNode.nodeType != nsIDOMNode.ELEMENT_NODE) {
> >+            if (aInstanceNode.textContent)
> >+              aInstanceNode.textContent += " ";
> 
> 
> In updateInstanceData you set node.textContent = "", so doesn't that mean that
> node.textContent will exist, but just have a length of 0?  Or am I wrong?  Just
> wondering if this test shouldn't be aInstanceNode.textContent.length > 0

Sorry, it's not clear for me what do node.textContent is not exist and what's bad here even it is not exist :)?

> >+          var fchild = aInstanceNode.firstChild;
> >+          if (fchild && (fchild.nodeType == nsIDOMNode.TEXT_NODE ||
> >+              fchild.nodeType == nsIDOMNode.CDATA_SECTION_NODE)) {
> >+            fchild.nodeValue += " " + aItem.value;
> 
> what if nodeValue is empty to start with?

I guess then nodeValue return empty string.

>  Won't we now have a value that
> starts with a space?  So instead of <boundNode>firstValue</boundNode>, we'd
> have <boundNode> firstValue</boundNode>.

Fixed.

> >+      <method name="updateInstanceNodeByItem">
> >+        <parameter name="aIncremental"/>
> >+        <parameter name="aItem"/>
> >+        <body>
> >+        <![CDATA[
> >+          // per http://www.w3.org/TR/xforms/slice4.html#evt-select
> >+          // we send the event to the itemset if it is a parent.
> >+          var parent = this.ownerDocument.getBindingParent(this);
> >+          var target = parent && parent.localName == "itemset" ? parent : this;
> >+
> >+          var eventName = aItem.selected ? "xforms-select" : "xforms-deselect";
> >+          this.dispatchXFormsNotificationEvent(eventName, target);
> >+
> >+          this.updateInstanceData(aIncremental);
> >+        ]]>
> >+        </body>
> >+      </method>
> 
> Like I mentioned previously, this won't dispatch the
> xforms-select/xforms-deselect correctly.  Also make sure that if there were
> like 4 items selected and then the user clicked on just one item, that all of
> the deselect events fire for the deselected items before the xforms-select
> fires for the selected item.
> (http://www.w3.org/TR/xforms/index-all.html#rpm-event-seq-swvc)

User can change selection of one item of select control in the same moment. Therefore I send events for that item user interact with.

> Also, according to the spec ->
> http://www.w3.org/TR/xforms/index-all.html#rpm-event-seq-swvc, deselect fires
> before select.

Fixed.

Comment 26

[aaronr]

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


>Index: extensions/xforms/resources/content/selects.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects.xml
>diff -N extensions/xforms/resources/content/selects.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects.xml	17 Nov 2006 12:22:35 -0000

>+      <method name="updateInstanceData">
>+        <parameter name="aIncremental"/>
>+        <body>
>+        <![CDATA[
>+          if (aIncremental != this.incremental)
>+            return;
>+
>+          var node = this.accessors.getBoundNode();
>+          if (!node)
>+            return;
>+
>+          node = node.cloneNode(false);
>+          node.textContent = "";
>+          this.traverseItems(this, this.buildInstanceData, node);
>+
>+          if (node.nodeType != Components.interfaces.nsIDOMNode.ELEMENT_NODE)
>+            this.accessors.setValue(node.textContent);
>+          else
>+            this.accessors.setContent(node, true);

UGH!  Sorry I didn't notice this before, but you can't do this.  Using setContent like this will generate a xforms-rebuild for even normal valueItem selections.  Select/Select1 item selection should only generate rebuild when a copyItem is selected.  Fixing this to use setValue in those cases will fix your xforms-value-changed problem, too.

Rest of your patch looks ok, though.

Comment 27

[aaronr]

Attachment: testcase

Here's a testcase that fails that didn't fail on 0.7.  In 0.7 it would generate the xforms-out-of-range event while it was initializing, but then it goes into range once everything is set up.  With your patch, it stays out of range until the user selects something.  Even selecting the selected item will get it back into range.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 29

[aaronr]

Comment on attachment 245928 [details] [diff] [review]
patch3


>Index: extensions/xforms/resources/content/selects.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects.xml
>diff -N extensions/xforms/resources/content/selects.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects.xml	19 Nov 2006 07:15:41 -0000

>+      <!-- Update instance node. This method is called when selection of select
>+        element is changed.
>+
>+        @param aIncremental - specifies if instance node should be updated
>+                              incrementally.
>+      -->
>+      <method name="updateInstanceData">
>+        <parameter name="aIncremental"/>
>+        <body>
>+        <![CDATA[
>+          if (aIncremental != this.incremental)
>+            return;
>+
>+          var node = this.accessors.getBoundNode();
>+          if (!node)
>+            return;
>+
>+          node = node.cloneNode(false);
>+          node.textContent = "";
>+          this.traverseItems(this, this.buildInstanceData, node);
>+
>+          var nsIDOMNode = Components.interfaces.nsIDOMNode;
>+
>+          if (node.nodeType != nsIDOMNode.ELEMENT_NODE ||
>+              node.childNodes.length == 1 &&
>+              node.firstChild.nodeType == nsIDOMNode.TEXT_NODE)
>+            this.accessors.setValue(node.textContent);
>+          else
>+            this.accessors.setContent(node, true);
>+        ]]>
>+        </body>
>+      </method>
>+

A couple of issues with this change.

1) You are only checking for text node, but you also need to check for cdata section node too, right?
2) If a user selects a copyItem, then setContent will be called correctly.  But if the user then selects/deselects regular valueItems after that, you'll still hit setContent (and thus rebuild) every time even though the copyItem selection hasn't changed.
3) still some compatibility issues.  I'll attach a new testcase.

Comment 30

[aaronr]

Attachment: testcase2

With this testcase and patch3 applied I see the following behavior:

Leave Strawberry selected and then choose Coconut.  Both should not be selected.  Then click on Mango (with Strawberry deselected).  Mango doesn't get selected AND the select1 goes out of range.  If you submit the instance data you'll notice that BOTH Coconut and Mango are submitted as well as Strawberry.

In the select testcase, you'll see that selecting/deselecting Mango or Coconut will cause xforms-rebuilds.  They are both valueItems, so should never cause that event.

After you fix the issues, please verify the submitted data for your selections, too.  Select1's should only ever have one item submitted but it is easy to end up with a valueItem AND a copyItem both submitted if things aren't just right.

Comment 31

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 32

[aaronr]

Attachment: testcase2

fixed testcase to use proper submission element in second part of the testcase.

Comment 33

[aaronr]

Comment on attachment 246149 [details] [diff] [review]
patch4


>Index: extensions/xforms/resources/content/selects.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects.xml
>diff -N extensions/xforms/resources/content/selects.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects.xml	21 Nov 2006 14:45:53 -0000

>+
>+    <!-- private -->
>+      <!-- Run through xforms:item elements and select them if needed. If
>+        xforms:item element contains xforms:value element value of which is
>+        matched with item of given values array or if it contains xforms:copy
>+        element value of which is matched with item of given nodes array then
>+        that xforms:item element will be selected.
>+        The method is used in conjunction with traverseItems method.
>+
>+        @param aItem - current traversed item element
>+      -->
>+      <method name="selectItemElements">
>+        <parameter name="aItem"/>
>+        <body>
>+        <![CDATA[
>+          aItem.disabled = this.accessors.isReadonly();
>+
>+          if (aItem.isCopyItem) {
>+            if (!this.nodes || !aItem.copyNode) {
>+              // aNodes is empty only if select controls is bound to non element
>+              // node. We just disable item element since it's value can't be
>+              // saved in bound node.
>+              aItem.disabled = true;
>+              return true;
>+            }
>+
>+            for (var i = 0; i < this.nodes.length; i++) {
>+              if (aItem.isCopyNode(this.nodes[i].node)) {
>+                aItem.selected = true;
>+                this.nodes[i].isused = true;
>+                return true;
>+              }
>+            }

The spec says that if an copyItem is selected and the select/select1 isn't bound to an element node that we should dispatch a xforms-binding-exception event.  That should probably happen even if the item is disabled.

...

>+
>+      <!-- Run through item elements and build content of instance node for
>+        selected item elements.
>+
>+        @param aItem - currently traversed item element
>+        @param aInstanceNode - new generated instance node
>+        @param aForceRebuild - object, its 'value' property should have value
>+                               'true' if copy item element was been selected or
>+                               deselected.
>+      -->
>+      <method name="buildInstanceData">
>+        <parameter name="aItem"/>
>+        <parameter name="aInstanceNode"/>
>+        <parameter name="aForceRebuild"/>
>+        <body>
>+        <![CDATA[
>+          if (aItem.isCopyItem) {
>+            var wasSelected = false;
>+            for (var i = 0; i < this.nodes.length; i++) {
>+              if (aItem.isCopyNode(this.nodes[i].node)) {
>+                wasSelected = true;
>+                break;
>+              }
>+            }
>+            if (wasSelected != aItem.selected)
>+              aForceRebuild.value = true;
>+
>+            if (aItem.selected)
>+              aInstanceNode.appendChild(aItem.copyNode.cloneNode(true));

what if aInstanceNode isn't an element node?  If we already know that aInstanceNode MUST be an element node (because we've already tested for it before), you should probably put a comment here saying why you know that this is safe.  For example, 'if aItem.selected is true, we've already tested to make sure that aInstanceNode is an element node.  So we can append child without further testing'.

...

>+      <!-- Cycally traverse item elements for specified direction and invokes
>+        given function for every item element.
>+
>+        @param aItemNode - start item element
>+        @param aUpDirection - if true then then item elements are traversed in
>+                              up direction
>+        @param aFunc, aFuncArg1, aFuncArg2 - invoked function and its arguments
>+        @param aCurrNode - current traversed node, used internally
>+      -->
>+      <method name="cycleTraverseItems">
>+        <parameter name="aItemNode"/>
>+        <parameter name="aUpDirection"/>
>+        <parameter name="aFunc"/>
>+        <parameter name="aFuncArg1"/>
>+        <parameter name="aFuncArg2"/>
>+        <parameter name="aCurrNode"/>
>+        <body>
>+          if (aCurrNode == aItemNode)
>+            return false;
>+
>+          if (!aCurrNode) {
>+            if (aItemNode.namespaceURI != this.XFORMS_NS ||
>+                aItemNode.localName != "item")
>+              return false;
>+            aCurrNode = aItemNode;
>+          }
>+
>+          var next = aUpDirection ? aCurrNode.previousSibling :
>+            aCurrNode.nextSibling;
>+          if (next) {
>+            var res = this.traverseItems(next, aFunc, aFuncArg1, aFuncArg2,
>+                                         aUpDirection);
>+            if (!res)
>+              return false;
>+
>+            return this.cycleTraverseItems(aItemNode, aUpDirection, aFunc,
>+                                           aFuncArg1, aFuncArg2, next);
>+          }
>+
>+          var parent = aCurrNode;
>+          while (parent = parent.parentNode) {
>+            if (parent.namespaceURI == this.XFORMS_NS) {
>+              switch (parent.localName) {
>+                case "choices": case "itemset":
>+                  next = parent;
>+                  break;
>+                case "select": case "select1":
>+                  next = aUpDirection ? parent.lastChild : parent.firstChild;
>+                  break;
>+              }
>+              if (next)
>+                return this.cycleTraverseItems(aItemNode, aUpDirection, aFunc,
>+                                               aFuncArg1, aFuncArg2, next);
>+            }
>+          }
>+
>+          return true;
>+        </body>
>+      </method>
>+
>+      <!-- Storage for previous value of 'in-range' state of select element -->
>+      <field name="outOfRange">null</field>
>+
>+      <!-- Arrays of values and nodes what are builded from instance node. -->

nit: comment should probably be something like 'Arrays of selected values and nodes that combine to build the content of the bound instance node'

I found a couple more testcases that don't work when I tested how copyItems work now when the select/select1 are not bound to an element node.  Probably simple to fix.  I'd also like to see that if you are going to disable the checkbox/radiobutton for copyItems (if the select/select1 bound node isn't an element), that the item text also take on the disabled styling.  Right now the item text is still regular black text.  But you can do that under another bug if you want.  Just make sure you put the bug number in this bug.

r-'ing for these two new testcases to be fixed.

Comment 34

[aaronr]

Attachment: testcase3

This testcase has an empty bound node to start with, so that means control should be out-of-range, but it isn't.  Also when you select a valueItem, it should be stored in the bound node and it doesn't if the bound node is an attribute node.

Comment 35

[aaronr]

(In reply to comment #33)
> (From update of attachment 246149 [details] [diff] [review] [edit])
> 
> >Index: extensions/xforms/resources/content/selects.xml
> >===================================================================
> >RCS file: extensions/xforms/resources/content/selects.xml
> >diff -N extensions/xforms/resources/content/selects.xml
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> >+++ extensions/xforms/resources/content/selects.xml	21 Nov 2006 14:45:53 -0000
> 
> >+
> >+    <!-- private -->
> >+      <!-- Run through xforms:item elements and select them if needed. If
> >+        xforms:item element contains xforms:value element value of which is
> >+        matched with item of given values array or if it contains xforms:copy
> >+        element value of which is matched with item of given nodes array then
> >+        that xforms:item element will be selected.
> >+        The method is used in conjunction with traverseItems method.
> >+
> >+        @param aItem - current traversed item element
> >+      -->
> >+      <method name="selectItemElements">
> >+        <parameter name="aItem"/>
> >+        <body>
> >+        <![CDATA[
> >+          aItem.disabled = this.accessors.isReadonly();
> >+
> >+          if (aItem.isCopyItem) {
> >+            if (!this.nodes || !aItem.copyNode) {
> >+              // aNodes is empty only if select controls is bound to non element
> >+              // node. We just disable item element since it's value can't be
> >+              // saved in bound node.
> >+              aItem.disabled = true;
> >+              return true;
> >+            }
> >+
> >+            for (var i = 0; i < this.nodes.length; i++) {
> >+              if (aItem.isCopyNode(this.nodes[i].node)) {
> >+                aItem.selected = true;
> >+                this.nodes[i].isused = true;
> >+                return true;
> >+              }
> >+            }
> 
> The spec says that if an copyItem is selected and the select/select1 isn't
> bound to an element node that we should dispatch a xforms-binding-exception
> event.  That should probably happen even if the item is disabled.
> 

Hmmm, on second thought, I don't know if my comment makes any sense.  Because if the item is disabled, I guess you can argue that the user selecting the item never happened.  But maybe add a comment here saying that if the item is ever NOT disabled, that a binding exception should occur here.

So just fix the bound attribute node testcase and I think that'll be it.

Comment 36

[aaronr]

(In reply to comment #24)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > 
> > > > nit: ugh, I'm not much of a fan of having a select-xhtml.xml, select.xml,
> > > > selects-xhtml.xml and selects.xml.  The names are too similar.  How about
> > > > renaming selects*.xml to select-full-*.xml if all they are going to contain is
> > > > the implementations for the select/select1 full bindings.
> > > 
> > > Once I have plans to use bindings of selects.xml for xhmlt minimal select1
> > > control and implement this control in selects-xhtml.xml file. What will we do
> > > with names?
> > 
> > well, if you put minimal select1 into selects.xml and get rid of select1.xml,
> > then we'll be left with select.xml and selects.xml (plus the xhtml and xul
> > ones, of course).  And both select.xml and selects.xml will have both select
> > and select1 implementations in them. 'select' and 'selects' are very similar
> > names and the files will hold similar types of content.  That is very
> > confusing.  We need to distinguish what types of bindings are in each file and
> > have a filename that reflects that.  For example, if selects.xml will hold full
> > and minimal and select.xml holds the compact implementations, then we can call
> > them selects-compact and selects-full-minimal.  Just something descriptive so
> > that at a glance we know what should live in each.
> 
> We have the next:
> 
> select.xml - base widgets for select/select1 controls that are hold native
> widgets.
> select-xhtml.xml - contains compact select/select1 controls
> select-xul.xml - contains compact select/select1 and minimal select1 for xul
> controls.
> 
> selects.xml - defines base widgets for select/select1 controls don't use any
> native widget and every item/choices element has representation.
> selects-xhtml.xml - will contains full select/select1 and minimal select1 for
> xhtml controls
> selects-xul.xml - contains full select/select1 controls.
> 
> Implementation of select/select1 controls of selects.xml files is more right
> (and I believe once we should get rid implementation that is used in select.xml
> files) and  'selects' name is more proper since this file contains
> implementation as for select element as well for select1 element. But again I
> agree that 'select' and 'selects' name are too similiar and will confuse. We
> can change name for 'selects' files or we can do cvs copy of 'select' files to
> someother one. Aaron, what way and file names would you prefer?
> 

I can't really say I've come up with a better idea.  Especially since the reorg doesn't sound like it is done.  If you plan to re-write the selects in select.xml and the select1 in select1.xml, then in the end I'd like to see what we have now...select.xml and select1.xml and perhaps a selectsbase.xml.  We should probably end up with all of the UI content bindings in select-*.xml or select1-*.xml  And I'd hate to lose select.xml or select1.xml...not just because of the names, but also because of all the nice cvs history we have for them.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> Created an attachment (id=246217) [edit]
> testcase3
> 
> This testcase has an empty bound node to start with, so that means control
> should be out-of-range, but it isn't.  Also when you select a valueItem, it
> should be stored in the bound node and it doesn't if the bound node is an
> attribute node.
> 

It means if bound node is empty then control should be in out-of-range state. Right? I can't get why. Specs says: "Dispatched in response to: the value of an instance data node has changed such that the value can not be represented by the form control." I guess select control can represent empty value, it is if no one item is selected. What did I miss?

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #36)

> I can't really say I've come up with a better idea.  Especially since the reorg
> doesn't sound like it is done.  If you plan to re-write the selects in
> select.xml and the select1 in select1.xml, then in the end I'd like to see what
> we have now...select.xml and select1.xml and perhaps a selectsbase.xml.  We
> should probably end up with all of the UI content bindings in select-*.xml or
> select1-*.xml  And I'd hate to lose select.xml or select1.xml...not just
> because of the names, but also because of all the nice cvs history we have for
> them.
> 

I like this but it's hard to do since I don't know even approximately when I can get rid implementation of select.xml files because I don't know how to do it.

We won't lose cvs history, we should do cvs copy. In that case cvs history is saved.

Comment 39

[aaronr]

(In reply to comment #37)
> (In reply to comment #34)
> > Created an attachment (id=246217) [edit]
> > testcase3
> > 
> > This testcase has an empty bound node to start with, so that means control
> > should be out-of-range, but it isn't.  Also when you select a valueItem, it
> > should be stored in the bound node and it doesn't if the bound node is an
> > attribute node.
> > 
> 
> It means if bound node is empty then control should be in out-of-range state.
> Right? I can't get why. Specs says: "Dispatched in response to: the value of an
> instance data node has changed such that the value can not be represented by
> the form control." I guess select control can represent empty value, it is if
> no one item is selected. What did I miss?
> 

Depends on how you look at it.  Is a bound node that contains an empty string value telling the select/select1 to select nothing or telling the select/select1 to select the item with the empty-string value?  I would argue the latter because if the select DID have an item with an empty string value, we'd select it in that case, right?  So in the case of the bound node holding an empty string and there being no item with an empty string value, then the item we are looking for isn't there and thus the control is out of range.

Novell and formsPlayer both generate a xforms-out-of-range for the testcase.  XSmiles does not.  But XSmiles also selects 'vanilla' in the testcase which also isn't correct, so I don't know that they have this scenario completely working, yet.

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch5

1) changed comment for this.values and this.nodes fields
2) added comment why we don't fire 'xforms-binding-exception' event for copy items
3) empty value suff

Comment 41

[alexander :surkov (:asurkov)]

Attachment: patch6

testcase3 looks like working

Comment 42

[alexander :surkov (:asurkov)]

Attachment: patch7

Comment 43

[aaronr]

Comment on attachment 246328 [details] [diff] [review]
patch7

>Index: extensions/xforms/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/jar.mn,v
>retrieving revision 1.22
>diff -u -8 -p -r1.22 jar.mn
>--- extensions/xforms/jar.mn	25 Sep 2006 09:59:13 -0000	1.22
>+++ extensions/xforms/jar.mn	22 Nov 2006 21:03:39 -0000
>@@ -21,16 +21,19 @@ xforms.jar:
>   content/xforms/input-xhtml.xml               (resources/content/input-xhtml.xml)
>   content/xforms/input-xul.xml                 (resources/content/input-xul.xml)
>   content/xforms/select1.xml                   (resources/content/select1.xml)
>   content/xforms/range.xml                     (resources/content/range.xml)
>   content/xforms/range-xhtml.xml               (resources/content/range-xhtml.xml)
>   content/xforms/select.xml                    (resources/content/select.xml)
>   content/xforms/select-xhtml.xml              (resources/content/select-xhtml.xml)
>   content/xforms/select-xul.xml                (resources/content/select-xul.xml)
>+  content/xforms/selects.xml                   (resources/content/selects.xml)
>+  content/xforms/selects-xhtml.xml             (resources/content/selects-xhtml.xml)
>+  content/xforms/selects-xul.xml               (resources/content/selects-xul.xml)
>   content/xforms/bindingex.css                 (resources/content/bindingex.css)
>   content/xforms/bindingex.xul                 (resources/content/bindingex.xul)
>   content/xforms/calendar.png                  (resources/content/calendar.png)
> * locale/en-US/xforms/contents.rdf             (resources/locale/en-US/contents.rdf)
>   locale/en-US/xforms/xforms.properties        (resources/locale/en-US/xforms.properties)
>   locale/en-US/xforms/xforms.dtd               (resources/locale/en-US/xforms.dtd)
> * skin/xforms/contents.rdf                     (resources/skin/contents.rdf)
>   skin/xforms/widgets-xhtml.css                (resources/skin/widgets-xhtml.css)
>Index: extensions/xforms/nsIXFormsItemElement.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsItemElement.idl,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 nsIXFormsItemElement.idl
>--- extensions/xforms/nsIXFormsItemElement.idl	17 Nov 2005 22:00:26 -0000	1.2
>+++ extensions/xforms/nsIXFormsItemElement.idl	22 Nov 2006 21:03:39 -0000
>@@ -78,9 +78,19 @@ interface nsIXFormsItemElement : nsISupp
>    */
>   attribute boolean isCopyItem;
> 
>   /*
>    * returns the node that the contained copy element is bound to
>    */
>   readonly attribute nsIDOMNode copyNode;
> 
>+  /**
>+   * Compare given DOM node and copy node (i.e. node that the contained copy
>+   * element is bound to.
>+   *
>+   * @param nsIDOMNode aNode - given DOM node to compare with copy node
>+   *
>+   * @return boolean - return true if given node is copy node
>+   */
>+  boolean isCopyNode(in nsIDOMNode aNode);
> };
>+
>Index: extensions/xforms/nsXFormsItemElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemElement.cpp,v
>retrieving revision 1.17
>diff -u -8 -p -r1.17 nsXFormsItemElement.cpp
>--- extensions/xforms/nsXFormsItemElement.cpp	8 Oct 2006 14:15:01 -0000	1.17
>+++ extensions/xforms/nsXFormsItemElement.cpp	22 Nov 2006 21:03:40 -0000
>@@ -482,16 +482,34 @@ nsXFormsItemElement::GetIsCopyItem(PRBoo
> 
> NS_IMETHODIMP
> nsXFormsItemElement::SetIsCopyItem(PRBool aIsCopyItem)
> {
>   mIsCopyItem = aIsCopyItem;
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsItemElement::IsCopyNode(nsIDOMNode *aNode, PRBool *aIsCopyNode)
>+{
>+  NS_ENSURE_ARG(aNode);
>+  NS_ENSURE_ARG_POINTER(aIsCopyNode);
>+
>+  *aIsCopyNode = PR_FALSE;
>+
>+  nsCOMPtr<nsIDOMNode> selectedNode;
>+  nsresult rv = SelectItemByNode(aNode, getter_AddRefs(selectedNode));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (selectedNode)
>+    *aIsCopyNode = PR_TRUE;
>+
>+  return NS_OK;
>+}
>+
> NS_HIDDEN_(nsresult)
> NS_NewXFormsItemElement(nsIXTFElement **aResult)
> {
>   *aResult = new nsXFormsItemElement();
>   if (!*aResult)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   NS_ADDREF(*aResult);
>Index: extensions/xforms/resources/content/select-xhtml.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select-xhtml.xml,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 select-xhtml.xml
>--- extensions/xforms/resources/content/select-xhtml.xml	27 Oct 2006 10:49:09 -0000	1.7
>+++ extensions/xforms/resources/content/select-xhtml.xml	22 Nov 2006 21:03:40 -0000
>@@ -49,78 +49,51 @@
>           xmlns:html="http://www.w3.org/1999/xhtml"
>           xmlns:xbl="http://www.mozilla.org/xbl"
>           xmlns:xforms="http://www.w3.org/2002/xforms">
> 
> 
> <!-- SELECT/SELECT1 CONTROLS
>   The section contains xforms select/select1 controls implementations for XHTML.
>   All controls are inherited from interface bindings realized in select.xml.
>+
>+  XXX: select of minimal appearance is not implemented (see bug 332928).
>+
>+  select1 of minimal appearance is implemented in select1.xml file.
>+  select/select1 of appearance="full" is implemented in selects-xhtml.xml file.
> -->
> 
>   <!-- SELECT APPEARANCE='COMPACT' : <DEFAULT> -->
>   <binding id="xformswidget-select-compact"
>            extends="chrome://xforms/content/select.xml#xformswidget-select-base">
>     <content>
>       <html:label>
>         <html:span class="label-container">
>            <children includes="label"/>
>         </html:span>
>         <html:span anonid="control" xbl:inherits="style, accesskey"/>
>         <children/>
>       </html:label>
>     </content>
>   </binding>
> 
>-  <!-- SELECT APPEARANCE='FULL' -->
>-  <binding id="xformswidget-select-full"
>-           extends="chrome://xforms/content/select.xml#xformswidget-select-base">
>-    <content>
>-      <html:table xbl:inherits="style">
>-        <html:tr>
>-          <html:td valign="top"><children includes="label"/></html:td>
>-          <html:td>
>-            <html:span anonid="control" xbl:inherits="style, accesskey"/>
>-          </html:td>
>-        </html:tr>
>-      </html:table>
>-      <children/>
>-    </content>
>-  </binding>
>-
>   <!-- SELECT1 APPEARANCE='COMPACT' : <DEFAULT> -->
>   <binding id="xformswidget-select1-compact"
>            extends="chrome://xforms/content/select.xml#xformswidget-select1-base">
>     <content>
>       <html:label>
>         <html:span class="label-container">
>            <children includes="label"/>
>         </html:span>
>         <html:span anonid="control" xbl:inherits="style, accesskey"/>
>         <children/>
>       </html:label>
>     </content>
>   </binding>
> 
>-  <!-- SELECT1 APPEARANCE='FULL' -->
>-  <binding id="xformswidget-select1-full"
>-           extends="chrome://xforms/content/select.xml#xformswidget-select1-base">
>-    <content>
>-      <html:table xbl:inherits="style">
>-        <html:tr>
>-          <html:td valign="top"><children includes="label"/></html:td>
>-          <html:td>
>-            <html:span anonid="control" xbl:inherits="style, accesskey"/>
>-          </html:td>
>-        </html:tr>
>-      </html:table>
>-      <children/>
>-    </content>
>-  </binding>
>-
> 
> <!-- CONTROL WIDGETS FOR SELECT/SELECT1 CONTROLS
>   All control widgets are underlying controls for select/select1 and serve to
>   realize functionality needed for select/select1 work in XHTML context. Thease
>   widgets are inherited from 'controlwidget-base' binding and implement the
>   interface what base widget for xforms select/select1 controls ask for. You can
>   find interface description in 'select.xml' file.
> -->
>@@ -224,245 +197,27 @@
>       </handler>
> 
>       <!--
>         XXX: since xforms:label can include arbitrary elements then 'focus',
>           'blur' and 'change' events should be listen from 'xhtml:select'
>           element only.
>       -->
>     </handlers>
>-
>-  </binding>
>-
>-
>-  <!-- BASE CONTROL WIDGET FOR SELECT/SELECT1 APPEARANCE='FULL' -->
>-  <binding id="controlwidget-selects-full-base"
>-           extends="chrome://xforms/content/select.xml#controlwidget-nonativewidget-base">
>-
>-    <implementation>
>-      <method name="removeAllItems">
>-        <body>
>-          for (var i = this.control.childNodes.length; i > 0; i--) {
>-           this.control.removeChild(this.control.childNodes[i-1]);
>-          }
>-        </body>
>-      </method>
>-
>-      <method name="appendItem">
>-        <parameter name="aLabel"/>
>-        <parameter name="aValue"/>
>-        <parameter name="aGroup"/>
>-        <body>
>-          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>-        </body>
>-      </method>
>-
>-      <method name="appendGroup">
>-        <parameter name="aLabel"/>
>-        <parameter name="aGroup"/>
>-        <body>
>-          var mainDiv = document.createElementNS(this.XHTML_NS, "div");
>-
>-          var labelDiv = document.createElementNS(this.XHTML_NS, "div");
>-          if (aLabel) {
>-            labelDiv.className = "select-choice-label";
>-            labelDiv.appendChild(aLabel.cloneNode(true));
>-          }
>-          mainDiv.appendChild(labelDiv);
>-
>-          var contentDiv = document.createElementNS(this.XHTML_NS, "div");
>-          contentDiv.className = "select-choice-content";
>-          mainDiv.appendChild(contentDiv);
>-
>-          if (aGroup) {
>-            aGroup.appendChild(mainDiv);
>-          } else {
>-            this.control.appendChild(mainDiv);
>-          }
>-
>-          return contentDiv;
>-        </body>
>-      </method>
>-
>-      <method name="addItemToSelection">
>-        <parameter name="aItem"/>
>-        <body>
>-          aItem.firstChild.checked = true;
>-        </body>
>-      </method>
>-
>-      <method name="removeItemFromSelection">
>-        <parameter name="aItem"/>
>-        <body>
>-          aItem.firstChild.checked = false;
>-        </body>
>-      </method>
>-
>-      <method name="isItemSelected">
>-        <parameter name="aItem"/>
>-        <body>
>-          return aItem.firstChild.checked;
>-        </body>
>-      </method>
>-
>-      <method name="advanceFocusToItem">
>-        <parameter name="aItem"/>
>-        <body>
>-          aItem.firstChild.focus();
>-        </body>
>-      </method>
>-
>-      <method name="isItemElement">
>-        <parameter name="aElement"/>
>-        <body>
>-          return aElement.getAttribute("anonid") == "wcontrol";
>-        </body>
>-      </method>
>-    </implementation>
>-
>-    <handlers>
>-      <handler event="change">
>-        if (event.originalTarget.getAttribute("anonid") == "wcontrol")
>-          this.updateInstanceData(true);
>-      </handler>
>-
>-      <handler event="blur" phase="capturing">
>-        if (event.originalTarget.getAttribute("anonid") == "wcontrol")
>-          this.updateInstanceData(false);
>-      </handler>
>-
>-      <!--
>-        XXX: need to send DOMFocusIn/DOMFocusOut events
>-        XXX: when incremental='false' then model data should be updated on 
>-        'blur' event for xforms:select only
>-      -->
>-    </handlers>
>-  </binding>
>-
>-
>-  <!-- CONTROL WIDGET FOR SELECT APPEARANCE='FULL' -->
>-  <binding id="controlwidget-select-full"
>-           extends="#controlwidget-selects-full-base">
>-
>-    <content>
>-      <html:div anonid="control"/>
>-    </content>
>-
>-    <implementation>
>-      <method name="appendItem">
>-        <parameter name="aLabel"/>
>-        <parameter name="aValue"/>
>-        <parameter name="aGroup"/>
>-        <body>
>-          var div = document.createElementNS(this.XHTML_NS, "div");
>-          div.setAttribute("anonid", "itemcontainer");
>-
>-          var input = document.createElementNS(this.XHTML_NS, "input");
>-          input.setAttribute("type", "checkbox");
>-          input.setAttribute("value", aValue);
>-          input.setAttribute("anonid", "wcontrol");
>-          if (this.readonly)
>-            input.setAttribute("disabled", "true");
>-
>-          div.appendChild(input);
>-          if (aLabel) {
>-            div.appendChild(aLabel.cloneNode(true));
>-          }
>-
>-          if (aGroup) {
>-            aGroup.appendChild(div);
>-          } else {
>-            this.control.appendChild(div);
>-          }
>-          return div;
>-        </body>
>-      </method>
>-    </implementation>
>-
>-    <handlers>
>-      <handler event="click">
>-        if (event.originalTarget.getAttribute("anonid") == "itemcontainer") {
>-          event.originalTarget.firstChild.checked =
>-            !event.originalTarget.firstChild.checked;
>-          this.updateInstanceData(true);
>-        }
>-      </handler>
>-    </handlers>
>   </binding>
> 
> 
> <!-- CONTROL WIDGETS FOR SELECT1 CONTROLS
>   The section contains underlying widgets implementations needed for select1
>   controls.
> -->
> 
>-
>-  <!-- CONTROL WIDGET FOR SELECT1 APPEARANCE='MINIMAL'
>-    Now select1 appearance='minimal' control is implemented in select1.xml
>-    as separate widget.
>-  -->
>-
>-
>   <!-- CONTROL WIDGET FOR SELECT1 APPEARANCE='COMPACT' -->
>   <binding id="controlwidget-select1-compact"
>            extends="#controlwidget-select-compact">
>     <content>
>       <html:select xbl:inherits="style, accesskey, disabled=readonly"
>                    class="xf-value" size="5" anonid="control"/>
>     </content>
>   </binding>
> 
>-
>-  <!-- CONTROL WIDGET FOR SELECT1 APPEARANCE='FULL' -->
>-  <binding id="controlwidget-select1-full"
>-           extends="#controlwidget-selects-full-base">
>-
>-    <content>
>-      <html:form anonid="control"/>
>-    </content>
>-
>-    <implementation>
>-      <method name="appendItem">
>-        <parameter name="aLabel"/>
>-        <parameter name="aValue"/>
>-        <parameter name="aGroup"/>
>-        <body>
>-          var div = document.createElementNS(this.XHTML_NS, "div");
>-          div.setAttribute("anonid", "itemcontainer");
>-
>-          var input = document.createElementNS(this.XHTML_NS, "input");
>-          input.setAttribute("type", "radio");
>-          input.setAttribute("value", aValue);
>-          input.setAttribute("name", "radioControlForXFormsSelect1");
>-          input.setAttribute("anonid", "wcontrol");
>-          if (this.readonly)
>-            input.setAttribute('disabled', 'true');
>-          div.appendChild(input);
>-
>-          if (aLabel)
>-            div.appendChild(aLabel.cloneNode(true));
>-
>-          if (aGroup) {
>-            aGroup.appendChild(div);
>-          } else {
>-            this.control.appendChild(div);
>-          }
>-
>-          return div;
>-        </body>
>-      </method>
>-    </implementation>
>-
>-    <handlers>
>-      <handler event="click">
>-      <![CDATA[
>-        if (event.originalTarget.getAttribute("anonid") == "itemcontainer" &&
>-            !event.originalTarget.firstChild.checked) {
>-          event.originalTarget.firstChild.checked = true;
>-          this.updateInstanceData(true);
>-        }
>-      ]]>
>-      </handler>
>-    </handlers>
>-  </binding>
>-
> </bindings>
> 
>Index: extensions/xforms/resources/content/select-xul.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select-xul.xml,v
>retrieving revision 1.6
>diff -u -8 -p -r1.6 select-xul.xml
>--- extensions/xforms/resources/content/select-xul.xml	6 Nov 2006 02:50:37 -0000	1.6
>+++ extensions/xforms/resources/content/select-xul.xml	22 Nov 2006 21:03:40 -0000
>@@ -49,16 +49,20 @@
>           xmlns:xforms="http://www.w3.org/2002/xforms"
>           xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> 
> 
> <!-- SELECT, SELECT1 CONTROLS
>   The section contains xforms select and select1 controls implementations for
>   XUL. All controls are inherited from interface bindings realized in
>   select.xml file.
>+
>+  XXX: Element select of minimal appearance is not implemented (see bug 332928).
>+
>+  select/select1 of full appearance are implemented in selects-xul.xml file.
> -->
> 
>   <binding id="xformswidget-select"
>            extends="chrome://xforms/content/select.xml#xformswidget-select-base">
>     <content>
>       <xul:hbox flex="1">
>         <children includes="label"/>
>         <xul:box anonid="control" xbl:inherits="style, accesskey"
>@@ -85,41 +89,29 @@
>     then we should call nsIXFormsDelegate.widgetAttached() method. If we'll have
>     separate binding for each appearance attribute value then it will be happen
>     automatically.
>   -->
> 
>   <binding id="xformswidget-select-compact"
>            extends="#xformswidget-select"/>
> 
>-  <binding id="xformswidget-select-full"
>-           extends="#xformswidget-select"/>
>-
>   <binding id="xformswidget-select1-minimal"
>            extends="#xformswidget-select1"/>
> 
>   <binding id="xformswidget-select1-compact"
>            extends="#xformswidget-select1"/>
> 
>-  <binding id="xformswidget-select1-full"
>-           extends="#xformswidget-select1"/>
>-
> <!-- CONTROL WIDGETS FOR SELECT CONTROLS
>   The section contains underlying widgets implementations needed for xforms
>   select controls. All underlying widgets implement the interface what base
>   widget for xforms select controls ask for. You can find interface description
>   in 'select.xml' file.
> -->
> 
>-
>-  <!-- CONTROL WIDGET FOR SELECT APPEARANCE='MINIMAL'
>-    XXX: Widget is not implemented.
>-  -->
>-
>-
>   <!-- CONTROL WIDGET FOR SELECT APPEARANCE='COMPACT' -->
>   <binding id="controlwidget-select-compact"
>            extends="chrome://xforms/content/select.xml#controlwidget-base">
> 
>     <content>
>       <xul:listbox xbl:inherits="style, accesskey, disabled=readonly"
>                    rows="5" flex="1"
>                    seltype="multiple"
>@@ -236,160 +228,16 @@
>         XXX: Since xforms:label can include arbitrary elements then 'focus',
>           'blur' and 'command' events should be listen from 'xul:listbox'
>           element only.
>       -->
>     </handlers>
>   </binding>
> 
> 
>-  <!-- CONTROL WIDGET FOR SELECT APPEARANCE='FULL' -->
>-  <binding id="controlwidget-select-full"
>-           extends="chrome://xforms/content/select.xml#controlwidget-nonativewidget-base">
>-
>-    <content>
>-      <xul:vbox xbl:inherits="orient" anonid="control"/>
>-    </content>
>-
>-    <implementation>
>-      <method name="removeAllItems">
>-        <body>
>-          for (var i = this.control.childNodes.length; i > 0; i--) {
>-            this.control.removeChild(this.control.childNodes[i-1]);
>-          }
>-        </body>
>-      </method>
>-
>-      <method name="appendItem">
>-        <parameter name="aLabel"/>
>-        <parameter name="aValue"/>
>-        <parameter name="aGroup"/>
>-        <body>
>-          var box = this.ownerDocument.createElementNS(this.XUL_NS, "hbox");
>-          box.setAttribute("align", "center");
>-
>-          var checkbox = this.ownerDocument.createElementNS(this.XUL_NS, "checkbox");
>-          checkbox.setAttribute("value", aValue);
>-          checkbox.setAttribute("anonid", "wcontrol");
>-          if (this.readonly)
>-            checkbox.setAttribute("disabled", "true");
>-
>-          box.appendChild(checkbox);
>-          if (aLabel) {
>-            // since label node can contains textnodes then we use
>-            // 'description' element as container for label node.
>-            var description = this.ownerDocument.
>-              createElementNS(this.XUL_NS, "description");
>-            description.setAttribute("anonid", "labelcontainer");
>-            description.appendChild(aLabel.cloneNode(true));
>-            box.appendChild(description);
>-          }
>-
>-          if (aGroup) {
>-            aGroup.appendChild(box);
>-          } else {
>-            this.control.appendChild(box);
>-          }
>-
>-          return box;
>-        </body>
>-      </method>
>-
>-      <method name="appendGroup">
>-        <parameter name="aLabel"/>
>-        <parameter name="aGroup"/>
>-        <body>
>-          var mainBox = this.ownerDocument.createElementNS(this.XUL_NS, "vbox");
>-
>-          var labelBox = this.ownerDocument.createElementNS(this.XUL_NS, "hbox");
>-          if (aLabel) {
>-            labelBox.className = "select-choice-label";
>-            labelBox.appendChild(aLabel.cloneNode(true));
>-          }
>-          mainBox.appendChild(labelBox);
>-
>-          var contentBox = this.ownerDocument.createElementNS(this.XUL_NS, "vbox");
>-          contentBox.className = "select-choice-content";
>-          mainBox.appendChild(contentBox);
>-
>-          if (aGroup) {
>-            aGroup.appendChild(mainBox);
>-          } else {
>-            this.control.appendChild(mainBox);
>-          }
>-
>-          return contentBox;
>-        </body>
>-      </method>
>-
>-      <method name="addItemToSelection">
>-        <parameter name="aItem"/>
>-        <body>
>-          aItem.firstChild.checked = true;
>-        </body>
>-      </method>
>-
>-      <method name="removeItemFromSelection">
>-        <parameter name="aItem"/>
>-        <body>
>-          aItem.firstChild.checked = false;
>-        </body>
>-      </method>
>-
>-      <method name="isItemSelected">
>-        <parameter name="aItem"/>
>-        <body>
>-          return aItem.firstChild.checked;
>-        </body>
>-      </method>
>-
>-      <method name="advanceFocusToItem">
>-        <parameter name="aItem"/>
>-        <body>
>-          aItem.firstChild.focus();
>-        </body>
>-      </method>
>-
>-      <method name="isItemElement">
>-        <parameter name="aElement"/>
>-        <body>
>-          return aElement.getAttribute("anonid") == "wcontrol";
>-        </body>
>-      </method>
>-    </implementation>
>-
>-    <handlers>
>-      <handler event="click">
>-        if (event.originalTarget.getAttribute("anonid") == "labelcontainer") {
>-          event.originalTarget.previousSibling.checked =
>-            !event.originalTarget.previousSibling.checked;
>-          this.updateInstanceData(true);
>-        }
>-      </handler>
>-
>-      <handler event="command">
>-        if (event.originalTarget.getAttribute("anonid") == "wcontrol")
>-          this.updateInstanceData(true);
>-      </handler>
>-
>-      <handler event="blur" phase="capturing">
>-        if (event.originalTarget.getAttribute("anonid") == "wcontrol")
>-          this.updateInstanceData(false);
>-      </handler>
>-
>-      <!--
>-        XXX: We need to send DOMFocusIn/DOMFocusOut events
>-        XXX: When incremental='false' then model data should be updated on 
>-          'blur' event for xforms:select only
>-      -->
>-    </handlers>
>-
>-  </binding>
>-
>-
> <!-- CONTROL WIDGETS FOR SELECT1 CONTROLS
>   The section contains underlying widgets implementations needed for xforms
>   select1 controls. All underlying widgets implement the interface what base
>   widget for xforms select1 controls ask for. You can find interface description
>   in 'select.xml' file.
> -->
> 
>   <!-- CONTROL WIDGET FOR SELECT1 APPEARANCE='MINIMAL' -->
>@@ -536,145 +384,9 @@
>   <binding id="controlwidget-select1-compact"
>            extends="#controlwidget-select-compact">
>     <content>
>       <xul:listbox xbl:inherits="style, accesskey, disabled=readonly"
>                    rows="5" anonid="control"/>
>     </content>
>   </binding>
> 
>-
>-  <!-- CONTROL WIDGET FOR SELECT1 APPEARANCE='FULL' -->
>-  <binding id="controlwidget-select1-full"
>-           extends="chrome://xforms/content/select.xml#controlwidget-base">
>-
>-    <content>
>-      <xul:radiogroup anonid="control"/>
>-    </content>
>-
>-    <implementation>
>-      <method name="removeAllItems">
>-        <body>
>-          for (var i = this.control.childNodes.length; i > 0; i--) {
>-             this.control.removeChild(this.control.childNodes[i-1]);
>-          }
>-        </body>
>-      </method>
>-
>-      <method name="appendItem">
>-        <parameter name="aLabel"/>
>-        <parameter name="aValue"/>
>-        <parameter name="aGroup"/>
>-        <body>
>-          var box = this.ownerDocument.createElementNS(this.XUL_NS, "hbox");
>-          box.setAttribute("align", "center");
>-
>-          var radio = this.ownerDocument.createElementNS(this.XUL_NS, "radio");
>-          radio.setAttribute("value", aValue);
>-          radio.setAttribute("anonid", "wcontrol");
>-          if (this.readonly)
>-            radio.setAttribute("disabled", "true");
>-
>-          box.appendChild(radio);
>-          if (aLabel) {
>-            // since label node can contains textnodes then we use
>-            // 'description' element as container for label node.
>-            var description = this.ownerDocument.
>-              createElementNS(this.XUL_NS, "description");
>-            description.setAttribute("anonid", "labelcontainer");
>-            description.appendChild(aLabel.cloneNode(true));
>-            box.appendChild(description);
>-          }
>-
>-          if (aGroup) {
>-            aGroup.appendChild(box);
>-          } else {
>-            this.control.appendChild(box);
>-          }
>-
>-          return box;
>-        </body>
>-      </method>
>-
>-      <method name="appendGroup">
>-        <parameter name="aLabel"/>
>-        <parameter name="aGroup"/>
>-        <body>
>-          var mainBox = this.ownerDocument.createElementNS(this.XUL_NS, "vbox");
>-
>-          var labelBox = this.ownerDocument.createElementNS(this.XUL_NS, "hbox");
>-          if (aLabel) {
>-            labelBox.className = "select-choice-label";
>-            labelBox.appendChild(aLabel.cloneNode(true));
>-          }
>-          mainBox.appendChild(labelBox);
>-
>-          var contentBox = this.ownerDocument.createElementNS(this.XUL_NS, "vbox");
>-          contentBox.className = "select-choice-content";
>-          mainBox.appendChild(contentBox);
>-
>-          if (aGroup) {
>-            aGroup.appendChild(mainBox);
>-          } else {
>-            this.control.appendChild(mainBox);
>-          }
>-
>-          return contentBox;
>-        </body>
>-      </method>
>-
>-      <method name="addItemToSelection">
>-        <parameter name="aItem"/>
>-        <body>
>-          // there are a cases when 'radio' binding isn't created yet, therefore
>-          // we use setTimeout.
>-          window.setTimeout(
>-            function(list, item) {
>-              list.selectedItem = item;
>-            },
>-            0,
>-            this.control, aItem.firstChild
>-          );
>-        </body>
>-      </method>
>-
>-      <method name="removeItemFromSelection">
>-        <parameter name="aItem"/>
>-        <body>
>-          if (aItem.firstChild.selected)
>-            this.control.selectedItem = null;
>-        </body>
>-      </method>
>-
>-      <method name="isItemSelected">
>-        <parameter name="aItem"/>
>-        <body>
>-          return aItem.firstChild.selected;
>-        </body>
>-      </method>
>-    </implementation>
>-
>-    <handlers>
>-      <handler event="click">
>-        if (event.originalTarget.getAttribute("anonid") == "labelcontainer") {
>-          this.control.selectedItem = event.originalTarget.previousSibling;
>-          this.updateInstanceData(true);
>-        }
>-      </handler>
>-
>-      <handler event="command">
>-        if (event.originalTarget.getAttribute("anonid") == "wcontrol")
>-          this.updateInstanceData(true);
>-      </handler>
>-
>-      <handler event="blur" phase="capturing">
>-        if (event.originalTarget.getAttribute("anonid") == "control")
>-          this.updateInstanceData(false);
>-      </handler>
>-
>-      <!--
>-        XXX: need to send DOMFocusIn/DOMFocusOut events
>-        XXX: when incremental='false' then model data should be updated on
>-          'blur' event for xforms:select1 only
>-      -->
>-    </handlers>
>-  </binding>
> </bindings>
>Index: extensions/xforms/resources/content/select.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select.xml,v
>retrieving revision 1.30
>diff -u -8 -p -r1.30 select.xml
>--- extensions/xforms/resources/content/select.xml	6 Nov 2006 02:50:37 -0000	1.30
>+++ extensions/xforms/resources/content/select.xml	22 Nov 2006 21:03:40 -0000
>@@ -34,18 +34,22 @@
>    - use your version of this file under the terms of the MPL, indicate your
>    - decision by deleting the provisions above and replace them with the notice
>    - and other provisions required by the GPL or the LGPL. If you do not delete
>    - the provisions above, a recipient may use your version of this file under
>    - the terms of any one of the MPL, the GPL or the LGPL.
>    -
>    - ***** END LICENSE BLOCK ***** -->
> 
>-<!--
>+<bindings id="xformsSelectBindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:html="http://www.w3.org/1999/xhtml"
>+          xmlns:xbl="http://www.mozilla.org/xbl">
> 
>+<!--
>   This file implements the "abstract" UI class for XForms select controls. It
>   have "pure virtual" functions that it expect to be implemented by concrete
>   application and returned in the getElementControl() call. An example is the
>   controls for XHTML in select-xhtml.xml.
> 
>   The "abstact" UI class parses the child nodes of the xforms:select and then
>   constructs the anonymous content programmatically.
> 
>@@ -80,21 +84,16 @@
> 
>     allowFreeEntry(allowed) - enable/disable free entry
>       @param allowed - boolean value
> 
>     appendFreeEntryItem(value) - append and select free entry item
>       @param value - item value
> -->
> 
>-<bindings id="xformsSelectBindings"
>-          xmlns="http://www.mozilla.org/xbl"
>-          xmlns:html="http://www.w3.org/1999/xhtml"
>-          xmlns:xbl="http://www.mozilla.org/xbl">
>-
>   <!-- BASE for select/select1 elements. -->
>   <binding id="xformswidget-selectcontrols-base"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-base">
> 
>     <implementation implements="nsIXFormsUIWidget, nsIXFormsNSEditableElement">
> 
>       <property name="editor" readonly="true"
>                 onget="return this.control.editor;"/>
>@@ -866,25 +865,23 @@
>           ]]>
>         </body>
>       </method>
> 
>       <method name="dispatchSelectEvent">
>         <parameter name="aElement"/>
>         <parameter name="aName"/>
>         <body>
>-          var ev = document.createEvent("Events");
>-          ev.initEvent(aName, true, false);
>-          var elm = aElement;
>+        <![CDATA[
>           // per http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#evt-select
>           // we send the event to the itemset if it is a parent.
>-          if (elm.parentNode.localName == "itemset")
>-            elm = elm.parentNode;
>-          elm.dispatchEvent(ev);
>-          return true;
>+          var parent = this.ownerDocument.getBindingParent(aElement);
>+          var elm = parent && parent.localName == "itemset" ? parent : aElement;
>+          return this.dispatchXFormsNotificationEvent(aName, elm);
>+        ]]>
>         </body>
>       </method>
> 
>       <!-- Array of objects that contains objects for all items of this control,
>         serves to keep xforms items and native items together.
>         Object has next properties:
>           control - xforms:item element
>           option - native widget for xforms:item
>@@ -1217,66 +1214,9 @@
>           this.allowFreeEntry(true);
>         else
>           this.allowFreeEntry(false);
>       </constructor>
> 
>     </implementation>
>   </binding>
> 
>-
>-  <!-- The 'controlwidget-nonativewidget-base' is the base binding for underlying
>-    controls which represents the xforms control. This binding is supposed to
>-    be used for underlying control that actually haven't native widget. For
>-    example, underlying control for select @appearance='full' is represented by
>-    checkboxes bundle.
>-    This binding implements the 'focus' method to advance focus to the
>-    underlying control.
>-
>-    The widget assumes successors bindings have additional methods:
>-
>-      advanceFocusToItem(aItem) - makes item focused
>-        @param aItem - item element
>-
>-      isItemElement(aElement) - returns true if element is item
>-        @param aElement - DOMElement
>-  -->
>-  <binding id="controlwidget-nonativewidget-base"
>-           extends="#controlwidget-base">
>-
>-    <implementation>
>-      <!-- Advance focus to native widget. -->
>-      <method name="focus">
>-        <body>
>-        <![CDATA[
>-          var item = this.currentItem;
>-          if (!item) {
>-            var options = this.parentControl._controlArray;
>-            // If there are selected items then advance focus to first of them
>-            for (var i = 0; i < options.length; i++) {
>-              item = options[i].option;
>-              var selected = this.isItemSelected(item);
>-              if (selected)
>-                break;
>-            }
>-            // otherwise we focus first item.
>-            if (options.length && i == options.length)
>-              item = options[0].option;
>-          }
>-
>-          if (item)
>-            this.advanceFocusToItem(item);
>-        ]]>
>-        </body>
>-      </method>
>-
>-      <field name="currentItem">null</field>
>-    </implementation>
>-
>-    <handlers>
>-      <handler event="focus" phase="capturing">
>-        if (this.isItemElement(event.originalTarget))
>-          this.currentItem = event.originalTarget;
>-      </handler>
>-    </handlers>
>-  </binding>
>-
> </bindings>
>Index: extensions/xforms/resources/content/select1.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select1.xml,v
>retrieving revision 1.27
>diff -u -8 -p -r1.27 select1.xml
>--- extensions/xforms/resources/content/select1.xml	6 Nov 2006 02:50:37 -0000	1.27
>+++ extensions/xforms/resources/content/select1.xml	22 Nov 2006 21:03:40 -0000
>@@ -1138,40 +1138,16 @@
>             if (this._selected)
>               this._selected.setActive(true);
>           }
>         }
>       </handler>
>     </handlers>
>   </binding>
> 
>-  <binding id="xformswidget-itemset"
>-           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>-    <content>
>-      <html:div style="display:none;">
>-        <children/>
>-      </html:div>
>-      <html:div anonid="insertion"/>
>-    </content>
>-
>-    <implementation implements="nsIXFormsItemSetUIElement">
>-      <field name="_anonymousItemSetContent">null</field>
>-
>-      <property name="anonymousItemSetContent" readonly="true">
>-        <getter>
>-          if (!this._anonymousItemSetContent) {
>-            this._anonymousItemSetContent =
>-              document.getAnonymousElementByAttribute(this, "anonid", "insertion");
>-          }
>-          return this._anonymousItemSetContent;
>-        </getter>
>-      </property>
>-    </implementation>
>-  </binding>
>-
> 
>   <!-- The binding for <item> is needed only because of the
>        scrollIntoView method. -->
>   <binding id="xformswidget-select1-item">
>     <content>
>       <html:div anonid="content">
>         <children/>
>       </html:div>
>Index: extensions/xforms/resources/content/selects-xhtml.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects-xhtml.xml
>diff -N extensions/xforms/resources/content/selects-xhtml.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects-xhtml.xml	22 Nov 2006 21:03:40 -0000
>@@ -0,0 +1,177 @@
>+<?xml version="1.0"?>
>+
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+   -
>+   - The contents of this file are subject to the Mozilla Public License Version
>+   - 1.1 (the "License"); you may not use this file except in compliance with
>+   - the License. You may obtain a copy of the License at
>+   - http://www.mozilla.org/MPL/
>+   -
>+   - Software distributed under the License is distributed on an "AS IS" basis,
>+   - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+   - for the specific language governing rights and limitations under the
>+   - License.
>+   -
>+   - The Original Code is Mozilla XForms support.
>+   -
>+   - The Initial Developer of the Original Code is
>+   - Mozilla Foundation.
>+   - Portions created by the Initial Developer are Copyright (C) 2006
>+   - the Initial Developer. All Rights Reserved.
>+   -
>+   - Contributor(s):
>+   -  Alexander Surkov <[e-mail]> (original author)
>+   -
>+   - Alternatively, the contents of this file may be used under the terms of
>+   - either the GNU General Public License Version 2 or later (the "GPL"), or
>+   - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+   - in which case the provisions of the GPL or the LGPL are applicable instead
>+   - of those above. If you wish to allow use of your version of this file only
>+   - under the terms of either the GPL or the LGPL, and not to allow others to
>+   - use your version of this file under the terms of the MPL, indicate your
>+   - decision by deleting the provisions above and replace them with the notice
>+   - and other provisions required by the GPL or the LGPL. If you do not delete
>+   - the provisions above, a recipient may use your version of this file under
>+   - the terms of any one of the MPL, the GPL or the LGPL.
>+   -
>+   - ***** END LICENSE BLOCK ***** -->
>+
>+<bindings id="xformsSelectBindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:html="http://www.w3.org/1999/xhtml"
>+          xmlns:xbl="http://www.mozilla.org/xbl">
>+
>+  <!-- SELECT: APPEARANCE FULL -->
>+  <binding id="select-full"
>+           extends="chrome://xforms/content/selects.xml#select-base">
>+    <content>
>+      <children includes="label"/>
>+      <html:div class="xf-value">
>+        <children/>
>+      </html:div>
>+    </content>
>+  </binding>
>+
>+  <!-- SELECT1: APPEARANCE FULL -->
>+  <binding id="select1-full"
>+           extends="chrome://xforms/content/selects.xml#select1-base">
>+    <content>
>+      <children includes="label"/>
>+      <html:div class="xf-value">
>+        <children/>
>+      </html:div>
>+    </content>
>+
>+    <handlers>
>+      <handler event="keypress" keycode="VK_UP" preventdefault="true">
>+        var item = this.selectedItem;
>+        if (item) {
>+          function selectPrevItem(aItem, aSelect) {
>+            aSelect.selectedItem = aItem;
>+            return false;
>+          }
>+          this.cycleTraverseItems(item, true, selectPrevItem, this);
>+        }
>+      </handler>
>+
>+      <handler event="keypress" keycode="VK_DOWN" preventdefault="true">
>+        var item = this.selectedItem;
>+        if (item) {
>+          function selectNextItem(aItem, aSelect) {
>+            aSelect.selectedItem = aItem;
>+            return false;
>+          }
>+          this.cycleTraverseItems(item, false, selectNextItem, this);
>+        }
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+
>+  <!-- ITEM of SELECT APPEARANCE FULL -->
>+  <binding id="select-full-item-base"
>+           extends="chrome://xforms/content/selects.xml#select-item-base">
>+
>+    <implementation>
>+      <property name="selected"
>+                onget="return this.control.checked;"
>+                onset="this.control.checked = val;"/>
>+
>+      <property name="disabled"
>+                onget="return this.control.disabled"
>+                onset="this.control.disabled = val;"/>
>+
>+      <method name="getControlElement">
>+        <body>
>+          return this.ownerDocument.
>+            getAnonymousElementByAttribute(this, "anonid", "control");
>+        </body>
>+      </method>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="blur" phase="capturing">
>+        this.updateInstanceData(false);
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+
>+  <!-- ITEM of SELECT APPEARANCE FULL -->
>+  <binding id="select-full-item"
>+           extends="#select-full-item-base">
>+    <content>
>+      <html:input type="checkbox" anonid="control"/>
>+      <children/>
>+    </content>
>+
>+    <handlers>
>+      <handler event="click">
>+        if (this.disabled)
>+          return;
>+
>+        if (event.originalTarget != this.control) {
>+          // Select/unselect checkbox that is representation of xforms item
>+          // element if user clicks xforms label element.
>+          this.selected = !this.selected;
>+          this.focus();
>+        }
>+
>+        this.updateInstanceData(true);
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+
>+  <!-- ITEM of SELECT1 APPEARANCE FULL -->
>+  <binding id="select1-full-item"
>+           extends="#select-full-item-base">
>+    <content>
>+      <html:input type="radio" anonid="control"/>
>+      <children/>
>+    </content>
>+
>+    <handlers>
>+      <handler event="click">
>+      <![CDATA[
>+        if (this.disabled)
>+          return;
>+
>+        if (event.originalTarget != this.control) {
>+          if (!this.selected) {
>+            // Select radio that is representation of xforms item element if
>+            // user clicks xforms label element.
>+            this.selected = true;
>+          }
>+          this.focus();
>+        }
>+
>+        this.updateInstanceData(true);
>+      ]]>
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+</bindings>
>+
>Index: extensions/xforms/resources/content/selects-xul.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects-xul.xml
>diff -N extensions/xforms/resources/content/selects-xul.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects-xul.xml	22 Nov 2006 21:03:40 -0000
>@@ -0,0 +1,233 @@
>+<?xml version="1.0"?>
>+
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+   -
>+   - The contents of this file are subject to the Mozilla Public License Version
>+   - 1.1 (the "License"); you may not use this file except in compliance with
>+   - the License. You may obtain a copy of the License at
>+   - http://www.mozilla.org/MPL/
>+   -
>+   - Software distributed under the License is distributed on an "AS IS" basis,
>+   - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+   - for the specific language governing rights and limitations under the
>+   - License.
>+   -
>+   - The Original Code is Mozilla XForms support.
>+   -
>+   - The Initial Developer of the Original Code is
>+   - Mozilla Foundation.
>+   - Portions created by the Initial Developer are Copyright (C) 2006
>+   - the Initial Developer. All Rights Reserved.
>+   -
>+   - Contributor(s):
>+   -  Alexander Surkov <[e-mail]> (original author)
>+   -
>+   - Alternatively, the contents of this file may be used under the terms of
>+   - either the GNU General Public License Version 2 or later (the "GPL"), or
>+   - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+   - in which case the provisions of the GPL or the LGPL are applicable instead
>+   - of those above. If you wish to allow use of your version of this file only
>+   - under the terms of either the GPL or the LGPL, and not to allow others to
>+   - use your version of this file under the terms of the MPL, indicate your
>+   - decision by deleting the provisions above and replace them with the notice
>+   - and other provisions required by the GPL or the LGPL. If you do not delete
>+   - the provisions above, a recipient may use your version of this file under
>+   - the terms of any one of the MPL, the GPL or the LGPL.
>+   -
>+   - ***** END LICENSE BLOCK ***** -->
>+
>+<bindings id="xformsSelectBindings"
>+          xmlns="http://www.mozilla.org/xbl"
>+          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+          xmlns:xbl="http://www.mozilla.org/xbl">
>+
>+  <!-- SELECT: APPEARANCE FULL -->
>+  <binding id="select-full"
>+           extends="chrome://xforms/content/selects.xml#select-base">
>+    <content>
>+      <children includes="label"/>
>+      <xul:vbox class="xf-value">
>+        <children/>
>+      </xul:vbox>
>+    </content>
>+  </binding>
>+
>+
>+  <!-- RADIOGROUP used as representation of SELECT1 element.
>+    XUL radiogroup element searches radio elements in explicit content. This
>+    binding redefines '_getRadioChildren' method to return correct list of
>+    radio elements that are used for representation of xforms item elements.
>+  -->
>+  <binding id="select1-full-radiogroup"
>+           extends="chrome://global/content/bindings/radio.xml#radiogroup">
>+
>+    <implementation>
>+      <method name="_getRadioChildren">
>+        <body>
>+          var select = this.ownerDocument.getBindingParent(this);
>+
>+          if (this.mRadioChildren)
>+            return this.mRadioChildren;
>+
>+          var radioChildren = [];
>+          function buildRadioChildren(aItem, aArray) {
>+            aArray.push(aItem.control);
>+            return true;
>+          }
>+          select.traverseItems(select, buildRadioChildren, radioChildren);
>+          return this.mRadioChildren = radioChildren;
>+        </body>
>+      </method>
>+    </implementation>
>+  </binding>
>+
>+  <!-- SELECT1: APPEARANCE FULL -->
>+  <binding id="select1-full"
>+           extends="chrome://xforms/content/selects.xml#select1-base">
>+    <content>
>+      <children includes="label"/>
>+      <xul:radiogroup flex="1" class="xf-value" anonid="control">
>+        <children/>
>+      </xul:radiogroup>
>+    </content>
>+
>+    <implementation>
>+      <method name="getControlElement">
>+        <body>
>+          return this.ownerDocument.
>+            getAnonymousElementByAttribute(this, "anonid", "control");
>+        </body>
>+      </method>
>+    </implementation>
>+  </binding>
>+
>+
>+  <!-- ITEM of SELECT/SELECT1 APPEARANCE FULL -->
>+  <binding id="select-full-item-base"
>+           extends="chrome://xforms/content/selects.xml#select-item-base">
>+
>+    <implementation>
>+      <property name="disabled"
>+                onget="return this.control.disabled"
>+                onset="this.control.disabled = val;"/>
>+
>+      <method name="getControlElement">
>+        <body>
>+          return this.ownerDocument.
>+            getAnonymousElementByAttribute(this, "anonid", "control");
>+        </body>
>+      </method>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="command">
>+        this.updateInstanceData(true);
>+      </handler>
>+
>+      <handler event="blur" phase="capturing">
>+        this.updateInstanceData(false);
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+
>+  <!-- ITEM of SELECT APPEARANCE FULL -->
>+  <binding id="select-full-item"
>+           extends="#select-full-item-base">
>+    <content>
>+      <xul:checkbox anonid="control"/>
>+      <children/>
>+    </content>
>+
>+    <implementation>
>+      <property name="selected"
>+                onget="return this.control.checked;"
>+                onset="this.control.checked = val;"/>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="click">
>+        if (this.disabled)
>+          return;
>+
>+        if (event.originalTarget != this.control) {
>+          // Select/unselect checkbox that is representation of xforms item
>+          // element if user clicks xforms label element.
>+          this.selected = !this.selected;
>+          this.updateInstanceData(true);
>+        }
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+
>+  <!-- XUL RADIO used as representation for ITEM of SELECT1 APPEARANCE FULL.
>+    Since original XUL radio element searches radiogroup element in explicit
>+    content and isn't work propertly if it failed to find radiogroup then
>+    this binding redefines 'radioGroup' proeprty to return radiogroup for
>+    radio element.
>+  -->
>+  <binding id="select1-full-item-radio"
>+           extends="chrome://global/skin/globalBindings.xml#radio">
>+
>+    <implementation>
>+      <property name="radioGroup" readonly="true">
>+        <getter>
>+          if (!this._radioGroup) {
>+            var item = this.ownerDocument.getBindingParent(this);
>+            return item.selectControl.control;
>+          }
>+          return this._radioGroup;
>+        </getter>
>+      </property>
>+      <field name="_radioGroup">null</field>
>+    </implementation>
>+  </binding>
>+
>+  <!-- ITEM of SELECT1 APPEARANCE FULL -->
>+  <binding id="select1-full-item"
>+           extends="#select-full-item-base">
>+    <content>
>+      <xul:hbox>
>+        <xul:radio anonid="control"/>
>+        <children/>
>+      </xul:hbox>
>+    </content>
>+
>+    <implementation>
>+      <property name="selected">
>+        <getter>
>+          return this.control.selected;
>+        </getter>
>+        <setter>
>+          if (this.control.selected != val) {
>+            var item = val ? this.control : null;
>+            this.selectControl.control.selectedItem = item;
>+          }
>+        </setter>
>+      </property>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="click">
>+      <![CDATA[
>+        if (this.disabled)
>+          return;
>+
>+        if (event.originalTarget != this.control) {
>+          // Select radio element that is representation of item element if
>+          // user clicks xforms label element.
>+          if (!this.selected)
>+            this.selected = true;
>+
>+          this.focus();
>+          this.updateInstanceData(true);
>+        }
>+      ]]>
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+</bindings>
>+
>Index: extensions/xforms/resources/content/selects.xml
>===================================================================
>RCS file: extensions/xforms/resources/content/selects.xml
>diff -N extensions/xforms/resources/content/selects.xml
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ extensions/xforms/resources/content/selects.xml	22 Nov 2006 21:03:40 -0000

>+
>+  <!-- BASE for select/select1 elements. -->
>+  <binding id="selectcontrols-base"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+
>+    <implementation implements="nsIXFormsUIWidget">
>+
>+    <!-- nsIXFormsUIWidget -->
>+      <method name="refresh">
>+        <body>
>+        <![CDATA[
>+          var boundNode = this.accessors.getBoundNode();
>+          if (!boundNode)
>+            return null;
>+
>+          this.nodes = this.getNodesArray(boundNode);
>+          this.values = this.getValuesArray(boundNode);

Looks like getValuesArray depends on getNodesArray being called first.  You definitely need a comment here to make sure that the order doesn't get changed.

>+
>+          this.traverseItems(this, this.selectItemElements);
>+
>+          var outOfRange = this.isOutOfRange(this.values) ||
>+            this.isOutOfRange(this.nodes);
>+          if (this.outOfRange == null || this.outOfRange != outOfRange) {
>+            this.accessors.setInRange(!outOfRange);
>+            this.outOfRange = outOfRange;
>+          }
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <method name="focus">
>+        <body>
>+          function focusItem(aItem) {
>+            if (!aItem.disabled)
>+              aItem.focus();
>+            return aItem.disabled;
>+          }
>+          this.traverseItems(this, focusItem);
>+        </body>
>+      </method>
>+
>+      <property name="incremental">
>+        <getter>
>+          return this.getAttribute("incremental") != "false";
>+        </getter>
>+        <setter>
>+          if (!val)
>+            this.setAttribute("incremental", "false");
>+          else
>+            this.removeAttribute("incremental");
>+        </setter>
>+      </property>
>+
>+    <!-- private -->
>+      <!-- Run through xforms:item elements and select them if needed. If
>+        xforms:item element contains xforms:value element value of which is
>+        matched with item of given values array or if it contains xforms:copy
>+        element value of which is matched with item of given nodes array then
>+        that xforms:item element will be selected.
>+        The method is used in conjunction with traverseItems method.
>+
>+        @param aItem - current traversed item element
>+      -->
>+      <method name="selectItemElements">
>+        <parameter name="aItem"/>
>+        <body>
>+        <![CDATA[
>+          aItem.disabled = this.accessors.isReadonly();
>+
>+          if (aItem.isCopyItem) {
>+            if (!this.nodes || !aItem.copyNode) {
>+              // aNodes is empty only if select controls is bound to non element
>+              // node. We just disable item element since it's value can't be
>+              // saved in bound node.
>+              aItem.disabled = true;
>+              return true;
>+            }
>+
>+            for (var i = 0; i < this.nodes.length; i++) {
>+              if (aItem.isCopyNode(this.nodes[i].node)) {
>+                aItem.selected = true;
>+                this.nodes[i].isused = true;
>+                return true;
>+              }
>+            }
>+          } else {
>+            for (var i = 0; i < this.values.length; i++) {
>+              if (this.values[i].value == aItem.value) {
>+                aItem.selected = true;
>+                this.values[i].isused = true;
>+                return true;
>+              }
>+            }
>+          }
>+
>+          if (aItem.selected)
>+            aItem.selected = false;
>+
>+          return true;
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <!-- Return array of values each of them item elemnent should be selected
>+        for. -->
>+      <method name="getValuesArray">
>+        <parameter name="aNode"/>
>+        <body>
>+        <![CDATA[
>+          var nsIDOMNode = Components.interfaces.nsIDOMNode;
>+          var whiteSpaceExpr = /\n|\t|\r/g;
>+
>+          var array = [];
>+          if (aNode.nodeType != nsIDOMNode.ELEMENT_NODE) {
>+            if (/\S/.test(aNode.nodeValue)) {
>+              var values = this.getValuesArrayFor(aNode.nodeValue,
>+                                                  whiteSpaceExpr);
>+
>+              for (var i = 0; i < values.length; i++)
>+                array.push({value: values[i], isused: false});
>+            } else {
>+              array.push({value: "", isused: false});
>+            }
>+            return array;
>+          }
>+
>+          if (!aNode.hasChildNodes()) {
>+            array.push({value: "", isused: false});
>+            return array;
>+          }
>+
>+          for (var child = aNode.firstChild; child; child = child.nextSibling) {
>+            if ((child.nodeType == nsIDOMNode.TEXT_NODE ||
>+                child.nodeType == nsIDOMNode.CDATA_SECTION_NODE) &&
>+                child.nodeValue) {
>+              if (/\S/.test(child.nodeValue)) {
>+                var values = this.getValuesArrayFor(child.nodeValue,
>+                                                    whiteSpaceExpr)
>+
>+                for (var i = 0; i < values.length; i++)
>+                  array.push({value: values[i], isused: false});
>+              } else if (!array.length && !this.nodes) {
>+                array.push({value: "", isused: false});
>+              }

for this 'else if' test to work correctly (particularly the !this.nodes part of this test), the code really assumes that getNodesArray was just called and that this.nodes is as up to date as it can be.  So you really need to add a comment inside this method saying that this method assumes that getNodesArray was just called to make this.nodes up to date prior to this method getting called.  Or combine getNodesArray and getValuesArray into one method to ensure that this is the case.

I can't say that I have a strong opinion either way.  So if you either add the comments mentioned above or combine getNodesArray with getValuesArray, r=me.

Comment 44

[alexander :surkov (:asurkov)]

Attachment: patch8

1) comment that getNodesArray should be called before getValuesArray added
2) labels of disabled items are grayed

Comment 45

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

Comment on attachment 246385 [details] [diff] [review]
patch8

I'll test and review this tomorrow.

Comment 46

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

Comment on attachment 246385 [details] [diff] [review]
patch8

>Index: extensions/xforms/nsIXFormsItemElement.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsItemElement.idl,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 nsIXFormsItemElement.idl
>--- extensions/xforms/nsIXFormsItemElement.idl	17 Nov 2005 22:00:26 -0000	1.2
>+++ extensions/xforms/nsIXFormsItemElement.idl	23 Nov 2006 11:56:01 -0000
>@@ -78,9 +78,19 @@ interface nsIXFormsItemElement : nsISupp

You're modifying an interface, please update the uuid

>    */
>   attribute boolean isCopyItem;
> 
>   /*
>    * returns the node that the contained copy element is bound to
>    */
>   readonly attribute nsIDOMNode copyNode;
> 
>+  /**
>+   * Compare given DOM node and copy node (i.e. node that the contained copy
>+   * element is bound to.
>+   *
>+   * @param nsIDOMNode aNode - given DOM node to compare with copy node
>+   *
>+   * @return boolean - return true if given node is copy node
>+   */
>+  boolean isCopyNode(in nsIDOMNode aNode);
> };

Why is this needed? What is the difference between
itemElement.copyNode == someOtherNode
and 
itemElement.isCopyNode(someOtherNode)
According to comments those should be the same, IMO.

Perhaps the method should be called something else?
copyNodeEquals(in nsIDOMNode aNode)? Or something like that. Not quite sure.
Depends on what it does :)


>+<bindings id="xformsSelectBindings"

you have several these. Perhaps better to use different id for different bindings.

>+                this.nodes[i].isused = true;

I'd have .isUsed, not .isused


Will re-review the new patch. (This is so big patch that it needs few re-reads ;) )

Comment 47

[alexander :surkov (:asurkov)]

Attachment: patch9

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #46)

> >+   * Compare given DOM node and copy node (i.e. node that the contained copy
> >+   * element is bound to.
> >+   *
> >+   * @param nsIDOMNode aNode - given DOM node to compare with copy node
> >+   *
> >+   * @return boolean - return true if given node is copy node
> >+   */
> >+  boolean isCopyNode(in nsIDOMNode aNode);
> > };
> 
> Why is this needed? What is the difference between
> itemElement.copyNode == someOtherNode
> and 
> itemElement.isCopyNode(someOtherNode)
> According to comments those should be the same, IMO.

No, it's not the same because copy node and child node of instance node are different. Method compares nodes by content by nsXFormsUtils::AreNodesEqual. How should I change comment to avoid likewise questions? :)

> Perhaps the method should be called something else?
> copyNodeEquals(in nsIDOMNode aNode)? Or something like that. Not quite sure.
> Depends on what it does :)

I like copyNodeEquals :)

Comment 49

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

Comment on attachment 246914 [details] [diff] [review]
patch9


> 
>+  /**
>+   * Compare given DOM node and copy node (i.e. node that the contained copy
>+   * element is bound to.
>+   *
>+   * @param nsIDOMNode aNode - given DOM node to compare with copy node
>+   *
>+   * @return boolean - return true if given node is copy node
>+   */
>+  boolean copyNodeEquals(in nsIDOMNode aNode);
> };
>+

Update the @return comment too to match the new method name

> 
>+NS_IMETHODIMP
>+nsXFormsItemElement::CopyNodeEquals(nsIDOMNode *aNode, PRBool *aIsCopyNode)
>+{
>+  NS_ENSURE_ARG(aNode);
>+  NS_ENSURE_ARG_POINTER(aIsCopyNode);
>+
>+  *aIsCopyNode = PR_FALSE;
>+
>+  nsCOMPtr<nsIDOMNode> selectedNode;
>+  nsresult rv = SelectItemByNode(aNode, getter_AddRefs(selectedNode));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (selectedNode)
>+    *aIsCopyNode = PR_TRUE;
>+
>+  return NS_OK;
>+}

Nit,
I'd write this something like:
  NS_ENSURE_ARG(aNode);
  NS_ENSURE_ARG_POINTER(aIsCopyNode);
  nsCOMPtr<nsIDOMNode> selectedNode;
  nsresult rv = SelectItemByNode(aNode, getter_AddRefs(selectedNode));
  *aIsCopyNode = !!(selectedNode);
  return rv;




>+      <!-- Return array of copy nodes each of them item element should be
>+        selected for. -->
>+      <method name="getNodesArray">
>+        <parameter name="aNode"/>
>+        <body>
>+          var nsIDOMNode = Components.interfaces.nsIDOMNode;
>+          if (aNode.nodeType != nsIDOMNode.ELEMENT_NODE)
>+            return;

Should this return null or empty array in this case?



>+          var parent = aCurrNode;
>+          while (parent = parent.parentNode) {
>+            if (parent.namespaceURI == this.XFORMS_NS) {
>+              switch (parent.localName) {
>+                case "choices": case "itemset":

Please put |case|s to separate lines. Also elsewhere in the patch.


>+      <method name="addItemToSelection">
>+        <parameter name="aItem"/>
>+        <body>
>+          if (aItem)
>+            aItem.selected = true;
>+        </body>
>+      </method>
>+
>+      <method name="removeItemFromSelection">
>+        <parameter name="aItem"/>
>+        <body>
>+          if (aItem)
>+            aItem.selected = false;
>+        </body>
>+      </method>

You didn't add these methods, but the method names are strange.
I think there should be something like markItemSelected(aItem, aSelected) method.
Fix either in this bug or in a new bug, if you agree that the names are a bit weird.


>+      <method name="isOutOfRange">
>+        <parameter name="aHitArray"/>
>+        <body>
>+        <![CDATA[
>+          if (!aHitArray)
>+            return false;
>+
>+          for (var i = 0; i < aHitArray.length && aHitArray[i].isUsed; i++);
>+          return i != aHitArray.length;

Nit, I know this works in JS, but I'd still prefer defining the |var i = 0;| outside |for(;;);|
And ++i, not i++ ;)

With those r=me
(But some more testing for this might be good. The patch is quite large :) )

Comment 50

[alexander :surkov (:asurkov)]

Attachment: patch10

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #49)
> 
> >+      <!-- Return array of copy nodes each of them item element should be
> >+        selected for. -->
> >+      <method name="getNodesArray">
> >+        <parameter name="aNode"/>
> >+        <body>
> >+          var nsIDOMNode = Components.interfaces.nsIDOMNode;
> >+          if (aNode.nodeType != nsIDOMNode.ELEMENT_NODE)
> >+            return;
> 
> Should this return null or empty array in this case?
> 
Yes, null. I added additional comment for method.

> >+      <method name="addItemToSelection">
> >+        <parameter name="aItem"/>
> >+        <body>
> >+          if (aItem)
> >+            aItem.selected = true;
> >+        </body>
> >+      </method>
> >+
> >+      <method name="removeItemFromSelection">
> >+        <parameter name="aItem"/>
> >+        <body>
> >+          if (aItem)
> >+            aItem.selected = false;
> >+        </body>
> >+      </method>
> 
> You didn't add these methods, but the method names are strange.
> I think there should be something like markItemSelected(aItem, aSelected)
> method.
> Fix either in this bug or in a new bug, if you agree that the names are a bit
> weird.

I open to new names. I don't think the current are weird because I got accustomed to them :). I stole these names from nsIDOMXULMultiSelectControlElement interface.

> 
> >+      <method name="isOutOfRange">
> >+        <parameter name="aHitArray"/>
> >+        <body>
> >+        <![CDATA[
> >+          if (!aHitArray)
> >+            return false;
> >+
> >+          for (var i = 0; i < aHitArray.length && aHitArray[i].isUsed; i++);
> >+          return i != aHitArray.length;
> 
> Nit, I know this works in JS, but I'd still prefer defining the |var i = 0;|
> outside |for(;;);|

News! That works in JS and in C++ I believe at least in MSVC. But in any way fixed.

> And ++i, not i++ ;)
Is there difference in our case?

Comment 52

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

(In reply to comment #51)
> > >+          for (var i = 0; i < aHitArray.length && aHitArray[i].isUsed; i++);
> > >+          return i != aHitArray.length;
> > 
> > Nit, I know this works in JS, but I'd still prefer defining the |var i = 0;|
> > outside |for(;;);|
> 
> News! That works in JS and in C++ I believe at least in MSVC. But in any way
> fixed.
> 

Really? You can use the variable defined in |for| also outside |for| in C++?

> > And ++i, not i++ ;)
> Is there difference in our case?
> 

++i is possibly faster. Though, compilers should optimize i++ in this case.

Comment 53

[alexander :surkov (:asurkov)]

(In reply to comment #52)
> (In reply to comment #51)
> > > >+          for (var i = 0; i < aHitArray.length && aHitArray[i].isUsed; i++);
> > > >+          return i != aHitArray.length;
> > > 
> > > Nit, I know this works in JS, but I'd still prefer defining the |var i = 0;|
> > > outside |for(;;);|
> > 
> > News! That works in JS and in C++ I believe at least in MSVC. But in any way
> > fixed.
> > 
> 
> Really? You can use the variable defined in |for| also outside |for| in C++?

yes, at least in MSVC. Though I agree that is the rather feature.

Comment 54

[Håkan Waara]

Checked in to trunk for Alex.

Comment 55

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16