Last Comment Bug 333619 - select generating too many messages
: select generating too many messages
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-11 12:30 PDT by aaronr
Modified: 2016-07-15 14:46 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase 1 (2.55 KB, application/xhtml+xml)
2006-04-11 12:32 PDT, aaronr
no flags Details
testcase 2 (2.55 KB, application/xhtml+xml)
2006-04-11 12:33 PDT, aaronr
no flags Details
patch (3.28 KB, patch)
2006-04-11 22:19 PDT, alexander :surkov
aaronr: review+
Details | Diff | Splinter Review
testcase 2 (2.36 KB, application/xhtml+xml)
2006-04-12 09:36 PDT, aaronr
no flags Details
patch2 (5.43 KB, patch)
2006-04-12 19:29 PDT, alexander :surkov
doronr: review+
Details | Diff | Splinter Review

Description aaronr 2006-04-11 12:30:27 PDT
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.
Comment 1 aaronr 2006-04-11 12:32:43 PDT
Created attachment 218067 [details]
testcase 1
Comment 2 aaronr 2006-04-11 12:33:14 PDT
Created attachment 218068 [details]
testcase 2
Comment 3 alexander :surkov 2006-04-11 22:19:40 PDT
Created attachment 218137 [details] [diff] [review]
patch
Comment 4 aaronr 2006-04-12 09:36:12 PDT
Created attachment 218178 [details]
testcase 2

looks like I accidently added testcase 1 twice.
Comment 5 aaronr 2006-04-12 10:23:43 PDT
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 6 aaronr 2006-04-12 11:32:43 PDT
Comment on attachment 218137 [details] [diff] [review]
patch

with those, r=me
Comment 7 alexander :surkov 2006-04-12 19:29:09 PDT
Created attachment 218246 [details] [diff] [review]
patch2

(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?
Comment 8 Doron Rosenberg (IBM) 2006-04-14 14:03:49 PDT
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>
>
Comment 9 aaronr 2006-04-14 14:13:14 PDT
(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.
Comment 10 Allan Beaufour 2006-04-18 01:48:37 PDT
Fixed on trunk

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