Last Comment Bug 332083 - expose interface xforms:select to control selection
: expose interface xforms:select to control selection
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
Mentors:
Depends on:
Blocks: 333690 xformsa11y
  Show dependency treegraph
 
Reported: 2006-03-29 02:15 PST by alexander :surkov
Modified: 2016-07-15 14:46 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (33.79 KB, patch)
2006-09-02 13:26 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (33.81 KB, patch)
2006-09-02 13:29 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
testcase (8.02 KB, application/xhtml+xml)
2006-09-03 09:23 PDT, alexander :surkov
no flags Details
patch3 (33.83 KB, patch)
2006-10-05 07:58 PDT, alexander :surkov
bugs: review-
Details | Diff | Splinter Review
patch4 (33.89 KB, patch)
2006-10-10 12:46 PDT, alexander :surkov
bugs: review+
aaronr: review+
Details | Diff | Splinter Review
patch5 (34.08 KB, patch)
2006-10-27 03:32 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description alexander :surkov 2006-03-29 02:15:35 PST
Now xforms:select binding defines interface property 'selectedIndex'. xforms:select[appearance='full'] doesn't implement it. Though I didn't know what 'selectedIndex' should return if xforms:select has some selected items. Probably it should return the first item of them. Maybe should xforms:select provide something more than just 'selectedIndex'?
Comment 1 Allan Beaufour 2006-03-29 03:42:56 PST
(In reply to comment #0)
> Now xforms:select binding defines interface property 'selectedIndex'.
> xforms:select[appearance='full'] doesn't implement it. Though I didn't know
> what 'selectedIndex' should return if xforms:select has some selected items.

as in: "more than one selected item"?

> Probably it should return the first item of them. Maybe should xforms:select
> provide something more than just 'selectedIndex'?

Well, what do we use it for?
Comment 2 Allan Beaufour 2006-03-29 23:55:58 PST
(In reply to comment #1)
> (In reply to comment #0)
> > Now xforms:select binding defines interface property 'selectedIndex'.
> > xforms:select[appearance='full'] doesn't implement it. Though I didn't know
> > what 'selectedIndex' should return if xforms:select has some selected items.
> 
> as in: "more than one selected item"?
> 
> > Probably it should return the first item of them. Maybe should xforms:select
> > provide something more than just 'selectedIndex'?
> 
> Well, what do we use it for?

surkov and I talked about it on IRC.

I cannot find any uses of it in our code? Is it only there for scriptable access? Then I think we should just remove it. The selected item(s) can be retrieved through the bound node. That should be enough.

So if no one has a really good use for it, imho, kill it.
Comment 3 alexander :surkov 2006-04-05 19:50:21 PDT
Note, there is the same problem for controls implementations in xul.
Comment 4 alexander :surkov 2006-05-15 22:26:44 PDT
(In reply to comment #1)
> 
> Well, what do we use it for?
> 

I guess it is unique way to know what item is selected if select isn't bound to instance data.
Comment 5 alexander :surkov 2006-09-02 09:05:09 PDT
Ally code wants some methods for select/select1 to implement nsIAccessibleSelectable interface for xforms accessible objects.
Comment 6 alexander :surkov 2006-09-02 09:47:09 PDT
html:select hasn't special interface to support selection methods. It has selectedIndex only. The rest of work should be done via options I guess. We should decide how we call needed methods. Below method names are listed for nsIDOMXULMultiSelectControlElement interface.

nsIAccessibleSelectable
GetSelectedChildren()

nsIDOMXULMultiSelectControlElement
selectedItems


nsIAccessibleSelectable
addChildToSelection (in long index)

nsIDOMXULMultiSelectControlElement
addItemToSelection ( nsIDOMXULSelectControlItemElement item )


nsIAccessibleSelectable
removeChildFromSelection (in long index)

nsIDOMXULMultiSelectControlElement
removeItemFromSelection ( nsIDOMXULSelectControlItemElement item )


nsIAccessibleSelectable, nsIDOMXULMultiSelectControlElement
clearSelection ()


nsIAccessibleSelectable
refSelection (in long index)

nsIDOMXULMultiSelectControlElement
selectedItems[index] or getSelectedItem()


nsIAccessibleSelectable
isChildSelected (in long index)

nsIDOMXULMultiSelectControlElement
none


nsIAccessibleSelectable
selectAllSelection ()

nsIDOMXULMultiSelectControlElement
selectAll ()
Comment 7 alexander :surkov 2006-09-02 13:26:05 PDT
Created attachment 236554 [details] [diff] [review]
patch
Comment 8 alexander :surkov 2006-09-02 13:29:44 PDT
Created attachment 236555 [details] [diff] [review]
patch2
Comment 9 alexander :surkov 2006-09-03 09:23:38 PDT
Created attachment 236620 [details]
testcase
Comment 10 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-09-07 22:55:40 PDT
If ally is the only use case for this, why not just implement
nsIAccessibleSelectable. What is the need for new interfaces?
Comment 11 alexander :surkov 2006-09-08 02:41:35 PDT
(In reply to comment #10)
> If ally is the only use case for this, why not just implement
> nsIAccessibleSelectable. What is the need for new interfaces?
> 

Do you ask to implement nsIAccessibleSelectable interface for xforms:select/xforms:select1? If yes then accessible objects implements nsIAccessibleSelectable interface only, not dom nodes. And I guess we should have interface to change selection via javascript because nsIXFormsAccessors.setValue() seems to be not very compfortable for this.
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-09-11 13:58:26 PDT
Could you still explain why you need this.
In #5 you talk about nsIAccessibleSelectable, but is that only for some
accessibily objects? (I don't know that code at all)
Comment 13 alexander :surkov 2006-09-11 19:18:45 PDT
(In reply to comment #12)
> Could you still explain why you need this.
> In #5 you talk about nsIAccessibleSelectable, but is that only for some
> accessibily objects? (I don't know that code at all)
> 

Firstly, I belive we need the interface to allow users to control items selection since now they can do it only by calling setValue()/setBoundNode() methods. It's not fine. Also I think we'd should ask w3c about the such interface because xforms spec support some scripting aspect.

Accessibility is just example why we need it. All accessibility objects that represents select control (like html:select/xul:menulist/xul:listbox/xforms:select/xforms:select1) should provide nsIAccessibleSelectable interface. In the end this interface will be used by 3d party software to get/set selection for selectable control.

So, the proposed interface is almost analogue of nsIXFormsAccessors::setValue() method, for example therefore, when selection is changed by this interface then instance data is updated too. The difference between them is nsIXFormsAccessors works with instance data directly but new interfaces work rather with control than with instance data.
Comment 14 aaronr 2006-09-12 14:41:03 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > Could you still explain why you need this.
> > In #5 you talk about nsIAccessibleSelectable, but is that only for some
> > accessibily objects? (I don't know that code at all)
> > 
> 
> Firstly, I belive we need the interface to allow users to control items
> selection since now they can do it only by calling setValue()/setBoundNode()
> methods. It's not fine. Also I think we'd should ask w3c about the such
> interface because xforms spec support some scripting aspect.

I don't see what is wrong with the current approach.  Making the user set the bound node value is very much in keeping with XForms and the whole concept of MVC (model-view-controller).  The controls merely represent the data values, so I think stressing to the user that they are changing the data, not the control, is a good thing.

> 
> Accessibility is just example why we need it. All accessibility objects that
> represents select control (like
> html:select/xul:menulist/xul:listbox/xforms:select/xforms:select1) should
> provide nsIAccessibleSelectable interface. In the end this interface will be
> used by 3d party software to get/set selection for selectable control.
> 
> So, the proposed interface is almost analogue of nsIXFormsAccessors::setValue()
> method, for example therefore, when selection is changed by this interface then
> instance data is updated too. The difference between them is nsIXFormsAccessors
> works with instance data directly but new interfaces work rather with control
> than with instance data.
> 

To clarify, in the case of nsIAccessibleSelectable, the values that you are getting and setting aren't actually the bound value(s), but rather the item labels for the selected values.  Right?
Comment 15 alexander :surkov 2006-09-12 22:13:17 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Could you still explain why you need this.
> > > In #5 you talk about nsIAccessibleSelectable, but is that only for some
> > > accessibily objects? (I don't know that code at all)
> > > 
> > 
> > Firstly, I belive we need the interface to allow users to control items
> > selection since now they can do it only by calling setValue()/setBoundNode()
> > methods. It's not fine. Also I think we'd should ask w3c about the such
> > interface because xforms spec support some scripting aspect.
> 
> I don't see what is wrong with the current approach.  Making the user set the
> bound node value is very much in keeping with XForms and the whole concept of
> MVC (model-view-controller).  The controls merely represent the data values, so
> I think stressing to the user that they are changing the data, not the control,
> is a good thing.

There is nothing wrong with current approach. I can assume just sometimes is not comfortable. When I working with data directly then I loose all specificity of control that represent the data. Probably you're right and I think in terms of existing xml languages (like xul and xhtml), but in any way I can't see any wrong with this approach since actually it adds to xforms one more way to change instance data.

I guess your point is XForms should allow to expose any specifity of certain xforms control but it disallow to have any methods to modify value of control that is bound to. It means that selection methods of default xform:input should be exposed. Right? So Let assume I'd like to replate selected text in input control on some other one. In this case I should get selection from interface, I should get instance data and change instance data according with selection. IMO it's bad mixing because if I like to modify selected text then I'd like to do it for control rather for bound instance node. And what's happens if user already changed control's value but instance data wasn't been updated yet. Here I should call nsIXFormsUIWidget::getCurrentValue() and then only modify instance data. Very complex. And it's unneeded because when I working with selection of control then I'd like to work with control's value. You assume that the user only is able to change control's value but when xforms author like to emulate user operation then with current approach he will fail or get more complex solution.

I'd vote to give xforms author a possibility to modify instance data via control like it can do user.

> > 
> > Accessibility is just example why we need it. All accessibility objects that
> > represents select control (like
> > html:select/xul:menulist/xul:listbox/xforms:select/xforms:select1) should
> > provide nsIAccessibleSelectable interface. In the end this interface will be
> > used by 3d party software to get/set selection for selectable control.
> > 
> > So, the proposed interface is almost analogue of nsIXFormsAccessors::setValue()
> > method, for example therefore, when selection is changed by this interface then
> > instance data is updated too. The difference between them is nsIXFormsAccessors
> > works with instance data directly but new interfaces work rather with control
> > than with instance data.
> > 
> 
> To clarify, in the case of nsIAccessibleSelectable, the values that you are
> getting and setting aren't actually the bound value(s), but rather the item
> labels for the selected values.  Right?
> 

I'm not sure how it is happen in the end. I guess either label or value. It will depend on how assistive software gives information to user. But nsIAccessibleSelectable works with accessible objects for item elements.
Comment 16 aaronr 2006-09-13 10:30:28 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > Could you still explain why you need this.
> > > > In #5 you talk about nsIAccessibleSelectable, but is that only for some
> > > > accessibily objects? (I don't know that code at all)
> > > > 
> > > 
> > > Firstly, I belive we need the interface to allow users to control items
> > > selection since now they can do it only by calling setValue()/setBoundNode()
> > > methods. It's not fine. Also I think we'd should ask w3c about the such
> > > interface because xforms spec support some scripting aspect.
> > 
> > I don't see what is wrong with the current approach.  Making the user set the
> > bound node value is very much in keeping with XForms and the whole concept of
> > MVC (model-view-controller).  The controls merely represent the data values, so
> > I think stressing to the user that they are changing the data, not the control,
> > is a good thing.
> 
> There is nothing wrong with current approach. I can assume just sometimes is
> not comfortable. When I working with data directly then I loose all specificity
> of control that represent the data. Probably you're right and I think in terms
> of existing xml languages (like xul and xhtml), but in any way I can't see any
> wrong with this approach since actually it adds to xforms one more way to
> change instance data.
> 
> I guess your point is XForms should allow to expose any specifity of certain
> xforms control but it disallow to have any methods to modify value of control
> that is bound to. It means that selection methods of default xform:input should
> be exposed. Right? So Let assume I'd like to replate selected text in input
> control on some other one. In this case I should get selection from interface,
> I should get instance data and change instance data according with selection.
> IMO it's bad mixing because if I like to modify selected text then I'd like to
> do it for control rather for bound instance node. And what's happens if user
> already changed control's value but instance data wasn't been updated yet. Here
> I should call nsIXFormsUIWidget::getCurrentValue() and then only modify
> instance data. Very complex. And it's unneeded because when I working with
> selection of control then I'd like to work with control's value. You assume
> that the user only is able to change control's value but when xforms author
> like to emulate user operation then with current approach he will fail or get
> more complex solution.
> 
> I'd vote to give xforms author a possibility to modify instance data via
> control like it can do user.
> 

I'm fine with the user setting the value via the control if modifying the instance data doesn't work for them.  They can already do this by using setValue or setContent in nsIXFormsAccessors.  What I'm wondering is why we need another interface to do the same thing?
Comment 17 alexander :surkov 2006-09-13 11:16:38 PDT
(In reply to comment #16)
> 
> I'm fine with the user setting the value via the control if modifying the
> instance data doesn't work for them.  They can already do this by using
> setValue or setContent in nsIXFormsAccessors.  What I'm wondering is why we
> need another interface to do the same thing?
> 

You are laconic, I try to follow you :). Because this another interface has another semantics. When user modfiy instance data via accessors interface then he thinks in terms of xforms model. When user modfiy instance data via proposed interface then he thinks in terms of select items (let call me terms of control). I can assume for some tasks it will be fine thing. At least I'd like to use it in accessibility and as user I can say it's really comfortable.
Comment 18 aaronr 2006-09-13 12:53:51 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > 
> > I'm fine with the user setting the value via the control if modifying the
> > instance data doesn't work for them.  They can already do this by using
> > setValue or setContent in nsIXFormsAccessors.  What I'm wondering is why we
> > need another interface to do the same thing?
> > 
> 
> You are laconic, I try to follow you :). Because this another interface has
> another semantics. When user modfiy instance data via accessors interface then
> he thinks in terms of xforms model. When user modfiy instance data via proposed
> interface then he thinks in terms of select items (let call me terms of
> control). I can assume for some tasks it will be fine thing. At least I'd like
> to use it in accessibility and as user I can say it's really comfortable.
> 

The only way I can see this used is by some kind of accessibility aid that recognizes speech.  Is this what you need it for?  If this is the case, then you'll probably need it for more than just xf:select.  You'd need it for xf:select1, xf:range, xf:input checkbox and the date picker.  I guess as long as the interfaces are only available through nsIXFormsUtilityService, then it is ok with me.
Comment 19 alexander :surkov 2006-09-15 08:54:01 PDT
(In reply to comment #18)

> The only way I can see this used is by some kind of accessibility aid that
> recognizes speech.  Is this what you need it for?  If this is the case, then
> you'll probably need it for more than just xf:select.  You'd need it for
> xf:select1, xf:range, xf:input checkbox and the date picker.  I guess as long
> as the interfaces are only available through nsIXFormsUtilityService, then it
> is ok with me.
> 

I can see only it will be useful for softwares that recognizes speech too. Probably aaronlev knows what more things that can use it. But in any way accessibility requires from me to implement nsIAccessibleSelectable interface. And the best way in my opinion is to support proposed interfaces.

For sure, I need in more additional interfaces. Just currenly I try to implement accessible objects for select/select1.
Comment 20 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-09-19 00:40:55 PDT
(In reply to comment #19)
> But in any way
> accessibility requires from me to implement nsIAccessibleSelectable interface.
> And the best way in my opinion is to support proposed interfaces.
> 
> For sure, I need in more additional interfaces. Just currenly I try to
> implement accessible objects for select/select1.
> 

So will you need still some more interfaces before you can implement nsIAccessibleSelectable?

Comment 21 alexander :surkov 2006-09-19 02:58:44 PDT
(In reply to comment #20)

> So will you need still some more interfaces before you can implement
> nsIAccessibleSelectable?
> 

No. That's different issue. But for example, I will need some interfaces when I will implement nsIAccessibleEditableText/nsIAccessibleText for xforms:input/xforms:textarea.
Comment 22 alexander :surkov 2006-10-05 07:58:44 PDT
Created attachment 241334 [details] [diff] [review]
patch3

updated to trunk
Comment 23 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-10-10 06:23:50 PDT
Comment on attachment 241334 [details] [diff] [review]
patch3

surkov convinced me that these interfaces are useful, especially for a11y.


>+[scriptable, uuid(5ead20bd-1462-4834-a5ad-46a80570de3e)]
>+interface nsIXFormsNSSelect1Element : nsISupports
>+{
>+  /**
>+   * Return selected / select xforms item element.
>+   */
>+  attribute nsIDOMNode selectedItem;

I'd say an |attribute| doesn't return anything. You can set or get the selected item
using .selectedItem. So a bit better comment.

>+  /**
>+   * Return true if given xforms:item element is selected.
>+   */
>+  boolean isItemSelected(in nsIDOMNode item);

Could you use the doxygen/javadoc style for parameters and for return values;
@parameter and @return
Not only in this method, but also elsewhere in .idl files.

>+      <method name="updateInstanceData">
>+        <parameter name="aIncremental"/>
>+        <body>
>+        <![CDATA[
>+          // Fire 'xfroms-select'/'xforms-deselect' events and unselect illegaly
>+          // selected items.
>+          var copySelectedOrDeselected = new Boolean();
>+          copySelectedOrDeselected.value = false;
>+          var contentEnvelope =
>+            this._processSelectedValues(copySelectedOrDeselected);
>+
>+          if (!aIncremental || this.incremental) {
>+            if (contentEnvelope) {
>+              this._setBoundValue(contentEnvelope,
>+                                  copySelectedOrDeselected.value);
>+            }
>+          }
>+        ]]>
>+        </body>
>+      </method>

Since you're changing things like this, could you please test xforms-select/deselect events properly, i.e.
go through the bugs which have test cases for those.

>@@ -946,40 +1128,30 @@
>         <parameter name="aIncremental"/>
>         <body>
>         <![CDATA[
>           if (!this.parentControl)
>             return;
> 
>           // No need to update the bound value if the control is incremental and
>           // we are losing focus. It should already be up to date.
>-          if (this.parentControl.incremental && !aIncremental)
>+          if (this.this.parentControl.incremental && !aIncremental)

er, what is "this.this." ?

I'd like to see still a new patch with the fixes.
Comment 24 alexander :surkov 2006-10-10 12:46:45 PDT
Created attachment 241859 [details] [diff] [review]
patch4

I did some tests of beaufour and some from bugs. Looks working.
Comment 25 aaronr 2006-10-26 13:32:57 PDT
Comment on attachment 241859 [details] [diff] [review]
patch4


>Index: extensions/xforms/resources/content/select.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select.xml,v
>retrieving revision 1.26
>diff -u -8 -p -r1.26 select.xml
>--- extensions/xforms/resources/content/select.xml	25 Sep 2006 09:50:54 -0000	1.26
>+++ extensions/xforms/resources/content/select.xml	10 Oct 2006 17:33:51 -0000

>@@ -83,26 +82,22 @@
>       @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">
> 
>-  <!-- SELECT BASE -->
>-  <binding id="xformswidget-select-base"
>+  <!-- BASE for select/select1 elements. -->
>+  <binding id="xformswidget-selects-base"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-base">
> 

Can you pick another name for this binding?  Like maybe xformswidget-selectcontrols-base or something like that?  xformswidget-selects-base is too close to xformswidget-select-base.  If I happen to miss one letter typing, then the person reading what I wrote will be thinking of a whole different binding than what I meant.


>@@ -846,33 +868,193 @@
>           // 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;
>         </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
>+          wasSelected - flag specifies whether native item was selected or not.
>+      -->
>       <field name="_controlArray">new Array()</field>
>+
>       <field name="_selectedElementArray">new Array()</field>
>       <field name="_defaultHash">null</field>
>       <field name="_accessorValueCache">null</field>
>       <field name="_outOfRange">false</field>
> 
>       <method name="getControlElement">
>         <body>
>           return document.getAnonymousElementByAttribute(this, "anonid", "control");
>         </body>
>       </method>
> 
>     </implementation>
> 
>   </binding>
> 
> 
>+  <!-- SELECT BASE
>+    Implements nsIXFormsNSSelectElement interface.
>+  -->
>+  <binding id="xformswidget-select-base"
>+           extends="#xformswidget-selects-base">
>+
>+    <implementation implemens="nsIXFormsNSSelectElement">
>+

should be 'implements'.  Missing a 't'.

>+    <!-- nsIXFormsNSSelectElement -->
>+      <property name="selectedItems" readonly="true">
>+        <getter>
>+        <![CDATA[
>+          var items = [];
>+          for (var i = 0; i < this._controlArray.length; i++) {
>+            var nativeitem = this._controlArray[i].option;
>+            if (this.control.isItemSelected(nativeitem)) {
>+              var item = this._controlArray[i].control;
>+              items.push(item);
>+            }
>+          }
>+          return items;
>+        ]]>
>+        </getter>
>+      </property>
>+
>+      <method name="addItemToSelection">
>+        <parameter name="aItem"/>
>+        <body>
>+          var nativeitem = this.getNativeItem(aItem);
>+          if (!this.control.isItemSelected(nativeitem)) {
>+            this.control.addItemToSelection(nativeitem);
>+            this.updateInstanceData();
>+          }
>+        </body>
>+      </method>
>+
>+      <method name="removeItemFromSelection">
>+        <parameter name="aItem"/>
>+        <body>
>+          var nativeitem = this.getNativeItem(aItem);
>+          if (this.control.isItemSelected(nativeitem)) {
>+            this.control.removeItemFromSelection(nativeitem);
>+            this.updateInstanceData();
>+          }
>+        </body>
>+      </method>
>+
>+      <method name="clearSelection">
>+        <body>
>+        <![CDATA[
>+          changed = false;
>+          for (var i = 0; i < this._controlArray.length; i++) {
>+            var nativeitem = this._controlArray[i].option;
>+            if (this.control.isItemSelected(nativeitem)) {
>+              this.control.removeItemFromSelection(nativeitem);
>+              changed = true;
>+            }
>+          }
>+          if (changed)
>+            this.updateInstanceData();
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <method name="selectAll">
>+        <body>
>+        <![CDATA[
>+          var changed = false;
>+          for (var i = 0; i < this._controlArray.length; i++) {
>+            var nativeitem = this._controlArray[i].option;
>+            if (!this.control.isItemSelected(nativeitem)) {
>+              this.control.addItemToSelection(nativeitem);
>+              changed = true;
>+            }
>+          }
>+          if (changed)
>+            this.updateInstanceData();
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <method name="isItemSelected">
>+        <parameter name="aItem"/>
>+        <body>
>+          return this.control.isItemSelected(this.getNativeItem(aItem));
>+        </body>
>+      </method>


Ugh, none of this stuff is going to be thread safe.  If someone calls one of these functions from another thread when a user is interacting with the control, we are hosed.  Not much of a problem with most controls since their timing windows when updating a control's value is so small, but with select/select1 there is a much larger window for mayhem.  I don't know what we can do to prevent it, though.  Maybe we should put some warning in the interface idl files?

>+
>+    <!-- private -->
>+      <method name="getNativeItem">
>+        <parameter name="aItem"/>
>+        <body>
>+          <![CDATA[
>+            for (var i = 0; i < this._controlArray.length; i++) {
>+              if (this._controlArray[i].control == aItem)
>+                return this._controlArray[i].option;
>+            }
>+            return null;
>+          ]]>
>+        </body>
>+      </method>
>+    </implementation>
>+  </binding>
>+
>+
>+  <!-- SELECT1 BASE
>+    Implements nsIXFormsNSSelect1Element interface.
>+  -->
>+  <binding id="xformswidget-select1-base"
>+           extends="#xformswidget-selects-base">
>+
>+    <implementation implemens="nsIXFormsNSSelect1Element">

'implements'.  Missing a 't'

>+      <property name="selectedItem">
>+        <getter>
>+        <![CDATA[
>+          for (var i = 0; i < this._controlArray.length; i++) {
>+            var nativeitem = this._controlArray[i].option;
>+            var item = this._controlArray[i].control;
>+            if (this.control.isItemSelected(nativeitem)) {
>+              return item;
>+            }
>+          }
>+          return null;
>+        ]]>
>+        </getter>
>+        <setter>
>+        <![CDATA[
>+          var changed = false;
>+          for (var i = 0; i < this._controlArray.length; i++) {
>+            var nativeitem = this._controlArray[i].option;
>+            var item = this._controlArray[i].control;
>+            if (!this.control.isItemSelected(nativeitem)) {
>+              if (item == val) {
>+                this.control.addItemToSelection(nativeitem);
>+                changed = true;
>+              }
>+            } else {
>+              if (item != val) {
>+                this.control.removeItemFromSelection(nativeitem);
>+                changed = true;
>+              }
>+            }
>+          }
>+          if (changed)
>+            this.updateInstanceData();
>+        ]]>
>+        </setter>
>+      </property>
>+    </implementation>
>+  </binding>

This is for select1, right?  You are walking the whole list every time.  What if you have 50 items in the list and the currently selected item might be the first item on the list and the one that you want to make the selected item might be the second item.  Might be more efficient to go through the list looking for the currently selected item and the value that you want to set and once you get those two pieces of information, you bounce out of the loop.


>Index: extensions/xforms/resources/content/select1.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select1.xml,v
>retrieving revision 1.24
>diff -u -8 -p -r1.24 select1.xml
>--- extensions/xforms/resources/content/select1.xml	12 Sep 2006 14:11:02 -0000	1.24
>+++ extensions/xforms/resources/content/select1.xml	10 Oct 2006 17:33:51 -0000
>@@ -73,17 +73,36 @@
>                     anonid="dropmarker"
>                     tabindex="-1"
>                     onmousedown="this.parentNode.parentNode.shouldHandleBlur = false;"
>                     onmouseup="this.parentNode.parentNode.shouldHandleBlur = true;"
>                     onclick="this.parentNode.parentNode.togglePopup();
>                              this.previousSibling.focus();"
>         /></html:span></content>
> 
>-    <implementation implements="nsIXFormsUIWidget">
>+    <implementation implements="nsIXFormsUIWidget, nsIXFormsNSSelect1Element">
>+    <!-- nsIXFormsNSSelect1Element -->
>+      <property name="selectedItem">
>+        <getter>
>+          return this._selected;
>+        </getter>
>+        <setter>
>+          if (this._selected)
>+            this._selected.setActive(false);

shouldn't you test to see if the currently selected value is the same as val before you deselect it and do all of this extra work?

>+
>+          this._selected =
>+            val.QueryInterface(Components.interfaces.nsIXFormsItemElement);
>+          if (this._selected) {
>+            this._selected.setActive(true);
>+            this.updateInputField();
>+            this._handleSelection(true);
>+          }
>+        </setter>
>+      </property>
>+

With those changes, r=me
Comment 26 alexander :surkov 2006-10-27 03:32:07 PDT
Created attachment 243769 [details] [diff] [review]
patch5

> Ugh, none of this stuff is going to be thread safe.  If someone calls one of
> these functions from another thread when a user is interacting with the
> control, we are hosed.  Not much of a problem with most controls since their
> timing windows when updating a control's value is so small, but with
> select/select1 there is a much larger window for mayhem.  I don't know what we
> can do to prevent it, though.  Maybe we should put some warning in the
> interface idl files?

Commonly I think we should update instance data right after when selection is changed via interface. I think we should add UpdateInstanceData() into nsXFormsUIWidget interface. So anybody who uses these interfaces is able to update instance node when it's needed. Sure it doesn't solve potential problem.
Comment 27 alexander :surkov 2006-10-27 04:08:56 PDT
checked in by smaug
Comment 28 aaronr 2007-04-23 15:45:28 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

Note You need to log in before you can comment on or make changes to this bug.