expose interface xforms:select to control selection

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
9 months ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
fixed1.8.0.12, fixed1.8.1.4
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

11 years ago
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

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

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

Comment 3

11 years ago
Note, there is the same problem for controls implementations in xul.
(Assignee)

Updated

11 years ago
Blocks: 333690
(Assignee)

Comment 4

11 years ago
(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.

Updated

11 years ago
Assignee: aaronr → xforms
(Assignee)

Comment 5

11 years ago
Ally code wants some methods for select/select1 to implement nsIAccessibleSelectable interface for xforms accessible objects.
Assignee: xforms → surkov.alexander
Blocks: 337249
(Assignee)

Comment 6

11 years ago
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 ()
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Summary: selectedIndex for xforms:select → expose interface xforms:select to control selection
(Assignee)

Comment 7

11 years ago
Created attachment 236554 [details] [diff] [review]
patch
Attachment #236554 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 8

11 years ago
Created attachment 236555 [details] [diff] [review]
patch2
Attachment #236554 - Attachment is obsolete: true
Attachment #236555 - Flags: review?(Olli.Pettay)
Attachment #236554 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 9

11 years ago
Created attachment 236620 [details]
testcase

Comment 10

11 years ago
If ally is the only use case for this, why not just implement
nsIAccessibleSelectable. What is the need for new interfaces?
(Assignee)

Comment 11

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

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

Comment 13

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

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

Comment 15

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

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

Comment 17

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

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

Comment 19

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

11 years ago
(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?

(Assignee)

Comment 21

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

Comment 22

11 years ago
Created attachment 241334 [details] [diff] [review]
patch3

updated to trunk
Attachment #236555 - Attachment is obsolete: true
Attachment #241334 - Flags: review?(Olli.Pettay)
Attachment #236555 - Flags: review?(Olli.Pettay)

Comment 23

11 years ago
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.
Attachment #241334 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 24

11 years ago
Created attachment 241859 [details] [diff] [review]
patch4

I did some tests of beaufour and some from bugs. Looks working.
Attachment #241334 - Attachment is obsolete: true
Attachment #241859 - Flags: review?(Olli.Pettay)

Updated

11 years ago
Attachment #241859 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

11 years ago
Attachment #241859 - Flags: review?(aaronr)

Comment 25

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

Comment 26

11 years ago
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.
Attachment #241859 - Attachment is obsolete: true
(Assignee)

Comment 27

11 years ago
checked in by smaug
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 28

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.