output with a value attribute sets wrong context for its children

RESOLVED WORKSFORME

Status

Core Graveyard
XForms
P2
normal
RESOLVED WORKSFORME
12 years ago
9 months ago

People

(Reporter: Allan Beaufour, Assigned: aaronr)

Tracking

({fixed1.8.0.4, fixed1.8.1, testcase})

Trunk
fixed1.8.0.4, fixed1.8.1, testcase
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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?
(Reporter)

Comment 1

12 years ago
Created attachment 193153 [details]
Testcase
(Reporter)

Updated

12 years ago
Keywords: testcase
(Reporter)

Comment 2

12 years ago
(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.
(Reporter)

Comment 3

12 years ago
Created attachment 197168 [details]
Fixed testcase

*ahem* labels are shown before controls...
Attachment #193153 - Attachment is obsolete: true
(Reporter)

Updated

12 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 4

12 years ago
Created attachment 197169 [details] [diff] [review]
Patch

Here's a go. I return the contextnode from EvaluateNodeBinding to
ProcessNodeBinding, and then store it as the mBoundNode on the output.
Assignee: aaronr → allan
Attachment #197169 - Flags: review?(aaronr)
(Assignee)

Comment 5

12 years ago
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.
Attachment #197169 - Flags: review?(aaronr) → review-
(Reporter)

Comment 6

12 years ago
(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...
(Assignee)

Comment 7

12 years ago
> > 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.
(Reporter)

Comment 8

12 years ago
(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?
(Reporter)

Updated

12 years ago
Priority: -- → P2
(Assignee)

Updated

11 years ago
Blocks: 326372
(Assignee)

Updated

11 years ago
Blocks: 326373
(Reporter)

Comment 9

11 years ago
Created attachment 216512 [details] [diff] [review]
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...)
Attachment #216512 - Flags: review?(smaug)

Comment 10

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

Is this patch rotten? 

Comment 11

11 years ago
Comment on attachment 216512 [details] [diff] [review]
New approach

yes, needs to be updated.
(Looks good btw, but I'd like to test it)
Attachment #216512 - Flags: review?(Olli.Pettay)
(Reporter)

Comment 12

11 years ago
Created attachment 217196 [details] [diff] [review]
updated patch
Attachment #197169 - Attachment is obsolete: true
Attachment #216512 - Attachment is obsolete: true
Attachment #217196 - Flags: review?(Olli.Pettay)

Comment 13

11 years ago
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
Attachment #217196 - Flags: review?(aaronr)
Attachment #217196 - Flags: review?(Olli.Pettay)
Attachment #217196 - Flags: review+
(Reporter)

Comment 14

11 years ago
Created attachment 217267 [details] [diff] [review]
Patch with smaug's comments
Attachment #217196 - Attachment is obsolete: true
Attachment #217267 - Flags: review?(aaronr)
Attachment #217196 - Flags: review?(aaronr)
(Assignee)

Comment 15

11 years ago
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
Attachment #217267 - Flags: review?(aaronr) → review+
(Reporter)

Comment 16

11 years ago
Created attachment 217390 [details] [diff] [review]
Checked-in version
Attachment #217267 - Attachment is obsolete: true
(Reporter)

Comment 17

11 years ago
Fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Summary: output with a value attribute sets wrong context for its children? → output with a value attribute sets wrong context for its children
Whiteboard: xf-to-branch
(Reporter)

Updated

11 years ago
Blocks: 332853
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.0.3, fixed1.8.1
(Reporter)

Updated

11 years ago
Whiteboard: xf-to-branch
(Reporter)

Comment 18

11 years ago
This has regressed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

10 years ago
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.
Assignee: allan → aaronr
Status: REOPENED → NEW
(Assignee)

Comment 20

10 years ago
(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.
(Assignee)

Comment 21

9 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago9 years ago
Resolution: --- → WORKSFORME
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.