Closed
Bug 333619
Opened 19 years ago
Closed 19 years ago
select generating too many messages
Categories
(Core Graveyard :: XForms, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: aaronr, Assigned: surkov)
Details
(Keywords: fixed1.8.0.5, fixed1.8.1)
Attachments
(3 files, 2 obsolete files)
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.
|
Assignee | |
Updated•19 years ago
|
Assignee: aaronr → surkov
|
|
Assignee | |
Comment 3•19 years ago
|
||
Attachment #218137 -
Flags: review?(aaronr)
|
|
Assignee | |
Updated•19 years ago
|
Status: NEW → ASSIGNED
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+
|
|
Assignee | |
Comment 7•19 years ago
|
||
(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 8•19 years ago
|
||
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.
|
||
Comment 10•19 years ago
|
||
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
|
|
||
Updated•18 years ago
|
Keywords: fixed1.8.1
|
|
||
Updated•18 years ago
|
Keywords: fixed1.8.0.5
|
|
||
Updated•18 years ago
|
Whiteboard: xf-to-branch
|
||
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•