Closed Bug 265931 Opened 20 years ago Closed 20 years ago

Problem with nested binds

Categories

(Core Graveyard :: XForms, defect)

Product:
Core Graveyard
Component:
XForms
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: allan, Assigned: allan)

References

(
URL
)

Details

Attachments

(1 file, 4 obsolete files)

Oops, comments now agree with aOuterBind parameter
41.97 KB, patch
bryner
: superreview+
Details | Diff | Splinter Review 
The patch in bug #265471, fits the description: "... the first node of the
parent's nodeset is used as the context node." David pointed out that there is 
a problem with that. In our opinion, the <input>'s in the following should be
bound to the same node. They are not.

<html xmlns="http://www.w3.org/1999/xhtml"
      xmlns:xsd="http://www.w3.org/2001/XMLSchema"
      xmlns:xforms="http://www.w3.org/2002/xforms">
<head>
  <title>Nest Bind Form</title>
  <xforms:model>
    <xforms:instance xmlns="">
      <data>
        <value/>
        <value>
          <data>value</data>
        </value>
      </data>
    </xforms:instance>
    <xforms:bind nodeset="value">
      <xforms:bind nodeset="data" id="bnd"/>
    </xforms:bind>
    <xforms:bind nodeset="value/data" id="bnd2"/>
  </xforms:model>
</head>
<body>
<h1>Nested Bind Test Form 2</h1>
<p>
<xforms:input bind="bnd">
  <xforms:label/>
</xforms:input>
<br/>
<xforms:input bind="bnd2">
  <xforms:label/>
</xforms:input>
</p>
</body>
</html>
(In reply to comment #0)
> The patch in bug #265471, fits the description: "... the first node of the
> parent's nodeset is used as the context node." David pointed out that there is 
> a problem with that. In our opinion, the <input>'s in the following should be
> bound to the same node. They are not.

The relevant section of the spec (7.4.2) states:
-----
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".
-----

I guess the difference is in the interpretation of "first node of the binding
expression".  My patch interprets it as "first node of the result nodeset".  It
seems like the interpretation you're asking for is that we basically use the
outer _expression_ as the context for the inner expression.  This doesn't seem
like what the spec calls for, nor do I see a way to support that in XPath.

Reading the spec, I have to agree with Brian on his interpretation.  That is
exactly how I read it.  But the Novell Java XForms processor and formsPlayer
both handle the example as Allan and David interpret it.  Oracle, Chiba and
X-Smiles don't handle the example form well at all.

Though not exactly on point, this discussion hints on how it should be handled
(as far as formsPlayer is concerned):
http://article.gmane.org/gmane.comp.web.xforms/1726
Hmmm, yeah I guess that is what it says in the specification. We had another
view on how it should work. We still mean that "our opinion is better". Then the
nested binds can be seen as shortcut for just concatenating the
nodeset-expressions. I think that is quite intuitive to understand, whereas the
current approach is not.

But ok, that is a problem for the WG... end of bug.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → WONTFIX
Well, 'your opinion is better' for one use case, the one where you want to 
concatenate two strings together to make a longer one ;).

However, whether the spec spells it out or not, the only way you can really 
make @nodeset behave consistenly in all situations is to see it is an iterator.

I do agree with your use case though, and that we should have a feature that 
allows XPath statements to be built up by concatenating smaller parts, and I 
also would like to see the ability to use xf:bind IDs within context -- i.e., 
to refer to those parts -- and I've raised both of these things with the WG, 
as has Mark Seaborne.

-- Mark Birbeck
After talking about this with TV Raman, I now think I may have been applying the
wrong section of the spec to this case.  Section 7.4, bullet 5, says:

For Nodeset binding expressions, the context size is the size of the node-set,
and the context position is the document order position of the node currently
being processed within the node-set.

which I would say describes the behavior of FormsPlayer and what Allan had asked
for.  So, in this respect, if <bind> qualifies as a "binding element", then
bullets 2 and 5 contradict each other, in my opinion.  It seems likely that any
clarification that would happen in the spec would go the way that
implementations have already gone, so I'm ok with changing our behavior to match
that.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attached patch patch (obsolete) — — Splinter Review 
This should make our behavior be what's discussed here.  When evaluating a
bind's nodeset, we will build up an XPath expression to evaluate by finding the
nearest ancestor which either (a) contains an absolute XPath expression, or (b)
is outermost, then build up an expression to evaluate by concatenating down to
the current bind element.
Comment on attachment 164030 [details] [diff] [review]
patch

Oh, also, the change to ParentChanged() is to prevent refreshing on shutdown
which seems to lead to a crash.
Attachment #164030 - Flags: review?(allan)
Comment on attachment 164030 [details] [diff] [review]
patch

Sneaking in the bug fix for nsXFormsInputElement, are you :)

More seriously, there are two problems:
1) Absoluteness
+    PRBool isAbsolute = (!nodeset.IsEmpty() &&
+			  nodeset.CharAt(0) == PRUnichar('/'));
This one's absolute too:
(/nodes)
And there are probably more cases (whitespace?). This is really a problem for
the XPath engine.

2) Appending
+    if (!isAbsolute) {
+      aExpr.AppendLiteral("/");
+      aExpr.Append(nodeset);
Simple appending will not work for functions, parentheses, etc:
<bind nodeset="/nodes">
  <bind nodeset="instance('i2')/other"/>
</bind>
Attachment #164030 - Flags: review?(allan) → review-
That's pretty much what I was worried about, and why I wanted someone mho knows
more about XPath than me to review.  I think I may have to go with my original
plan for this, which was to add an extenion to nsIXPathExpression::evaluate that
allows you to supply a nodeset-context for the evaluation instead of just a
single-node-context.
I would let the issue rest for a couple of weeks. The WG has a face-to-face
meeting next week, where they, hopefully, will discuss this issue. What do you
know? They might solve the bug for us :)
David tells me that the end result is, that references to ids on nested binds
should always return an empty nodeset. Only references to outermost binds can
give a result. This will be in the errata for 1.0.
Here goes my idea about a solution: We create a XTFElement for bind (bug 257468)
and then use nsIXFormsContextControl to set the context for nested binds. We are
then handling contexts in the same way through out the code.

We could also change the current nsXFormsModel::ProcessBind() to iterate over
the nodeset and do the same, but I vote for the above. What do you say?
(In reply to comment #12)
> Here goes my idea about a solution: We create a XTFElement for bind (bug 257468)
> and then use nsIXFormsContextControl to set the context for nested binds. We are
> then handling contexts in the same way through out the code.
> 
> We could also change the current nsXFormsModel::ProcessBind() to iterate over
> the nodeset and do the same, but I vote for the above. What do you say?

Hmmm, I might change my vote here... it will be very expensive, I'll change
ProcessBind() tomorrow.
Attached patch Patch for nsXFormsUtils (obsolete) — — Splinter Review 
Here's a patch for nsXFormsUtil that reflects the WG decision. It simplifies
the code a lot. EvaluateNodeset() now handles binds via GetNodeContext() gets
the context for bind elements too through a call to a modified
FindBindContext().

The premise is that changes to @nodeset for a bind is only applied after a
xforms-rebuild, which I think makes sense.
Comment on attachment 168138 [details] [diff] [review]
Patch for nsXFormsUtils

r=darin

might want to fixup the XXX comment though.

also, the todo about ELEMENT_WITH_BIND_ATTR can probably be deleted.

otherwise, this looks good to me.
Attachment #168138 - Flags: review+
(In reply to comment #15)
> (From update of attachment 168138 [details] [diff] [review])
> might want to fixup the XXX comment though.

Check. Taking a bit longer, than I thought (tm), but I will fix bug 272225 on
the way, as it is closely related.

BTW, here is the actual wording from the WG:
"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."
(Source: http://www.w3.org/MarkUp/Group/2004/11/f2f/2004Nov11#resolution6)
Assignee: bryner → allan
Blocks: 272225
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — — Splinter Review 
Fixed the XXX comment from Darin's review, and changed ProcessBind too. Changes
the following:

* nsXFormsModelElement::ProcessBind()

- Evaluates each child bind for each node in the resulting nodeset of the
parent. Expensive, but necessary to set correct context position.

* nsXFormsUtils::GetNodeContext()

- Also sets the context node, if the node has a @bind.

* nsXFormsUtils::GetParentModel()

- Returns PRBool to tell whether bind is outermost or not.

* nsXFormsUtils::EvaluateNodeBinding()

- Only evaluates outermost bindings. It returns nsnull as the result otherwise,
but still sets the correct model and context.

- Handles expressions from binds too, so EvaluateNodeset() is no longer needed.


- Removed aBindElement as a return parameter, nobody was using it. (the reason
behind changes to that many files)

- Returns a nsresult, so its possible to see whether the nsIDOMXPathResult is
empty because of an error or that there are no binding expressions present (bug
272225).
Attachment #168138 - Attachment is obsolete: true
Attachment #168691 - Flags: review?(darin)
Comment on attachment 168691 [details] [diff] [review]
Patch v2

>--- xforms/nsXFormsModelElement.cpp	2004-11-26 09:10:26.000000000 +0100

>@@ -677,46 +678,49 @@ nsXFormsModelElement::ProcessBind(nsIDOM

>+          /// @todo Should we continue processing on failure, or give up?
>+          /// (XXX)
>+          if (!ProcessBind(aEvaluator, node,
>+                           snapItem + 1, snapLen,
>+                           nsCOMPtr<nsIDOMElement>(do_QueryInterface(child))))
>+            return PR_FALSE;

failing makes sense doesn't it?  afterall, you want to generate
a binding exception, right?


>--- xforms/nsXFormsOutputElement.cpp	2004-12-08 11:52:29.000000000 +0100

>+    rv = nsXFormsUtils::EvaluateNodeBinding(mElement,
>+                                            nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR,
>+                                            NS_LITERAL_STRING("ref"),
>+                                            EmptyString(),
>+                                            nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE,
>+                                            getter_AddRefs(modelNode),
>+                                            getter_AddRefs(result));
>   } else {
>+    rv = nsXFormsUtils::EvaluateNodeBinding(mElement,
>+                                            nsXFormsUtils::ELEMENT_WITH_MODEL_ATTR,
>+                                            NS_LITERAL_STRING("value"),
>+                                            EmptyString(),
>+                                            nsIDOMXPathResult::STRING_TYPE,
>+                                            getter_AddRefs(modelNode),
>+                                            getter_AddRefs(result));
>   }

hmm... the only thing that's different between these two function
calls is the value of two parameters.  maybe it would be worth it
to use temporary variables for those parameters, and then just code
up one call to EvaluateNodeBinding?  whatever, not a big deal.


>+    // Model is not the immediate parent, this is reference to a nested

"this is _a_ reference"


>   nsCOMPtr<nsIDOMElement> contextNode;
>+  nsCOMPtr<nsIDOMElement> bindElement;
>   PRInt32 contextPosition;
>   PRInt32 contextSize;
>   nsresult rv = GetNodeContext(aElement,
>                                aElementFlags,
>                                aModel,
>-                               aBind,
>+                               getter_AddRefs(bindElement),
>                                getter_AddRefs(contextNode),
>                                &contextPosition,
>                                &contextSize);
> 
>+  if (bindElement && rv == NS_ERROR_ABORT) {

It is a big odd to return out params with a NS_FAILED(rv), but
of course this is not an XPCOM method, so those rules don't need
to apply.  But, for conventions-sake you might want to consider
a different way of indicating this condition.  (e.g., maybe a 
success code instead of a failure code?)

>+  nsCOMPtr<nsIDOMXPathResult> res = EvaluateXPath(expr,
>+                                                  contextNode,
>+                                                  aElement,
>+                                                  aResultType,
>+                                                  contextSize,
>+                                                  contextPosition);
>+
>+  res.swap(*aResult); // addrefs

/addrefs/exchanges ref/


Nothing major, r=darin
Attachment #168691 - Flags: review?(darin) → review+
(In reply to comment #18)
> >@@ -677,46 +678,49 @@ nsXFormsModelElement::ProcessBind(nsIDOM
> 
> >+          /// @todo Should we continue processing on failure, or give up?
> >+          /// (XXX)
> >+          if (!ProcessBind(aEvaluator, node,
> >+                           snapItem + 1, snapLen,
> >+                           nsCOMPtr<nsIDOMElement>(do_QueryInterface(child))))
> >+            return PR_FALSE;
> 
> failing makes sense doesn't it?  afterall, you want to generate
> a binding exception, right?

(I had forgotten that comment...) Yeah, I was just wondering whether we should
continue with other binds. But reading the spec, it says: Fatal error. So
probably not :) I'll delete that.

I'll fix the rest, and a new patch will be on its way.
Attached patch Fixes Darins comments (obsolete) — — Splinter Review 
Who was the joker who made checking of non-outermost bind a part of the return
result? *ahem*, it's an out parameter now.
Attachment #164030 - Attachment is obsolete: true
Attachment #168691 - Attachment is obsolete: true

        Attachment #168754 -
        Attachment is obsolete: true

    
  

        Attachment #168770 -
        Flags: superreview?(bryner)

        Attachment #168770 -
        Flags: superreview?(bryner) → superreview+

    
    

    
  
Checked in via bug 278896.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED

    
    

    
  
*** Bug 272225 has been marked as a duplicate of this bug. ***
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: