Last Comment Bug 316895 - copy element needs to cause rebuilds
: copy element needs to cause rebuilds
Status: RESOLVED FIXED
: fixed1.8.0.2, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
Mentors:
: 319404 (view as bug list)
Depends on:
Blocks: 326372 326373 326555
  Show dependency treegraph
 
Reported: 2005-11-17 14:22 PST by aaronr
Modified: 2016-07-15 14:46 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase w/ copy (2.48 KB, application/xhtml+xml)
2005-11-17 16:39 PST, aaronr
no flags Details
select/select1 w/o copy testcase (2.49 KB, application/xhtml+xml)
2005-11-17 16:44 PST, aaronr
no flags Details
testcase tracking rebuild messages (2.42 KB, application/xhtml+xml)
2005-11-18 00:24 PST, aaronr
no flags Details
proposed fix (9.41 KB, patch)
2005-11-18 00:59 PST, aaronr
allan: review-
Details | Diff | Splinter Review
next try (12.48 KB, patch)
2005-11-18 16:49 PST, aaronr
bugs: review+
Details | Diff | Splinter Review
setContent testcase (5.64 KB, application/xhtml+xml)
2006-02-17 14:20 PST, aaronr
no flags Details
nother fix attempt (22.73 KB, patch)
2006-02-17 18:43 PST, aaronr
allan: review+
bugs: review+
Details | Diff | Splinter Review
fixes extraneous rebuild on blur plus comments (23.77 KB, patch)
2006-02-22 02:23 PST, aaronr
bugs: review+
Details | Diff | Splinter Review
updated with comments fixed (24.78 KB, patch)
2006-02-22 15:31 PST, aaronr
allan: review+
bugs: review+
Details | Diff | Splinter Review
patch for checkin (24.69 KB, patch)
2006-02-23 09:51 PST, aaronr
no flags Details | Diff | Splinter Review

Description aaronr 2005-11-17 14:22:31 PST
As per 9.3.4 in the spec (http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#ui-adv-copy), copy needs to cause a "A full computational dependency rebuild" when a copyItem is selected or deselected.  It doesn't say that this holds off if @incremental="false" but I assume that it should.  What do you think Allan?
Comment 1 aaronr 2005-11-17 16:39:34 PST
Created attachment 203466 [details]
testcase w/ copy
Comment 2 aaronr 2005-11-17 16:41:24 PST
last minute removal of MarkNodeAsChanged from my copy patch broke select and select1 refreshing other controls.  That should be fixed with the patch that fixes this bug.
Comment 3 aaronr 2005-11-17 16:44:08 PST
Created attachment 203467 [details]
select/select1 w/o copy testcase
Comment 4 aaronr 2005-11-18 00:24:49 PST
Created attachment 203498 [details]
testcase tracking rebuild messages

all the non-cherry items are NOT copyItems.  Selecting/deselecting any of the other items should cause a rebuild.
Comment 5 aaronr 2005-11-18 00:59:58 PST
Created attachment 203503 [details] [diff] [review]
proposed fix

This fix seems to work fine for me
Comment 6 Allan Beaufour 2005-11-18 02:12:23 PST
(In reply to comment #0)
> As per 9.3.4 in the spec
> (http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#ui-adv-copy),
> copy needs to cause a "A full computational dependency rebuild" when a copyItem
> is selected or deselected.  It doesn't say that this holds off if
> @incremental="false" but I assume that it should.  What do you think Allan?

On the rationale that there should be no difference in the user experience whether or not an item is a copy or not, then the rebuild should wait for blur on @incremental="false", yes.
Comment 7 Allan Beaufour 2005-11-18 02:23:20 PST
Comment on attachment 203503 [details] [diff] [review]
proposed fix

You cannot optimize the rebuild away. The spec clearly states:
"* A full computational dependency rebuild is done."
(http://www.w3.org/TR/2005/PER-xforms-20051006/slice9.html#ui-adv-copy)
So even though it makes no sense to do it from the viewpoint of copy, the form author might be expecting xforms-rebuild events and possible side-effect of that.

This of course assumes that "a full computational rebuild" is a "xforms-rebuild". I have always read it like that. It's arguable whether the event should be sent though, or it should just be done. But that's another issue.
Comment 8 aaronr 2005-11-18 14:46:24 PST
(In reply to comment #7)
> (From update of attachment 203503 [details] [diff] [review] [edit])
> You cannot optimize the rebuild away. The spec clearly states:
> "* A full computational dependency rebuild is done."
> (http://www.w3.org/TR/2005/PER-xforms-20051006/slice9.html#ui-adv-copy)
> So even though it makes no sense to do it from the viewpoint of copy, the form
> author might be expecting xforms-rebuild events and possible side-effect of
> that.
> 
> This of course assumes that "a full computational rebuild" is a
> "xforms-rebuild". I have always read it like that. It's arguable whether the
> event should be sent though, or it should just be done. But that's another
> issue.
> 

I guess that I need to comment my code better.  setContent is an accessor, so it can be called by any form/widget author, so always doing a rebuild isn't ideal (since in those cases I doubt that it will be because of a copyItem selection).  And I changed the code in select and select1 to use setContent for most of the item selections, whether copyItem or valueItem to keep the code simpler.  So setContent isn't called just because of a copyItem selection/deselection.

So given that setContent isn't called just due to users interacting with copyItems, I think that my code still addresses your concerns.  In my code you'll see that I'll force rebuilds in every situation except:

1) when bound node contains only a single text node AND the replacement content is a single text node.  This means that a copyItem was not selected beforehand and setContent is NOT getting called because of a copyItem selection.  If either of these were the case, then there would be an element node under the bound node or in the replacement content.

2) there is no content under the bound node and no content replacing the nothingness.  In that case we just return.

So in every case where there was a copyItem selection (there is an element node under the replacement content) or a copyItem deselection (there is an element node under the bound node) we will do a rebuild.

Now, as to whether we use xforms-rebuild event or just call rebuild on the model inside setContent, I don't really care much either way.  I guess I prefer the xforms-rebuild event for now since model->Rebuild doesn't generate a xforms-rebuild notification, yet.

If I add comments to my patch to clarify how setContent works w.r.t. selecting/deselecting a copyItem, Allan, does that address all of your concerns about this patch?
Comment 9 aaronr 2005-11-18 15:07:57 PST
ack.  Missed one.  Shouldn't be causing a rebuild if there is no content under the bound node and we are replacing that nothingness with a text node.  Same scenario that setValue handles and it doesn't cause a rebuild, so setContent shouldn't either.
Comment 10 aaronr 2005-11-18 16:49:08 PST
Created attachment 203597 [details] [diff] [review]
next try

I added comments, fixed a couple of coding holes that I found since the last patch that caused some extra xforms-rebuilds.  Also tried to make more obvious where setContent was behaving like setValue().
Comment 11 aaronr 2005-12-07 10:21:08 PST
*** Bug 319404 has been marked as a duplicate of this bug. ***
Comment 12 Doron Rosenberg (IBM) 2006-01-05 14:06:12 PST
just noting that the patch fixes all select bugs I found :)
Comment 13 Allan Beaufour 2006-01-09 00:17:21 PST
(In reply to comment #12)
> just noting that the patch fixes all select bugs I found :)

*hint* *hint* for me and smaug? ;-) I'll get to the review soon.
Comment 14 Olli Pettay [:smaug] 2006-01-10 11:48:22 PST
Comment on attachment 203466 [details]
testcase w/ copy

This crashes with the patch. Without patch I see quite many ASSERTIONs, but not crash.
Comment 15 Olli Pettay [:smaug] 2006-01-10 11:50:55 PST
(In reply to comment #14)
> (From update of attachment 203466 [details] [edit])
> This crashes with the patch. Without patch I see quite many ASSERTIONs, but not
> crash.
> 
Hmm, was there a fix related to those assertions lately? Have to check out the 
very latest code and re-test. But if no new comments, the crash is there...
Comment 16 Olli Pettay [:smaug] 2006-01-10 13:21:59 PST
(In reply to comment #14)
> (From update of attachment 203466 [details] [edit])
> This crashes with the patch. Without patch I see quite many ASSERTIONs, but not
> crash.
> 
No crash after updating and recompiling. Still assertions.
Comment 17 Olli Pettay [:smaug] 2006-01-10 13:32:44 PST
Comment on attachment 203597 [details] [diff] [review]
next try

And the assertions are Bug 317497. So r=me, I think ;)
Comment 18 Allan Beaufour 2006-01-11 01:33:02 PST
(apologies for slowness in my answer, and I might not be 100% on track yet...)

(In reply to comment #8)
> 1) when bound node contains only a single text node AND the replacement content
> is a single text node.  This means that a copyItem was not selected beforehand
> and setContent is NOT getting called because of a copyItem selection.  If
> either of these were the case, then there would be an element node under the
> bound node or in the replacement content.

Why is setContent() being called at all then?
 
> 2) there is no content under the bound node and no content replacing the
> nothingness.  In that case we just return.

What if a select has a copy element that points to empty content, and the user clicks it, that's "copy selection", and should cause rebuild shouldn't it?
Comment 19 aaronr 2006-01-11 12:04:42 PST
(In reply to comment #18)
> (apologies for slowness in my answer, and I might not be 100% on track yet...)
> 
> (In reply to comment #8)
> > 1) when bound node contains only a single text node AND the replacement content
> > is a single text node.  This means that a copyItem was not selected beforehand
> > and setContent is NOT getting called because of a copyItem selection.  If
> > either of these were the case, then there would be an element node under the
> > bound node or in the replacement content.
> 
> Why is setContent() being called at all then?

The reason that I use setContent this way is to keep the code in select1 as consistent as possible.  The code is already rife with special casing, so I didn't want to introduce any more and I tried to reduce the amount of special casing when I could.  So setValue will only be used inside select1 when it @selection="open" or the boundNode isn't an element node.  In all other cases we use setContent.

And we'd still have to anticipate this possibility anyhow since we can't guarentee that we'll be the only callers of setContent since it is in the nsIXFormsAccessors interface.

> 
> > 2) there is no content under the bound node and no content replacing the
> > nothingness.  In that case we just return.
> 
> What if a select has a copy element that points to empty content, and the user
> clicks it, that's "copy selection", and should cause rebuild shouldn't it?
> 

According to the spec, I guess that it should, strictly speaking.  But I looked at it like refresh.  We don't refresh a model if we select a value for a node that already has that value.

If you think we should rebuild, we certainly can.  Just trying to be as efficient as possible.
Comment 20 Allan Beaufour 2006-01-12 01:09:28 PST
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #8)
> > > 1) when bound node contains only a single text node AND the replacement content
> > > is a single text node.  This means that a copyItem was not selected beforehand
> > > and setContent is NOT getting called because of a copyItem selection.  If
> > > either of these were the case, then there would be an element node under the
> > > bound node or in the replacement content.
> > 
> > Why is setContent() being called at all then?
> 
> The reason that I use setContent this way is to keep the code in select1 as
> consistent as possible.  The code is already rife with special casing, so I
> didn't want to introduce any more and I tried to reduce the amount of special
> casing when I could.  So setValue will only be used inside select1 when it
> @selection="open" or the boundNode isn't an element node.  In all other cases
> we use setContent.

Hmmm. I guess I have to look at that code again. I'm not happy about the usage of setContent() then. (but that said, I've never looked to much into select/select1).

