The context node for non-outermost binding elements is the first node of the binding expression of the immediately enclosing element. An element is "immediately enclosing" when it is the first binding element node in the node-set returned by the XPath expression ancestor::*. This is also referred to as "scoped resolution". [http://www.w3.org/MarkUp/Forms/Group/Drafts/Sources/index-all.html#expr-eval] But what about an output with @value="'test'"? There is no "first node of the binding expression". If I look at the value attribute for the output element in the spec it states: The evaluation context is the same as would be applied to the evaluation of the single node binding. [http://www.w3.org/MarkUp/Forms/Group/Drafts/Sources/index-all.html#ui-output] So I would say that the context of the output with (only) a value attribute should be that: "the same as would be applied to the evaluation of the single node binding". Or?

(In reply to comment #1) > Created an attachment (id=193153) [edit] > Testcase I think that output using @value, should set it's mBoundNode to it's context node. Then the "find context" functionality always just have to find it's first parent with the same model and get the context node from it (as it is doing it now). Otherwise we need a "find parent in same model, not being output with value"... this will also simplify fixing bug 305060 I think.

*ahem* labels are shown before controls...

Here's a go. I return the contextnode from EvaluateNodeBinding to ProcessNodeBinding, and then store it as the mBoundNode on the output.

Comment on attachment 197169 [details] [diff] [review] Patch sorry that it took me so long to get around to commenting on this bug. Something was nagging me in the back of my head about it but I never took the time to sit down and figure out what it was before now. I'm a little concerned that with this patch we'd be setting mBoundNode where we currently don't. There are a lot of potential side effects here. If someone gets an output element (like via skinning) and starts playing around with its delegate interface, it could end up calling methods that look at mBoundNode and could end up altering this context node's value even though it is only peripherially related to the output element. I don't think that you have to try to add a test for an xf:output that uses @value instead of a SNB node. I think the logic that you need to look for is whether the containing element has mBoundNode to use for a context and if it doesn't, then test for mBindAttrsCount. If it has mBindAttrsCount, then look for @ref or @bind. If not either of those, then go up the parent chain again and run the same tests (mBoundNode, mBindAttrsCount, @ref and @bind) until you find one that does. Then there is your context. Odds are that xf:output won't be the last XForms control created that can live nicely without a SNB attribute.

(In reply to comment #5) > (From update of attachment 197169 [details] [diff] [review] [edit]) > sorry that it took me so long to get around to commenting on this bug. > Something was nagging me in the back of my head about it but I never took the > time to sit down and figure out what it was before now. As long as it's a good review, it's worth the waiting time :) > I'm a little concerned that with this patch we'd be setting mBoundNode where we > currently don't. There are a lot of potential side effects here. If someone > gets an output element (like via skinning) and starts playing around with its > delegate interface, it could end up calling methods that look at mBoundNode and > could end up altering this context node's value even though it is only > peripherially related to the output element. Good point. > I don't think that you have to try to add a test for an xf:output that uses > @value instead of a SNB node. I think the logic that you need to look for is > whether the containing element has mBoundNode to use for a context and if it > doesn't, then test for mBindAttrsCount. If it has mBindAttrsCount, then look > for @ref or @bind. If not either of those, then go up the parent chain again > and run the same tests (mBoundNode, mBindAttrsCount, @ref and @bind) until you > find one that does. Then there is your context. Odds are that xf:output won't > be the last XForms control created that can live nicely without a SNB > attribute. > Good point in using mBindingAttrsCount, but why would I need to check the attributes directly? I'll look at using mBindingAttrsCount. Actually, there might be a problem with mBindingAttrsCount and output... will output not be removed from the model if it only has a value attribute ... might be influencing bug 310276...

> > I don't think that you have to try to add a test for an xf:output that uses > > @value instead of a SNB node. I think the logic that you need to look for is > > whether the containing element has mBoundNode to use for a context and if it > > doesn't, then test for mBindAttrsCount. If it has mBindAttrsCount, then look > > for @ref or @bind. If not either of those, then go up the parent chain again > > and run the same tests (mBoundNode, mBindAttrsCount, @ref and @bind) until you > > find one that does. Then there is your context. Odds are that xf:output won't > > be the last XForms control created that can live nicely without a SNB > > attribute. > > > > Good point in using mBindingAttrsCount, but why would I need to check the > attributes directly? > You need to check the attributes individually because I think that you or Smaug needed to alter how mBindingAttrsCount works a while back, if I remember right. It now takes into account @model, too. So you can't rely just on mBindingAttrsCount to tell you that it is a control with SNB.

(In reply to comment #7) > > > I don't think that you have to try to add a test for an xf:output that uses > > > @value instead of a SNB node. I think the logic that you need to look for is > > > whether the containing element has mBoundNode to use for a context and if it > > > doesn't, then test for mBindAttrsCount. If it has mBindAttrsCount, then look > > > for @ref or @bind. If not either of those, then go up the parent chain again > > > and run the same tests (mBoundNode, mBindAttrsCount, @ref and @bind) until you > > > find one that does. Then there is your context. Odds are that xf:output won't > > > be the last XForms control created that can live nicely without a SNB > > > attribute. > > > > > > > Good point in using mBindingAttrsCount, but why would I need to check the > > attributes directly? > > > > You need to check the attributes individually because I think that you or Smaug > needed to alter how mBindingAttrsCount works a while back, if I remember right. > It now takes into account @model, too. So you can't rely just on > mBindingAttrsCount to tell you that it is a control with SNB. Ah, that was me. I forgot about that. But the change makes sense because: <group model="m1" ref="x"> <group model="m2" ref="y"> <output model="m1" value="z"> <label ref="."> here label should be bound to "x", which breaks your algorithm. How about still using my approach but make output override getBoundNode, and not return mBoundNode if it only has a @value?

Output with a value saves the parent's context node in mBoundNode, which means that the correct context node (the parent's) will be passed on to children. But it also ensures that it is not returned in GetBoundNode(), as it should not incur refreshes or receive events for it, etc. It also includes a few fixes for unecessary QIs, and makes the output member vars packed bool, and initializes them (you never know...)

(In reply to comment #9) > Created an attachment (id=216512) [edit] Is this patch rotten?

Comment on attachment 216512 [details] [diff] [review] New approach yes, needs to be updated. (Looks good btw, but I'd like to test it)

Comment on attachment 217196 [details] [diff] [review] updated patch >- CallQueryInterface(mBoundNode, aContextNode); // addrefs >- NS_ASSERTION(*aContextNode, "could not QI context node from bound node?"); >+ NS_ADDREF(*aContextNode = mBoundNode); // addrefs Remove "// addrefs", I guess it is quite clear that NS_ADDREF addrefs ;) > /** >- * Binds the control to the model. Does _not_ set mBoundNode, etc. Just sets >- * mModel, and handle attaching to the model (including reattaching from any >- * old model). >+ * Binds the control to the model. Does _not_ establish a node binding. Just >+ * sets mModel, and handle attaching to the model (including reattaching >+ * from any old model). >+ * >+ * @param aSetBoundNode Set mBoundNode too? (only that, still no proper >+ * node binding) Could you still add a comment why that (mBoundNode) is not a proper node binding. with those, r=me

Comment on attachment 217267 [details] [diff] [review] Patch with smaug's comments >Index: nsXFormsUtils.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.cpp,v >retrieving revision 1.74 >diff -u -p -U8 -r1.74 nsXFormsUtils.cpp >--- nsXFormsUtils.cpp 30 Mar 2006 18:40:51 -0000 1.74 >+++ nsXFormsUtils.cpp 5 Apr 2006 08:11:48 -0000 >@@ -362,17 +362,18 @@ nsXFormsUtils::GetNodeContext(nsIDOMElem > } > > return NS_OK; > } > > /* static */ already_AddRefed<nsIModelElementPrivate> > nsXFormsUtils::GetModel(nsIDOMElement *aElement, > nsIXFormsControl **aParentControl, >- PRUint32 aElementFlags) >+ PRUint32 aElementFlags, >+ nsIDOMNode **aContextNode) > > { > nsCOMPtr<nsIModelElementPrivate> model; > nsCOMPtr<nsIDOMNode> contextNode; > nsCOMPtr<nsIDOMElement> bind; > PRBool outerbind; > > GetNodeContext(aElement, >@@ -380,19 +381,27 @@ nsXFormsUtils::GetModel(nsIDOMElement > getter_AddRefs(model), > getter_AddRefs(bind), > &outerbind, > aParentControl, > getter_AddRefs(contextNode)); > > NS_ENSURE_TRUE(model, nsnull); > >+ if (aContextNode) { >+ if (contextNode) { >+ NS_ADDREF(*aContextNode = contextNode); >+ } else { >+ *aContextNode = nsnull; >+ } >+ } >+ nit: can't you just change this to: if (aContextNode) { NS_IF_ADDREF(*aContextNode = contextNode); } either way, r=me

Fixed on trunk

This has regressed.

XSmiles and formsPlayer don't support this. I guess we should make sure that someone else is seeing this our way before trying to fix this again. I asked the WG what the answer is: http://lists.w3.org/Archives/Public/www-forms/2007Apr/0000.html I've already started looking at this so I guess I'll assign it to myself to resolve this one way or the other.

(In reply to comment #19) > XSmiles and formsPlayer don't support this. I guess we should make sure that > someone else is seeing this our way before trying to fix this again. I asked > the WG what the answer is: > http://lists.w3.org/Archives/Public/www-forms/2007Apr/0000.html > > I've already started looking at this so I guess I'll assign it to myself to > resolve this one way or the other. > MarkB replied and said formsPlayer fixed their behavior and that it should work as Allan thought.

I can not reproduce this bug on the trunk or in 0.8.0.3. This bug was probably fixed by my patch to bug 376217.