Closed Bug 333619 Opened 18 years ago Closed 18 years ago

select generating too many messages

Categories

(Core Graveyard :: XForms, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: aaronr, Assigned: surkov)

Details

(Keywords: fixed1.8.0.5, fixed1.8.1)

Attachments

(3 files, 2 obsolete files)

testcase 1
2.55 KB, application/xhtml+xml
Details
testcase 2
2.36 KB, application/xhtml+xml
Details
patch2
5.43 KB, patch
doronr
: review+
Details | Diff | Splinter Review 
With the reorganizing of xf:select, we are now generating too many events for xforms-binding-exceptions and xforms-select and xforms-deselect when we have message popups.  That is because we are going through the event generation codepath for "blur" when the control is incremental.  We really shouldn't do anything if the select is incremental and it loses focus.  The current value should already be correct.
Attached file testcase 1 — 

    
    

    
  

      
      
      
      

        Attached file
          
        testcase 2 (obsolete)
        — 
      

    


  

    
  
Assignee: aaronr → surkov

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #218137 -
        Flags: review?(aaronr)

    
  
Status: NEW → ASSIGNED

    
    

    
  

      
      
      
      

        Attached file
          
        testcase 2
        — 
      

    


  
looks like I accidently added testcase 1 twice.

        Attachment #218068 -
        Attachment is obsolete: true

    
    

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

It looks like with this patch getSelectedValues doesn't just build a list of selected values anymore, but also dispatches selection events (and also a binding exception from a previous patch).  So getSelectedValues has changed from behaving like a stand alone function to one that is tied pretty much into the user selection process.  So I guess we should change the name to be something different than getSelectedValues.  Maybe "buildListAndNotify"?  Also, please put a comment near the beginning of this function describing what it does since the function name won't quite describe what it does.  Thanks.

>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-12 14:18:11.000000000 +0900

>@@ -828,16 +832,24 @@
>       <method name="updateInstanceData">
>         <parameter name="aIncremental"/>
>         <body>
>-          if (this.parentControl) {
>-            if (!aIncremental || this.parentControl.incremental) {
>-              this.parentControl._setBoundValue();
>-            } else {
>-              // we should call _getSelectedValues() method since we probably
>-              // should unselect illegaly selected items.
>-              this.parentControl._getSelectedValues();
>-            }
>-            this.parentControl._dispatchSelectEvents();
>+        <![CDATA[
>+          if (!this.parentControl)
>+            return;
>+
>+          // We skip non-incremental updates if select is incremental since
>+          // native control widgets ask for non-incremental update right away
>+          // after incremental update.
>+          if (this.parentControl.incremental && !aIncremental)
>+            return;
>+
>+          if (!aIncremental || this.parentControl.incremental) {
>+            this.parentControl._setBoundValue();
>+          } else {
>+            // we should call _getSelectedValues() method since we probably
>+            // should unselect illegaly selected items.
>+            this.parentControl._getSelectedValues();
>           }
>+        ]]>
>         </body>
>       </method>
> 

I have NO idea what your comment means.  You are using the word 'incremental' too many times in one sentence and it is confusing me :=)

Suggestions: 

1) add a comment as to what aIncremental means.  For example, 'aIncremental will be true if this function is called because the user selected a new value in the native control widget.  aIncremental will be false if this function is called due to focus leaving the native control widget.'
2) change your confusing comment to read more like, 'no need to update the bound value if the control is incremental and we are losing focus.  It should already be up to date'.

    
    

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

with those, r=me

        Attachment #218137 -
        Flags: review?(aaronr) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch2
        — 
          — Splinter Review
      

    


  