> And we'd still have to anticipate this possibility anyhow since we can't
> guarentee that we'll be the only callers of setContent since it is in the
> nsIXFormsAccessors interface.

I've never been 100% happy with that, and David is also commenting about that part in bug 279063 comment 38. I tend to agree with him.

> > 
> > > 2) there is no content under the bound node and no content replacing the
> > > nothingness.  In that case we just return.
> > 
> > What if a select has a copy element that points to empty content, and the user
> > clicks it, that's "copy selection", and should cause rebuild shouldn't it?
> > 
> 
> According to the spec, I guess that it should, strictly speaking.  But I looked
> at it like refresh.  We don't refresh a model if we select a value for a node
> that already has that value.
> 
> If you think we should rebuild, we certainly can.  Just trying to be as
> efficient as possible.

I know it's nitpicking, but according to the spec. you can not optimize that away. People do funky stuff with XForms, and somebody might actually expect the events to be sent.
Comment 21 aaronr 2006-01-12 09:27:53 PST
> 
> Hmmm. I guess I have to look at that code again. I'm not happy about the usage
> of setContent() then. (but that said, I've never looked to much into
> select/select1).

That's fine.  We can put all the special testing back in to eliminate as many calls as possible to setContent if you want.  Which will cause any similarity between select and select1 to disappear.  But I need some examples of what you want the interface to look like and how you want it to behave.  We can't get around the fact that there will have to be an interface in nsIXFormsAccessors that can set the content of an element node with one or more elements.

> 
> > And we'd still have to anticipate this possibility anyhow since we can't
> > guarentee that we'll be the only callers of setContent since it is in the
> > nsIXFormsAccessors interface.
> 
> I've never been 100% happy with that, and David is also commenting about that
> part in bug 279063 comment 38. I tend to agree with him.
> 

I guess I'm confused.  You don't like the way that setContent works, or the fact that setContent is in nsIXFormsAccessors or both?  Please let me know.  I thought that David wanted a seperate setContent for select vs. select1.  Which means a setContent that just deep copies one node under the bound node and one that deep copies multiple nodes under the bound node.

> > > 
> > > > 2) there is no content under the bound node and no content replacing the
> > > > nothingness.  In that case we just return.
> > > 
> > > What if a select has a copy element that points to empty content, and the user
> > > clicks it, that's "copy selection", and should cause rebuild shouldn't it?
> > > 
> > 
> > According to the spec, I guess that it should, strictly speaking.  But I looked
> > at it like refresh.  We don't refresh a model if we select a value for a node
> > that already has that value.
> > 
> > If you think we should rebuild, we certainly can.  Just trying to be as
> > efficient as possible.
> 
> I know it's nitpicking, but according to the spec. you can not optimize that
> away. People do funky stuff with XForms, and somebody might actually expect the
> events to be sent.
> 

Fair enough.  I'll fix this when you let me know what other changes you are looking for.

