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?

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.

all the non-cherry items are NOT copyItems. Selecting/deselecting any of the other items should cause a rebuild.

This fix seems to work fine for me

(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 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.

(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?

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.

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().

*** Bug 319404 has been marked as a duplicate of this bug. ***

just noting that the patch fixes all select bugs I found :)

(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 on attachment 203466 [details] testcase w/ copy This crashes with the patch. Without patch I see quite many ASSERTIONs, but not crash.

(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...

(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 on attachment 203597 [details] [diff] [review] next try And the assertions are Bug 317497. So r=me, I think ;)

(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?

(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.

(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.

> > 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.

as per comment in bug 279063, I'll change setContent to handle the envelope content cloning rather than the script author.

(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 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.

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.

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?

(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 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 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 on attachment 212282 [details] [diff] [review] nother fix attempt With Allan's comments, r=me

(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.

(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".

(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.

(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 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);

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 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 on attachment 212821 [details] [diff] [review] updated with comments fixed Fix Allan's comments and r=me

Doh! Of course. Made the change suggested by Allan

checked into trunk

checked into 1.8.1, a=doronr

checked into 1.8.0