305196 - output with a value attribute sets wrong context for its children

Status: RESOLVED WORKSFORME

(Core Graveyard :: XForms, defect, P2)

Description

[Allan Beaufour]

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?

Comment 1

[Allan Beaufour]

Attachment: Testcase

Comment 2

[Allan Beaufour]

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

Comment 3

[Allan Beaufour]

Attachment: Fixed testcase

*ahem* labels are shown before controls...

Comment 4

[Allan Beaufour]

Attachment: Patch

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

Comment 5

[aaronr]

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.

Comment 6

[Allan Beaufour]

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

Comment 7

[aaronr]

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

Comment 8

[Allan Beaufour]

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

Comment 9

[Allan Beaufour]

Attachment: New approach

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #9)
> Created an attachment (id=216512) [edit]

Is this patch rotten? 

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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 12

[Allan Beaufour]

Attachment: updated patch

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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 14

[Allan Beaufour]

Attachment: Patch with smaug's comments

Comment 15

[aaronr]

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

Comment 16

[Allan Beaufour]

Attachment: Checked-in version

Comment 17

[Allan Beaufour]

Fixed on trunk

Comment 18

[Allan Beaufour]

This has regressed.

Comment 19

[aaronr]

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.

Comment 20

[aaronr]

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

Comment 21

[aaronr]

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.