(In reply to comment #6)
> (From update of attachment 218137 [details] [diff] [review] [edit])
> with those, r=me
> 

I'm about _getSelectedValues. I like more _processSelectedValues. What do you think?

        Attachment #218137 -
        Attachment is obsolete: true

        Attachment #218246 -
        Flags: review?(doronr)

    
    

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

>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-13 11:25:06.000000000 +0900
>@@ -544,7 +544,7 @@
>           // textnode under the bound node.
>           var copySelectedOrDeselected = new Boolean();
>           copySelectedOrDeselected.value = false;
>-          var contentEnvelope = this._getSelectedValues(copySelectedOrDeselected);
>+          var contentEnvelope = this._processSelectedValues(copySelectedOrDeselected);
>           if (contentEnvelope) {
>             if (boundNode.nodeType == Node.ELEMENT_NODE) {
>               // we shouldn't call setContent if we haven't selected any
>@@ -568,11 +568,11 @@
>               }
>             } else {
>               // if some copyItems were selected by the user prior to the call
>-              // to _getSelectedValues, then we would not have set up
>+              // to _processSelectedValues, then we would not have set up
>               // _accessorValueCache.  Since the node we are bound to can't
>               // be set by copyItems (its not an ELEMENT_NODE), any copyItems
>               // in this select would have been deselected during
>-              // _getSelectedValues.  Thus, anything in the contentEnvelope at
>+              // _processSelectedValues.  Thus, anything in the contentEnvelope at
>               // this point should just be strings and so we can set
>               // delegate.value directly and use _accessorValueCache after all.
> 
>@@ -584,7 +584,13 @@
>         </body>
>       </method>
> 
>-      <method name="_getSelectedValues">
>+      <!--
>+        The method serves to
>+          1) unselect illegally selected items
>+          2) fire "xforms-select"/"xforms-deselect" events
>+          3) return list of selected values
>+      -->
>+      <method name="_processSelectedValues">
>         <parameter name="aIsACopyItemSelectedOrDeselected"/>
>         <body>
>         <![CDATA[
>@@ -598,8 +604,10 @@
>           }
> 
>           var boundNode = this.accessors.getBoundNode();
>-          if (!boundNode)
>+          if (!boundNode) {
>+            this._dispatchSelectEvents();
>             return;
>+          }
> 
>           // we are cloning boundNode to create a node that we will return.
>           // By the end of this function, assuming all went well,
>@@ -610,6 +618,7 @@
>           var contentEnvelope = null;
>           contentEnvelope = boundNode.cloneNode(false);
>           if (!contentEnvelope) {
>+            this._dispatchSelectEvents();
>             return;
>           }
>           var boundType = boundNode.nodeType;
>@@ -702,6 +711,7 @@
>             contentEnvelope.nodeValue = selectedValues;
>           }
> 
>+          this._dispatchSelectEvents();
>           return contentEnvelope;
>         ]]>
>         </body>
>@@ -715,16 +725,16 @@
>           for (var i = 0; i < options.length; i++) {
>             var selected = this.control.isItemSelected(options[i].option);
>             if (options[i].wasSelected && !selected) {
>-              this.dispatchSelectEvent(options[i].control, "xforms-deselect");
>               options[i].wasSelected = false;
>+              this.dispatchSelectEvent(options[i].control, "xforms-deselect");
>             }
>           }
> 
>           for (var i = 0; i < options.length; i++) {
>             var selected = this.control.isItemSelected(options[i].option);
>             if (!options[i].wasSelected && selected) {
>-              this.dispatchSelectEvent(options[i].control, "xforms-select");
>               options[i].wasSelected = true;
>+              this.dispatchSelectEvent(options[i].control, "xforms-select");
>             }
>           }
>           ]]>
>@@ -824,20 +834,32 @@
>         </setter>
>       </property>
> 
>-      <!-- Update instance data to native control widget value. -->
>+      <!-- Update instance data to native control widget value.
>+        @param aIncremental - will be true if this function is called because
>+          the user selected a new value in the native control widget. It will be
>+          false if this function is called due to focus leaving the native
>+          control widget.
>+      -->
>       <method name="updateInstanceData">
>         <parameter name="aIncremental"/>
>         <body>
>-          if (this.parentControl) {
>-            if (!aIncremental || this.parentControl.incremental) {
>-              this.parentControl._setBoundValue();
>-            } else {
>-              // we should call _getSelectedValues() method since we probably
>-              // should unselect illegaly selected items.
>-              this.parentControl._getSelectedValues();
>-            }
>-            this.parentControl._dispatchSelectEvents();
>+        <![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)
>+            return;
>+
>+          if (!aIncremental || this.parentControl.incremental) {
>+            this.parentControl._setBoundValue();
>+          } else {
>+            // we should call _processSelectedValues() method since we probably
>+            // should unselect illegaly selected items.
>+            this.parentControl._processSelectedValues();
>           }
>+        ]]>
>         </body>
>       </method>
>

        Attachment #218246 -
        Flags: review?(doronr) → review+

    
    

    
  
(In reply to comment #7)
> Created an attachment (id=218246) [edit]
> patch2
> 
> (In reply to comment #6)
> > (From update of attachment 218137 [details] [diff] [review] [edit] [edit])
> > with those, r=me
> > 
> 
> I'm about _getSelectedValues. I like more _processSelectedValues. What do you
> think?
> 


Sounds ok to me.

    
    

    
  
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

    
  
Keywords: fixed1.8.1

    
  
Keywords: fixed1.8.0.5

    
  
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: