Closed Bug 337250 Opened 18 years ago Closed 18 years ago

need a method to get current value of xforms controls

Categories

(Core Graveyard :: XForms, defect)

Product:
Core Graveyard
Component:
XForms
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access, fixed1.8.0.8, fixed1.8.1.1)

Attachments

(2 files, 3 obsolete files)

expose getCurrentValue4 [for checkin]
12.94 KB, patch
Details | Diff | Splinter Review
misspelling
1.18 KB, patch
Details | Diff | Splinter Review 
Since xforms controls are based on xhtml/xul controls then it would be nice if accessible objects for xforms controls will be based on accessible objects for xhtml/xul controls. We reach it if we'll create nsIAccessible object stub that will redirect all accessibility methods to methods of accessible object for underlying xhtml/xul control. Then for every accessible xforms control we can create accessible object inherited from this stub object. It allows us to be independent on internal xhtml/xul object. Aaron L, what do you think about it?
Depends on: 284068
No longer depends on: 284068
Keywords: access
XForms input controls (xf:input, xf:textarea, xf:secret) have pretty simple implementation and they are based on controls from host language. I prefer to implement accessible object for them before their huge fellows (like xf:select/xf:select1). Therefore I change bug summary.
Summary: Support nsIAccessible object for XForms controls → implement accessible objects for xforms input controls
(In reply to comment #0)
> Since xforms controls are based on xhtml/xul controls then it would be nice if
> accessible objects for xforms controls will be based on accessible objects for
> xhtml/xul controls. We reach it if we'll create nsIAccessible object stub that
> will redirect all accessibility methods to methods of accessible object for
> underlying xhtml/xul control. Then for every accessible xforms control we can
> create accessible object inherited from this stub object. It allows us to be
> independent on internal xhtml/xul object. Aaron L, what do you think about it?
> 

Now I'm not sure it's good idea. If we redirect accessibility calls to underlying widgets then we depend on internal xforms widgets implementation.

In this case xul:textbox is like input xforms controls from point of view of accessible code. If I get right xul:texbox provides interface that accessilbe code uses and it adress itself to underlying html:input to get state properties. State properties for XForms controls are defined by model item properties, therefore we shouldn't ask underlying widget about state. The other issues like selection are handled by accessibility code outside of textbox accessible object. AaronL, if I'm wrong please correct me.

Therefore we can provide additional interfaces for xfroms control for accessibility code (for example bug 333690 is about it) and we able to not use any kinds of redirection.
Attached patch expose getCurrentValue method (obsolete) — — Splinter Review 
Extending of nsIXFormsUIWidget interface by getCurrentValue() adding. The method will be used by accessible object to obtain current control's value via nsIXFormsUtilityService.
Attachment #232427 - Flags: review?(aaronr)
Status: NEW → ASSIGNED
Comment on attachment 232427 [details] [diff] [review]
expose getCurrentValue method


You'll notice some confusion below on my part.  I don't get a really good idea what a control should return because your comments don't really explain how getCurrentValue will be used.  You'll want to comment the .idl a little better so that a control author will have a good idea what to return for new controls.


>Index: extensions/xforms/resources/content/input.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/input.xml,v
>retrieving revision 1.5
>diff -u -8 -p -r1.5 input.xml
>--- extensions/xforms/resources/content/input.xml	26 Jul 2006 21:51:50 -0000	1.5
>+++ extensions/xforms/resources/content/input.xml	6 Aug 2006 17:30:32 -0000
>@@ -131,16 +131,22 @@
> 
>       <method name="focus">
>         <body>
>           this.control.focus();
>           return true;
>         </body>
>       </method>
> 
>+      <method name="getCurrentValue">
>+        <body>
>+          return this.control.value ? "true" : "false";
>+        </body>
>+      </method>
>+

What about if this.changed is false?  It could show as being unchecked, but that doesn't mean that false is the current value.  What if the current bound value is "blue"?  What do you want to represent to the user?  If you want it to be represented to the user as 'false', then please put a comment here so we know that this is exactly what we want.

>Index: extensions/xforms/resources/content/range.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/range.xml,v
>retrieving revision 1.4
>diff -u -8 -p -r1.4 range.xml
>--- extensions/xforms/resources/content/range.xml	30 May 2006 09:30:31 -0000	1.4
>+++ extensions/xforms/resources/content/range.xml	6 Aug 2006 17:30:32 -0000
>@@ -395,16 +395,22 @@
> 
>       <method name="focus">
>         <body>
>           this.canvas.focus();
>           return true;
>         </body>
>       </method>
> 
>+      <method name="getCurrentValue">
>+        <body>
>+          return this.rVal;
>+        </body>
>+      </method>
>+

It is possible that this.rVal will be null if the control is out-of-range.  You'd probably want to return an empty string in that case, I'd guess.

>Index: extensions/xforms/resources/content/select.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select.xml,v
>retrieving revision 1.22
>diff -u -8 -p -r1.22 select.xml
>--- extensions/xforms/resources/content/select.xml	12 Jul 2006 17:24:57 -0000	1.22
>+++ extensions/xforms/resources/content/select.xml	6 Aug 2006 17:30:33 -0000
>@@ -172,16 +172,23 @@
>         <body>
>           <![CDATA[
>           this.control.focus();
>           return true;
>           ]]>
>         </body>
>       </method>
> 
>+      <method name="getCurrentValue">
>+        <body>
>+          var contentEnvelope = this._processSelectedValues();
>+          return contentEnvelope.nodeValue;
>+        </body>
>+      </method>
>+

I'm confused.  The value you are returning for select1 is the label of the value selected.  But here you are returning the actual selected values?  And contentEnvelope.nodeValue is only the xf:values selected, so might not be all of the selected values (i.e. won't return anything for copyItems).

>Index: extensions/xforms/resources/content/select1.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select1.xml,v
>retrieving revision 1.20
>diff -u -8 -p -r1.20 select1.xml
>--- extensions/xforms/resources/content/select1.xml	26 May 2006 08:20:05 -0000	1.20
>+++ extensions/xforms/resources/content/select1.xml	6 Aug 2006 17:30:33 -0000
>@@ -705,16 +705,22 @@
> 
>       <method name="focus">
>         <body>
>           this.inputField.focus();
>           return true;
>         </body>
>       </method>
> 
>+      <method name="getCurrentValue">
>+        <body>
>+          return this.inputField.value;
>+        </body>
>+      </method>
>+

What if the bound value is out of range?  The input field won't have a value, but there will be a value in the bound node.  Plus you are returning the label of the selected item, not the actual selected value.

>Index: extensions/xforms/resources/content/xforms-xhtml.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms-xhtml.xml,v
>retrieving revision 1.12
>diff -u -8 -p -r1.12 xforms-xhtml.xml
>--- extensions/xforms/resources/content/xforms-xhtml.xml	2 Aug 2006 18:29:37 -0000	1.12
>+++ extensions/xforms/resources/content/xforms-xhtml.xml	6 Aug 2006 17:30:33 -0000
>@@ -76,16 +76,19 @@
>             __proto__: this.ownerDocument.
>               getAnonymousElementByAttribute(this, 'anonid', 'control'),
> 
>           // XXX changing from setting textContent to setting nodeValue of
>           // first child (text node created by space character initializer
>           // above).  Workaround for bug 322975.  Probably should be changed
>           // back after repeat is xbl-ized
> 
>+            get value() {
>+              return this.firstChild.nodeValue;
>+            },
>             set value(val) {
>               this.firstChild.nodeValue = val;
>             }
>           };
>         </body>
>       </method>
> 
>     </implementation>
>@@ -182,16 +188,20 @@
>             _labelControl: this,
>             _implicitContent: this.ownerDocument.
>               getAnonymousElementByAttribute(this, 'anonid', 'implicitcontent'),
>             _explicitContent: this.ownerDocument.
>               getAnonymousElementByAttribute(this, 'anonid', 'explicitcontent'),
> 
>             _ownerDocument: this.ownerDocument,
> 
>+            get value() {
>+              return this._currentTextValue;
>+            },
>+
>             set value(val) {
>               var textnode = null;
> 
>               if (val != null) {
>                 this._implicitContent.textContent = val;
>                 this._explicitContent.style.display = 'none';
>                 textnode = this._implicitContent.firstChild;
>               } else {

if val != null, you won't set this._currentTextValue.  So the control will have a value but you won't get that value using get value.  Same if !underline below.

>@@ -205,16 +215,18 @@
>                   }
>                 }
>                 if (!underline) {
>                   this._implicitContent.textContent = '';
>                   this._explicitContent.style.display = 'inline';
>                   return;
>                 }
> 
>+                this._currentTextValue = textnode.nodeValue;
>+
>                 this._implicitContent.textContent = this._labelControl.textContent;
>                 this._explicitContent.style.display = 'none';
>                 textnode = this._implicitContent.firstChild;
>               }
> 
>               var accesskey = this._labelControl.parentNode.getAttribute("accesskey");
>               if (accesskey.length == 1 && textnode) {
>                 this.setAccesskeyOnNode(accesskey, textnode);

>@@ -269,16 +283,19 @@
> 
>     <implementation>
>       <method name="getControlElement">
>       <body>
>         return {
>           __proto__: this.ownerDocument.
>             getAnonymousElementByAttribute(this, 'anonid', 'control'),
> 
>+          get value() {
>+            return "";
>+          },

You don't want to pass back anything for a trigger?  Not even the label?

>           set disabled(val) {
>             if (val) {
>               this.setAttribute('disabled', 'disabled');
>             } else {
>               this.removeAttribute('disabled');
>             }
>           }
>         };
>@@ -314,16 +331,19 @@
> 
>     <implementation>
>       <method name="getControlElement">
>       <body>
>         return {
>           __proto__: this.ownerDocument.
>             getAnonymousElementByAttribute(this, 'anonid', 'control'),
> 
>+          get value() {
>+            return "";
>+          },
>           set disabled(val) {
>             this.isDisabled = val;
>           },
>           isDisabled: false
>         };
>       </body>
>       </method>
> 

>@@ -471,16 +496,19 @@
>       </html:button>
>       <children/>
>     </content>
> 
>     <implementation implements="nsIXFormsUIWidget, nsIXFormsUploadUIElement">
>       <method name="getControlElement">
>         <body>
>           return {
>+            get value() {
>+              return this._textControl.value;
>+            },
>             set value(val) {
>               this._textControl.value = val;
>             },
>             set readonly(val) {
>               if (val) {
>                 this._browseButton.setAttribute('disabled', 'disabled');
>                 this._clearButton.setAttribute('disabled', 'disabled');
>               } else {
>@@ -541,16 +569,17 @@
>       </html:button>
>       <children/>
>     </content>
> 
>     <implementation>
>       <method name="getControlElement">
>         <body>
>           return {
>+            get value(){ return ""; }
>             set value(val){},
>             set readonly(val){},
>             focus: function(){}
>           };
>         </body>
>       </method>
>     </implementation>
>   </binding>
>Index: extensions/xforms/resources/content/xforms-xul.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms-xul.xml,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 xforms-xul.xml
>--- extensions/xforms/resources/content/xforms-xul.xml	2 Aug 2006 18:29:37 -0000	1.7
>+++ extensions/xforms/resources/content/xforms-xul.xml	6 Aug 2006 17:30:33 -0000

>@@ -181,18 +187,22 @@
>       <xul:button anonid="control" xbl:inherits="accesskey" flex="1">
>         <children/>
>       </xul:button>
>     </content>
> 
>     <implementation>
>       <method name="getControlElement">
>         <body>
>-          return this.ownerDocument.
>-            getAnonymousElementByAttribute(this, 'anonid', 'control');
>+          return {
>+            __proto__: this.ownerDocument.
>+              getAnonymousElementByAttribute(this, 'anonid', 'control'),
>+
>+            get value() { return ""; }
>+          };

same as with xhtml trigger...sure you don't want to return the label content or something?

>         </body>
>       </method>
>     </implementation>
> 
>     <handlers>
>       <handler event="command">
>         // XXX: we need to fire 'DOMActivate' event to get xforms:submit to
>         // work, since xul:button do not do it (see a bug 323005
>@@ -215,16 +225,17 @@
> 
>     <implementation>
>       <method name="getControlElement">
>         <body>
>           return {
>             __proto__: this.ownerDocument.
>               getAnonymousElementByAttribute(this, 'anonid', 'control'),
> 
>+            get value() { return ""; },
>             set disabled(val) {
>               this.isDisabled = val;
>             },
>             isDisabled: false
>           }
>         </body>
>       </method>
>     </implementation>
>@@ -301,29 +312,38 @@
> 
> 
>   <!-- ALERT: <DEFAULT> -->
>   <binding id="xformswidget-alert"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-alert-base">
>     <content>
>       <xul:deck anonid="contentswitcher" flex="1" selectedIndex="1">
>         <xul:label anonid="implicitcontent" xbl:inherits="orient"/>
>-        <xul:label xbl:inherits="orient"><children/></xul:label>
>+        <xul:label xbl:inherits="orient" anonid="explicitcontent">
>+          <children/>
>+        </xul:label>
>       </xul:deck>
>     </content>
> 
>     <implementation>
>       <method name="getControlElement">
>         <body>
>           return {
>             _contentSwitcher: this.ownerDocument.
>               getAnonymousElementByAttribute(this, 'anonid', 'contentswitcher'),
>             _implicitContent: this.ownerDocument.
>               getAnonymousElementByAttribute(this, 'anonid', 'implicitcontent'),
>+            _explicitContent: this.ownerDocument.
>+              getAnonymousElementByAttribute(this, 'anonid', 'explicitcontent'),
> 
>+            get value() {
>+              if (this._contentSwitcher.selectedIndex == 1)
>+                return this._explicitContent.textContent;
>+              return this._implicitContent.textContent;
>+            },
>             setValue: function setValue(aUseInlineValue, aValue) {
>               if (aUseInlineValue) {
>                 this._contentSwitcher.selectedIndex = 1;
>               } else {
>                 this._implicitContent.textContent = aValue;
>                 this._contentSwitcher.selectedIndex = 0;
>               }
>             }
>Index: extensions/xforms/resources/content/xforms.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms.xml,v
>retrieving revision 1.41
>diff -u -8 -p -r1.41 xforms.xml
>--- extensions/xforms/resources/content/xforms.xml	2 Aug 2006 18:29:37 -0000	1.41
>+++ extensions/xforms/resources/content/xforms.xml	6 Aug 2006 17:30:33 -0000
>@@ -183,17 +183,21 @@
>       <field name="_delegate">null</field>
>       <field name="_accessors">null</field>
>     </implementation>
>   </binding>
> 
> 
>   <!--
>     Base binding is for xforms controls that implement nsIXFormsUIWidget
>-    interface.
>+    interface. The binding assumes getElementControl() method returns the
>+    object:
>+    {
>+      get value(){}, // return current element "string" value
>+    }

You might want to explain this comment more.  If you describe what this is used for, people creating custom controls, etc. will have a better idea of what value to return for their control.  You notice some of my confusion above :-)
Attachment #232427 - Flags: review?(aaronr) → review-
Attached patch expose getCurrentValue2 (obsolete) — — Splinter Review 
> You'll notice some confusion below on my part.  I don't get a really good idea
> what a control should return because your comments don't really explain how
> getCurrentValue will be used.  You'll want to comment the .idl a little better
> so that a control author will have a good idea what to return for new controls.

Sorry for that. I tried to document better in new patch what getCurrentValue() should do.

> What about if this.changed is false?  It could show as being unchecked, but
> that doesn't mean that false is the current value.  What if the current bound
> value is "blue"?  What do you want to represent to the user?  If you want it to
> be represented to the user as 'false', then please put a comment here so we
> know that this is exactly what we want.

It's not important what do control value means. getCurrentValue() returns the
value that control actually shows, it returns what user actually can see. Nothing more.

> It is possible that this.rVal will be null if the control is out-of-range. 
> You'd probably want to return an empty string in that case, I'd guess.

> I'm confused.  The value you are returning for select1 is the label of the
> value selected.  But here you are returning the actual selected values?  And
> contentEnvelope.nodeValue is only the xf:values selected, so might not be all
> of the selected values (i.e. won't return anything for copyItems).

> What if the bound value is out of range?  The input field won't have a value,
> but there will be a value in the bound node.  Plus you are returning the label
> of the selected item, not the actual selected value.

I remove range/select1/select support while I will not be there.

> You don't want to pass back anything for a trigger?  Not even the label?

> same as with xhtml trigger...sure you don't want to return the label content or
> something?

Trigger hasn't value even if it is bound to instance node. Label means 'name' in
accessibility terms, it's not value actually.
Attachment #232427 - Attachment is obsolete: true
Attachment #233451 - Flags: review?(aaronr)
Depends on: 337690
Comment on attachment 233451 [details] [diff] [review]
expose getCurrentValue2

>Index: extensions/xforms/nsIXFormsUIWidget.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsUIWidget.idl,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 nsIXFormsUIWidget.idl
>--- extensions/xforms/nsIXFormsUIWidget.idl	5 Nov 2005 16:11:42 -0000	1.2
>+++ extensions/xforms/nsIXFormsUIWidget.idl	13 Aug 2006 12:20:53 -0000
>@@ -41,26 +41,39 @@

>+
>+  /**
>+   * Return "string" value of xforms element that element actually shows.
>+   *
>+   * From time to time current element value can be different from current value
>+   * of instance node that element is bound to. For example:
>+   * 1) When element has non incremental update and elements value is changed
>+   * due to user interaction then instance node is not updated at the same time.

I would change this wording a little bit.  Just to make it as clear as possible.  Maybe something like:
1) When a XForms control has non-incremental update and its value is changed due to user interaction, then getCurrentValue will return the control's new value even though the bound node has not be updated yet.


>Index: extensions/xforms/resources/content/xforms.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms.xml,v
>retrieving revision 1.41
>diff -u -8 -p -r1.41 xforms.xml
>--- extensions/xforms/resources/content/xforms.xml	2 Aug 2006 18:29:37 -0000	1.41
>+++ extensions/xforms/resources/content/xforms.xml	13 Aug 2006 12:20:54 -0000
>@@ -214,16 +214,28 @@
>       </method>
> 
>       <!-- Method is called when control should be focused -->
>       <method name="focus">
>         <body>
>           return false;
>         </body>
>       </method>
>+
>+      <!-- Method is called to get current value of xforms element.
>+
>+        Here the method is implemented for those xforms elements that don't
>+        allow to change instance value. The rest of xforms elements should
>+        override this implementation.
>+      -->

nit: Maybe give an example here.  How about this?  "Here the method is implemented for those xforms elements that can't be used to change the bound instance value.  For example, xforms:output.  The rest of the xforms elements should override this implementation."

You might also want to mention trigger here and when an element's override should return "".

with those nits, r=me
Attachment #233451 - Flags: review?(aaronr) → review+
Attached patch expose getCurrentValue3 (obsolete) — — Splinter Review 

        Attachment #233451 -
        Attachment is obsolete: true

        Attachment #234777 -
        Flags: review?(Olli.Pettay)

    
    

    
  
Comment on attachment 234777 [details] [diff] [review]
expose getCurrentValue3


> 
>+      <method name="getCurrentValue">
>+        <body>
>+          return this.control.value != "" ? "--" + this.control.value : "";
>+        </body>
>+      </method>

Hmm, I don't understand the "--"

>+      <method name="getCurrentValue">
>+        <body>
>+          return this.control.value != "" ? "---" + this.control.value : "";
>+        </body>
>+      </method>

here "---", not "--" like earlier. Please add comment and make those consistent.

        Attachment #234777 -
        Flags: review?(Olli.Pettay) → review+

    
    

    
  


  

        Attachment #234777 -
        Attachment is obsolete: true

    
    

    
  
checked in getCurrentValue4 patch to trunk for surkov.  Leaving this bug open at his request.
Whiteboard: xf-to-branch

    
  
Blocks: 349644

    
    

    
  
I filed new bug 350186 to support accessible objects for xforms controls to keep these things separated.
Blocks: 350186
No longer blocks: 349644
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Component: Disability Access APIs → XForms
Resolution: --- → FIXED
Summary: implement accessible objects for xforms input controls → need a method to get current value of xforms controls

    
  
No longer blocks: xformsa11y

    
    

    
  
Comment typo:
"even though the bound node has not be updated yet"
should be:
be --> been

    
    

    
  

      
      
      
      

        Attached patch
          
          
        misspelling
        — 
          — Splinter Review
      

    


  
(In reply to comment #12)
> Comment typo:
> "even though the bound node has not be updated yet"
> should be:
> be --> been
> 

Thank you.

    
    

    
  
Comment on attachment 236442 [details] [diff] [review]
misspelling

The patch needs checkin.

    
    

    
  
(In reply to comment #14)
> (From update of attachment 236442 [details] [diff] [review] [edit])
> The patch needs checkin.
> 

Done.

    
    

    
  
checked both patches into 1.8.0 branch on 2006/09/21
Keywords: fixed1.8.0.8

    
    

    
  
checked into 1.8 branch on 2006/11/21
Keywords: fixed1.8.1.1
Whiteboard: xf-to-branch
Product: Core → Core Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: