Right now, controls that use @bind just copy the XPath expression found there and evaluate it. That's not correct behaviour. Binds are static, and should only be reevaluated when rebuild is done. So we need to store the xpathresult and get that from the bind, when a controls uses @bind (it might be worthwhile to look at bug 307420 at the same time)

Ok, I'll head down this path. I'll take the approach I described in comment 0, and fix bug 307420 on the way. The "resolution" of inner binds is, afaik, that it should in fact be possible to reference them, and you will get the first node of the nodeset that the bind is bound to. That means that for that reason we should store a reference to that node on the bind. It might also make sense to save the XPathExpression on the bind, instead of re-creating that on each rebuild... that might end up in a follow bug.

Hmm, how this should actually should work is a bit questionable. I might have to delay this. I've written to the WG. http://lists.w3.org/Archives/Member/w3c-forms/2006JanMar/0149.html (for those who have access)

As bug 307420 has "undefined behaviour" I'm skipping that for the moment. Enough questions arises with static binds in the first place regarding lazy instances. Nevertheless here's the patch so far. I'm not overly satisfied with the handling of the different types of XPath result types, but I'm not sure I can avoid it. The patch makes the binds static alright, but for some reason the controls need two rebuilds to get the idea that they have changed into their heads.

(In reply to comment #1) > Created an attachment (id=211857) [edit] > Testcase Hmm, I think I have taken the concept of "static binds" a bit too strict. The staticness should only regard the nodeset that the bind is bound to, the values should be updated in the controls. Would be weird otherwise.

Comment on attachment 212485 [details] [diff] [review] Patch >diff -X patch-excludes -uprN -U8 xforms/nsXFormsModelElement.cpp xforms.staticbind/nsXFormsModelElement.cpp >--- xforms/nsXFormsModelElement.cpp 2006-02-17 11:11:30.000000000 +0100 >+++ xforms.staticbind/nsXFormsModelElement.cpp 2006-02-20 17:12:45.000000000 +0100 >@@ -860,20 +872,27 @@ nsXFormsModelElement::Refresh() > for (PRInt32 j = 0; j < mChangedNodes.Count(); ++j) { > curChanged = do_QueryInterface(mChangedNodes[j]); > > // Check whether the bound node is dirty. If so, we need to refresh the > // control (get updated node value from the bound node) > if (!refresh && boundNode) { > curChanged->IsSameNode(boundNode, &refresh); > >- if (refresh) >- // We need to refresh the control. We cannot break out of the loop >- // as we need to check dependencies >+ if (refresh ^ usesModelBinding) >+ // We need to refresh, but have to keep checking for >+ // dependencies, or the control uses a model binding, which >+ // means that no dependencies should be checked. Either way, no >+ // more to do for this changed node. > continue; >+ if (refresh && usesModelBinding) >+ // a node that uses model bindings do not need to have its >+ // dependencies ehcked, so as soon as we have found out that it >+ // needs a refresh, break out. >+ break; > } nit: spelling, ehcked -> checked. Also, maybe add a comment explaining why a control bound via @bind doesn't care whether a dependency is dirty or not but @ref does. Might even be able to do away with a comment on every 'if' test if there is a kind of 'this is what we are trying to accomplish' type of comment above the beginning of the for loop. >diff -X patch-excludes -uprN -U8 xforms/nsXFormsOutputElement.cpp xforms.staticbind/nsXFormsOutputElement.cpp >--- xforms/nsXFormsOutputElement.cpp 2005-08-03 20:43:44.000000000 +0200 >+++ xforms.staticbind/nsXFormsOutputElement.cpp 2006-02-20 13:17:57.000000000 +0100 >@@ -119,17 +119,22 @@ nsXFormsOutputElement::Bind() > } > > if (NS_FAILED(rv)) { > nsXFormsUtils::ReportError(NS_LITERAL_STRING("controlBindError"), mElement); > return rv; > } > > if (result) { >- result->GetSingleNodeValue(getter_AddRefs(mBoundNode)); >+ if (mUsesModelBinding) { >+ // When bound via @bind, we'll get a snapshot back >+ result->SnapshotItem(0, getter_AddRefs(mBoundNode)); >+ } else { >+ result->GetSingleNodeValue(getter_AddRefs(mBoundNode)); >+ } > } would it be worth setting mHasBinding = mUsesModelBinding and testing it before calling HasAttribute for both 'ref' and 'bind' earlier in this function? Just got to make sure that we still check for @bind even if mUsesModelBinding is false, otherwise we'll end up using @value when we shouldn't (i.e. when there is a @bind that points to an inner bind). This is as far as I got. I'll finish it up tomorrow. Looks really good so far.

Comment on attachment 212485 [details] [diff] [review] Patch >diff -X patch-excludes -uprN -U8 xforms/nsXFormsUtils.cpp xforms.staticbind/nsXFormsUtils.cpp >--- xforms/nsXFormsUtils.cpp 2006-02-17 11:11:30.000000000 +0100 >+++ xforms.staticbind/nsXFormsUtils.cpp 2006-02-20 13:19:46.000000000 +0100 >@@ -465,25 +465,27 @@ nsXFormsUtils::EvaluateXPath(const nsASt > /* static */ nsresult > nsXFormsUtils::EvaluateNodeBinding(nsIDOMElement *aElement, > PRUint32 aElementFlags, > const nsString &aBindingAttr, > const nsString &aDefaultRef, > PRUint16 aResultType, > nsIModelElementPrivate **aModel, > nsIDOMXPathResult **aResult, >+ PRBool *aUsesModelBind, > nsCOMArray<nsIDOMNode> *aDeps, > nsStringArray *aIndexesUsed) > { >- if (!aElement || !aModel || !aResult) { >- return NS_OK; >+ if (!aElement || !aModel || !aResult || !aUsesModelBind) { >+ return NS_ERROR_FAILURE; > } > > *aModel = nsnull; > *aResult = nsnull; >+ *aUsesModelBind = PR_FALSE; > > nsCOMPtr<nsIDOMNode> contextNode; > nsCOMPtr<nsIDOMElement> bindElement; > PRBool outerBind; > PRInt32 contextPosition; > PRInt32 contextSize; > nsresult rv = GetNodeContext(aElement, > aElementFlags, >@@ -495,52 +497,67 @@ nsXFormsUtils::EvaluateNodeBinding(nsIDO > &contextSize); > > NS_ENSURE_SUCCESS(rv, rv); > > if (!contextNode) { > return NS_OK; // this will happen if the doc is still loading > } > >- nsAutoString expr; >+ //////////////////// >+ // STEP 1: Handle @bind'ings > if (bindElement) { >- if (!outerBind) { >- // "When you refer to @id on a nested bind it returns an emtpy nodeset >- // because it has no meaning. The XForms WG will assign meaning in the >- // future." >- // @see http://www.w3.org/MarkUp/Group/2004/11/f2f/2004Nov11#resolution6 >+ if (outerBind) { >+ // If there is an outer bind element, we retrieve its nodeset. >+ nsCOMPtr<nsIContent> content(do_QueryInterface(bindElement)); >+ NS_ASSERTION(content, "nsIDOMElement not implementing nsIContent?!"); >+ >+ NS_IF_ADDREF(*aResult = >+ NS_STATIC_CAST(nsIDOMXPathResult*, >+ content->GetProperty(nsXFormsAtoms::bind))); >+ *aUsesModelBind = PR_TRUE; >+ } >+ >+ // References to inner binds are not defined. >+ // "When you refer to @id on a nested bind it returns an emtpy nodeset >+ // because it has no meaning. The XForms WG will assign meaning in the >+ // future." >+ // @see http://www.w3.org/MarkUp/Group/2004/11/f2f/2004Nov11#resolution6 > I think that we should report a warning to the JS Console or something. Because if someone is using a form that works in another processor and it doesn't work here because they handle inner binds differently than we do, the poor form author is going to have a tough time tracking down why the form doesn't work. Especially if they didn't even author it but copied it from somewhere else and modified it for their own usage. with this and my review comments from yesterday, r=me

> >@@ -119,17 +119,22 @@ nsXFormsOutputElement::Bind() > > } > > > > if (NS_FAILED(rv)) { > > nsXFormsUtils::ReportError(NS_LITERAL_STRING("controlBindError"), mElement); > > return rv; > > } > > > > if (result) { > >- result->GetSingleNodeValue(getter_AddRefs(mBoundNode)); > >+ if (mUsesModelBinding) { > >+ // When bound via @bind, we'll get a snapshot back > >+ result->SnapshotItem(0, getter_AddRefs(mBoundNode)); > >+ } else { > >+ result->GetSingleNodeValue(getter_AddRefs(mBoundNode)); > >+ } > > } > > would it be worth setting mHasBinding = mUsesModelBinding and testing it before > calling HasAttribute for both 'ref' and 'bind' earlier in this function? Problem is that mUsesModelBinding is maintained by ProcessNodeBinding(), called after the checks.

(In reply to comment #10) > >+ // References to inner binds are not defined. > >+ // "When you refer to @id on a nested bind it returns an emtpy nodeset > >+ // because it has no meaning. The XForms WG will assign meaning in the > >+ // future." > >+ // @see http://www.w3.org/MarkUp/Group/2004/11/f2f/2004Nov11#resolution6 > > > > I think that we should report a warning to the JS Console or something. > Because if someone is using a form that works in another processor and it > doesn't work here because they handle inner binds differently than we do, the > poor form author is going to have a tough time tracking down why the form > doesn't work. Especially if they didn't even author it but copied it from > somewhere else and modified it for their own usage. Good point. Noted.

Fixed Aaron's comments. I also added an extra check in nsXFormsModelElement::Refresh(), dunno if you want to look it through too Aaron?

Comment on attachment 212716 [details] [diff] [review] With Aaron's comments >diff -X patch-excludes -uprN -U8 xforms/nsIXFormsControl.idl xforms.staticbind/nsIXFormsControl.idl >--- xforms/nsIXFormsControl.idl 2005-06-21 18:47:29.000000000 +0200 >+++ xforms.staticbind/nsIXFormsControl.idl 2006-02-20 16:52:48.000000000 +0100 >@@ -81,9 +81,15 @@ interface nsIXFormsControl : nsIXFormsCo Because you're modifying an interface, change its uuid >@@ -163,16 +165,21 @@ protected: > PRBool mHasParent; > > /** State to prevent infinite loop when generating and handling xforms-next > * and xforms-previous events > */ > PRBool mPreventLoop; > > /** >+ * Does the control use a model bind? That is, does it have a @bind. >+ */ >+ PRBool mUsesModelBinding; While you're here, could you please change PRBool members to use PRPackedBool. >@@ -859,26 +878,33 @@ nsXFormsModelElement::Refresh() > >+ >+ // Two ways to go here. Keep in mind that controls using model >+ // binding expressions never needs to have dependencies checked as >+ // they only rebind on xforms-rebuild >+ if (refresh ^ usesModelBinding) >+ // 1) If either the control needs a refresh or it uses a model >+ // binding we can continue to next changed node > continue; >+ if (refresh && usesModelBinding) >+ // 2) If the control needs a refresh, and uses model bindings, >+ // we can stop checking here >+ break; > } Could you write this in a bit different way... Something like: if (refresh && usesModelBinding) { // ... // ... break } else if (refresh || usesModelBinding) { // ... // ... continue; } If nothing else, use at least {} -brackets after |if|.

w/smaug's comments fixed

Checked in on trunk