332197 - support @selection="open" for xf:select1[appearance='minimal'] for xul

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

It's need to support @selection="open" for xf:select1[appearance='minimal'] for xul context. The bug I want to fix after bug 323851 fixing.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

The patch doesn't support free enrty items when value is element node. I don't know how can item corresponding such value should be displayed.

Comment 2

[aaronr]

could you please attach a testcase so that I can play around with the patch?

Comment 3

[alexander :surkov (:asurkov)]

Attachment: testcase

(In reply to comment #2)
> could you please attach a testcase so that I can play around with the patch?
> 

Sorry, I forgot to do it :).

Comment 4

[aaronr]

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

>diff -uNrap mozilla.orig/extensions/xforms/resources/content/select.xml mozilla/extensions/xforms/resources/content/select.xml
>--- mozilla.orig/extensions/xforms/resources/content/select.xml	2006-04-04 17:11:26.000000000 +0900
>+++ mozilla/extensions/xforms/resources/content/select.xml	2006-04-14 19:17:11.000000000 +0900
>@@ -73,6 +73,14 @@
> 
>     isItemSelected(item) - return true if item is selected
>       @param item - item control returned by appendItem()
>+
>+    getFreeEntryValues() - return space delimited list of free entry items values
>+
>+    allowFreeEntry(allowed) - enable/disable free enrty
>+      @param allowed - boolean value

nit: spelling s/enrty/entry

>+
>+    appendFreeEntryItem(value) - append and select free entry item
>+      @param value - item value
> -->
> 
> <bindings id="xformsSelectBindings"
>@@ -185,9 +193,20 @@
>         </getter>
>       </property>
> 
>-      <property name="selection"
>-                onget="return this.getAttribute('selection'); "
>-                onset="this.setAttribute('selection', val); "/>
>+      <property name="selection">
>+        <getter>
>+          return this.getAttribute("selection");
>+        </getter>
>+        <setter>
>+          if (val == "open")
>+            this.setAttribute("selection", "open");
>+            this.control.allowFreeEnry(true);
>+          } else {
>+            this.removeAttribute("selection");
>+            this.control.allowFreeEnry(false);
>+          }
>+        </setter>
>+      </property>
> 

I would think that you should set the attribute to 'val' no matter what it is since you have no idea what other values a custom control author might want to use here.  But you can still use the test for 'open' to determine whether to allowFreeEntry or not.

Otherwise the patch looks good.

Also, since the select1 is incremental by default, as we type in the select1, the bound value should change (like for a xhtml xforms select1).  Currently this patch doesn't provide that for XUL.  Do you want to provide that in this patch or do you want to do that in a seperate bug?

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