Comment 22 aaronr 2006-01-19 15:45:40 PST
as per comment in bug 279063, I'll change setContent to handle the envelope content cloning rather than the script author.
Comment 23 Allan Beaufour 2006-02-06 03:34:23 PST
(In reply to comment #22)
> as per comment in bug 279063, I'll change setContent to handle the envelope
> content cloning rather than the script author.

So, should I wait for that before doing a (much too long waited for) review?
Comment 24 aaronr 2006-02-06 09:53:45 PST
Comment on attachment 203597 [details] [diff] [review]
next try

You might as well wait.  I've got to come up with a testcase that uses setContent (outside of XForms) so that I can make sure that we don't generate the errors that David mentioned if the contentEnvelope is from a different document and I just haven't had time to do that, yet.
Comment 25 aaronr 2006-02-17 14:20:55 PST
Created attachment 212256 [details]
setContent testcase

I don't know why the trigger that I skinned is screwed up looking in this testcase, but it works.  This testcase shows that can do setContent in one document with content from another document.
Comment 26 aaronr 2006-02-17 18:43:32 PST
Created attachment 212282 [details] [diff] [review]
nother fix attempt

ok.  This fix attempt takes Allan's and David's recommendations in addition to fixing the problem that this bug was for (missing some rebuilds, etc. when selecting an item from an itemset).  

To address Allan's and David's comments, setContent now takes an envelope from any document (doesn't have to be a clone of the bound node any longer) and will import and THEN append the nodes from the envelope.  I changed setContent to now take a flag to indicate whether setContent should call the rebuild, recalc, reval, refresh before returning or allow the caller to do it (presumably after batching up more changes so only need to do it once).  I also changed select and select1 to only use setContent when selecting or deslecting copyItems.  It will use accessor's setValue at all other times.

David/Allan: anything that I missed that you wanted me to do with this patch?
Comment 27 Allan Beaufour 2006-02-20 09:04:16 PST
(In reply to comment #26)
> Created an attachment (id=212282) [edit]
> nother fix attempt

I wanted to try this, but something's not working in my trunk build... selects seem to change instance data, but xforms-value-changed is not fired on the select, or other controls bound to the same node... ? Without this patch, that is.
Comment 28 Allan Beaufour 2006-02-21 05:18:59 PST
Comment on attachment 212282 [details] [diff] [review]
nother fix attempt

Code-wise I only have nits.

Functionality-wise something is weird? In attachment 203498 [details]:
1. Click select and choose vanilla
2. Click select to open dropdown
3. Click select to close dropdown
4. Click somewhere else

Why do I get an xforms-rebuild there?

> Index: nsIModelElementPrivate.idl
> ===================================================================
>    /**
> -   * Insert a node under an instance node
> -   */
> -  void setNodeContent(in nsIDOMNode contextNode,
> -                      in nsIDOMNode nodeContent,
> -                      out boolean nodeChanged);
> +   * Insert a node under an instance 
> +   * @param aContextNode       The instance node
> +   * @param aNodeContent       Node that holds the contents to insert under
> +   *                           the instance node
> +   * @param aNodeChanged       Indicates whether the contents of the instance
> +   *                           node really did change due to this action
> +   */


Something is wrong here. Your main comment say that you insert a node under an instance, but aNodeContent says that it holds "contents to insert". I guess you mean the latter.

> Index: nsIXFormsAccessors.idl
> ===================================================================
>    /**
>     * Used to set the complete contents of the bound node.  This function is
>     * meant to be used like setValue() except that it can be used to set more
>     * than just the first textnode contained under the bound node.  If there
>     * is nothing contained under aNode, then all children of the bound node
>     * will be eliminated.
>     *
> -   * @param aNode         aNode should be a copy of the bound node.  setContent
> -   *                      will take the contents of aNode and move them under
> -   *                      bound node.
> +   * @param aNode         setContent will take the contents of aNode and move
> +   *                      them under the control's bound node.

Moves?

> Index: nsXFormsMDGEngine.cpp
> ===================================================================
> @@ -785,20 +777,66 @@ nsXFormsMDGEngine::SetNodeContent(nsIDOM
>    NS_ENSURE_SUCCESS(rv, rv);
 
>    if (nodeType != nsIDOMNode::ELEMENT_NODE) {
>      // got to return something pretty unique that we can check down the road in
>      // order to dispatch any error events
>      return NS_ERROR_DOM_WRONG_TYPE_ERR;
>    }
 
> -  PRBool nodesEqual = nsXFormsUtils::AreNodesEqual(aContextNode,
> -                                                   aContentEnvelope,
> -                                                   PR_FALSE);
> -  if (nodesEqual) {
> +  // Need to determine if the contents of the context node and content envelope
> +  // are already the same.  If so, don't need to do any work.
> +
> +  PRBool hasChildren1, hasChildren2, contentsEqual = PR_FALSE;
> +  nsresult rv1 = aContextNode->HasChildNodes(&hasChildren1);
> +  nsresult rv2 = aContentEnvelope->HasChildNodes(&hasChildren2);
> +  if (NS_SUCCEEDED(rv1) && NS_SUCCEEDED(rv2) && hasChildren1 == hasChildren2) {
> +    // First test passed.  Both have the same number of children nodes.

Not entirely correct comment :)


> Index: resources/content/select.xml
> ===================================================================
>            // add to the control array
>            this._controlArray[this._controlArraySize] =
> -            {control: aItemElement, option: option, type: "item", wasSelected: false}
> +            {control: aItemElement, option: option, type: "item", wasSelected: option.selected}

When is option selected, isn't this only used on initialization? (I might have missed something...)

> @@ -549,20 +549,37 @@
>        <method name="_handleSelection">
>          <body>
>          <![CDATA[
>            var boundNode = this.accessors.getBoundNode();
>            if (!boundNode) {
>              return;
>            }
 
> -          var contentEnvelope = this._getSelectedValues();
> +          // if a copy item is selected or deselected, then we need to replace
> +          // ALL of the current content with the newly selected content.  Which
> +          // means calling setContent.  setValue only messes with the first
> +          // textnode under the bound node.
> +          var copySelectedOrDeselected = new Boolean();
> +          copySelectedOrDeselected.value = false;

Congrats, you are in good shape for "the longest variable name contest" ;-)
Comment 29 Allan Beaufour 2006-02-21 09:47:52 PST
Comment on attachment 212282 [details] [diff] [review]
nother fix attempt

(In reply to comment #28)
> (From update of attachment 212282 [details] [diff] [review] [edit])
> Code-wise I only have nits.
> 
> Functionality-wise something is weird? In attachment 203498 [details] [edit]:
> 1. Click select and choose vanilla
> 2. Click select to open dropdown
> 3. Click select to close dropdown
> 4. Click somewhere else
> 
> Why do I get an xforms-rebuild there?

As per irc, r=me with a fix or a follow-up bug for this.
Comment 30 Olli Pettay [:smaug] 2006-02-21 11:15:59 PST
Comment on attachment 212282 [details] [diff] [review]
nother fix attempt

With Allan's comments, r=me
Comment 31 aaronr 2006-02-22 02:18:59 PST
(In reply to comment #29)
> (From update of attachment 212282 [details] [diff] [review] [edit])
> (In reply to comment #28)
> > (From update of attachment 212282 [details] [diff] [review] [edit] [edit])
> > Code-wise I only have nits.
> > 
> > Functionality-wise something is weird? In attachment 203498 [details] [edit] [edit]:
> > 1. Click select and choose vanilla
> > 2. Click select to open dropdown
> > 3. Click select to close dropdown
> > 4. Click somewhere else
> > 
> > Why do I get an xforms-rebuild there?
> 
> As per irc, r=me with a fix or a follow-up bug for this.
> 

Looks like select1 is blindly updating the bound node during handleBlur().  So I changed this to only update bound node during blur if incremental == false (like select does).  I'm going to ask for reviews again to make sure I'm not overlooking a scenario where we might want to update the bound node even when we are already updating on every selection.

I fixed the other comments from above, too.
Comment 32 aaronr 2006-02-22 02:23:12 PST
Created attachment 212721 [details] [diff] [review]
fixes extraneous rebuild on blur plus comments
Comment 33 Allan Beaufour 2006-02-22 06:55:33 PST
(In reply to comment #28)
> > Index: nsIXFormsAccessors.idl
> > ===================================================================
> >    /**
> >     * Used to set the complete contents of the bound node.  This function is
> >     * meant to be used like setValue() except that it can be used to set more
> >     * than just the first textnode contained under the bound node.  If there
> >     * is nothing contained under aNode, then all children of the bound node
> >     * will be eliminated.
> >     *
> > -   * @param aNode         aNode should be a copy of the bound node.  setContent
> > -   *                      will take the contents of aNode and move them under
> > -   *                      bound node.
> > +   * @param aNode         setContent will take the contents of aNode and move
> > +   *                      them under the control's bound node.
> 
> Moves?

Ok. I could have been more explicit, but what I meant was "I guess you do not _move_ the nodes, you clone/import them".
Comment 34 Allan Beaufour 2006-02-22 07:10:04 PST
(In reply to comment #31)
> Looks like select1 is blindly updating the bound node during handleBlur().  So
> I changed this to only update bound node during blur if incremental == false
> (like select does).  I'm going to ask for reviews again to make sure I'm not
> overlooking a scenario where we might want to update the bound node even when
> we are already updating on every selection.

It seems to work... but I really miss a comment about why. And why the functionality is in handleBlur() and not in _handleSelection.
Comment 35 aaronr 2006-02-22 11:23:36 PST
(In reply to comment #33)
> (In reply to comment #28)
> > > Index: nsIXFormsAccessors.idl
> > > ===================================================================
> > >    /**
> > >     * Used to set the complete contents of the bound node.  This function is
> > >     * meant to be used like setValue() except that it can be used to set more
> > >     * than just the first textnode contained under the bound node.  If there
> > >     * is nothing contained under aNode, then all children of the bound node
> > >     * will be eliminated.
> > >     *
> > > -   * @param aNode         aNode should be a copy of the bound node.  setContent
> > > -   *                      will take the contents of aNode and move them under
> > > -   *                      bound node.
> > > +   * @param aNode         setContent will take the contents of aNode and move
> > > +   *                      them under the control's bound node.
> > 
> > Moves?
> 
> Ok. I could have been more explicit, but what I meant was "I guess you do not
> _move_ the nodes, you clone/import them".
> 


Doh!  Of course.  Will correct comment.

>It seems to work... but I really miss a comment about why. And why
>the functionality is in handleBlur() and not in _handleSelection.

I will add this comment.
Comment 36 Olli Pettay [:smaug] 2006-02-22 13:07:00 PST
Comment on attachment 212721 [details] [diff] [review]
fixes extraneous rebuild on blur plus comments


>+            if (this.selectionOpen && !this._selected) {
>+              this.accessors.setValue(this.inputField.value);

Could you call this._handleSelection(); here. 
It has the check whether select1 is bound to anywhere. 
And only if it is bound, then it calls this.accessors.setValue(this.inputField.value);
Comment 37 aaronr 2006-02-22 15:31:07 PST
Created attachment 212821 [details] [diff] [review]
updated with comments fixed

moved my new incremental test from handleBlur to inside handleSelection and removed my other changes from handleBlur.  Added parameter to _handleSelection so that we know if it was called during blurring or not.  So now handleBlur will still call handleSelection in every case, but handleSelection won't setvalue and setcontent during blurring unless incremental == false.
Comment 38 Allan Beaufour 2006-02-23 03:30:35 PST
Comment on attachment 212821 [details] [diff] [review]
updated with comments fixed

There' no real need to use a Boolean() is there, if _handleSelection is called without a parameter, aInBlur will evaluate to false, right?

Thanks for fixing the select1.xml comment btw.

r=me with a "normal bool"
Comment 39 Olli Pettay [:smaug] 2006-02-23 06:20:00 PST
Comment on attachment 212821 [details] [diff] [review]
updated with comments fixed

Fix Allan's comments and r=me
Comment 40 aaronr 2006-02-23 09:51:38 PST
Created attachment 212909 [details] [diff] [review]
patch for checkin

Doh!  Of course.  Made the change suggested by Allan
Comment 41 aaronr 2006-02-23 16:11:03 PST
checked into trunk
Comment 42 aaronr 2006-02-23 16:52:53 PST
checked into 1.8.1, a=doronr
Comment 43 aaronr 2006-02-23 17:25:06 PST
checked into 1.8.0

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