From ef65fa07cc537ccc98354fd94e575d41f073243d Mon Sep 17 00:00:00 2001 From: "aaronr%us.ibm.com" Date: Thu, 23 Feb 2006 23:53:23 +0000 Subject: [PATCH] [XForms] Fixes issue with selects and itemsets. Bug 316895, r=allan+smaug --- extensions/xforms/nsIModelElementPrivate.idl | 15 ++-- extensions/xforms/nsIXFormsAccessors.idl | 10 ++- extensions/xforms/nsXFormsAccessors.cpp | 7 +- extensions/xforms/nsXFormsMDGEngine.cpp | 84 ++++++++++++++----- .../xforms/resources/content/select.xml | 47 +++++++++-- .../xforms/resources/content/select1.xml | 36 +++++--- 6 files changed, 154 insertions(+), 45 deletions(-) diff --git a/extensions/xforms/nsIModelElementPrivate.idl b/extensions/xforms/nsIModelElementPrivate.idl index 336aab017edf..c1693434fc22 100644 --- a/extensions/xforms/nsIModelElementPrivate.idl +++ b/extensions/xforms/nsIModelElementPrivate.idl @@ -47,7 +47,7 @@ interface nsIDOMNode; * Private interface implemented by the model element for other * elements to use. */ -[uuid(ccba3717-ef05-41cd-b831-f7f5ab3b921c)] +[uuid(a6522c1a-a343-4b36-a130-75eb279a667a)] interface nsIModelElementPrivate : nsIXFormsModelElement { /** @@ -100,11 +100,16 @@ interface nsIModelElementPrivate : nsIXFormsModelElement out AString nodeValue); /** - * Insert a node under an instance node + * Insert a set of nodes underneath an instance node. + * @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 */ - void setNodeContent(in nsIDOMNode contextNode, - in nsIDOMNode nodeContent, - out boolean nodeChanged); + void setNodeContent(in nsIDOMNode aContextNode, + in nsIDOMNode aNodeContent, + out boolean aNodeChanged); /** * Validates the instance node against the schemas loaded by the model. diff --git a/extensions/xforms/nsIXFormsAccessors.idl b/extensions/xforms/nsIXFormsAccessors.idl index 8d532198f791..29fae1fed2d3 100644 --- a/extensions/xforms/nsIXFormsAccessors.idl +++ b/extensions/xforms/nsIXFormsAccessors.idl @@ -95,9 +95,11 @@ interface nsIXFormsAccessors : nsISupports * 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 copy + * them under the control's bound node. + * @param aForceUpdate Indicates whether setContent should rebuild, + * recalculate, revalidate and refresh the model that + * this control is bound to prior to returning */ - void setContent(in nsIDOMNode aNode); + void setContent(in nsIDOMNode aNode, in boolean aForceUpdate); }; diff --git a/extensions/xforms/nsXFormsAccessors.cpp b/extensions/xforms/nsXFormsAccessors.cpp index e0f30c81f9e2..366f5ae14b7b 100644 --- a/extensions/xforms/nsXFormsAccessors.cpp +++ b/extensions/xforms/nsXFormsAccessors.cpp @@ -115,9 +115,10 @@ nsXFormsAccessors::IsValid(PRBool *aStateVal) } NS_IMETHODIMP -nsXFormsAccessors::SetContent(nsIDOMNode *aNode) +nsXFormsAccessors::SetContent(nsIDOMNode *aNode, PRBool aForceUpdate) { NS_ENSURE_STATE(mElement); + NS_ENSURE_ARG(aNode); nsCOMPtr boundNode; nsresult rv = GetBoundNode(getter_AddRefs(boundNode)); @@ -129,10 +130,12 @@ nsXFormsAccessors::SetContent(nsIDOMNode *aNode) PRBool changed; rv = modelPriv->SetNodeContent(boundNode, aNode, &changed); NS_ENSURE_SUCCESS(rv, rv); - if (changed) { + if (aForceUpdate) { nsCOMPtr model = do_QueryInterface(modelPriv); if (model) { + rv = nsXFormsUtils::DispatchEvent(model, eEvent_Rebuild); + NS_ENSURE_SUCCESS(rv, rv); rv = nsXFormsUtils::DispatchEvent(model, eEvent_Recalculate); NS_ENSURE_SUCCESS(rv, rv); rv = nsXFormsUtils::DispatchEvent(model, eEvent_Revalidate); diff --git a/extensions/xforms/nsXFormsMDGEngine.cpp b/extensions/xforms/nsXFormsMDGEngine.cpp index b39a43c0de00..f49558bc0183 100644 --- a/extensions/xforms/nsXFormsMDGEngine.cpp +++ b/extensions/xforms/nsXFormsMDGEngine.cpp @@ -753,17 +753,9 @@ nsXFormsMDGEngine::SetNodeContent(nsIDOMNode *aContextNode, NS_ENSURE_ARG(aContentEnvelope); // ok, this is tricky. This function will REPLACE the contents of - // aContextNode with the CONTENTS of aContentEnvelope. No, not a clone of - // the contents, but the contents themselves. If aContentEnvelope has no - // contents, then any contents that aContextNode has will still be removed. - // In order to determine whether the incoming node content is the same as what - // is already contained in aContextNode, aContentEnvelope MUST be a clone (not - // deep) of aContextNode, otherwise aNodeChanged will always be returned as - // being PR_TRUE. I took this approach because I think it is much more - // efficient for the caller to build a complete list of what goes in the - // contents in one go rather than allowing any number of appends to existing - // content one node at a time. There are quite a few links in the call chain - // to go from nsXFormsDelegateStub to here. + // aContextNode with the a clone of the contents of aContentEnvelope. If + // aContentEnvelope has no contents, then any contents that aContextNode + // has will still be removed. if (aNodeChanged) { *aNodeChanged = PR_FALSE; @@ -790,10 +782,56 @@ nsXFormsMDGEngine::SetNodeContent(nsIDOMNode *aContextNode, 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, we can avoid some unnecessary 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. + if (hasChildren1) { + nsCOMPtr children1, children2; + PRUint32 childrenLength1, childrenLength2; + + rv1 = aContextNode->GetChildNodes(getter_AddRefs(children1)); + rv2 = aContentEnvelope->GetChildNodes(getter_AddRefs(children2)); + + if (NS_SUCCEEDED(rv1) && NS_SUCCEEDED(rv2) && children1 && children2) { + + // Both have child nodes. + rv1 = children1->GetLength(&childrenLength1); + rv2 = children2->GetLength(&childrenLength2); + if (NS_SUCCEEDED(rv1) && NS_SUCCEEDED(rv2) && + (childrenLength1 == childrenLength2)) { + + // both have the same number of child nodes. Now checking to see if + // each of the children are equal. + for (PRUint32 i = 0; i < childrenLength1; ++i) { + nsCOMPtr child1, child2; + + rv1 = children1->Item(i, getter_AddRefs(child1)); + rv2 = children2->Item(i, getter_AddRefs(child2)); + if (NS_FAILED(rv1) || NS_FAILED(rv2)) { + // Unexpected error. Not as many children in the list as we + // were told. + return NS_ERROR_UNEXPECTED; + } + + contentsEqual = nsXFormsUtils::AreNodesEqual(child1, child2, PR_TRUE); + if (!contentsEqual) { + break; + } + } + } + } + } else { + // neither have children + contentsEqual = PR_TRUE; + } + } + + if (contentsEqual) { return NS_OK; } @@ -821,12 +859,22 @@ nsXFormsMDGEngine::SetNodeContent(nsIDOMNode *aContextNode, nsCOMPtr childNode; rv = aContentEnvelope->GetFirstChild(getter_AddRefs(childNode)); NS_ENSURE_SUCCESS(rv, rv); + + nsCOMPtr document; + rv = aContextNode->GetOwnerDocument(getter_AddRefs(document)); + NS_ENSURE_STATE(document); + while (childNode) { - rv = aContextNode->AppendChild(childNode, getter_AddRefs(resultNode)); + nsCOMPtr importedNode; + rv = document->ImportNode(childNode, PR_TRUE, getter_AddRefs(importedNode)); + NS_ENSURE_STATE(importedNode); + rv = aContextNode->AppendChild(importedNode, getter_AddRefs(resultNode)); NS_ENSURE_SUCCESS(rv, rv); - rv = aContentEnvelope->GetFirstChild(getter_AddRefs(childNode)); + rv = childNode->GetNextSibling(getter_AddRefs(resultNode)); NS_ENSURE_SUCCESS(rv, rv); + + resultNode.swap(childNode); } // NB: Never reached for Readonly nodes. @@ -834,8 +882,6 @@ nsXFormsMDGEngine::SetNodeContent(nsIDOMNode *aContextNode, *aNodeChanged = PR_TRUE; } - // Not calling MarkNodeAsChanged since caller will do a full rebuild - return NS_OK; } diff --git a/extensions/xforms/resources/content/select.xml b/extensions/xforms/resources/content/select.xml index 2fbc8c81657c..ed44f84ae52d 100644 --- a/extensions/xforms/resources/content/select.xml +++ b/extensions/xforms/resources/content/select.xml @@ -514,7 +514,7 @@ // 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} this._controlArraySize++; this.uiElement.appendChild(option); @@ -554,10 +554,27 @@ 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; + var contentEnvelope = this._getSelectedValues(copySelectedOrDeselected); if (contentEnvelope) { if (boundNode.nodeType == Node.ELEMENT_NODE) { - this.accessors.setContent(contentEnvelope); + // we shouldn't call setContent if we haven't selected any + // copyItems. We can't just test for a single text node under + // the bound node because this could still have been the result + // of a copyItem being selected. And if a copyItem is selected, + // then we need to do the whole rebuild, recalculate, revalidate, + // refresh process according to the spec...whether we really need + // to or not. + if (copySelectedOrDeselected.value == false) { + this.accessors.setValue(contentEnvelope.textContent); + } else { + this.accessors.setContent(contentEnvelope, true); + } } else { // if some copyItems were selected by the user prior to the call // to _getSelectedValues, then we would not have set up @@ -577,6 +594,7 @@ + - // _handleSelection updates the bound node with the value from the - // currently selected item's value element or copy element. + +