(In reply to comment #4)

> nit: spelling s/enrty/entry
Fixed

> I would think that you should set the attribute to 'val' no matter what it is
> since you have no idea what other values a custom control author might want to
> use here.  But you can still use the test for 'open' to determine whether to
> allowFreeEntry or not.

Let it will be.

> 
> Also, since the select1 is incremental by default, as we type in the select1,
> the bound value should change (like for a xhtml xforms select1).  Currently
> this patch doesn't provide that for XUL.  Do you want to provide that in this
> patch or do you want to do that in a seperate bug?
> 

Fixed.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: updated

Updated patch to trunk

Comment 7

[Doron Rosenberg (IBM)]

Comment on attachment 221660 [details] [diff] [review]
updated

>diff -uNrap mozilla.orig/extensions/xforms/resources/content/select.xml mozilla/extensions/xforms/resources/content/select.xml
>--- mozilla.orig/extensions/xforms/resources/content/select.xml	2006-04-18 17:51:45.000000000 +0900
>+++ mozilla/extensions/xforms/resources/content/select.xml	2006-05-11 12:13:51.000000000 +0900
>@@ -73,6 +73,14 @@
> 
>     isItemSelected(item) - return true if item is selected
>       @param item - item control returned by appendItem()
>+
>+    getFreeEntryValues() - return space delimited list of free entry items values
>+
>+    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"
>@@ -185,9 +193,19 @@
>         </getter>
>       </property>
> 
>-      <property name="selection"
>-                onget="return this.getAttribute('selection'); "
>-                onset="this.setAttribute('selection', val); "/>
>+      <property name="selection">
>+        <getter>
>+          return this.getAttribute("selection");
>+        </getter>
>+        <setter>
>+          this.setAttribute("selection", val);
>+
>+          if (val == "open")
>+            this.control.allowFreeEnry(true);
>+          else
>+            this.control.allowFreeEnry(false);
>+        </setter>
>+      </property>
> 
>       <method name="_buildSelect">
>         <body>
>@@ -230,8 +248,7 @@
>           for (var index in this._defaultHash) {
>             if (this._defaultHash[index].hits == 0) {
>               if (this.selection == 'open') {
>-                // XXX: If the select is open, the missing elements should be added
>-                // and selected per 8.1.10 in the spec.
>+                this.control.appendFreeEntryItem(index);
>               } else {
>                 // XXX: some of default values not found, we need to throw an
>                 // xforms-out-of-range event, but only if the select is 'closed'.
>@@ -594,11 +611,6 @@
>         <parameter name="aIsACopyItemSelectedOrDeselected"/>
>         <body>
>         <![CDATA[
>-          var selectedValues = "";
>-
>-          // select if found, unselect if not
>-          var options = this._controlArray;
>-
>           if (aIsACopyItemSelectedOrDeselected) {
>             aIsACopyItemSelectedOrDeselected.value = false;
>           }
>@@ -621,6 +633,13 @@
>             this._dispatchSelectEvents();
>             return;
>           }
>+
>+          var selectedValues = "";
>+          if (this.selection == "open") {
>+            selectedValues = this.control.getFreeEntryValues();
>+          }
>+
>+          var options = this._controlArray;
>           var boundType = boundNode.nodeType;
>           var copyNode;
> 
>@@ -871,6 +890,33 @@
>         </body>
>       </method>
> 
>+      <!-- Return values of free entry. The method is a stub and it should
>+        be removed when all native controls will support @selection="open".
>+      -->
>+      <method name="getFreeEntryValues">
>+        <body>
>+          return "";
>+        </body>
>+      </method>
>+
>+      <!-- Allow free entry. The method is a stub and it should be removed when
>+        all native controls will support @selection="open".
>+      -->
>+      <method name="allowFreeEntry">
>+        <parameter name="aAllowed"/>
>+        <body>
>+        </body>
>+      </method>
>+
>+      <!-- Append and select free entry item. The method is a stub and it should
>+        be removed when all native controls will support @selection="open".
>+      -->
>+      <method name="appendFreeEntryItem">
>+        <parameter name="aValue"/>
>+        <body>
>+        </body>
>+      </method>
>+
>       <property name="XFORMS_NS" readonly="true"
>                 onget="return 'http://www.w3.org/2002/xforms';"/>
> 
>@@ -883,6 +929,13 @@
>       <property name="XBL_NS" readonly="true"
>                 onget="return 'http://www.mozilla.org/xbl';"/>
> 
>+      <constructor>
>+        if (this.parentControl.selection == "open")
>+          this.allowFreeEntry(true);
>+        else
>+          this.allowFreeEntry(false);
>+      </constructor>
>+
>     </implementation>
>   </binding>
> 
>diff -uNrap mozilla.orig/extensions/xforms/resources/content/select-xul.xml mozilla/extensions/xforms/resources/content/select-xul.xml
>--- mozilla.orig/extensions/xforms/resources/content/select-xul.xml	2006-04-04 17:07:12.000000000 +0900
>+++ mozilla/extensions/xforms/resources/content/select-xul.xml	2006-05-11 12:13:51.000000000 +0900
>@@ -442,9 +442,42 @@
>           return aItem.getAttribute("selected") == "true";
>         </body>
>       </method>
>+
>+      <method name="getFreeEntryValues">
>+        <body>
>+          if (this.control.getAttribute("editable") == "true") {
>+            if (!this.control.selectedItem)
>+              return this.control.value;
>+            return "";
>+          }
>+        </body>
>+      </method>
>+
>+      <method name="allowFreeEntry">
>+        <parameter name="aAllowed"/>
>+        <body>
>+          if (aAllowed)
>+            this.control.setAttribute("editable", "true");
>+          else
>+            this.control.removeAttribute("editable");
>+        </body>
>+      </method>
>+
>+      <method name="appendFreeEntryItem">
>+        <parameter name="aValue"/>
>+        <body>
>+          if (this.control.getAttribute("editable") == "true") {
>+            this.control.value = aValue;
>+          }
>+        </body>
>+      </method>
>     </implementation>
> 
>     <handlers>
>+      <handler event="input">
>+        this.updateInstanceData(true);
>+      </handler>
>+
>       <handler event="focus" phase="capturing">
>         this.dispatchDOMUIEvent("DOMFocusIn");
>       </handler>
>@@ -460,9 +493,10 @@
> 
>       <!--
>         XXX: Since xforms:label can include arbitrary elements then 'focus',
>-          'blur' and 'command' events should be listen from 'xul:menulist'
>-          element only. The described problem is not actual now. Note we will
>-          get it when we will use node instead of text content for a label.
>+          'blur', 'command' and 'input' events should be listen from
>+          'xul:menulist' element only. The described problem is not actual now.
>+          Note we will get it when we will use node instead of text content for
>+          a label.
>       -->
>     </handlers>
>   </binding>

Comment 8

[Allan Beaufour]

Fixed on trunk